Add browser path options to testing scripts.

tools/testing/dart/browser_controller.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 library browser;
 
 import "dart:async";
 import "dart:convert" show LineSplitter, UTF8;
 import "dart:core";
 import "dart:io";
 
@@ skipping 13 matching lines @@
 
   // This is called after the process is closed, before the done future
   // is completed.
   // Subclasses can use this to cleanup any browser specific resources
   // (temp directories, profiles, etc). The function is expected to do
   // it's work synchronously.
   Function _cleanup;
 
   /** 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
    * special threatment to not leak).
    */
   Process process;
 
   Function logger;
 
   /**
    * Id of the browser
    */
   String id;
 
   /** Print everything (stdout, stderr, usageLog) whenever we add to it */
   bool debugPrint = false;
 
   // This future returns when the process exits. It is also the return value
   // of close()
   Future done;
 
   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')) {
       return new IE();
     } else {
       throw "Non supported browser";
     }
   }
 
   static const List<String> SUPPORTED_BROWSERS =
@@ skipping 110 matching lines @@
     _testBrowserOutput = new BrowserOutput();
   }
 
   String toString();
 
   /** Starts the browser loading the given url */
   Future<bool> start(String url);
 }
 
 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
    */
   static const String versionFile =
       "/Applications/Safari.app/Contents/version.plist";
 
   /**
    * Directories where safari stores state. We delete these if the deleteCache
    * is set
@@ skipping 100 matching lines @@
           _logEvent("Could not clear cache");
           return false;
         }
         // Get the version and log that.
         return getVersion().then((version) {
           _logEvent("Got version: $version");
           return new Directory('').createTemp().then((userDir) {
             _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;
         });
       });
     });
   }
 
   String toString() => "Safari";
 
   // Delete the user specific browser cache and profile data.
   // Safari only have one per user, and you can't specify one by command line.
   static bool deleteCache = false;
 
 }
 
 
 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;
 
   Future<bool> _getVersion() {
     if (Platform.isWindows) {
       // The version flag does not work on windows.
       // See issue:
       // https://code.google.com/p/chromium/issues/detail?id=158372
       // The registry hack does not seem to work.
@@ skipping 37 matching lines @@
     }).catchError((e) {
       _logEvent("Running $_binary --version failed with $e");
       return false;
     });
   }
 
   String toString() => "Chrome";
 }
 
 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() {
     var environment = new Map<String,String>.from(Platform.environment);
     // By setting this environment variable, dartium will forward "print()"
     // calls in dart to the top-level javascript function "dartPrint()" if
     // available.
     environment['DART_FORWARDING_PRINT'] = '1';
     if (checkedMode) {
       environment['DART_FLAGS'] = '--checked';
     }
     return environment;
   }
 
   String toString() => "Dartium";
 }
 
 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",
                 "HKEY_LOCAL_MACHINE\\Software\\Microsoft\\Internet Explorer",
                 "/v",
                 "version"];
     return Process.run("reg", args).then((result) {
       if (result.exitCode == 0) {
         // The string we get back looks like this:
         // HKEY_LOCAL_MACHINE\Software\Microsoft\Internet Explorer
         //    version    REG_SZ    9.0.8112.16421
         var findString = "REG_SZ";
         var index = result.stdout.indexOf(findString);
         if (index > 0) {
           return result.stdout.substring(index + findString.length).trim();
         }
       }
       return "Could not get the version of internet explorer";
     });
   }
 
   Future<bool> start(String url) {
     _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";
 }
 
 
 class AndroidBrowserConfig {
   final String name;
   final String package;
   final String activity;
@@ skipping 126 matching lines @@
 
 
 class Firefox extends Browser {
   static const String enablePopUp =
       'user_pref("dom.disable_open_during_load", false);';
   static const String disableDefaultCheck =
       'user_pref("browser.shell.checkDefaultBrowser", false);';
   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);
     randomFile.writeStringSync(enablePopUp);
     randomFile.writeStringSync(disableDefaultCheck);
     randomFile.writeStringSync(disableScriptTimeLimit);
     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) {
     _logEvent("Starting firefox browser on: $url");
     // Get the version and log that.
     return Process.run(_binary, ["--version"]).then((var versionResult) {
       if (versionResult.exitCode != 0) {
         _logEvent("Failed to firefox get version");
         _logEvent("Make sure $_binary is a valid program for running firefox");
         return new Future.value(false);
@@ skipping 386 matching lines @@
     }
     return Future.wait(futures).then((values) {
       testingServer.httpServer.close();
       testingServer.errorReportingServer.close();
       printDoubleReportingTests();
       return !values.contains(false);
     });
   }
 
   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;
   }
 }
 
 class BrowserTestingServer {
   final Map globalConfiguration;
   /// Interface of the testing server:
   ///
   /// GET /driver/BROWSER_ID -- This will get the driver page to fetch
@@ skipping 308 matching lines @@
     Dart test driver, number of tests: <div id="number"></div><br>
     Currently executing: <div id="currently_executing"></div><br>
     Unhandled error: <div id="unhandled_error"></div>
     <iframe id="embedded_iframe"></iframe>
   </body>
 </html>
 """;
     return driverContent;
   }
 }