Bug 1466573 - Start Firefox with -foreground and -no-remote. r=jgraham
authorAndreas Tolfsen <ato@sny.no>
Tue, 05 Jun 2018 18:02:34 +0100
changeset 422626 c5c9371e1527ac814503222f80bd6f4983eed373
parent 422625 d264fabbfb132061f8154739e740077f8111663e
child 422627 9ca74dd454400190188c6646cf94441f198bbc24
push id34139
push useraciure@mozilla.com
push dateFri, 15 Jun 2018 09:48:05 +0000
treeherdermozilla-central@dc997a4e045e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjgraham
bugs1466573
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1466573 - Start Firefox with -foreground and -no-remote. r=jgraham Start Firefox with -foreground and -no-remote arguments if they have not already been given by the user. -foreground will ensure the application window gets focus when Firefox is started, and -no-remote will prevent remote commands to this instance of Firefox and also ensure we always start a new instance. MozReview-Commit-ID: LGEqgyHYapc
testing/mozbase/rust/mozrunner/src/firefox_args.rs
testing/mozbase/rust/mozrunner/src/lib.rs
testing/mozbase/rust/mozrunner/src/runner.rs
new file mode 100644
--- /dev/null
+++ b/testing/mozbase/rust/mozrunner/src/firefox_args.rs
@@ -0,0 +1,175 @@
+//! Argument string parsing and matching functions for Firefox.
+//!
+//! Which arguments Firefox accepts and in what style depends on the platform.
+//! On Windows only, arguments can be prefixed with `/` (slash), such as
+//! `/foreground`.  Elsewhere, including Windows, arguments may be prefixed
+//! with both single (`-foreground`) and double (`--foreground`) dashes.
+//!
+//! An argument's name is determined by a space or an assignment operator (`=`)
+//! so that for the string `-foo=bar`, `foo` is considered the argument's
+//! basename.
+
+use std::ffi::{OsStr, OsString};
+
+use runner::platform;
+
+fn parse_arg_name<T>(arg: T) -> Option<String>
+where
+    T: AsRef<OsStr>,
+{
+    let arg_os_str: &OsStr = arg.as_ref();
+    let arg_str = arg_os_str.to_string_lossy();
+
+    let mut start = 0;
+    let mut end = 0;
+
+    for (i, c) in arg_str.chars().enumerate() {
+        if i == 0 {
+            if !platform::arg_prefix_char(c) {
+                break;
+            }
+        } else if i == 1 {
+            if name_end_char(c) {
+                break;
+            } else if c != '-' {
+                start = i;
+                end = start + 1;
+            } else {
+                start = i + 1;
+                end = start;
+            }
+        } else {
+            end += 1;
+            if name_end_char(c) {
+                end -= 1;
+                break;
+            }
+        }
+    }
+
+    if start > 0 && end > start {
+        Some(arg_str[start..end].into())
+    } else {
+        None
+    }
+}
+
+fn name_end_char(c: char) -> bool {
+    c == ' ' || c == '='
+}
+
+/// Represents a Firefox command-line argument.
+#[derive(Debug, PartialEq)]
+pub enum Arg {
+    /// `-foreground` ensures application window gets focus, which is not the
+    /// default on macOS.
+    Foreground,
+
+    /// `-no-remote` prevents remote commands to this instance of Firefox, and
+    /// ensure we always start a new instance.
+    NoRemote,
+
+    /// `-P NAME` starts Firefox with a profile with a given name.
+    NamedProfile,
+
+    /// `-profile PATH` starts Firefox with the profile at the specified path.
+    Profile,
+
+    /// `-ProfileManager` starts Firefox with the profile chooser dialogue.
+    ProfileManager,
+
+    /// All other arguments.
+    Other(String),
+
+    /// Not an argument.
+    None,
+}
+
+impl<'a> From<&'a OsString> for Arg {
+    fn from(arg_str: &OsString) -> Arg {
+        if let Some(basename) = parse_arg_name(arg_str) {
+            match &*basename {
+                "profile" => Arg::Profile,
+                "P" => Arg::NamedProfile,
+                "ProfileManager" => Arg::ProfileManager,
+                "foreground" => Arg::Foreground,
+                "no-remote" => Arg::NoRemote,
+                _ => Arg::Other(basename),
+            }
+        } else {
+           Arg::None
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::{Arg, parse_arg_name};
+    use std::ffi::OsString;
+
+    fn parse(arg: &str, name: Option<&str>) {
+        let result = parse_arg_name(arg);
+        assert_eq!(result, name.map(|x| x.to_string()));
+    }
+
+    #[test]
+    fn test_parse_arg_name() {
+        parse("-p", Some("p"));
+        parse("--p", Some("p"));
+        parse("--profile foo", Some("profile"));
+        parse("--profile", Some("profile"));
+        parse("--", None);
+        parse("", None);
+        parse("-=", None);
+        parse("--=", None);
+        parse("-- foo", None);
+        parse("foo", None);
+        parse("/ foo", None);
+        parse("/- foo", None);
+        parse("/=foo", None);
+        parse("foo", None);
+        parse("-profile", Some("profile"));
+        parse("-profile=foo", Some("profile"));
+        parse("-profile = foo", Some("profile"));
+        parse("-profile abc", Some("profile"));
+        parse("-profile /foo", Some("profile"));
+    }
+
+    #[cfg(target_os = "windows")]
+    #[test]
+    fn test_parse_arg_name_windows() {
+        parse("/profile", Some("profile"));
+    }
+
+    #[cfg(not(target_os = "windows"))]
+    #[test]
+    fn test_parse_arg_name_non_windows() {
+        parse("/profile", None);
+    }
+
+    #[test]
+    fn test_arg_from_osstring() {
+        assert_eq!(Arg::from(&OsString::from("-- profile")), Arg::None);
+        assert_eq!(Arg::from(&OsString::from("profile")), Arg::None);
+        assert_eq!(Arg::from(&OsString::from("profile -P")), Arg::None);
+        assert_eq!(Arg::from(&OsString::from("-profiled")), Arg::Other("profiled".into()));
+        assert_eq!(Arg::from(&OsString::from("-PROFILEMANAGER")), Arg::Other("PROFILEMANAGER".into()));
+
+        assert_eq!(Arg::from(&OsString::from("--profile")), Arg::Profile);
+        assert_eq!(Arg::from(&OsString::from("-profile foo")), Arg::Profile);
+
+        assert_eq!(Arg::from(&OsString::from("--ProfileManager")), Arg::ProfileManager);
+        assert_eq!(Arg::from(&OsString::from("-ProfileManager")), Arg::ProfileManager);
+
+        // TODO: -Ptest is valid
+        //assert_eq!(Arg::from(&OsString::from("-Ptest")), Arg::NamedProfile);
+        assert_eq!(Arg::from(&OsString::from("-P")), Arg::NamedProfile);
+        assert_eq!(Arg::from(&OsString::from("-P test")), Arg::NamedProfile);
+
+        assert_eq!(Arg::from(&OsString::from("--foreground")), Arg::Foreground);
+        assert_eq!(Arg::from(&OsString::from("-foreground")), Arg::Foreground);
+
+        assert_eq!(Arg::from(&OsString::from("--no-remote")), Arg::NoRemote);
+        assert_eq!(Arg::from(&OsString::from("-no-remote")), Arg::NoRemote);
+    }
+}
--- a/testing/mozbase/rust/mozrunner/src/lib.rs
+++ b/testing/mozbase/rust/mozrunner/src/lib.rs
@@ -1,9 +1,11 @@
-#[macro_use] extern crate log;
+#[macro_use]
+extern crate log;
 extern crate mozprofile;
 #[cfg(target_os = "windows")]
 extern crate winreg;
 
+pub mod firefox_args;
 pub mod path;
 pub mod runner;
 
 pub use runner::platform::firefox_default_path;
--- a/testing/mozbase/rust/mozrunner/src/runner.rs
+++ b/testing/mozbase/rust/mozrunner/src/runner.rs
@@ -8,16 +8,18 @@ use std::fmt;
 use std::io;
 use std::io::ErrorKind;
 use std::path::{Path, PathBuf};
 use std::process;
 use std::process::{Child, Command, Stdio};
 use std::thread;
 use std::time;
 
+use firefox_args::Arg;
+
 pub trait Runner {
     type Process;
 
     fn arg<'a, S>(&'a mut self, arg: S) -> &'a mut Self
     where
         S: AsRef<OsStr>;
 
     fn args<'a, I, S>(&'a mut self, args: I) -> &'a mut Self
@@ -257,91 +259,46 @@ impl Runner for FirefoxRunner {
         let stderr = self.stderr.unwrap_or_else(|| Stdio::inherit());
 
         let mut cmd = Command::new(&self.binary);
         cmd.args(&self.args[..])
             .envs(&self.envs)
             .stdout(stdout)
             .stderr(stderr);
 
-        if !self.args.iter().any(|x| is_profile_arg(x)) {
+        let mut seen_foreground = false;
+        let mut seen_no_remote = false;
+        let mut seen_profile = false;
+        for arg in self.args.iter() {
+            match arg.into() {
+                Arg::Foreground => seen_foreground = true,
+                Arg::NoRemote => seen_no_remote = true,
+                Arg::Profile | Arg::NamedProfile | Arg::ProfileManager => seen_profile = true,
+                Arg::Other(_) | Arg::None => {},
+            }
+        }
+        if !seen_foreground {
+            cmd.arg("-foreground");
+        }
+        if !seen_no_remote {
+            cmd.arg("-no-remote");
+        }
+        if !seen_profile {
             cmd.arg("-profile").arg(&self.profile.path);
         }
 
         info!("Running command: {:?}", cmd);
         let process = cmd.spawn()?;
         Ok(FirefoxProcess {
             process,
-            profile: self.profile
+            profile: self.profile,
         })
     }
 }
 
-fn parse_arg_name<T>(arg: T) -> Option<String>
-where
-    T: AsRef<OsStr>,
-{
-    let arg_os_str: &OsStr = arg.as_ref();
-    let arg_str = arg_os_str.to_string_lossy();
-
-    let mut start = 0;
-    let mut end = 0;
-
-    for (i, c) in arg_str.chars().enumerate() {
-        if i == 0 {
-            if !platform::arg_prefix_char(c) {
-                break;
-            }
-        } else if i == 1 {
-            if name_end_char(c) {
-                break;
-            } else if c != '-' {
-                start = i;
-                end = start + 1;
-            } else {
-                start = i + 1;
-                end = start;
-            }
-        } else {
-            end += 1;
-            if name_end_char(c) {
-                end -= 1;
-                break;
-            }
-        }
-    }
-
-    if start > 0 && end > start {
-        Some(arg_str[start..end].into())
-    } else {
-        None
-    }
-}
-
-fn name_end_char(c: char) -> bool {
-    c == ' ' || c == '='
-}
-
-/// Check if an argument string affects the Firefox profile
-///
-/// Returns a boolean indicating whether a given string
-/// contains one of the `-P`, `-Profile` or `-ProfileManager`
-/// arguments, respecting the various platform-specific conventions.
-pub fn is_profile_arg<T>(arg: T) -> bool
-where
-    T: AsRef<OsStr>,
-{
-    if let Some(name) = parse_arg_name(arg) {
-        name.eq_ignore_ascii_case("profile") || name.eq_ignore_ascii_case("p")
-            || name.eq_ignore_ascii_case("profilemanager")
-    } else {
-        false
-    }
-}
-
 #[cfg(target_os = "linux")]
 pub mod platform {
     use path::find_binary;
     use std::path::PathBuf;
 
     /// Searches the system path for `firefox`.
     pub fn firefox_default_path() -> Option<PathBuf> {
         find_binary("firefox")
@@ -461,65 +418,8 @@ pub mod platform {
     pub fn firefox_default_path() -> Option<PathBuf> {
         None
     }
 
     pub fn arg_prefix_char(c: char) -> bool {
         c == '-'
     }
 }
-
-#[cfg(test)]
-mod tests {
-    use super::{is_profile_arg, parse_arg_name};
-
-    fn parse(arg: &str, name: Option<&str>) {
-        let result = parse_arg_name(arg);
-        assert_eq!(result, name.map(|x| x.to_string()));
-    }
-
-    #[test]
-    fn test_parse_arg_name() {
-        parse("-p", Some("p"));
-        parse("--p", Some("p"));
-        parse("--profile foo", Some("profile"));
-        parse("--profile", Some("profile"));
-        parse("--", None);
-        parse("", None);
-        parse("-=", None);
-        parse("--=", None);
-        parse("-- foo", None);
-        parse("foo", None);
-        parse("/ foo", None);
-        parse("/- foo", None);
-        parse("/=foo", None);
-        parse("foo", None);
-        parse("-profile", Some("profile"));
-        parse("-profile=foo", Some("profile"));
-        parse("-profile = foo", Some("profile"));
-        parse("-profile abc", Some("profile"));
-    }
-
-    #[cfg(target_os = "windows")]
-    #[test]
-    fn test_parse_arg_name_windows() {
-        parse("/profile", Some("profile"));
-    }
-
-    #[cfg(not(target_os = "windows"))]
-    #[test]
-    fn test_parse_arg_name_non_windows() {
-        parse("/profile", None);
-    }
-
-    #[test]
-    fn test_is_profile_arg() {
-        assert!(is_profile_arg("--profile"));
-        assert!(is_profile_arg("-p"));
-        assert!(is_profile_arg("-PROFILEMANAGER"));
-        assert!(is_profile_arg("-ProfileMANAGER"));
-        assert!(!is_profile_arg("-- profile"));
-        assert!(!is_profile_arg("-profiled"));
-        assert!(!is_profile_arg("-p1"));
-        assert!(is_profile_arg("-p test"));
-        assert!(is_profile_arg("-profile /foo"));
-    }
-}