Skip to content

Fix cef to follow new Browser::new() interface#17548

Merged
bors-servo merged 1 commit intoservo:masterfrom
sadmansk:cef-update
Jul 4, 2017
Merged

Fix cef to follow new Browser::new() interface#17548
bors-servo merged 1 commit intoservo:masterfrom
sadmansk:cef-update

Conversation

@sadmansk
Copy link
Copy Markdown
Contributor

@sadmansk sadmansk commented Jun 28, 2017 •
edited by larsbergstrom
Loading

Changes the call to Browser::new() in cef to contain the target_url.

cc @paulrouget

  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link
Copy Markdown

highfive commented Jun 28, 2017

warning Warning warning

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

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 28, 2017
@jdm jdm assigned paulrouget and unassigned jdm Jun 28, 2017
paulrouget
paulrouget suggested changes Jun 30, 2017
View reviewed changes
ports/cef/browser.rs
use std::rc::Rc;
use std::sync::atomic::{AtomicIsize, Ordering};
use std::thread;

Copy link
Copy Markdown
Contributor

@paulrouget paulrouget Jun 30, 2017

Choose a reason for hiding this comment

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

No need to remove this line.

Copy link
Copy Markdown
Contributor Author

@sadmansk sadmansk Jul 1, 2017

Choose a reason for hiding this comment

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

sorry, that was an oversight, i'll fix it

ports/cef/browser.rs Outdated
let target_url = match url_string {
Some(val) => ServoUrl::parse(&val[..]).unwrap_or(blank_url),
None => blank_url
};
Copy link
Copy Markdown
Contributor

@paulrouget paulrouget Jun 30, 2017

Choose a reason for hiding this comment

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

Could you do something like this:

    let target_url = url_string
        .and_then(|val| ServoUrl::parse(&val[..]).ok())
        .or(ServoUrl::parse("about:blank").ok())
        .unwrap();

Copy link
Copy Markdown
Contributor

@KiChjang KiChjang Jul 1, 2017

Choose a reason for hiding this comment

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

I think &val[..] here can be simplified to &val as well.

@sadmansk
Fix cef to follow new Browser::new() interface
185aa20 
(with passing of ServoUrl)

Review changes
@sadmansk
Copy link
Copy Markdown
Contributor Author

sadmansk commented Jul 1, 2017

@paulrouget fixed according to your review comments.

@sadmansk
Copy link
Copy Markdown
Contributor Author

sadmansk commented Jul 3, 2017

so ./mach test-unit is failing on Travis, but it's passing on my local machine, any idea why that could be the case?

@jdm
Copy link
Copy Markdown
Member

jdm commented Jul 3, 2017

I'm pretty sure the error while linking is caused by travis running out of disk space.

@sadmansk
Copy link
Copy Markdown
Contributor Author

sadmansk commented Jul 3, 2017

Can that error be ignored then? Or is there a fix that I can do?

@jdm
Copy link
Copy Markdown
Member

jdm commented Jul 3, 2017

It can be ignored.

@jdm
Copy link
Copy Markdown
Member

jdm commented Jul 3, 2017

@bors-servo: delegate=paul

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 3, 2017

✌️ @paul can now approve this pull request

@cbrewster
Copy link
Copy Markdown
Contributor

cbrewster commented Jul 3, 2017 •
edited
Loading

@bors-servo delegate=paulrouget

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 3, 2017

✌️ @paulrouget can now approve this pull request

@servo servo deleted a comment from bors-servo Jul 3, 2017
@paulrouget
Copy link
Copy Markdown
Contributor

paulrouget commented Jul 4, 2017

@bors-servo r+

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 4, 2017

📌 Commit 185aa20 has been approved by paulrouget

@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 Jul 4, 2017
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 4, 2017

⌛ Testing commit 185aa20 with merge b55b9dc...

bors-servo pushed a commit that referenced this pull request Jul 4, 2017
Auto merge of #17548 - sadmansk:cef-update, r=paulrouget
b55b9dc 
Fix cef to follow new Browser::new() interface

Changes the call to `Browser::new()` in cef to contain the target_url.

cc @paulrouget
<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #17273.

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/17548)
<!-- Reviewable:end -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jul 4, 2017

☀️ 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: paulrouget
Pushing b55b9dc to master...

@bors-servo bors-servo merged commit 185aa20 into servo:master Jul 4, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@KiChjang KiChjang KiChjang left review comments

@paulrouget paulrouget paulrouget approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@paulrouget paulrouget

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Browser::new() changes CEF follow up

7 participants

@sadmansk @highfive @jdm @bors-servo @cbrewster @paulrouget @KiChjang