Use NetworkConnector directly to account for replaced hosts #16131

nox · 2017-03-25T14:17:24Z

This change is

highfive · 2017-03-25T14:17:27Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/url.rs, components/script/dom/htmlformelement.rs, components/script/dom/urlhelper.rs
@KiChjang: components/net/fetch/methods.rs, components/net/http_loader.rs, components/script/dom/url.rs, components/net/websocket_loader.rs, components/script/dom/htmlformelement.rs, components/script/dom/urlhelper.rs, components/net_traits/hosts.rs, components/net_traits/hosts.rs

nox · 2017-03-25T14:17:53Z

@bors-servo try

bors-servo · 2017-03-25T14:17:57Z

⌛ Trying commit 8df5669 with merge cd6bc6f...


        Auto merge of #16131 - nox:tungstenite, r=<try>

Use NetworkConnector directly to account for replaced hosts

bors-servo · 2017-03-25T14:41:14Z

💔 Test failed - mac-rel-wpt1

nox · 2017-03-25T15:54:16Z

@bors-servo try

bors-servo · 2017-03-25T15:54:20Z

⌛ Trying commit af19004 with merge 152d6ce...


        Auto merge of #16131 - nox:tungstenite, r=<try>

Use NetworkConnector directly to account for replaced hosts  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16131)

bors-servo · 2017-03-25T16:24:24Z

💔 Test failed - linux-rel-wpt

nox · 2017-03-26T12:19:09Z

The failure is #14323.

nox · 2017-03-26T12:19:20Z

r? @jdm

jdm

This is a good change. I only have one comment about the changes in 22e4c9c.

jdm · 2017-03-26T13:12:03Z

components/url/lib.rs

@@ -49,8 +49,8 @@ impl ServoUrl {

    // NOTE: These methods return options that are always true temporarily until
    // we special-case some urls to avoid going through rust-url.


This comment should go away if we're making this change. I don't understand it though; any comments, @emilio?

@jdm This was from a time where we thought ServoUrl wouldn't always contain an Url, will remove it.


        Rework replace_hosts and host_replacement

They do less cloning now.


        Remove some useless Option<T> wrappers from ServoUrl methods


        Use NetworkConnector directly to account for replaced hosts

This let us remove a hack where we had to replace the host in the request URL.


        Use Url::port instead of Url::port_or_known_default to set Host

AFAIK, if there is no explicit port in the request URL, there should be no explicit port in the Host header.


        Put websocket_loader::init first in its module

nox · 2017-03-26T14:16:21Z

@jdm r=you?

jdm · 2017-03-26T14:46:12Z

@bors-servo: r+

bors-servo · 2017-03-26T14:46:13Z

📌 Commit 8796a82 has been approved by jdm

bors-servo · 2017-03-26T14:46:17Z

⌛ Testing commit 8796a82 with merge 1154bf5...


        Auto merge of #16131 - nox:tungstenite, r=jdm

Use NetworkConnector directly to account for replaced hosts  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16131)

Comment addressed.

bors-servo · 2017-03-26T15:24:48Z

💔 Test failed - linux-rel-css

nox · 2017-03-26T16:21:00Z

@bors-servo retry #14237

bors-servo · 2017-03-26T17:17:18Z

⌛ Testing commit 8796a82 with merge 1105100...


        Auto merge of #16131 - nox:tungstenite, r=jdm

Use NetworkConnector directly to account for replaced hosts  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16131)

bors-servo · 2017-03-26T17:56:51Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: jdm
Pushing 1105100 to master...

highfive assigned glennw Mar 25, 2017

highfive added the S-awaiting-review label Mar 25, 2017

bors-servo added a commit that referenced this pull request Mar 25, 2017

Auto merge of #16131 - nox:tungstenite, r=<try>

Loading status checks…

cd6bc6f

Use NetworkConnector directly to account for replaced hosts

highfive added the S-tests-failed label Mar 25, 2017

nox force-pushed the nox:tungstenite branch from 8df5669 to af19004 Mar 25, 2017

highfive removed the S-tests-failed label Mar 25, 2017

highfive added the S-tests-failed label Mar 25, 2017

nox removed the S-tests-failed label Mar 26, 2017

highfive assigned jdm and unassigned glennw Mar 26, 2017

jdm reviewed Mar 26, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Mar 26, 2017

nox added 5 commits Mar 25, 2017

Rework replace_hosts and host_replacement

bc5c4fa

They do less cloning now.

Remove some useless Option<T> wrappers from ServoUrl methods

54d37d9

Use NetworkConnector directly to account for replaced hosts

92c4a43

This let us remove a hack where we had to replace the host in the request URL.

Use Url::port instead of Url::port_or_known_default to set Host

b096bf4

AFAIK, if there is no explicit port in the request URL, there should be no explicit port in the Host header.

Put websocket_loader::init first in its module

Loading status checks…

8796a82

nox force-pushed the nox:tungstenite branch from af19004 to 54103b7 Mar 26, 2017

highfive removed the S-needs-code-changes label Mar 26, 2017

highfive added the S-awaiting-review label Mar 26, 2017

nox force-pushed the nox:tungstenite branch from 54103b7 to 8796a82 Mar 26, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 26, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 26, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Mar 26, 2017

bors-servo merged commit 8796a82 into servo:master Mar 26, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Mar 26, 2017

servo / servo

Use NetworkConnector directly to account for replaced hosts #16131

Use NetworkConnector directly to account for replaced hosts #16131

nox commented Mar 25, 2017 •

edited by larsbergstrom

highfive commented Mar 25, 2017

nox commented Mar 25, 2017

bors-servo commented Mar 25, 2017

bors-servo commented Mar 25, 2017

nox commented Mar 25, 2017

bors-servo commented Mar 25, 2017

bors-servo commented Mar 25, 2017

nox commented Mar 26, 2017

nox commented Mar 26, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

nox commented Mar 26, 2017

jdm commented Mar 26, 2017

bors-servo commented Mar 26, 2017

bors-servo commented Mar 26, 2017

bors-servo commented Mar 26, 2017

nox commented Mar 26, 2017

bors-servo commented Mar 26, 2017

bors-servo commented Mar 26, 2017

		@@ -49,8 +49,8 @@ impl ServoUrl {

		// NOTE: These methods return options that are always true temporarily until
		// we special-case some urls to avoid going through rust-url.

servo / servo

Join GitHub today

Use NetworkConnector directly to account for replaced hosts #16131

Use NetworkConnector directly to account for replaced hosts #16131

Conversation

nox commented Mar 25, 2017 • edited by larsbergstrom

highfive commented Mar 25, 2017

nox commented Mar 25, 2017

bors-servo commented Mar 25, 2017

bors-servo commented Mar 25, 2017

nox commented Mar 25, 2017

bors-servo commented Mar 25, 2017

bors-servo commented Mar 25, 2017

nox commented Mar 26, 2017

nox commented Mar 26, 2017

jdm left a comment

This comment has been minimized.

jdm Mar 26, 2017

This comment has been minimized.

nox Mar 26, 2017

nox commented Mar 26, 2017

jdm commented Mar 26, 2017

bors-servo commented Mar 26, 2017

bors-servo commented Mar 26, 2017

bors-servo commented Mar 26, 2017

nox commented Mar 26, 2017

bors-servo commented Mar 26, 2017

bors-servo commented Mar 26, 2017

nox commented Mar 25, 2017 •

edited by larsbergstrom