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

Initial fetch refactor #7275

Merged
merged 1 commit into from
Sep 28, 2015
Merged

Initial fetch refactor #7275

merged 1 commit into from
Sep 28, 2015

Conversation

KiChjang
Copy link
Contributor

Partial #4576
I am really unsure of how things would look like, so would really appreciate some feedback on this.

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 18, 2015
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #7265) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 20, 2015
@jdm jdm self-assigned this Aug 25, 2015
@jdm
Copy link
Member

jdm commented Aug 25, 2015

This is a good start! Having looked further at the fetch spec and the existing code, I propose the following:

  • we should only require the FetchContext value when we go to the network in http_network_or_cache_fetch (which obviously is not actually implemented yet).
  • spawn a thread in fetch_async, call fetch from it, and invoke listener.response_available with the Response return value

fetch_async now runs asynchronously from the point of view of the caller, and the listener will need to be implemented such that it sends the Response value that is passed over a channel back to the thread that invoked fetch_async (just like http://mxr.mozilla.org/servo/source/components/script/network_listener.rs#16)

Does that make sense?

@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 Aug 25, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Sep 21, 2015
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #7710) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 24, 2015
@jdm
Copy link
Member

jdm commented Sep 25, 2015

Looking better!
-S-awaiting-review +S-needs-code-changes


Reviewed 3 of 3 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


components/net/fetch/request.rs, line 152 [r2] (raw file):
If we make this method take self by value, we shouldn't need this clone.


components/net/fetch/request.rs, line 153 [r2] (raw file):
format!("fech for {:?}", self.url.serialize())


components/net_traits/lib.rs, line 167 [r1] (raw file):
I'd go with:

/// Interface for observing the final response for an asynchronous fetch operation.

Comments from the review on Reviewable.io

@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 Sep 25, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Sep 27, 2015
@jdm
Copy link
Member

jdm commented Sep 28, 2015

Great! Let's squash all these commits and then merge them :)
-S-needs-rebase -S-awaiting-review +S-needs-squash


Reviewed 3 of 3 files at r3, 2 of 2 files at r4, 2 of 2 files at r5.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@jdm jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Sep 28, 2015
@KiChjang
Copy link
Contributor Author

Just one sec, currently it doesn't compile and it generates this error:

servo/components/net/fetch/request.rs:153:9: 153:20 error: the trait `core::marker::Send` is not implemented for the type `fetch::cors_cache::CORSCache + 'static` [E0277]
servo/components/net/fetch/request.rs:153         spawn_named(format!("fetch for {:?}", self.url.serialize()), move || {

and goes on to say that fetch::cors_cache::CORSCache + 'static' cannot be sent between threads safely. Not sure what the best way to fix this.

@jdm
Copy link
Member

jdm commented Sep 28, 2015

https://github.com/servo/servo/pull/7275/files#diff-4039fbc0ced622014645f0d0d9c4e443R117 will need to be Box<CORSCache + Send> instead, I believe.

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 28, 2015
@KiChjang
Copy link
Contributor Author

Squashed and fixed build errors. Little guy should be ready to merge, apologies for taking so long to finish this simple task.

@jdm
Copy link
Member

jdm commented Sep 28, 2015

@bors-servo: r+
No worries! Thanks for doing this!

@bors-servo
Copy link
Contributor

📌 Commit ecf02a3 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Sep 28, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit ecf02a3 with merge 0fc9410...

bors-servo pushed a commit that referenced this pull request Sep 28, 2015
Initial fetch refactor

Partial #4576
I am really unsure of how things would look like, so would really appreciate some feedback on this.

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

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit ecf02a3 into servo:master Sep 28, 2015
@KiChjang KiChjang deleted the fetch-refactor branch September 28, 2015 20:35
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.

4 participants