Bug 1466573 - Convert Firefox args matching to predicate. r?whimboo,jgraham draft
authorAndreas Tolfsen <ato@sny.no>
Tue, 05 Jun 2018 18:02:34 +0100
changeset 804257 ff0d995f4f6cb6e50a4b7eb088ede1176098c660
parent 804256 c7f4d1e9c695dd65f66cd8e69927cf4b6206f338
child 804258 64a41281bce3c781d70657ef5344f43f2399e65e
push id112328
push userbmo:ato@sny.no
push dateTue, 05 Jun 2018 17:17:40 +0000
reviewerswhimboo, jgraham
bugs1466573
milestone62.0a1
Bug 1466573 - Convert Firefox args matching to predicate. r?whimboo,jgraham In order to support matching other arguments than profile-related arguments without re-parsing the argument string for each match, this patch generalises is_profile_arg() so that the parse-unwrapping code can be reused. MozReview-Commit-ID: Faclkzv2l99
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,157 @@
+//! 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;
+
+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 == '='
+}
+
+/// Determines if an argument's basename, that is the `profile` part of
+/// `--profile`, matches the predicate.
+pub fn basename_match<T, F>(arg_str: T, predicate: F) -> bool
+where
+    T: AsRef<OsStr>,
+    F: Fn(&str) -> bool,
+{
+    if let Some(basename) = parse_arg_name(arg_str) {
+        predicate(&basename)
+    } else {
+        false
+    }
+}
+
+pub mod predicates {
+    /// Check if an argument string affects the Firefox profile.
+    ///
+    /// Returns a boolean indicating whether the given argument string contains
+    /// one of the `P`, `Profile` or `-ProfileManager` arguments,
+    /// respecting the various platform specific conventions.
+    pub fn profile(x: &str) -> bool {
+        x.eq_ignore_ascii_case("profile") || x.eq_ignore_ascii_case("p")
+            || x.eq_ignore_ascii_case("profilemanager")
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::{basename_match, parse_arg_name, predicates};
+
+    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_basename_match() {
+        assert!(basename_match("-foo", |_| true));
+        assert!(!basename_match("-foo", |_| false));
+
+        assert!(basename_match("--profile", predicates::profile));
+        assert!(basename_match("-p", predicates::profile));
+        assert!(basename_match("-PROFILEMANAGER", predicates::profile));
+        assert!(basename_match("-ProfileMANAGER", predicates::profile));
+        assert!(!basename_match("-- profile", predicates::profile));
+        assert!(!basename_match("-profield", predicates::profile));
+        assert!(!basename_match("-p1", predicates::profile));
+        assert!(basename_match("-p test", predicates::profile));
+        assert!(basename_match("-profile /foo", predicates::profile));
+    }
+
+    #[test]
+    fn test_predicates_profile() {
+        assert!(predicates::profile("profile"));
+        assert!(predicates::profile("p"));
+        assert!(predicates::profile("profilemanager"));
+        assert!(predicates::profile("pROFiLe"));
+        assert!(predicates::profile("P"));
+        assert!(predicates::profile("PROFILEMANAGER"));
+        assert!(!predicates::profile("p1"));
+    }
+}
--- a/testing/mozbase/rust/mozrunner/src/lib.rs
+++ b/testing/mozbase/rust/mozrunner/src/lib.rs
@@ -1,8 +1,9 @@
 #[macro_use] extern crate log;
 extern crate mozprofile;
 #[cfg(target_os = "windows")]
 extern crate winreg;
 
+pub mod firefox_args;
 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
@@ -9,16 +9,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::{basename_match, predicates};
+
 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
@@ -258,90 +260,29 @@ 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)) {
+        if !self.args.iter().any(|arg| basename_match(arg, predicates::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
-    }
-}
 
 fn find_binary(name: &str) -> Option<PathBuf> {
     env::var("PATH").ok().and_then(|path_env| {
         for mut path in env::split_paths(&*path_env) {
             path.push(name);
             if path.exists() {
                 return Some(path);
             }
@@ -461,65 +402,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"));
-    }
-}