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

Add HTTP compression #5299

Merged
merged 1 commit into from
Apr 2, 2015
Merged

Add HTTP compression #5299

merged 1 commit into from
Apr 2, 2015

Conversation

mattnenterprise
Copy link
Contributor

No description provided.

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@hoppipolla-critic-bot
Copy link

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

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 jdm added the S-awaiting-review There is new code that needs to be reviewed. label Mar 21, 2015
@jdm
Copy link
Member

jdm commented Mar 21, 2015

Nice work! I've left a few comments on Critic.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 21, 2015
@jdm
Copy link
Member

jdm commented Mar 25, 2015

Two small issues left :)

@jdm jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 26, 2015
@jdm
Copy link
Member

jdm commented Mar 26, 2015

Go ahead and squash these changes!

@mattnenterprise
Copy link
Contributor Author

Ok got it.

@jdm jdm added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-squash Some (or all) of the commits in the PR should be combined. labels Mar 26, 2015
@jdm
Copy link
Member

jdm commented Mar 26, 2015

/XMLHttpRequest/response-data-gzip.htm
--------------------------------------
PASS expected FAIL XMLHttpRequest: GZIP response was correctly inflated
/home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/flate2-0.2.0/src/lib.rs:33:1: 33:33 error: can't find crate for `ffi`
/home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/flate2-0.2.0/src/lib.rs:33 extern crate "miniz-sys" as ffi;
                                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The latter is from the CEF build, which can be replicated using ./mach build-cef.

@mattnenterprise
Copy link
Contributor Author

@jdm So the response-data-gzip.htm test passed but it was expected to fail? Also I don't quite understand what is happening with the CEF build err.

@jdm
Copy link
Member

jdm commented Mar 28, 2015

The test failure is because the test is currently annotated to fail in Servo right now (http://mxr.mozilla.org/servo/source/tests/wpt/metadata/XMLHttpRequest/response-data-gzip.htm.ini), and your changes invalidate that. You should remove that ini file now, since it contains no more test annotations once removing this one.

As for the CEF error, that's... mysterious.

@metajack
Copy link
Contributor

Are you able to build CEF on master successfully? i.e. are you able to reproduce that failure locally?

@mattnenterprise
Copy link
Contributor Author

I can reproduce the failure on my branch locally.

@jdm
Copy link
Member

jdm commented Apr 1, 2015

Me too, and strangely enough the flate2-rs clone that Cargo downloaded doesn't contain the miniz-sys folder that's present in https://github.com/alexcrichton/flate2-rs/tree/88cb8cfc12bd5990d846851482b83450822f8703 . I'm super confused.

@jdm
Copy link
Member

jdm commented Apr 1, 2015

Aha, I figured it out! For some reason the cef/Cargo.lock uses a version of flate2 that depends on miniz-sys 0.1.4, whereas all of the other ones use miniz-sys 0.1.3. If you use ./mach update-cargo -p miniz-sys --precise 0.1.3 the build will succeed.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 1, 2015
@mattnenterprise
Copy link
Contributor Author

@jdm It should work now.

@jdm jdm added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 2, 2015
@bors-servo bors-servo closed this Apr 2, 2015
@bors-servo bors-servo merged commit 818f1c5 into servo:master Apr 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants