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

6 files changed, 288 insertions, 163 deletions

diff --git a/components/servo/Cargo.lock b/components/servo/Cargo.lock
index f53a156d553..49f2dc6585c 100644
--- a/components/servo/Cargo.lock
+++ b/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",
diff --git a/components/servo/Cargo.toml b/components/servo/Cargo.toml
index 37644696810..045b54da226 100644
--- a/components/servo/Cargo.toml
+++ b/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
diff --git a/components/servo/lib.rs b/components/servo/lib.rs
index 8109f23e3f9..107f5e14c0f 100644
--- a/components/servo/lib.rs
+++ b/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,46 +108,18 @@ impl Browser  {
             webdriver_server::start_server(port);
         }
 
-        // Create a Servo instance.
-        let resource_task = new_resource_task(opts.user_agent.clone());
-
-        // If we are emitting an output file, then we need to block on
-        // image load or we risk emitting an output file missing the
-        // image.
-        let image_cache_task: ImageCacheTask = if opts.output_file.is_some() {
-            ImageCacheTaskFactory::new_sync(resource_task.clone(), shared_task_pool,
-                                            time_profiler_chan.clone(), LoadPlaceholder::Preload)
-        } else {
-            ImageCacheTaskFactory::new(resource_task.clone(), shared_task_pool,
-                                       time_profiler_chan.clone(), LoadPlaceholder::Preload)
-        };
-
-        let font_cache_task = FontCacheTask::new(resource_task.clone());
-        let storage_task: StorageTask = StorageTaskFactory::new();
-
-        let constellation_chan = Constellation::<layout::layout_task::LayoutTask,
-                                                 script::script_task::ScriptTask>::start(
+        // 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(),
-                                                      resource_task,
-                                                      image_cache_task,
-                                                      font_cache_task,
                                                       time_profiler_chan.clone(),
-                                                      mem_profiler_chan.clone(),
                                                       devtools_chan,
-                                                      storage_task);
-
-        // Send the URL command to the constellation.
-        let cwd = env::current_dir().unwrap();
-        let url = match url::Url::parse(&opts.url) {
-            Ok(url) => url,
-            Err(url::ParseError::RelativeUrlWithoutBase)
-            => url::Url::from_file_path(&*cwd.join(&opts.url)).unwrap(),
-            Err(_) => panic!("URL parsing failed"),
-        };
-
-        let ConstellationChan(ref chan) = constellation_chan;
-        chan.send(ConstellationMsg::InitLoadUrl(url)).unwrap();
+                                                      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,
@@ -161,3 +152,55 @@ impl Browser  {
         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());
+
+    // If we are emitting an output file, then we need to block on
+    // image load or we risk emitting an output file missing the
+    // image.
+    let image_cache_task: ImageCacheTask = if opts.output_file.is_some() {
+        ImageCacheTaskFactory::new_sync(resource_task.clone(), shared_task_pool,
+                                        time_profiler_chan.clone(), LoadPlaceholder::Preload)
+    } else {
+        ImageCacheTaskFactory::new(resource_task.clone(), shared_task_pool,
+                                   time_profiler_chan.clone(), LoadPlaceholder::Preload)
+    };
+
+    let font_cache_task = FontCacheTask::new(resource_task.clone());
+    let storage_task: StorageTask = StorageTaskFactory::new();
+
+    let constellation_chan = Constellation::<layout::layout_task::LayoutTask,
+    script::script_task::ScriptTask>::start(
+        compositor_proxy.clone_compositor_proxy(),
+        resource_task,
+        image_cache_task,
+        font_cache_task,
+        time_profiler_chan.clone(),
+        mem_profiler_chan.clone(),
+        devtools_chan,
+        storage_task);
+
+    // Send the URL command to the constellation.
+    let cwd = env::current_dir().unwrap();
+    let url = match url::Url::parse(&opts.url) {
+        Ok(url) => url,
+        Err(url::ParseError::RelativeUrlWithoutBase)
+        => url::Url::from_file_path(&*cwd.join(&opts.url)).unwrap(),
+        Err(_) => panic!("URL parsing failed"),
+    };
+
+    {
+        let ConstellationChan(ref chan) = constellation_chan;
+        chan.send(ConstellationMsg::InitLoadUrl(url)).unwrap();
+    }
+
+    constellation_chan
+}
diff --git a/components/servo/main.rs b/components/servo/main.rs
index 6acaf013572..1bf329920e3 100644
--- a/components/servo/main.rs
+++ b/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/. */
 
-#![feature(start)]
+//! 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
 
-#[cfg(target_os="android")]
-extern crate libc;
+#![feature(start)]
 
+// The Servo engine
 extern crate servo;
-extern crate time;
-extern crate util;
+// Window graphics compositing and message dispatch
+extern crate compositing;
+// Servo networking
 extern crate net;
-
-#[cfg(not(test))]
+// Servo common utilitiess
+extern crate util;
+// The window backed by glutin
 extern crate "glutin_app" as app;
-
-#[cfg(not(test))]
-extern crate compositing;
+extern crate time;
 
 #[cfg(target_os="android")]
 #[macro_use]
 extern crate android_glue;
 
-#[cfg(target_os="android")]
-use libc::c_int;
-
-#[cfg(not(test))]
+use std::rc::Rc;
 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 {
-            None => {}
-            Some(ref window) => {
-                unsafe {
-                    window.set_nested_event_loop_listener(&mut browser);
-                }
-            }
-        }
+        maybe_register_glutin_resize_handler(&window, &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,14 +81,7 @@ fn main() {
             }
         };
 
-        match window {
-            None => {}
-            Some(ref window) => {
-                unsafe {
-                    window.remove_nested_event_loop_listener();
-                }
-            }
-        }
+        maybe_unregister_glutin_resize_handler(&window);
 
         let BrowserWrapper {
             browser
@@ -164,6 +90,33 @@ fn main() {
     }
 }
 
+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 {
+                window.remove_nested_event_loop_listener();
+            }
+        }
+    }
+}
+
+struct BrowserWrapper {
+    browser: Browser,
+}
+
 impl app::NestedEventLoopListener for BrowserWrapper {
     fn handle_event_from_nested_event_loop(&mut self, event: WindowEvent) -> bool {
         let is_resize = match event {
@@ -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),
+                        _ => {}
+                    }
+                }
+            });
+        }
+    }
+
+}
diff --git a/ports/gonk/src/lib.rs b/ports/gonk/src/lib.rs
index 3c258a991b7..d319d76574c 100644
--- a/ports/gonk/src/lib.rs
+++ b/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,
diff --git a/ports/gonk/src/main.rs b/ports/gonk/src/main.rs
index b7480994062..5a6af9db9ef 100644
--- a/ports/gonk/src/main.rs
+++ b/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),