Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

M1456: Implement MIME Sniffing #4209

Closed
wants to merge 7 commits into from
Closed

M1456: Implement MIME Sniffing #4209

wants to merge 7 commits into from

Conversation

@djzager
Copy link

djzager commented Dec 4, 2014

Issue: #3144

Bring on the review!

To show mime-sniffing feature:

Open gif in servo.
./mach run tests/content/parsable_mime/image/gif/test87a
Copy image outside of version control
cp tests/content/parsable_mime/image/gif/test87a ..
Checkout changeset without feature.
git checkout 0d2251510fb9ad8f4974c99cadafbd1a9a81e30f
Attempt to open gif in servo.
./mach run ../test87a

@djzager
Copy link
Author

djzager commented Dec 4, 2014

Leaving all the commits...we can squash later.

@jdm jdm closed this Dec 4, 2014
@jdm jdm reopened this Dec 4, 2014
@jdm jdm closed this Dec 4, 2014
@jdm jdm reopened this Dec 4, 2014
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Dec 4, 2014

Critic review: https://critic.hoppipolla.co.uk/r/3367

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jdm
Copy link
Member

jdm commented Dec 4, 2014

This has my blessing. Please rebase and squash!

@djzager djzager force-pushed the t29:mime-sniffing branch from 35ecf22 to c18c287 Dec 4, 2014
@djzager
Copy link
Author

djzager commented Dec 4, 2014

Squashed.

@jdm jdm removed the S-needs-squash label Dec 4, 2014
@jdm
Copy link
Member

jdm commented Dec 4, 2014

Still needs a rebase.

@djzager djzager force-pushed the t29:mime-sniffing branch from c18c287 to 49903b2 Dec 4, 2014
@IdeaHat
Copy link
Contributor

IdeaHat commented Dec 4, 2014

@jdm Looks like there is a bunch of net unit tests that are failing now. Some of them are simply because the fact that we will return a type for None...the system currently hangs instead of finishing the tests. These tests range from the data loader, to the image_cache_tasks, to the resource_tasks...

This pull request is not ready for merge.

@jdm
Copy link
Member

jdm commented Dec 4, 2014

Thanks for letting me know!

@IdeaHat
Copy link
Contributor

IdeaHat commented Dec 4, 2014

@jdm It would appear to be due to the pass-through logic...something with the start call, I would guess with opening our own channels. Hopefully we can unwind this.

@djzager djzager force-pushed the t29:mime-sniffing branch from 5cdd080 to 0d64290 Dec 5, 2014
@jdm
Copy link
Member

jdm commented Dec 5, 2014

Could you point out changes you've made in the most recent update? Furthermore, if there are additional changes forthcoming, can you make them in separate commits?

@djzager djzager force-pushed the t29:mime-sniffing branch from 0d64290 to d9bee1f Dec 5, 2014
@IdeaHat
Copy link
Contributor

IdeaHat commented Dec 5, 2014

@jdm Docs for demo: https://www.youtube.com/watch?v=ybX0hOkDJ8I&feature=youtu.be,
https://docs.google.com/document/d/1uhJUdlSzbLXkYnG8MRTnvPCRnksCc4EdkRGTHP6G4VU/edit?usp=sharing

The changes are all commits after 8af107c, but now we are passing unit tests but cannot load files...still not ready to merge :-/

@jdm jdm added the S-needs-rebase label Dec 19, 2014
@jdm jdm mentioned this pull request Feb 21, 2015
@jdm
Copy link
Member

jdm commented Feb 21, 2015

Rebased in #5005.

@jdm jdm closed this Feb 21, 2015
bors-servo pushed a commit that referenced this pull request Apr 7, 2015
This rebases and integrates #4209, removing the sniffer task (turns out it wasn't a great idea), and adds a `--sniff-mime-types` command line flag to enable sniffing for file:// and http:// resources. Tested against a random picture file on my harddrive. The actual MIME sniffing implementation can be extracted into a separate library separately.
bors-servo pushed a commit that referenced this pull request Apr 7, 2015
This rebases and integrates #4209, removing the sniffer task (turns out it wasn't a great idea), and adds a `--sniff-mime-types` command line flag to enable sniffing for file:// and http:// resources. Tested against a random picture file on my harddrive. The actual MIME sniffing implementation can be extracted into a separate library separately.
bors-servo pushed a commit that referenced this pull request Apr 7, 2015
bors-servo
This rebases and integrates #4209, removing the sniffer task (turns out it wasn't a great idea), and adds a `--sniff-mime-types` command line flag to enable sniffing for file:// and http:// resources. Tested against a random picture file on my harddrive. The actual MIME sniffing implementation can be extracted into a separate library separately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.