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.

Sign up for GitHub

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

Jump to bottom

Use hosts-replaced URL only when loading resources #6416

Merged
merged 1 commit into from
Aug 5, 2015
Merged

Use hosts-replaced URL only when loading resources #6416

merged 1 commit into from
Aug 5, 2015

Conversation

jgraham
Copy link
Contributor

@jgraham jgraham commented Jun 18, 2015

Review on Reviewable

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

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/5320

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jgraham
Copy link
Contributor Author

jgraham commented Jun 18, 2015

r? @Manishearth I'm not really sure that this is perfect; I couldn't decide which urls to put in error messages, and it generally feels a bit fragile.

jdm
jdm reviewed Jun 19, 2015
View reviewed changes
components/script/Cargo.toml
@@ -21,6 +21,9 @@ path = "../util"
[dependencies.msg]
path = "../msg"

[dependencies.net]
path = "../net"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! net_traits exists for a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh look, a @jdm!

You want the whole of the HOSTS_FILE stuff moved into net_traits then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Or make it polymorphic on a trait in net_traits. The former is okay.

@Manishearth
Copy link
Member

Mostly LGTM except for the dependency.

Also, there should be some comments about what the various _url variables mean.

@Manishearth Manishearth 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 Jun 19, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 20, 2015 

components/compositing/constellation.rs:602: (much) overlong line
components/compositing/constellation.rs:605: (much) overlong line

@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 Jun 22, 2015
waddlesplash
waddlesplash reviewed Jun 23, 2015
View reviewed changes
components/net_traits/Cargo.toml
@@ -24,4 +24,5 @@ log = "*"
url = "0.2.35"
hyper = "0.5"
euclid = "0.1"

regex = "0.1.33"
regex_macros = "0.1.19"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you got rid of the EOL at the end of the file?

@bors-servo
Copy link
Contributor

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

@jdm jdm added the S-needs-rebase There are merge conflict errors. label Jun 26, 2015
@jgraham jgraham force-pushed the hosts_replaced branch 2 times, most recently from 8cd704c to 15ca46f Compare June 29, 2015 18:19
@jgraham jgraham removed the S-needs-rebase There are merge conflict errors. label Jun 29, 2015
@jdm
Copy link
Member

jdm commented Jul 6, 2015

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 15ca46f has been approved by jdm

@jdm jdm 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 Jul 6, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 15ca46f with merge 00116d6...

bors-servo pushed a commit that referenced this pull request Jul 6, 2015
Auto merge of #6416 - jgraham:hosts_replaced, r=jdm
00116d6 
Use hosts-replaced URL only when loading resources



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

💔 Test failed - gonk

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 4, 2015
@jdm
Copy link
Member

jdm commented Aug 5, 2015

@bors-servo: r+
Makes sense.

@bors-servo
Copy link
Contributor

📌 Commit 49a1cfe has been approved by jdm

@jdm jdm 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 Aug 5, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 49a1cfe with merge aaa98d1...

bors-servo pushed a commit that referenced this pull request Aug 5, 2015
Auto merge of #6416 - jgraham:hosts_replaced, r=jdm
aaa98d1 
Use hosts-replaced URL only when loading resources



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

💔 Test failed - mac1

@jdm
Copy link
Member

jdm commented Aug 5, 2015 



/XMLHttpRequest/timeout-cors-async.htm
--------------------------------------
FAIL XMLHttpRequest: timeout event and cross-origin request

@jdm jdm 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 Aug 5, 2015
@jdm
Copy link
Member

jdm commented Aug 5, 2015

This PR really doesn't want to merge!

@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 Aug 5, 2015
@jgraham
Copy link
Contributor Author

jgraham commented Aug 5, 2015

Seems like there shouldn't have been any metadata updates at all. Which makes some sense as wptrunner hasn't been updated to use the right host name.

@jdm
Copy link
Member

jdm commented Aug 5, 2015

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit f52276d has been approved by jdm

@jdm jdm 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 Aug 5, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit f52276d with merge 8602d01...

bors-servo pushed a commit that referenced this pull request Aug 5, 2015
Auto merge of #6416 - jgraham:hosts_replaced, r=jdm
8602d01 
Use hosts-replaced URL only when loading resources



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

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3

@bors-servo bors-servo merged commit f52276d into servo:master Aug 5, 2015
This was referenced Aug 5, 2015
Closed
Closed
@mbrubeck mbrubeck mentioned this pull request Aug 5, 2015
Merged
bors-servo pushed a commit that referenced this pull request Aug 6, 2015
Auto merge of #6998 - mbrubeck:upgrade, r=Manishearth
e302474 
Update to unicode-bidi 0.2.1

Also includes some missing changes from #6416.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6998)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this pull request Aug 6, 2015
Auto merge of #6998 - mbrubeck:upgrade, r=Manishearth
d2ba81e 
Update to unicode-bidi 0.2.1

Also includes some missing changes from #6416.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6998)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Manishearth Manishearth

Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@jgraham @highfive @hoppipolla-critic-bot @Manishearth @Ms2ger @bors-servo @jdm @metajack @waddlesplash