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

[WIP] Move asynchronous network loads to the fetch stack and remove unused code. #13714

Closed
wants to merge 10 commits into from

Conversation

Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Oct 12, 2016

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/document.rs, components/net/resource_thread.rs, components/script/network_listener.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/dom/servoparser/mod.rs, components/script/document_loader.rs, components/script/script_thread.rs, components/net/image_cache_thread.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 12, 2016
@highfive
Copy link

warning Warning warning

  • These commits modify net, gfx, and script code, but no tests are modified. Please consider adding a test!

@Manishearth
Copy link
Member

I need spec links on each of the RequestInit ctors.


Reviewed 2 of 2 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, 2 of 2 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 2 of 2 files at r9, 1 of 1 files at r10, 1 of 1 files at r11, 1 of 1 files at r12, 2 of 2 files at r13.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


components/net/image_cache_thread.rs, line 536 at r8 (raw file):

                                FetchResponseMsg::ProcessRequestEOF => return,
                                FetchResponseMsg::ProcessResponse(meta_result) => {
                                    ResponseAction::HeadersAvailable(meta_result.map(|m| {

Do we need ResponseAction anymore? We can just proxy the FetchResponseMsg through


components/net/image_cache_thread.rs, line 228 at r13 (raw file):

}

/// Data for passing between threads/processes to indicate a particular action to

If we're not removing this, make this comment reflect what specifically this is used for here


components/script/dom/servoparser/mod.rs, line 345 at r3 (raw file):

            Ok(meta) => {
                Some(match meta {
                    FetchMetadata::Unfiltered(m) => m,

might be useful as a convenience method, FetchMetadata.unwrap_ufiltered() or something


Comments from Reviewable

@Ms2ger Ms2ger force-pushed the more-fetch branch 2 times, most recently from ac2bd28 to 8fbf382 Compare October 12, 2016 10:47
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Oct 12, 2016

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

📌 Commit 8fbf382 has been approved by Manishearth

@highfive highfive assigned Manishearth and unassigned mbrubeck Oct 12, 2016
@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. labels Oct 12, 2016
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Oct 12, 2016

@bors-servo r-

Apparently not

@Manishearth
Copy link
Member

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 8fbf382 has been approved by Manishearth

@Ms2ger
Copy link
Contributor Author

Ms2ger commented Oct 12, 2016

@bors-servo p=1

@bors-servo
Copy link
Contributor

⌛ Testing commit 8fbf382 with merge 7c295d7...

bors-servo pushed a commit that referenced this pull request Oct 12, 2016
Move asynchronous network loads to the fetch stack and remove unused code.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13714)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 12, 2016
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Oct 12, 2016

Looks like I'll need to support blob URLs. I'll check tomorrow if I can land some parts of this PR independently.

@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Oct 18, 2016
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 20, 2016
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Oct 20, 2016

@bors-servo r+

@Ms2ger Ms2ger changed the title Move asynchronous network loads to the fetch stack and remove unused code. [WIP] Move asynchronous network loads to the fetch stack and remove unused code. Oct 21, 2016
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Oct 24, 2016

Currently, this is hitting a timeout in tests/wpt/mozilla/tests/mozilla/mime_sniffing_font_context.html; if I understand correctly, that's because there is sniffing code in the old networking stack that isn't in the fetch stack. I'd argue it would be better to break the test for now, for two reasons in particular. First, it really isn't the networking stack's job to sniff bytes; the consumer should ignore the metadata rather than expecting the networking layer to mess with it. Second, I'd generally prefer not to add too much code to the fetch stack before the old stack is gone, and we've been able to simplify the fetch code a little.

I'll leave it to @jdm to decide whether that's a good idea.

@KiChjang
Copy link
Contributor

Perhaps it makes sense to open a tracking issue for this?

@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Nov 2, 2016
@jdm jdm added S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 2, 2016
@jdm
Copy link
Member

jdm commented Nov 2, 2016

I reviewed these commits by accident in #13961.

bors-servo pushed a commit that referenced this pull request Nov 2, 2016
Move remaining users of the legacy networking stack to fetch.

Fixes #13931.
Fixes #13714.
@bors-servo
Copy link
Contributor

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

@Ms2ger Ms2ger deleted the more-fetch branch November 4, 2016 11:11
@Ms2ger Ms2ger mentioned this pull request Nov 7, 2016
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. S-needs-rebase There are merge conflict errors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants