Pass URL to Browser::new(), delegate url checking logic to 3rd party by sadmansk · Pull Request #16477 · servo/servo

sadmansk · 2017-04-15T19:27:42Z

Move the logic of computing the initial url from opts.rs to /ports/servo/main.rs
Add a ServoUrl argument to Browser::new

Based on the requested changes by @paulrouget:

We can read the pref in main() instead. shell.homepage would be used if the url is not passed as an argument. I'm trying to decouple the "app" logic and the "web engine" logic. I think it's up to the app to set the initial URL, and I'm not sure the initial url should be part of opts.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Pass the initial URL as an argument to Browser::new, not as a opts property #15636

There are tests for these changes

This change is

highfive · 2017-04-15T19:27:48Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @cbrewster (or someone else) soon.

sadmansk · 2017-04-15T20:16:28Z

components/config/opts.rs

        Some(&opt_match.free[0][..])
    } else {
-        homepage_pref.as_string()
+        None


Flag: what should the url_opt be set to if it can't be parsed from the cmdline?

Also, Servo exits early if it can't find a URL. See https://github.com/sadmansk/servo/blob/95047114eeed77c4c9b310e6d4f04d3a3295a9e2/components/config/opts.rs#L678

This is not necessary anymore. Because the initial URL is now handled in main.rs, main.rs can do 2 things: either use about:blank or exit with the same error message (now sure which option is best).

How would you like me to handle the case if it is null? Should I print the usage or simply assign None?

Assign None.

Forgive my newbiness, but I'm having some problem with assigning None on a matching statement. I couldn't search up exactly why it does not work. I was wondering if you could give me a pointer for the syntax.

error[E0308]: match arms have incompatible types --> /home/sadman/repos/servo/components/config/opts.rs:673:15 | 673 | let url = match url_opt { | _______________^ starting here... 674 | | Some(url_string) => { 675 | | parse_url_or_filename(&cwd, url_string) 676 | | .unwrap_or_else(|()| args_fail("URL parsing failed")) 677 | | }, 678 | | None => None, 679 | | }; | |_____^ ...ending here: expected struct `servo_url::ServoUrl`, found enum `std::option::Option` | = note: expected type `servo_url::ServoUrl` found type `std::option::Option<_>` note: match arm with an incompatible type --> /home/sadman/repos/servo/components/config/opts.rs:678:17 | 678 | None => None, | ^^^^

Forgive my newbiness

I've been through this too ^^

but I'm having some problem with assigning None on a matching statement. I couldn't search up exactly why it does not work. I was wondering if you could give me a pointer for the syntax.

Sure. The issue here is that the variable url is a ServoUrl. So the first branch of the match does return a ServoUrl. But the None branch returns a Option<ServoUrl> (which is None in this case).

Before your PR, we had a url_opt (optional url: Option<ServoUrl>) that was transformed to url (ServoUrl). Because we didn't want url to be optional. Now we want. So we want to keep url optional.

So in this function, we don't want to unwrap the url (which mean moving from Option<X> to X), we want to keep it optional.

So this block of code your working on, was doing 2 things: 1) calling parse_url_or_filename to make sure it's a valid url and that a file path is transformed into a url, and 2) unwrap the url to enforce it (not make it optional).

Now we only want 1) to happen.

Let me know if you need more info. You can also ping me on irc (paul).

First of all, thanks so much for giving me the time!
I do understand what we now want, I'm just having a bit of trouble understanding how to do it. Like, for instance, do I need to completely replace the match block and try unwrapping url_opt and save the value in url_string, and then only call parse_url_or_file on url_string if it isn't equal to either None or empty string?

Sounds about right.

The match block should be replaced with something that would achieve this:

if url_opt is None, keep it None.
If url_opt is Some(foo), url_opt should become Some(parse_url_or_filename(foo).unwrap()), but if the result of parse_url_or_filename(foo) is Err(), we should call args_fail (which will exit).

Achieving this should be easy with some combinaison of unwrap and map calls (methods of Option and Result types).

parse_url_or_filename return a Result that is more or less like an Option.

Make sure to understand:

https://doc.rust-lang.org/std/option/

https://doc.rust-lang.org/std/result/

This should be one or two lines of code. I let you try to understand what this should look like. Manipulating options is a core concept of Rust. It's worth the time understanding it.

Do not hesitate to ask more questions.

The map function for Option should work very well here.

sadmansk · 2017-04-15T20:17:50Z

components/servo/lib.rs

-    if let Some(url) = url {
-        constellation_chan.send(ConstellationMsg::InitLoadUrl(url)).unwrap();
-    };
+    constellation_chan.send(ConstellationMsg::InitLoadUrl(url)).unwrap();


Flag: should we do a check here to see if url is None?

It can't be None at this point.

paulrouget · 2017-04-16T07:03:54Z

Thank you for taking care of this. I'll look at this PR early next week.

paulrouget · 2017-04-17T07:33:13Z

components/servo/lib.rs

-    if let Some(url) = url {
-        constellation_chan.send(ConstellationMsg::InitLoadUrl(url)).unwrap();
-    };
+    constellation_chan.send(ConstellationMsg::InitLoadUrl(url)).unwrap();


It can't be None at this point.

paulrouget · 2017-04-17T07:33:24Z

components/config/opts.rs

        Some(&opt_match.free[0][..])
    } else {
-        homepage_pref.as_string()
+        None


paulrouget · 2017-04-17T07:44:48Z

ports/servo/main.rs

-        browser: Browser::new(window.clone()),
+        browser: Browser::new(window.clone(), opts::get().url.clone()
+            .unwrap_or(ServoUrl::parse(PREFS.get("shell.homepage").as_string().unwrap())
+            .unwrap()))


opts::get().url default value should be None, not Some("about:blank") (see here).

If PREFS.get("shell.homepage") doesn't exist, we should use "about:blank" (or exist early).

So you always have a valid URL.

about:blank sounds like a good idea to me, I'll go with that.

paulrouget · 2017-04-17T07:48:59Z

components/config/opts.rs

        Some(&opt_match.free[0][..])
    } else {
-        homepage_pref.as_string()
+        None


Also, Servo exits early if it can't find a URL. See https://github.com/sadmansk/servo/blob/95047114eeed77c4c9b310e6d4f04d3a3295a9e2/components/config/opts.rs#L678

This is not necessary anymore. Because the initial URL is now handled in main.rs, main.rs can do 2 things: either use about:blank or exit with the same error message (now sure which option is best).

cbrewster · 2017-04-17T14:08:12Z

r? @paulrouget

cbrewster · 2017-04-17T14:47:24Z

@bors-servo delegate=paulrouget

bors-servo · 2017-04-17T14:47:25Z

✌️ @paulrouget can now approve this pull request

paulrouget · 2017-04-17T16:29:46Z

@sadmansk let me know if you have any question about my comments.

sadmansk · 2017-04-17T20:13:42Z

@paulrouget just give me a few days, I have exams this week, I'll address your comments as soon as that's done 😅

sadmansk · 2017-04-24T00:28:10Z

@paulrouget Made that change you requested. Please take another look and let me know if there was anything else you wanted me to change. Thanks.

KiChjang · 2017-04-24T04:10:27Z

ports/servo/main.rs

+        browser: Browser::new(window.clone(),
+                opts::get().url.clone()
+                    .unwrap_or(ServoUrl::parse(PREFS.get("shell.homepage").as_string()
+                    .unwrap_or("about:blank")).unwrap()))


This line should get at least one more level of indentation to show that the method isn't operating at the same level as the previous line.

paulrouget

opts.rs' from_cmdline_args method needs some more work. url_opt can be None, and this is valid now as we fallback to the preference or about:blank in main.rs. See

servo/components/config/opts.rs

Line 681 in 7338205

args_fail("servo asks that you provide a URL")

<- remove this

Also, the default url should be None I think:

servo/components/config/opts.rs

Line 506 in 7338205

url: Some(ServoUrl::parse("about:blank").unwrap()),

paulrouget · 2017-04-24T08:30:48Z

ports/servo/main.rs

+        browser: Browser::new(window.clone(),
+                opts::get().url.clone()
+                    .unwrap_or(ServoUrl::parse(PREFS.get("shell.homepage").as_string()
+                        .unwrap_or("about:blank")).unwrap()))


Can you split that in a way that it's easier to understand?
Maybe move the url resolution above, and add a comment to explain the logic.

sadmansk · 2017-05-07T07:40:59Z

@paulrouget I made the requested changes as I understood. Please let me know if I misunderstood anything. Also, if the parsing of the URL fails, i.e. parse_url_or_filename() returns an error, then I'm assigning None to url without any feedback. Should I print a message indicating that the parsing of the url failed?

bors-servo · 2017-05-07T23:19:19Z

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

paulrouget

Address my comments, squash and rebase, and I think that should do it.

Thank you!

paulrouget · 2017-05-08T14:11:16Z

components/config/opts.rs

+            match parse_url_or_filename(&cwd, url_string) {
+                Ok(v) => Some(v),
+                Err(_) => None,
+            }


You can transform an Result into an Option with parse_url_or_filename().ok().
See https://doc.rust-lang.org/std/result/enum.Result.html#method.ok

paulrouget · 2017-05-08T14:23:45Z

components/config/opts.rs

    let opts = Opts {
        is_running_problem_test: is_running_problem_test,
-        url: Some(url),
+        url: url,


Keep url_opt:

url: url_opt

paulrouget · 2017-05-08T14:45:45Z

components/config/opts.rs

-            args_fail("servo asks that you provide a URL")
-        }
+        None => None,
    };


I think you can combine all of this with and_then, something like this:

let url_opt = url_opt.and_then(|url_string| parse_url_or_filename(…).ok());

Also, rename url to url_opt (still an option at this point)

paulrouget · 2017-05-08T14:47:51Z

Also, if the parsing of the URL fails, i.e. parse_url_or_filename() returns an error, then I'm assigning None to url without any feedback. Should I print a message indicating that the parsing of the url failed?

Yes, maybe a warning.

paulrouget · 2017-05-10T06:40:07Z

Ok. It looks good now.

You also need to update ports/cef. You can build it with ./mach build-cef.

sadmansk · 2017-05-11T02:14:13Z

updated ports/cef. I had to set the home URL before making the call to the browser new. Is this okay?

paulrouget · 2017-05-11T05:23:27Z

ports/cef/browser.rs

-            let servo_browser = Browser::new(glutin_window.clone());
+            let home_url = ServoUrl::parse(PREFS.get("shell.homepage").as_string()
+                    .unwrap_or("about:blank")).unwrap();
+            let servo_browser = Browser::new(glutin_window.clone(), home_url);


If I'm not mistaken, ServoCefBrowser::new is called by browser_host_create, which comes with a url. So you should use it instead of use about:blank.

Sorry a bit confused, should I change the function signature of ServoCefBrowser::new so that it gets the url from browser_host_create and use that instead?

@paulrouget Hey, still waiting on this clarification. Please let me know at your earliest convenience

should I change the function signature of ServoCefBrowser::new so that it gets the url from browser_host_create and use that instead?

Yes.

And then I don't think it's necessary to call frame.set_url:

servo/ports/cef/browser.rs

Line 274 in d767e60

unsafe { browser.downcast().frame.set_url(CefWrap::to_rust(url)); }

paulrouget · 2017-05-11T05:23:48Z

ports/cef/browser.rs

 use interfaces::{cef_browser_t, cef_browser_host_t, cef_client_t, cef_frame_t};
 use interfaces::{cef_request_context_t};
 use servo::Browser;
+use servo::servo_config::prefs::PREFS;


I don't know much about CEF, but I think you should not use PREFS here. Just use about:blank directly.

paulrouget · 2017-05-11T05:27:56Z

ports/cef/browser.rs

            window.set_browser(self.clone());
-            let servo_browser = Browser::new(window.clone());
+            let home_url = ServoUrl::parse(PREFS.get("shell.homepage").as_string()
+                    .unwrap_or("about:blank")).unwrap();


assign about:blank directly.

paulrouget · 2017-05-18T08:27:54Z

@sadmansk anything I can do to help?

sadmansk · 2017-05-18T20:58:21Z

@paulrouget Sorry about that, it's been a really busy week at work, so didn't get a chance to work on it. I will get to it as soon as I catch a breath

paulrouget · 2017-05-19T06:13:46Z

No worries. Just wanted to make sure you were not stuck.

paulrouget · 2017-06-07T03:53:52Z

@sadmansk if you want, we can land this as it is, and add the url to ServoCefBrowser::new in another PR.

bors-servo · 2017-06-07T13:50:15Z

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

sadmansk · 2017-06-08T17:10:00Z

@paulrouget yeah ofcourse, that sounds like a good plan, i'll rebase tonight and ping you for the merge, and also start working on the cef browser stuff.

Use Option instead of ServoUrl for url in opts Cleaner mapping from parse_url to url_opt Add comment about url parsing error Print reason for parsing error Remove comment about warning Add home url when openning new browser window in CEF Fix merge error

sadmansk · 2017-06-09T03:06:10Z

@paulrouget merge is ready now, i'll work on the cef browser things on a different PR

paulrouget · 2017-06-12T04:16:04Z

@bors-servo r+

bors-servo · 2017-06-12T04:16:04Z

📌 Commit 708c561 has been approved by paulrouget

bors-servo · 2017-06-12T04:16:09Z

⌛ Testing commit 708c561 with merge 8714064...

@paulrouget

Pass URL to Browser::new(), delegate url checking logic to 3rd party  1. Move the logic of computing the initial url from `opts.rs` to `/ports/servo/main.rs` 2. Add a `ServoUrl` argument to `Browser::new` Based on the requested changes by @paulrouget: >We can read the pref in main() instead. shell.homepage would be used if the url is not passed as an argument. I'm trying to decouple the "app" logic and the "web engine" logic. I think it's up to the app to set the initial URL, and I'm not sure the initial url should be part of opts. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #15636  - [ ] There are tests for these changes    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16477)

paulrouget · 2017-06-12T04:18:47Z

CEF Follow up: #17273

bors-servo · 2017-06-12T05:16:24Z

☀️ 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 8714064 to master...

highfive assigned cbrewster Apr 15, 2017

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

sadmansk force-pushed the url_param_browser branch from 8cb16b0 to 9504711 Compare April 15, 2017 20:14

sadmansk commented Apr 15, 2017

View reviewed changes

sadmansk changed the title ~~WIP: Pass URL to Browser::new(), delegate url checking logic to 3rd party~~ Pass URL to Browser::new(), delegate url checking logic to 3rd party Apr 16, 2017

paulrouget reviewed Apr 17, 2017

View reviewed changes

highfive assigned paulrouget and unassigned cbrewster Apr 17, 2017

sadmansk closed this Apr 23, 2017

sadmansk force-pushed the url_param_browser branch from 9504711 to a26079d Compare April 23, 2017 23:23

sadmansk reopened this Apr 24, 2017

sadmansk force-pushed the url_param_browser branch from cb61240 to fdaa887 Compare April 24, 2017 00:27

KiChjang reviewed Apr 24, 2017

View reviewed changes

paulrouget reviewed Apr 24, 2017

View reviewed changes

sadmansk force-pushed the url_param_browser branch from 6c66110 to c867f1e Compare May 7, 2017 07:37

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

sadmansk force-pushed the url_param_browser branch from b5ccecb to 656bf8f Compare May 8, 2017 03:02

paulrouget suggested changes May 8, 2017

View reviewed changes

paulrouget suggested changes May 11, 2017

View reviewed changes

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

paulrouget mentioned this pull request Jun 8, 2017

Tabs support: add methods to create additional top level browsing context #17226

Closed

sadmansk force-pushed the url_param_browser branch from 3831b6a to 13b1815 Compare June 9, 2017 01:38

sadmansk force-pushed the url_param_browser branch from be1d68f to 708c561 Compare June 9, 2017 02:06

paulrouget approved these changes Jun 12, 2017

View reviewed changes

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. S-needs-rebase There are merge conflict errors. labels Jun 12, 2017

paulrouget mentioned this pull request Jun 12, 2017

Browser::new() changes CEF follow up #17273

Closed

bors-servo merged commit 708c561 into servo:master Jun 12, 2017

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

bors-servo mentioned this pull request Jun 12, 2017

Remove compositor forwarding code and use dedicated mechanism #17269

Merged

5 tasks

jdm mentioned this pull request Jun 13, 2017

Windows nightly fails on open #17290

Closed

Uh oh!

Conversation

sadmansk commented Apr 15, 2017 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Apr 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulrouget Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulrouget Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulrouget commented Apr 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulrouget Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbrewster commented Apr 17, 2017

Uh oh!

cbrewster commented Apr 17, 2017

Uh oh!

bors-servo commented Apr 17, 2017

Uh oh!

paulrouget commented Apr 17, 2017

Uh oh!

sadmansk commented Apr 17, 2017

Uh oh!

sadmansk commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulrouget left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadmansk commented May 7, 2017

Uh oh!

bors-servo commented May 7, 2017

Uh oh!

paulrouget left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadmansk commented Apr 15, 2017 •

edited by larsbergstrom

Loading

paulrouget Apr 17, 2017 •

edited

Loading

paulrouget Apr 25, 2017 •

edited

Loading

paulrouget Apr 17, 2017 •

edited

Loading

paulrouget left a comment •

edited

Loading

sadmansk May 20, 2017 •

edited

Loading