Merge functionality of WebsocketConnect into Fetch #18871

KiChjang · 2017-10-14T02:30:15Z

Partial #14897.

This change is

highfive · 2017-10-14T02:30:19Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/network_listener.rs
@paulrouget: components/constellation/network_listener.rs
@fitzgen: components/script/dom/request.rs, components/script/dom/websocket.rs, components/script/fetch.rs, components/script/dom/xmlhttprequest.rs, components/script/document_loader.rs and 1 more
@KiChjang: components/script/dom/request.rs, components/net/resource_thread.rs, components/script/dom/websocket.rs, components/script/fetch.rs, components/script/dom/xmlhttprequest.rs and 7 more

highfive · 2017-10-14T02:30:20Z

Warning

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

KiChjang · 2017-10-14T02:33:06Z

@bors-servo try

bors-servo · 2017-10-14T02:33:11Z

⌛ Trying commit 644e34b with merge b4de070...

Merge functionality of WebsocketConnect into Fetch Partial #14897.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18871)

bors-servo · 2017-10-14T04:31:29Z

💔 Test failed - mac-rel-wpt2

KiChjang · 2017-10-14T18:30:48Z

@bors-servo retry

bors-servo · 2017-10-14T18:30:53Z

⌛ Trying commit 644e34b with merge 9aadce6...

Merge functionality of WebsocketConnect into Fetch Partial #14897.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18871)

bors-servo · 2017-10-15T04:31:22Z

💥 Test timed out

jdm · 2017-10-15T12:26:25Z

@bors-servo: retry

bors-servo · 2017-10-15T12:26:30Z

⌛ Trying commit 644e34b with merge c0430a1...

Merge functionality of WebsocketConnect into Fetch Partial #14897.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18871)

bors-servo · 2017-10-15T12:49:01Z

💔 Test failed - linux-dev

jdm · 2017-10-15T13:41:17Z

Everything else passed.

bors-servo · 2017-10-16T17:55:30Z

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

KiChjang · 2017-10-19T18:23:40Z

@bors-servo try

bors-servo · 2017-10-19T18:23:45Z

⌛ Trying commit 252a6d4 with merge f3b6e06...

Merge functionality of WebsocketConnect into Fetch Partial #14897.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18871)

bors-servo · 2017-10-19T19:21:41Z

💔 Test failed - arm32

KiChjang · 2017-10-19T20:00:42Z

I have no idea why it always fails on the manifest_update.sh script, but other than that, r? @jdm

jdm · 2017-10-24T12:26:32Z

Also redirecting this. r? @avadacatavra

avadacatavra · 2017-10-24T22:46:19Z

components/net_traits/lib.rs

-    pub resource_url: ServoUrl,
-    pub origin: String,
-    pub protocols: Vec<String>,
+pub enum FetchChannels {


can you comment these to make it more clear what they are

avadacatavra · 2017-10-24T22:49:57Z

components/net_traits/request.rs

 pub enum RequestMode {
    Navigate,
    SameOrigin,
    NoCors,
    CorsMode,
-    WebSocket
+    WebSocket(Vec<String>)


can you make it more clear that this vec is the protocols?

avadacatavra · 2017-10-24T22:50:47Z

components/script/dom/request.rs

@@ -556,7 +556,7 @@ impl RequestMethods for Request {

    // https://fetch.spec.whatwg.org/#dom-request-mode
    fn Mode(&self) -> RequestMode {
-        self.request.borrow().mode.into()
+        self.request.borrow().mode.clone().into()


why is the clone needed?

RequestMode used to implement Copy, but since I've added the WebSocket(Vec<String>) variant, it cannot implement Copy, hence this clone.

KiChjang · 2017-10-24T23:39:30Z

Comments addressed.

bors-servo · 2017-10-24T23:41:36Z

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

jdm · 2017-10-25T02:30:45Z

error[E0432]: unresolved import `net_traits::request::Type`

--> /home/travis/build/servo/servo/components/net/websocket_loader.rs:21:66

|

21 | use net_traits::request::{Destination, RequestInit, RequestMode, Type};

| ^^^^ no `Type` in `request`

KiChjang · 2017-10-25T03:08:12Z

Comments addressed for real this time.

avadacatavra · 2017-10-25T13:28:18Z

@bors-servo r+

bors-servo · 2017-10-25T13:28:19Z

📌 Commit 99f9696 has been approved by avadacatavra

bors-servo · 2017-10-25T14:00:43Z

⌛ Testing commit 99f9696 with merge a69e333...

…atavra Merge functionality of WebsocketConnect into Fetch Partial #14897.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18871)

bors-servo · 2017-10-25T15:28:14Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: avadacatavra
Pushing a69e333 to master...

highfive assigned mbrubeck Oct 14, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 14, 2017

KiChjang force-pushed the websocket-fetch-integration branch from edc7039 to 644e34b Compare October 14, 2017 02:32

highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 14, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 15, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Oct 16, 2017

KiChjang force-pushed the websocket-fetch-integration branch from 644e34b to 252a6d4 Compare October 19, 2017 18:22

highfive removed the S-tests-failed The changes caused existing tests to fail. label Oct 19, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 19, 2017

highfive assigned avadacatavra and unassigned jdm Oct 24, 2017

avadacatavra reviewed Oct 24, 2017

View reviewed changes

KiChjang force-pushed the websocket-fetch-integration branch 3 times, most recently from 0052896 to 0874f2b Compare October 24, 2017 23:02

highfive added the S-needs-rebase There are merge conflict errors. label Oct 24, 2017

KiChjang force-pushed the websocket-fetch-integration branch from 0874f2b to 46817ca Compare October 25, 2017 00:58

jdm removed the S-needs-rebase There are merge conflict errors. label Oct 25, 2017

KiChjang force-pushed the websocket-fetch-integration branch 4 times, most recently from 25b6a72 to 559769a Compare October 25, 2017 03:06

Merge functionality of WebsocketConnect into Fetch

99f9696

KiChjang force-pushed the websocket-fetch-integration branch from 559769a to 99f9696 Compare October 25, 2017 03:54

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 25, 2017

bors-servo merged commit 99f9696 into servo:master Oct 25, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 25, 2017

KiChjang deleted the websocket-fetch-integration branch March 31, 2018 05:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge functionality of WebsocketConnect into Fetch #18871

Merge functionality of WebsocketConnect into Fetch #18871

KiChjang commented Oct 14, 2017 •

edited by SimonSapin

Loading

highfive commented Oct 14, 2017

highfive commented Oct 14, 2017

KiChjang commented Oct 14, 2017

bors-servo commented Oct 14, 2017

bors-servo commented Oct 14, 2017

KiChjang commented Oct 14, 2017

bors-servo commented Oct 14, 2017

bors-servo commented Oct 15, 2017

jdm commented Oct 15, 2017

bors-servo commented Oct 15, 2017

bors-servo commented Oct 15, 2017

jdm commented Oct 15, 2017

bors-servo commented Oct 16, 2017

KiChjang commented Oct 19, 2017

bors-servo commented Oct 19, 2017

bors-servo commented Oct 19, 2017

KiChjang commented Oct 19, 2017

jdm commented Oct 24, 2017

avadacatavra Oct 24, 2017

avadacatavra Oct 24, 2017

avadacatavra Oct 24, 2017

KiChjang Oct 24, 2017

KiChjang commented Oct 24, 2017

bors-servo commented Oct 24, 2017

jdm commented Oct 25, 2017

KiChjang commented Oct 25, 2017

avadacatavra commented Oct 25, 2017

bors-servo commented Oct 25, 2017

bors-servo commented Oct 25, 2017

bors-servo commented Oct 25, 2017

Merge functionality of WebsocketConnect into Fetch #18871

Merge functionality of WebsocketConnect into Fetch #18871

Conversation

KiChjang commented Oct 14, 2017 • edited by SimonSapin Loading

highfive commented Oct 14, 2017

highfive commented Oct 14, 2017

KiChjang commented Oct 14, 2017

bors-servo commented Oct 14, 2017

bors-servo commented Oct 14, 2017

KiChjang commented Oct 14, 2017

bors-servo commented Oct 14, 2017

bors-servo commented Oct 15, 2017

jdm commented Oct 15, 2017

bors-servo commented Oct 15, 2017

bors-servo commented Oct 15, 2017

jdm commented Oct 15, 2017

bors-servo commented Oct 16, 2017

KiChjang commented Oct 19, 2017

bors-servo commented Oct 19, 2017

bors-servo commented Oct 19, 2017

KiChjang commented Oct 19, 2017

jdm commented Oct 24, 2017

avadacatavra Oct 24, 2017

Choose a reason for hiding this comment

avadacatavra Oct 24, 2017

Choose a reason for hiding this comment

avadacatavra Oct 24, 2017

Choose a reason for hiding this comment

KiChjang Oct 24, 2017

Choose a reason for hiding this comment

KiChjang commented Oct 24, 2017

bors-servo commented Oct 24, 2017

jdm commented Oct 25, 2017

KiChjang commented Oct 25, 2017

avadacatavra commented Oct 25, 2017

bors-servo commented Oct 25, 2017

bors-servo commented Oct 25, 2017

bors-servo commented Oct 25, 2017

KiChjang commented Oct 14, 2017 •

edited by SimonSapin

Loading