Auto merge of #5489 - larsbergstrom:brson_cleanup, r=metajack,ms2ger,jdm,manish

All of the commits by brson have been reviewed. Just the android fixups and Rustup one (which also cleans up some of the duplication due to a complete rewrite of code that had been moved in the original PR).

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

components/servo/Cargo.lock

@@ -6,6 +6,7 @@ dependencies = [
  "bitflags 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "compositing 0.0.1",
  "devtools 0.0.1",
+ "devtools_traits 0.0.1",
  "gfx 0.0.1",
  "glutin_app 0.0.1",
  "layout 0.0.1",

components/servo/Cargo.toml

@@ -78,6 +78,9 @@ path = "../devtools"
 [dependencies.webdriver_server]
 path = "../webdriver_server"
 
+[dependencies.devtools_traits]
+path = "../devtools_traits"
+
 [dependencies.glutin_app]
 path = "../../ports/glutin"
 optional = true

components/servo/lib.rs

@@ -2,14 +2,26 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+// Servo, the mighty web browser engine from the future.
+//
+// This is a very simple library that wires all of Servo's components
+// together as type `Browser`, along with a generic client
+// implementing the `WindowMethods` trait, to create a working web
+// browser.
+//
+// The `Browser` type is responsible for configuring a
+// `Constellation`, which does the heavy lifting of coordinating all
+// of Servo's internal subsystems, including the `ScriptTask` and the
+// `LayoutTask`, as well maintains the navigation context.
+//
+// The `Browser` is fed events from a generic type that implements the
+// `WindowMethods` trait.
 #![feature(libc, rustc_private, thread_local)]
 #![cfg_attr(not(test), feature(path))]
 
-#[macro_use]
-extern crate log;
-
 extern crate compositing;
 extern crate devtools;
+extern crate devtools_traits;
 extern crate net;
 extern crate net_traits;
 extern crate msg;
@@ -26,57 +38,64 @@ extern crate webdriver_server;
 use compositing::CompositorEventListener;
 use compositing::windowing::WindowEvent;
 
-#[cfg(not(test))]
 use compositing::windowing::WindowMethods;
-#[cfg(not(test))]
 use compositing::{CompositorProxy, CompositorTask, Constellation};
-#[cfg(not(test))]
+
 use msg::constellation_msg::Msg as ConstellationMsg;
-#[cfg(not(test))]
 use msg::constellation_msg::ConstellationChan;
-#[cfg(not(test))]
+
 use script::dom::bindings::codegen::RegisterBindings;
 
-#[cfg(not(test))]
 use net::image_cache_task::{ImageCacheTaskFactory, LoadPlaceholder};
-#[cfg(not(test))]
 use net::storage_task::StorageTaskFactory;
-#[cfg(not(test))]
 use net::resource_task::new_resource_task;
-#[cfg(not(test))]
 use net_traits::image_cache_task::ImageCacheTask;
-#[cfg(not(test))]
 use net_traits::storage_task::StorageTask;
-#[cfg(not(test))]
+
 use gfx::font_cache_task::FontCacheTask;
-#[cfg(not(test))]
 use profile::mem;
-#[cfg(not(test))]
 use profile::time;
-#[cfg(not(test))]
 use util::opts;
-#[cfg(not(test))]
 use util::taskpool::TaskPool;
 
-#[cfg(not(test))]
 use std::rc::Rc;
+use std::sync::mpsc::Sender;
 
 pub struct Browser {
     compositor: Box<CompositorEventListener + 'static>,
 }
 
+/// The in-process interface to Servo.
+///
+/// It does everything necessary to render the web, primarily
+/// orchestrating the interaction between JavaScript, CSS layout,
+/// rendering, and the client window.
+///
+/// Clients create a `Browser` for a given reference-counted type
+/// implementing `WindowMethods`, which is the bridge to whatever
+/// application Servo is embedded in. Clients then create an event
+/// loop to pump messages between the embedding application and
+/// various browser components.
 impl Browser  {
-    #[cfg(not(test))]
     pub fn new<Window>(window: Option<Rc<Window>>) -> Browser
     where Window: WindowMethods + 'static {
-        use std::env;
-
         ::util::opts::set_experimental_enabled(opts::get().enable_experimental);
+
+        // Global configuration options, parsed from the command line.
         let opts = opts::get();
+
+        // Create the global vtables used by the (generated) DOM
+        // bindings to implement JS proxies.
         RegisterBindings::RegisterProxyHandlers();
 
+        // Use this thread pool to load-balance simple tasks, such as
+        // image decoding.
         let shared_task_pool = TaskPool::new(8);
 
+        // Get both endpoints of a special channel for communication between
+        // the client window and the compositor. This channel is unique because
+        // messages to client may need to pump a platform-specific event loop
+        // to deliver the message.
         let (compositor_proxy, compositor_receiver) =
             WindowMethods::create_compositor_channel(&window);
         let time_profiler_chan = time::Profiler::create(opts.time_profiler_period);
@@ -89,6 +108,58 @@ impl Browser  {
             webdriver_server::start_server(port);
         }
 
+        // Create the constellation, which maintains the engine
+        // pipelines, including the script and layout threads, as well
+        // as the navigation context.
+        let constellation_chan = create_constellation(opts.clone(),
+                                                      compositor_proxy.clone_compositor_proxy(),
+                                                      time_profiler_chan.clone(),
+                                                      devtools_chan,
+                                                      mem_profiler_chan.clone(),
+                                                      shared_task_pool);
+
+        // The compositor coordinates with the client window to create the final
+        // rendered page and display it somewhere.
+        let compositor = CompositorTask::create(window,
+                                                compositor_proxy,
+                                                compositor_receiver,
+                                                constellation_chan.clone(),
+                                                time_profiler_chan,
+                                                mem_profiler_chan);
+
+        Browser {
+            compositor: compositor,
+        }
+    }
+
+    pub fn handle_event(&mut self, event: WindowEvent) -> bool {
+        self.compositor.handle_event(event)
+    }
+
+    pub fn repaint_synchronously(&mut self) {
+        self.compositor.repaint_synchronously()
+    }
+
+    pub fn pinch_zoom_level(&self) -> f32 {
+        self.compositor.pinch_zoom_level()
+    }
+
+    pub fn get_title_for_main_frame(&self) {
+        self.compositor.get_title_for_main_frame()
+    }
+
+    pub fn shutdown(mut self) {
+        self.compositor.shutdown();
+    }
+}
+fn create_constellation(opts: opts::Opts,
+                        compositor_proxy: Box<CompositorProxy+Send>,
+                        time_profiler_chan: time::ProfilerChan,
+                        devtools_chan: Option<Sender<devtools_traits::DevtoolsControlMsg>>,
+                        mem_profiler_chan: mem::ProfilerChan,
+                        shared_task_pool: TaskPool) -> ConstellationChan {
+    use std::env;
+
     // Create a Servo instance.
     let resource_task = new_resource_task(opts.user_agent.clone());
 
@@ -126,38 +197,10 @@ impl Browser  {
         Err(_) => panic!("URL parsing failed"),
     };
 
+    {
         let ConstellationChan(ref chan) = constellation_chan;
         chan.send(ConstellationMsg::InitLoadUrl(url)).unwrap();
-
-        let compositor = CompositorTask::create(window,
-                                                compositor_proxy,
-                                                compositor_receiver,
-                                                constellation_chan.clone(),
-                                                time_profiler_chan,
-                                                mem_profiler_chan);
-
-        Browser {
-            compositor: compositor,
-        }
     }
 
-    pub fn handle_event(&mut self, event: WindowEvent) -> bool {
+    constellation_chan
-        self.compositor.handle_event(event)
-    }
-
-    pub fn repaint_synchronously(&mut self) {
-        self.compositor.repaint_synchronously()
-    }
-
-    pub fn pinch_zoom_level(&self) -> f32 {
-        self.compositor.pinch_zoom_level()
-    }
-
-    pub fn get_title_for_main_frame(&self) {
-        self.compositor.get_title_for_main_frame()
-    }
-
-    pub fn shutdown(mut self) {
-        self.compositor.shutdown();
-    }
 }

components/servo/main.rs

@@ -2,116 +2,52 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+//! The `servo` test application.
+//!
+//! Creates a `Browser` instance with a simple implementation of
+//! the compositor's `WindowMethods` to create a working web browser.
+//!
+//! This browser's implementation of `WindowMethods` is built on top
+//! of [glutin], the cross-platform OpenGL utility and windowing
+//! library.
+//!
+//! For the engine itself look next door in lib.rs.
+//!
+//! [glutin]: https://github.com/tomaka/glutin
+
 #![feature(start)]
 
-#[cfg(target_os="android")]
+// The Servo engine
-extern crate libc;
-
 extern crate servo;
-extern crate time;
+// Window graphics compositing and message dispatch
-extern crate util;
-extern crate net;
-
-#[cfg(not(test))]
-extern crate "glutin_app" as app;
-
-#[cfg(not(test))]
 extern crate compositing;
+// Servo networking
+extern crate net;
+// Servo common utilitiess
+extern crate util;
+// The window backed by glutin
+extern crate "glutin_app" as app;
+extern crate time;
 
 #[cfg(target_os="android")]
 #[macro_use]
 extern crate android_glue;
 
-#[cfg(target_os="android")]
+use std::rc::Rc;
-use libc::c_int;
-
-#[cfg(not(test))]
 use util::opts;
-
-#[cfg(not(test))]
 use net::resource_task;
-
-#[cfg(not(test))]
 use servo::Browser;
-#[cfg(not(test))]
 use compositing::windowing::WindowEvent;
 
 #[cfg(target_os="android")]
 use std::borrow::ToOwned;
 
-#[cfg(not(test))]
-struct BrowserWrapper {
-    browser: Browser,
-}
-
-#[cfg(target_os="android")]
-android_start!(main);
-
-#[cfg(target_os="android")]
-fn get_args() -> Vec<String> {
-    vec![
-        "servo".to_owned(),
-        "http://en.wikipedia.org/wiki/Rust".to_owned()
-    ]
-}
-
-#[cfg(not(target_os="android"))]
-fn get_args() -> Vec<String> {
-    use std::env;
-    env::args().collect()
-}
-
-#[cfg(target_os="android")]
-struct FilePtr(*mut libc::types::common::c95::FILE);
-
-#[cfg(target_os="android")]
-unsafe impl Send for FilePtr {}
-
-#[cfg(target_os="android")]
-fn redirect_output(file_no: c_int) {
-    use libc::funcs::posix88::unistd::{pipe, dup2};
-    use libc::funcs::posix88::stdio::fdopen;
-    use libc::funcs::c95::stdio::fgets;
-    use util::task::spawn_named;
-    use std::mem;
-    use std::ffi::CString;
-    use std::str::from_utf8;
-
-    unsafe {
-        let mut pipes: [c_int; 2] = [ 0, 0 ];
-        pipe(pipes.as_mut_ptr());
-        dup2(pipes[1], file_no);
-        let mode = CString::new("r").unwrap();
-        let input_file = FilePtr(fdopen(pipes[0], mode.as_ptr()));
-        spawn_named("android-logger".to_owned(), move || {
-            loop {
-                let mut read_buffer: [u8; 1024] = mem::zeroed();
-                let FilePtr(input_file) = input_file;
-                fgets(read_buffer.as_mut_ptr() as *mut i8, read_buffer.len() as i32, input_file);
-                match from_utf8(&read_buffer) {
-                    Ok(s) => android_glue::write_log(s.trim_right_matches('\0')),
-                    _ => {}
-                }
-            }
-        });
-    }
-}
-
-#[cfg(target_os="android")]
-fn setup_logging() {
-    use libc::consts::os::posix88::{STDERR_FILENO, STDOUT_FILENO};
-    //os::setenv("RUST_LOG", "servo,gfx,msg,util,layers,js,std,rt,extra");
-    redirect_output(STDERR_FILENO);
-    redirect_output(STDOUT_FILENO);
-}
-
-#[cfg(not(target_os="android"))]
-fn setup_logging() {
-}
-
 fn main() {
+    // Parse the command line options and store them globally
     if opts::from_cmdline_args(&*get_args()) {
         setup_logging();
+
+        // Possibly interpret the `HOST_FILE` environment variable
         resource_task::global_init();
 
         let window = if opts::get().headless {
@@ -120,21 +56,18 @@ fn main() {
             Some(app::create_window())
         };
 
+        // Our wrapper around `Browser` that also implements some
+        // callbacks required by the glutin window implementation.
         let mut browser = BrowserWrapper {
             browser: Browser::new(window.clone()),
         };
 
-        match window {
+        maybe_register_glutin_resize_handler(&window, &mut browser);
-            None => {}
-            Some(ref window) => {
-                unsafe {
-                    window.set_nested_event_loop_listener(&mut browser);
-                }
-            }
-        }
 
         browser.browser.handle_event(WindowEvent::InitializeCompositing);
 
+        // Feed events from the window to the browser until the browser
+        // says to stop.
         loop {
             let should_continue = match window {
                 None => browser.browser.handle_event(WindowEvent::Idle),
@@ -148,7 +81,29 @@ fn main() {
             }
         };
 
-        match window {
+        maybe_unregister_glutin_resize_handler(&window);
+
+        let BrowserWrapper {
+            browser
+        } = browser;
+        browser.shutdown();
+    }
+}
+
+fn maybe_register_glutin_resize_handler(window: &Option<Rc<app::window::Window>>,
+                                        browser: &mut BrowserWrapper) {
+    match *window {
+        None => {}
+        Some(ref window) => {
+            unsafe {
+                window.set_nested_event_loop_listener(browser);
+            }
+        }
+    }
+}
+
+fn maybe_unregister_glutin_resize_handler(window: &Option<Rc<app::window::Window>>) {
+    match *window {
         None => {}
         Some(ref window) => {
             unsafe {
@@ -156,12 +111,10 @@ fn main() {
             }
         }
     }
+}
 
-        let BrowserWrapper {
+struct BrowserWrapper {
-            browser
+    browser: Browser,
-        } = browser;
-        browser.shutdown();
-    }
 }
 
 impl app::NestedEventLoopListener for BrowserWrapper {
@@ -180,3 +133,83 @@ impl app::NestedEventLoopListener for BrowserWrapper {
     }
 }
 
+#[cfg(target_os="android")]
+fn setup_logging() {
+    android::setup_logging();
+}
+
+#[cfg(not(target_os="android"))]
+fn setup_logging() {
+}
+
+#[cfg(target_os="android")]
+fn get_args() -> Vec<String> {
+    vec![
+        "servo".to_owned(),
+        "http://en.wikipedia.org/wiki/Rust".to_owned()
+    ]
+}
+
+#[cfg(not(target_os="android"))]
+fn get_args() -> Vec<String> {
+    use std::env;
+    env::args().collect()
+}
+
+// This macro must be used at toplevel because it defines a nested
+// module, but macros can only accept identifiers - not paths -
+// preventing the expansion of this macro within the android module
+// without use of an additionl stub method or other hackery.
+#[cfg(target_os = "android")]
+android_start!(main);
+
+#[cfg(target_os = "android")]
+mod android {
+    extern crate libc;
+    extern crate android_glue;
+
+    use self::libc::c_int;
+    use std::borrow::ToOwned;
+
+    pub fn setup_logging() {
+        use self::libc::consts::os::posix88::{STDERR_FILENO, STDOUT_FILENO};
+        //os::setenv("RUST_LOG", "servo,gfx,msg,util,layers,js,std,rt,extra");
+        redirect_output(STDERR_FILENO);
+        redirect_output(STDOUT_FILENO);
+    }
+
+    struct FilePtr(*mut self::libc::types::common::c95::FILE);
+
+    unsafe impl Send for FilePtr {}
+
+    fn redirect_output(file_no: c_int) {
+        use self::libc::funcs::posix88::unistd::{pipe, dup2};
+        use self::libc::funcs::posix88::stdio::fdopen;
+        use self::libc::funcs::c95::stdio::fgets;
+        use util::task::spawn_named;
+        use std::mem;
+        use std::ffi::CString;
+        use std::str::from_utf8;
+
+        unsafe {
+            let mut pipes: [c_int; 2] = [ 0, 0 ];
+            pipe(pipes.as_mut_ptr());
+            dup2(pipes[1], file_no);
+            let mode = CString::from_slice("r".as_bytes());
+            let input_file = FilePtr(fdopen(pipes[0], mode.as_ptr()));
+            spawn_named("android-logger".to_owned(), move || {
+                loop {
+                    let mut read_buffer: [u8; 1024] = mem::zeroed();
+                    let FilePtr(input_file) = input_file;
+                    fgets(read_buffer.as_mut_ptr() as *mut i8, read_buffer.len() as i32, input_file);
+                    let cs = CString::from_slice(&read_buffer);
+                    match from_utf8(cs.as_bytes()) {
+                        Ok(s) => android_glue::write_log(s),
+                        _ => {}
+                    }
+                }
+            });
+        }
+    }
+
+}

ports/gonk/src/lib.rs

@@ -66,16 +66,37 @@ pub struct Browser {
     compositor: Box<CompositorEventListener + 'static>,
 }
 
+/// The in-process interface to Servo.
+///
+/// It does everything necessary to render the web, primarily
+/// orchestrating the interaction between JavaScript, CSS layout,
+/// rendering, and the client window.
+///
+/// Clients create a `Browser` for a given reference-counted type
+/// implementing `WindowMethods`, which is the bridge to whatever
+/// application Servo is embedded in. Clients then create an event
+/// loop to pump messages between the embedding application and
+/// various browser components.
 impl Browser {
     #[cfg(not(test))]
     pub fn new<Window>(window: Option<Rc<Window>>) -> Browser
         where Window: WindowMethods + 'static {
         ::util::opts::set_experimental_enabled(opts::get().enable_experimental);
+        // Global configuration options, parsed from the command line.
         let opts = opts::get();
+
+        // Create the global vtables used by the (generated) DOM
+        // bindings to implement JS proxies.
         RegisterBindings::RegisterProxyHandlers();
 
+        // Use this thread pool to load-balance simple tasks, such as
+        // image decoding.
         let shared_task_pool = TaskPool::new(8);
 
+        // Get both endpoints of a special channel for communication between
+        // the client window and the compositor. This channel is unique because
+        // messages to client may need to pump a platform-specific event loop
+        // to deliver the message.
         let (compositor_proxy, compositor_receiver) =
             WindowMethods::create_compositor_channel(&window);
         let time_profiler_chan = time::Profiler::create(opts.time_profiler_period);
@@ -100,6 +121,10 @@ impl Browser {
 
         let font_cache_task = FontCacheTask::new(resource_task.clone());
         let storage_task = StorageTaskFactory::new();
+
+        // Create the constellation, which maintains the engine
+        // pipelines, including the script and layout threads, as well
+        // as the navigation context.
         let constellation_chan = Constellation::<layout::layout_task::LayoutTask,
                                                  script::script_task::ScriptTask>::start(
                                                       compositor_proxy.clone_compositor_proxy(),
@@ -123,6 +148,8 @@ impl Browser {
         let ConstellationChan(ref chan) = constellation_chan;
         chan.send(ConstellationMsg::InitLoadUrl(url)).unwrap();
 
+        // The compositor coordinates with the client window to create the final
+        // rendered page and display it somewhere.
         let compositor = CompositorTask::create(window,
                                                 compositor_proxy,
                                                 compositor_receiver,

ports/gonk/src/main.rs

@@ -11,6 +11,19 @@
 // For FFI
 #![allow(non_snake_case, dead_code)]
 
+//! The `servo` test application.
+//!
+//! Creates a `Browser` instance with a simple implementation of
+//! the compositor's `WindowMethods` to create a working web browser.
+//!
+//! This browser's implementation of `WindowMethods` is built on top
+//! of [glutin], the cross-platform OpenGL utility and windowing
+//! library.
+//!
+//! For the engine itself look next door in lib.rs.
+//!
+//! [glutin]: https://github.com/tomaka/glutin
+
 extern crate servo;
 extern crate time;
 extern crate util;
@@ -42,6 +55,7 @@ struct BrowserWrapper {
 }
 
 fn main() {
+    // Parse the command line options and store them globally
     if opts::from_cmdline_args(env::args().collect::<Vec<_>>().as_slice()) {
         resource_task::global_init();
 
@@ -51,6 +65,8 @@ fn main() {
             Some(window::Window::new())
         };
 
+        // Our wrapper around `Browser` that also implements some
+        // callbacks required by the glutin window implementation.
         let mut browser = BrowserWrapper {
             browser: Browser::new(window.clone()),
         };
@@ -62,6 +78,8 @@ fn main() {
 
         browser.browser.handle_event(WindowEvent::InitializeCompositing);
 
+        // Feed events from the window to the browser until the browser
+        // says to stop.
         loop {
             let should_continue = match window {
                 None => browser.browser.handle_event(WindowEvent::Idle),