Add browser path options to testing scripts.

tools/testing/dart/browser_controller.dart

Index: tools/testing/dart/browser_controller.dart
diff --git a/tools/testing/dart/browser_controller.dart b/tools/testing/dart/browser_controller.dart
index e74cc2c19a1e16c31a08ac995645e7e6e1105521..f8d050e47c6eb9d1bd60b3bf6abdb2ba24ec1a6f 100644
--- a/tools/testing/dart/browser_controller.dart
+++ b/tools/testing/dart/browser_controller.dart
@@ -31,6 +31,18 @@ abstract class Browser {
 
   /** The version of the browser - normally set when starting a browser */
   String version = "";
+
+  String _binary;
+
+  setBinary(String browserName, Map globalConfiguration) {
+    _binary = globalConfiguration[browserName];
+    if (_binary == null || _binary == '') {
+      _binary = _getBinary();
+    }
+  }
+
+  String _getBinary();

[kustermann, 2013/11/06 09:33:01]

In effect, this method is used for getting the default path to the browser.

Why can't we just do that in test_options.dart ?
We have default values for other options as well.
This would eliminate all _Binary() methods.

+
   /**
    * The underlying process - don't mess directly with this if you don't
    * know what you are doing (this is an interactive process that needs
@@ -55,14 +67,13 @@ abstract class Browser {
   Browser();
 
   factory Browser.byName(String name,
-                         [Map globalConfiguration = const {},
-                          bool checkedMode = false]) {
-    if (name == 'ff' || name == 'firefox') {
+                         [bool checkedMode = false]) {
+    if (name == 'firefox') {
       return new Firefox();
     } else if (name == 'chrome') {
       return new Chrome();
     } else if (name == 'dartium') {
-      return new Dartium(globalConfiguration, checkedMode);
+      return new Dartium(checkedMode);
     } else if (name == 'safari') {
       return new Safari();
     } else if (name.startsWith('ie')) {
@@ -193,11 +204,13 @@ abstract class Browser {
 }
 
 class Safari extends Browser {
-  /**
-   * The binary used to run safari - changing this can be nececcary for
-   * testing or using non standard safari installation.
-   */
-  static const String binary = "/Applications/Safari.app/Contents/MacOS/Safari";
+  String _getBinary() {
+    if (Platform.isMacOS) {
+      return "/Applications/Safari.app/Contents/MacOS/Safari";
+    } else {
+      throw "Safari browser not supported on ${Platform.operatingSystem}";
+    }
+  }
 
   /**
    * We get the safari version by parsing a version file
@@ -318,10 +331,10 @@ class Safari extends Browser {
             _cleanup = () { userDir.deleteSync(recursive: true); };
             _createLaunchHTML(userDir.path, url);
             var args = ["${userDir.path}/launch.html"];
-            return startBrowser(binary, args);
+            return startBrowser(_binary, args);
           });
         }).catchError((error) {
-          _logEvent("Running $binary --version failed with $error");
+          _logEvent("Running $_binary --version failed with $error");
           return false;
         });
       });
@@ -338,21 +351,18 @@ class Safari extends Browser {
 
 
 class Chrome extends Browser {
-  String _binary;
   String _version = "Version not found yet";
 
-  Chrome() {
-    _binary = _getBinary();
-  }
-
   String _getBinary() {
     if (Platform.isWindows) {
       return "C:\\Program Files (x86)\\Google\\Chrome\\Application\\chrome.exe";
     } else if (Platform.isMacOS) {
       return "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome";
+    } else if (Platform.isLinux) {
+      return 'google-chrome';
+    } else {
+      throw "Chrome is not supported on ${Platform.operatingSystem}";
     }
-    assert(Platform.isLinux);
-    return 'google-chrome';
   }
 
   Map<String, String> _getEnvironment() => null;
@@ -410,13 +420,16 @@ class Chrome extends Browser {
 }
 
 class Dartium extends Chrome {
-  final Map globalConfiguration;
   final bool checkedMode;
 
-  Dartium(this.globalConfiguration, this.checkedMode);
+  Dartium(this.checkedMode);
 
   String _getBinary() {
-    return Locations.getDartiumLocation(globalConfiguration);
+    if (Platform.operatingSystem == 'macos') {
+      return 'client/tests/dartium/Chromium.app/Contents/MacOS/Chromium';
+    } else {
+      return new Uri.file('client/tests/dartium/chrome').toFilePath();
+    }
   }
 
   Map<String, String> _getEnvironment() {
@@ -435,9 +448,7 @@ class Dartium extends Chrome {
 }
 
 class IE extends Browser {
-
-  static const String binary =
-      "c:\\Program Files\\Internet Explorer\\iexplore.exe";
+  String _getBinary() => "C:\\Program Files\\Internet Explorer\\iexplore.exe";
 
   Future<String> getVersion() {
     var args = ["query",
@@ -463,7 +474,7 @@ class IE extends Browser {
     _logEvent("Starting ie browser on: $url");
     return getVersion().then((version) {
       _logEvent("Got version: $version");
-      return startBrowser(binary, [url]);
+      return startBrowser(_binary, [url]);
     });
   }
   String toString() => "IE";
@@ -610,8 +621,6 @@ class Firefox extends Browser {
   static const String disableScriptTimeLimit =
       'user_pref("dom.max_script_run_time", 0);';
 
-  static String _binary = _getBinary();
-
   Future _createPreferenceFile(var path) {
     var file = new File("${path.toString()}/user.js");
     var randomFile = file.openSync(mode: FileMode.WRITE);
@@ -621,15 +630,16 @@ class Firefox extends Browser {
     randomFile.close();
   }
 
-  // This is extracted to a function since we may need to support several
-  // locations.
-  static String _getWindowsBinary() {
-    return "C:\\Program Files (x86)\\Mozilla Firefox\\firefox.exe";
-  }
-
-  static String _getBinary() {
-    if (Platform.isWindows) return _getWindowsBinary();
-    if (Platform.isLinux) return 'firefox';
+  String _getBinary() {
+    if (Platform.isWindows) {
+      return "C:\\Program Files (x86)\\Mozilla Firefox\\firefox.exe";
+    } else if (Platform.isLinux) {
+      return 'firefox';
+    } else if (Platform.isWindows) {
+      throw 'No path to firefox browser supplied on Windows';
+    } else {
+      throw 'Firefox not supported on ${Platform.operatingSystem}';
+    }
   }
 
   Future<bool> start(String url) {
@@ -1036,8 +1046,9 @@ class BrowserTestRunner {
   }
 
   Browser getInstance() {
-    var browser =
-        new Browser.byName(browserName, globalConfiguration, checkedMode);
+    if (browserName == 'ff') browserName = 'firefox';
+    var browser = new Browser.byName(browserName, checkedMode);
+    browser.setBinary(browserName, globalConfiguration);

[kustermann, 2013/11/06 09:33:01]

There's one thing I don't like about this:
If we create a new browser instance we always have to remember calling
'browser.SetBinary()' afterwards.

If we moved the default path of browsers to test_options.dart we can get rid of
this globalConfiguration parameter and just pass the executable to the
constructor:

"var browser = new Browser.byName(browserName,
globalConfiguration[browserName])"

     browser.logger = logger;
     return browser;
   }