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

Pass URL to Browser::new(), delegate url checking logic to 3rd party #16477

Merged
merged 1 commit into from Jun 12, 2017
+29 −23
Merged

Pass URL to Browser::new(), delegate url checking logic to 3rd party #16477

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

22 components/config/opts.rs
View file
Open in desktop
@@ -480,7 +480,7 @@ const DEFAULT_USER_AGENT: UserAgent = UserAgent::Desktop;
pub fn default_opts() -> Opts {
Opts {
is_running_problem_test: false,
url: Some(ServoUrl::parse("about:blank").unwrap()),
url: None,
tile_size: 512,
device_pixels_per_px: None,
time_profiling: None,
@@ -631,11 +631,10 @@ pub fn from_cmdline_args(args: &[String]) -> ArgumentParsingResult {
}

let cwd = env::current_dir().unwrap();
let homepage_pref = PREFS.get("shell.homepage");
let url_opt = if !opt_match.free.is_empty() {
Some(&opt_match.free[0][..])
} else {
homepage_pref.as_string()
None

This comment has been minimized.

Sign in to view
@sadmansk

sadmansk Apr 15, 2017

Author Contributor

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

This comment has been minimized.

Sign in to view
@paulrouget

paulrouget Apr 17, 2017

Contributor

None.

This comment has been minimized.

Sign in to view
@paulrouget

paulrouget Apr 17, 2017 •
edited

Contributor

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).

This comment has been minimized.

Sign in to view
@sadmansk

sadmansk Apr 25, 2017

Author Contributor

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

This comment has been minimized.

Sign in to view
@paulrouget

paulrouget Apr 25, 2017

Contributor

Assign None.

This comment has been minimized.

Sign in to view
@sadmansk

sadmansk Apr 25, 2017

Author Contributor

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,
    |                 ^^^^

This comment has been minimized.

Sign in to view
@paulrouget

paulrouget Apr 25, 2017 •
edited

Contributor

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).

This comment has been minimized.

Sign in to view
@sadmansk

sadmansk Apr 25, 2017

Author Contributor

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?

This comment has been minimized.

Sign in to view
@paulrouget

paulrouget Apr 25, 2017

Contributor

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:

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.

This comment has been minimized.

Sign in to view
@KiChjang

KiChjang Apr 25, 2017

Member

The map function for Option should work very well here.
};
let is_running_problem_test =
url_opt
@@ -645,16 +644,11 @@ pub fn from_cmdline_args(args: &[String]) -> ArgumentParsingResult {
url.starts_with("http://web-platform.test:8000/_mozilla/mozilla/canvas/") ||
url.starts_with("http://web-platform.test:8000/_mozilla/css/canvas_over_area.html"));

let url = match url_opt {
Some(url_string) => {
parse_url_or_filename(&cwd, url_string)
.unwrap_or_else(|()| args_fail("URL parsing failed"))
},
None => {
print_usage(app_name, &opts);
args_fail("servo asks that you provide a URL")
}
};
let url_opt = url_opt.and_then(|url_string| parse_url_or_filename(&cwd, url_string)
.or_else(|error| {
warn!("URL parsing failed ({:?}).", error);
Err(error)
}).ok());

let tile_size: usize = match opt_match.opt_str("s") {
Some(tile_size_str) => tile_size_str.parse()
@@ -775,7 +769,7 @@ pub fn from_cmdline_args(args: &[String]) -> ArgumentParsingResult {

let opts = Opts {
is_running_problem_test: is_running_problem_test,
url: Some(url),
url: url_opt,
tile_size: tile_size,
device_pixels_per_px: device_pixels_per_px,
time_profiling: time_profiling,
10 components/servo/lib.rs
View file
Open in desktop
@@ -122,7 +122,7 @@ pub struct Browser<Window: WindowMethods + 'static> {
}

impl<Window> Browser<Window> where Window: WindowMethods + 'static {
pub fn new(window: Rc<Window>) -> Browser<Window> {
pub fn new(window: Rc<Window>, target_url: ServoUrl) -> Browser<Window> {
// Global configuration options, parsed from the command line.
let opts = opts::get();

@@ -203,7 +203,7 @@ impl<Window> Browser<Window> where Window: WindowMethods + 'static {
// as the navigation context.
let (constellation_chan, sw_senders) = create_constellation(opts.user_agent.clone(),
opts.config_dir.clone(),
opts.url.clone(),
target_url,
compositor_proxy.clone_compositor_proxy(),
time_profiler_chan.clone(),
mem_profiler_chan.clone(),
@@ -287,7 +287,7 @@ fn create_compositor_channel(event_loop_waker: Box<compositor_thread::EventLoopW

fn create_constellation(user_agent: Cow<'static, str>,
config_dir: Option<PathBuf>,
url: Option<ServoUrl>,
url: ServoUrl,
compositor_proxy: CompositorProxy,
time_profiler_chan: time::ProfilerChan,
mem_profiler_chan: mem::ProfilerChan,
@@ -337,9 +337,7 @@ fn create_constellation(user_agent: Cow<'static, str>,
constellation_chan.send(ConstellationMsg::SetWebVRThread(webvr_thread)).unwrap();
}

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

This comment has been minimized.

Sign in to view
@sadmansk

sadmansk Apr 15, 2017

Author Contributor

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

This comment has been minimized.

Sign in to view
@paulrouget

paulrouget Apr 17, 2017

Contributor

It can't be None at this point.

// channels to communicate with Service Worker Manager
let sw_senders = SWManagerSenders {
10 ports/cef/browser.rs
View file
Open in desktop
@@ -9,6 +9,8 @@ use interfaces::{CefBrowser, CefBrowserHost, CefClient, CefFrame, CefRequestCont
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;

This comment has been minimized.

Sign in to view
@paulrouget

paulrouget May 11, 2017

Contributor

I don't know much about CEF, but I think you should not use PREFS here. Just use about:blank directly.
use servo::servo_url::ServoUrl;
use types::{cef_browser_settings_t, cef_string_t, cef_window_info_t, cef_window_handle_t};
use window;
use wrappers::CefWrap;
@@ -130,7 +132,9 @@ impl ServoCefBrowser {
let (glutin_window, servo_browser) = if window_info.windowless_rendering_enabled == 0 {
let parent_window = glutin_app::WindowID::new(window_info.parent_window as *mut _);
let glutin_window = glutin_app::create_window(Some(parent_window));
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);

This comment has been minimized.

Sign in to view
@paulrouget

paulrouget May 11, 2017

Contributor

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.

This comment has been minimized.

Sign in to view
@sadmansk

sadmansk May 20, 2017 •
edited

Author Contributor

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?

This comment has been minimized.

Sign in to view
@sadmansk

sadmansk Jun 2, 2017

Author Contributor

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

This comment has been minimized.

Sign in to view
@paulrouget

paulrouget Jun 5, 2017

Contributor

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)); }

window_handle = glutin_window.platform_window().window as cef_window_handle_t;
(Some(glutin_window), ServoBrowser::OnScreen(servo_browser))
} else {
@@ -171,7 +175,9 @@ impl ServoCefBrowserExtensions for CefBrowser {
if window_info.windowless_rendering_enabled != 0 {
let window = window::Window::new(window_info.width, window_info.height);
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();

This comment has been minimized.

Sign in to view
@paulrouget

paulrouget May 11, 2017

Contributor

assign about:blank directly.
let servo_browser = Browser::new(window.clone(), home_url);
*self.downcast().servo_browser.borrow_mut() = ServoBrowser::OffScreen(servo_browser);
}

10 ports/servo/main.rs
View file
Open in desktop
@@ -37,6 +37,8 @@ use servo::compositing::windowing::WindowEvent;
use servo::config;
use servo::config::opts::{self, ArgumentParsingResult};
use servo::config::servo_version;
use servo::servo_config::prefs::PREFS;
use servo::servo_url::ServoUrl;
use std::env;
use std::panic;
use std::process;
@@ -143,10 +145,16 @@ fn main() {

let window = app::create_window(None);

// If the url is not provided, we fallback to the homepage in PREFS,
// or a blank page in case the homepage is not set either.
let target_url = opts::get().url.clone()
.unwrap_or(ServoUrl::parse(PREFS.get("shell.homepage").as_string()
.unwrap_or("about:blank")).unwrap());

// Our wrapper around `Browser` that also implements some
// callbacks required by the glutin window implementation.
let mut browser = BrowserWrapper {
browser: Browser::new(window.clone()),
browser: Browser::new(window.clone(), target_url)
};

browser.browser.setup_logging();
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.