Skip to content

/ servo

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

opts.rs: don't accept multiple URLs. Fixes #5520. #5527

Merged
merged 1 commit into from Apr 8, 2015
Merged

opts.rs: don't accept multiple URLs. Fixes #5520. #5527

merged 1 commit into from Apr 8, 2015

Conversation

@jagtalon
Copy link
Contributor

jagtalon commented Apr 4, 2015

Switched from opts.urls from being of type Vec to type String and changing the name to url as well. Changed the other files that are using opts.urls accordingly.
@highfive
Copy link

highfive commented Apr 4, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon.
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Apr 4, 2015

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

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.
@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 6, 2015

Looks great, thanks! I noted one minor issue at the Critic link above. After that's addressed, this can land with r=mbrubeck.
@mbrubeck mbrubeck self-assigned this Apr 6, 2015
@jagtalon
Copy link
Contributor Author

jagtalon commented Apr 6, 2015

@mbrubeck done! Hope it's okay.
@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 6, 2015

Thanks! Please squash these commits together and then we will land them.
@jagtalon
Copy link
Contributor Author

jagtalon commented Apr 6, 2015

@mbrubeck squashed!
@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 6, 2015

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Apr 6, 2015

📌 Commit 3c3f067 has been approved by mbrubeck
@jdm jdm added S-awaiting-merge and removed S-needs-squash labels Apr 6, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Apr 7, 2015

⌛ Testing commit 3c3f067 with merge 1161d83...
bors-servo pushed a commit that referenced this pull request Apr 7, 2015
bors-servo
Auto merge of #5527 - jagtalon:jag/multiple-urls, r=mbrubeck
1161d83 
Switched from opts.urls from being of type Vec to type String and changing the name to `url` as well. Changed the other files that are using opts.urls accordingly.
@bors-servo
Copy link
Contributor

bors-servo commented Apr 7, 2015

💔 Test failed - linux2
@Manishearth
Copy link
Member

Manishearth commented Apr 7, 2015 

   Compiling embedding v0.0.1 (file:///home/servo/buildbot/slave/linux2/build/ports/cef)
browser.rs:19:5: 19:25 warning: unused import, #[warn(unused_imports)] on by default
browser.rs:19 use std::borrow::ToOwned;
                  ^~~~~~~~~~~~~~~~~~~~
browser.rs:207:9: 207:16 warning: variable does not need to be mutable, #[warn(unused_mut)] on by default
browser.rs:207     let mut url = "http://s27.postimg.org/vqbtrolyr/servo.jpg".to_string();
                       ^~~~~~~
browser.rs:207:19: 207:75 error: str.to_owned() is more efficient than str.to_string(), please use it instead, #[deny(str_to_string)] on by default
browser.rs:207     let mut url = "http://s27.postimg.org/vqbtrolyr/servo.jpg".to_string();
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
core.rs:73:21: 73:41 error: str.to_owned() is more efficient than str.to_string(), please use it instead, #[deny(str_to_string)] on by default
core.rs:73     temp_opts.url = HOME_URL.to_string();
                               ^~~~~~~~~~~~~~~~~~~~
error: aborting due to 2 previous errors
Could not compile `embedding`.
@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 7, 2015

Use ./mach build-cef to reproduce that build
@jagtalon
Copy link
Contributor Author

jagtalon commented Apr 7, 2015

Thanks will make the fix.
@jagtalon
opts.rs: don't accept multiple URLs. Fixes #5520.
Loading status checks…
74cd4cd 
Switched from opts.urls from being of type Vec to type String and changing the name to `url` as well. Changed the other files that are using opts.urls accordingly.

servo/lib.rs + gonk/src/lib.rs: no need for a block scope.

cef: fix compiler errors.

- remove the use of `mut` since it's not needed.
- use `to_owned` instead of `to_string` because it's more efficient.
@jagtalon
Copy link
Contributor Author

jagtalon commented Apr 7, 2015

@mbrubeck fixed issue and rebased. Should be good!
@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 7, 2015

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Apr 7, 2015

📌 Commit 74cd4cd has been approved by mbrubeck
@bors-servo
Copy link
Contributor

bors-servo commented Apr 7, 2015

⌛ Testing commit 74cd4cd with merge c642632...
bors-servo pushed a commit that referenced this pull request Apr 7, 2015
bors-servo
Auto merge of #5527 - jagtalon:jag/multiple-urls, r=mbrubeck
c642632 
Switched from opts.urls from being of type Vec to type String and changing the name to `url` as well. Changed the other files that are using opts.urls accordingly.
@bors-servo
Copy link
Contributor

bors-servo commented Apr 7, 2015

💔 Test failed - mac1
@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 7, 2015

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Apr 8, 2015

⌛ Testing commit 74cd4cd with merge df57af1...
bors-servo pushed a commit that referenced this pull request Apr 8, 2015
bors-servo
Auto merge of #5527 - jagtalon:jag/multiple-urls, r=mbrubeck
Loading status checks…
df57af1 
Switched from opts.urls from being of type Vec to type String and changing the name to `url` as well. Changed the other files that are using opts.urls accordingly.
@bors-servo
Copy link
Contributor

bors-servo commented Apr 8, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2
This was referenced Apr 8, 2015
Merged
Merged
@bors-servo bors-servo merged commit 74cd4cd into servo:master Apr 8, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@jagtalon jagtalon deleted the jagtalon:jag/multiple-urls branch Apr 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mbrubeck mbrubeck

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@jagtalon @highfive @hoppipolla-critic-bot @mbrubeck @bors-servo @Manishearth @Ms2ger @jdm
You can’t perform that action at this time.