address review comments

ports/libsimpleservo/Cargo.toml

@@ -31,6 +31,7 @@ winapi = "0.3.2"
 
 [build-dependencies]
 gl_generator = "0.9"
+cc = "1.0"
 
 [features]
 default = ["unstable", "default-except-unstable"]

ports/libsimpleservo/build.rs

@@ -2,14 +2,13 @@
  * 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/. */
 
+extern crate cc;
 extern crate gl_generator;
 
 use gl_generator::{Api, Fallbacks, Profile, Registry};
 use std::env;
 use std::fs::File;
 use std::path::Path;
-use std::process;
-use std::process::{Command, Stdio};
 
 fn main() {
     let target = env::var("TARGET").unwrap();
@@ -24,18 +23,6 @@ fn android_main() {
     let ndk_path = env::var_os("ANDROID_NDK").expect("Please set the ANDROID_NDK environment variable");
     let ndk_path = Path::new(&ndk_path);
 
-    // Build up the path to the NDK compilers
-    // Options for host are:  "linux-x86_64" "linux-x86" "darwin-x86_64" "darwin-x86"
-    // per: https://android.googlesource.com/platform/ndk/+/ics-mr0/docs/STANDALONE-TOOLCHAIN.html
-
-    let host = env::var("HOST").unwrap();
-    let google_host = match host.as_ref() {
-        "i686-unknown-linux-gnu" => "linux-x86",
-        "x86_64-apple-darwin" => "darwin-x86_64",
-        "x86_64-unknown-linux-gnu" => "linux-x86_64",
-        _ => panic!("Unknown support android cross-compile host: {}", host)
-    };
-
     let target = env::var("TARGET").unwrap();
     let arch = if target.contains("arm") {
         "arch-arm"
@@ -55,49 +42,17 @@ fn android_main() {
         "android-18"
     };
 
-    let (toolchain, prefix) = if target.contains("armv7") {
-        let toolchain = "arm-linux-androideabi";
-        (toolchain.into(), toolchain.into())
-    } else if target.contains("i686") {
-        ("x86".into(), target)
-    } else {
-        (target.clone(), target)
-    };
-
-    let toolchain_path = ndk_path.join("toolchains").join(format!("{}-4.9", toolchain)).join("prebuilt").
-        join(google_host);
-    println!("toolchain path is: {}", toolchain_path.to_str().unwrap());
+    // compiling android_native_app_glue.c
+    let c_file = ndk_path.join("sources").join("android").join("native_app_glue").join("android_native_app_glue.c");
+    let sysroot = ndk_path.join("platforms").join(platform).join(arch);
+    cc::Build::new()
+        .file(c_file)
+        .flag("--sysroot")
+        .flag(sysroot.to_str().unwrap())
+        .compile("android_native_app_glue");
 
     // Get the output directory.
     let out_dir = env::var("OUT_DIR").expect("Cargo should have set the OUT_DIR environment variable");
-    let directory = Path::new(&out_dir);
-
-    // compiling android_native_app_glue.c
-    if Command::new(toolchain_path.join("bin").join(format!("{}-gcc", prefix)))
-        .arg(ndk_path.join("sources").join("android").join("native_app_glue").join("android_native_app_glue.c"))
-        .arg("-c")
-        .arg("-o").arg(directory.join("android_native_app_glue.o"))
-        .arg("--sysroot").arg(ndk_path.join("platforms").join(platform).join(arch))
-        .stdout(Stdio::inherit())
-        .stderr(Stdio::inherit())
-        .status().unwrap().code().unwrap() != 0
-    {
-        println!("Error while executing gcc");
-        process::exit(1)
-    }
-
-    // compiling libandroid_native_app_glue.a
-    if Command::new(toolchain_path.join("bin").join(format!("{}-ar", prefix)))
-        .arg("rcs")
-        .arg(directory.join("libandroid_native_app_glue.a"))
-        .arg(directory.join("android_native_app_glue.o"))
-        .stdout(Stdio::inherit())
-        .stderr(Stdio::inherit())
-        .status().unwrap().code().unwrap() != 0
-    {
-        println!("Error while executing ar");
-        process::exit(1)
-    }
 
     println!("cargo:rustc-link-lib=static=android_native_app_glue");
     println!("cargo:rustc-link-search=native={}", out_dir);

ports/libsimpleservo/src/api.rs

@@ -175,63 +175,37 @@ impl ServoGlue {
             .map_err(|_| "Can't parse URL")
             .and_then(|url| {
                 let event = WindowEvent::LoadUrl(self.browser_id, url);
-                self.events.push(event);
-                if !self.batch_mode {
-                    self.perform_updates()
-                } else {
-                    Ok(())
-                }
+                self.process_event(event)
             })
     }
 
     /// Reload the page.
     pub fn reload(&mut self) -> Result<(), &'static str> {
         info!("reload");
         let event = WindowEvent::Reload(self.browser_id);
-        self.events.push(event);
-        if !self.batch_mode {
-            self.perform_updates()
-        } else {
-            Ok(())
-        }
+        self.process_event(event)
     }
 
     /// Go back in history.
     pub fn go_back(&mut self) -> Result<(), &'static str> {
         info!("go_back");
         let event = WindowEvent::Navigation(self.browser_id, TraversalDirection::Back(1));
-        self.events.push(event);
-        if !self.batch_mode {
-            self.perform_updates()
-        } else {
-            Ok(())
-        }
+        self.process_event(event)
     }
 
     /// Go forward in history.
     pub fn go_forward(&mut self) -> Result<(), &'static str> {
         info!("go_forward");
         let event = WindowEvent::Navigation(self.browser_id, TraversalDirection::Forward(1));
-        self.events.push(event);
-        if !self.batch_mode {
-            self.perform_updates()
-        } else {
-            Ok(())
-        }
+        self.process_event(event)
     }
 
     /// Let Servo know that the window has been resized.
     pub fn resize(&mut self, width: u32, height: u32) -> Result<(), &'static str> {
         info!("resize");
         self.callbacks.width.set(width);
         self.callbacks.height.set(height);
-        let event = WindowEvent::Resize;
-        self.events.push(event);
-        if !self.batch_mode {
-            self.perform_updates()
-        } else {
-            Ok(())
-        }
+        self.process_event(WindowEvent::Resize)
     }
 
     /// Start scrolling.
@@ -245,12 +219,7 @@ impl ServoGlue {
             TypedPoint2D::new(x as i32, y as i32),
             TouchEventType::Down,
         );
-        self.events.push(event);
-        if !self.batch_mode {
-            self.perform_updates()
-        } else {
-            Ok(())
-        }
+        self.process_event(event)
     }
 
     /// Scroll.
@@ -264,12 +233,7 @@ impl ServoGlue {
             TypedPoint2D::new(x as i32, y as i32),
             TouchEventType::Move,
         );
-        self.events.push(event);
-        if !self.batch_mode {
-            self.perform_updates()
-        } else {
-            Ok(())
-        }
+        self.process_event(event)
     }
 
     /// End scrolling.
@@ -283,20 +247,19 @@ impl ServoGlue {
             TypedPoint2D::new(x as i32, y as i32),
             TouchEventType::Up,
         );
-        self.events.push(event);
-        if !self.batch_mode {
-            self.perform_updates()
-        } else {
-            Ok(())
-        }
+        self.process_event(event)
     }
 
     /// Perform a click.
     pub fn click(&mut self, x: u32, y: u32) -> Result<(), &'static str> {
         let mouse_event =
             MouseWindowEvent::Click(MouseButton::Left, TypedPoint2D::new(x as f32, y as f32));
         let event = WindowEvent::MouseWindowEventClass(mouse_event);
-        self.servo.handle_events(vec![event]);
+        self.process_event(event)
+    }
+
+    fn process_event(&mut self, event: WindowEvent) -> Result<(), &'static str> {
+        self.events.push(event);
         if !self.batch_mode {
             self.perform_updates()
         } else {

ports/libsimpleservo/src/capi.rs

@@ -36,6 +36,7 @@ pub struct CHostCallbacks {
     pub on_animating_changed: extern fn(animating: bool),
 }
 
+/// The returned string is not freed. This will leak.
 #[no_mangle]
 pub extern "C" fn servo_version() -> *const c_char {
     let v = api::servo_version();

ports/libsimpleservo/src/jniapi.rs

@@ -2,6 +2,8 @@
  * 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/. */
 
+#![allow(non_snake_case)]
+
 use android_logger::{self, Filter};
 use api::{self, EventLoopWaker, ServoGlue, SERVO, HostTrait, ReadFileTrait};
 use gl_glue;
@@ -34,15 +36,13 @@ where
 }
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_version(env: JNIEnv, _class: JClass) -> jstring {
     let v = api::servo_version();
     let output = env.new_string(v).expect("Couldn't create java string");
     output.into_inner()
 }
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_init(
     env: JNIEnv,
     _: JClass,
@@ -99,7 +99,6 @@ pub fn Java_com_mozilla_servoview_NativeServo_init(
 }
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_setBatchMode(
     env: JNIEnv,
     _: JClass,
@@ -110,7 +109,6 @@ pub fn Java_com_mozilla_servoview_NativeServo_setBatchMode(
 }
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_resize(
     env: JNIEnv,
     _: JClass,
@@ -122,43 +120,37 @@ pub fn Java_com_mozilla_servoview_NativeServo_resize(
 }
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_performUpdates(env: JNIEnv, _class: JClass) {
     debug!("performUpdates");
     call(env, |s| s.perform_updates());
 }
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_loadUri(env: JNIEnv, _class: JClass, url: JString) {
     debug!("loadUri");
     let url: String = env.get_string(url).unwrap().into();
     call(env, |s| s.load_uri(&url));
 }
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_reload(env: JNIEnv, _class: JClass) {
     debug!("reload");
     call(env, |s| s.reload());
 }
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_goBack(env: JNIEnv, _class: JClass) {
     debug!("goBack");
     call(env, |s| s.go_back());
 }
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_goForward(env: JNIEnv, _class: JClass) {
     debug!("goForward");
     call(env, |s| s.go_forward());
 }
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_scrollStart(
     env: JNIEnv,
     _: JClass,
@@ -172,7 +164,6 @@ pub fn Java_com_mozilla_servoview_NativeServo_scrollStart(
 }
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_scrollEnd(
     env: JNIEnv,
     _: JClass,
@@ -187,7 +178,6 @@ pub fn Java_com_mozilla_servoview_NativeServo_scrollEnd(
 
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_scroll(
     env: JNIEnv,
     _: JClass,
@@ -201,7 +191,6 @@ pub fn Java_com_mozilla_servoview_NativeServo_scroll(
 }
 
 #[no_mangle]
-#[allow(non_snake_case)]
 pub fn Java_com_mozilla_servoview_NativeServo_click(env: JNIEnv, _: JClass, x: jint, y: jint) {
     debug!("click");
     call(env, |s| s.click(x as u32, y as u32));

python/servo/command_base.py

@@ -334,10 +334,8 @@ def get_apk_path(self, release):
         base_path = self.get_target_dir()
         base_path = path.join(base_path, self.config["android"]["target"])
         apk_name = "servo.apk"
-        if release:
-            return path.join(base_path, "release", apk_name)
-        else:
-            return path.join(base_path, "debug", apk_name)
+        build_type = "release" if release else "debug"
+        return path.join(base_path, build_type, apk_name)
 
     def get_binary_path(self, release, dev, android=False):
         # TODO(autrilla): this function could still use work - it shouldn't