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

Partial implementation of HTTP-network Fetch step #9153

Merged
merged 1 commit into from Jan 5, 2016

Conversation

@nikkisquared
Copy link
Contributor

nikkisquared commented Jan 5, 2016

I've been working on making a partial implementation of HTTP-network Fetch[1] so that I can begin testing the entire Fetch protocol as implemented in Servo. The empty steps are intentionally so, including the incomplete Step 5, which I started without realizing I don't need to do basic testing.

[1] https://fetch.spec.whatwg.org/#http-network-fetch

Review on Reviewable

@@ -4,16 +4,20 @@

use fetch::cors_cache::{BasicCORSCache, CORSCache, CacheRequestDetails};
use fetch::response::ResponseMethods;
use http_loader::{NetworkHttpRequestFactory, WrappedHttpResponse};
use http_loader::{create_http_connector, obtain_response};

This comment has been minimized.

Copy link
@KiChjang

KiChjang Jan 5, 2016

Member

I'm not sure whether it's a good idea for the fetch infrastructure to depend upon http_loader.

@frewsxcv
Copy link
Member

frewsxcv commented Jan 5, 2016

components/net_traits/lib.rs, line 84 [r1] (raw file):
A couple things:

  1. Considering you're using markdown, it might be helpful to use /// so that it gets rendered in the documentation
  2. You only need one set of [ and ] for links in markdown

Comments from the review on Reviewable.io

@frewsxcv
Copy link
Member

frewsxcv commented Jan 5, 2016

components/net/fetch/request.rs, line 784 [r1] (raw file):
This could be:

if let Some(encoding) = response.headers.get::<ContentEncoding>() {
    ...
}

Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Jan 5, 2016

Nice work! With @frewsxcv's comments addressed, the first commit removed and the remaining ones squashed together, and the result rebased against master, I'd be happy to merge these changes.

@nikkisquared nikkisquared force-pushed the nikkisquared:http_network_fetch branch from 48eb508 to 1e8b840 Jan 5, 2016
@jdm
Copy link
Member

jdm commented Jan 5, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jan 5, 2016

📌 Commit 1e8b840 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jan 5, 2016

Testing commit 1e8b840 with merge 573995e...

bors-servo added a commit that referenced this pull request Jan 5, 2016
Partial implementation of HTTP-network Fetch step

I've been working on making a partial implementation of HTTP-network Fetch[1] so that I can begin testing the entire Fetch protocol as implemented in Servo. The empty steps are intentionally so, including the incomplete Step 5, which I started without realizing I don't need to do basic testing.

[1] https://fetch.spec.whatwg.org/#http-network-fetch

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9153)
<!-- Reviewable:end -->
@nikkisquared nikkisquared force-pushed the nikkisquared:http_network_fetch branch from 1e8b840 to af310f7 Jan 5, 2016
@jdm
Copy link
Member

jdm commented Jan 5, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jan 5, 2016

📌 Commit af310f7 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jan 5, 2016

Testing commit af310f7 with merge 1ea2341...

bors-servo added a commit that referenced this pull request Jan 5, 2016
Partial implementation of HTTP-network Fetch step

I've been working on making a partial implementation of HTTP-network Fetch[1] so that I can begin testing the entire Fetch protocol as implemented in Servo. The empty steps are intentionally so, including the incomplete Step 5, which I started without realizing I don't need to do basic testing.

[1] https://fetch.spec.whatwg.org/#http-network-fetch

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9153)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 5, 2016

@bors-servo bors-servo merged commit af310f7 into servo:master Jan 5, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nikkisquared nikkisquared deleted the nikkisquared:http_network_fetch branch Jan 5, 2016
@jdm jdm removed the S-awaiting-merge label May 8, 2017
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

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