Add support for ie in the browser controller

tools/testing/dart/browser_controller.dart

Index: tools/testing/dart/browser_controller.dart
===================================================================
--- tools/testing/dart/browser_controller.dart	(revision 26047)
+++ tools/testing/dart/browser_controller.dart	(working copy)
@@ -54,13 +54,15 @@
       return new Chrome();
     } 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 =
-      const ['safari', 'ff', 'firefox', 'chrome'];
+    const ['safari', 'ff', 'firefox', 'chrome', 'ie9', 'ie10'];
 
   static const List<String> BROWSERS_WITH_WINDOW_SUPPORT =
       const ['safari', 'ff', 'firefox', 'chrome'];
@@ -144,7 +146,7 @@
           if (_cleanup != null) {
             _cleanup();
           }
-          doneCompleter.complete(exitCode);
+          doneCompleter.complete(exitCode == 0);

[kustermann, 2013/08/13 07:44:03]

Since we always kill the browser (sooner or later) exitCode will never be zero,
right?

[ricow1, 2013/08/14 08:13:51]

That is true, change and added error handling

         });
       });
       return true;
@@ -347,6 +349,44 @@
   String toString() => "Chrome";
 }
 
+class IE extends Browser {
+
+  static const String binary =
+      "c:\\Program Files\\Internet Explorer\\iexplore.exe";

[kustermann, 2013/08/13 07:44:03]

Is this the same path for IE9 and IE10?

[ricow1, 2013/08/14 08:13:51]

Yes this is the standard location, but as I said previously I will change all
browsers to accept a specific location

+
+  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) + findString.length;
+	if (index > 0) {
+	  return result.stdout.substring(index).trim();

[kustermann, 2013/08/13 07:44:03]

I indexOf(findString) is -1 and you add 6 it will be positive -- so this
condition will alsways be met if it matches or not.

You could just use a regexp here (it's not performance critical ...)

[ricow1, 2013/08/14 08:13:51]

Changed to not add the length until inside the if

+        }
+      }
+      return "Could not get the version of internet explorer";
+    });
+  }
+
+
+  Future<bool> start(String url) {
+    _logEvent("Starting ie browser on: $url");
+    // Get the version and log that.

[kustermann, 2013/08/13 07:44:03]

That comment is not really necessary, the code is self-explaining.

[ricow1, 2013/08/14 08:13:51]

Done.

+    return getVersion().then((version) {
+      _logEvent("Got version: $version");
+      return startBrowser(binary, [url]);
+    });
+  }
+  String toString() => "IE";
+}
+
+
 class AndroidChrome extends Browser {
   static const String viewAction = 'android.intent.action.VIEW';
   static const String mainAction = 'android.intent.action.MAIN';
@@ -609,7 +649,6 @@
 
   void handleResults(String browserId, String output, int testId) {
     var status = browserStatus[browserId];
-    DebugLogger.info("Handling result for browser ${browserId}");
     if (testCache.containsKey(testId)) {
       doubleReportingTests.add(testId);
       return;
@@ -631,12 +670,10 @@
         throw("This should never happen, wrong test id");
       }
       testCache[testId] = status.currentTest.url;
-      DebugLogger.info("Size of output for test $testId : ${output.length}");
       Stopwatch watch = new Stopwatch()..start();
       status.currentTest.doneCallback(output,
                                       status.currentTest.stopwatch.elapsed);
       watch.stop();
-      DebugLogger.info("Handling of test $testId took : ${watch.elapsed}");
       status.lastTest = status.currentTest;
       status.currentTest = null;
     } else {
@@ -652,7 +689,6 @@
   void handleTimeout(BrowserTestingStatus status) {
     // We simply kill the browser and starts up a new one!
     // We could be smarter here, but it does not seems like it is worth it.
-    DebugLogger.info("Handling timeout for browser ${status.browser.id}");
     status.timeout = true;
     timedOut.add(status.currentTest.url);
     var id = status.browser.id;
@@ -701,8 +737,6 @@
     if (testQueue.isEmpty) return null;
     var status = browserStatus[browserId];
     if (status == null) return null;
-    DebugLogger.info("Handling getNext for browser "
-                     "${browserId} timeout status: ${status.timeout}");
 
     // We are currently terminating this browser, don't start a new test.
     if (status.timeout) return null;
@@ -801,10 +835,13 @@
     return HttpServer.bind(local_ip, 0).then((createdServer) {
       httpServer = createdServer;
       void handler(HttpRequest request) {
-        DebugLogger.info("Handling request to: ${request.uri.path}");
+        // Don't allow caching of resources from the browser controller, i.e.,
+        // we don't want the browser to cache the result of getNextTest.
+        request.response.headers.set("Cache-Control",
+                                     "no-cache, no-store, must-revalidate");
         if (request.uri.path.startsWith(reportPath)) {
           var browserId = request.uri.path.substring(reportPath.length + 1);
-          var testId = 
+          var testId =
               int.parse(request.uri.queryParameters["id"].split("=")[1]);
           handleReport(request, browserId, testId);
           // handleReport will asynchroniously fetch the data and will handle
@@ -819,14 +856,11 @@
           var browserId = request.uri.path.substring(nextTestPath.length + 1);
           textResponse = getNextTest(browserId);
         } else {
-          DebugLogger.info("Handling non standard request to: "
-                           "${request.uri.path}");
+          // /favicon.ico requests
         }
         request.response.write(textResponse);
         request.listen((_) {}, onDone: request.response.close);
-        request.response.done.then((_) {
-          DebugLogger.info("Done handling request to: ${request.uri.path}");
-        }).catchError((error) {
+        request.response.done.catchError((error) {
           if (!underTermination) {
             print("URI ${request.uri}");
             print("Textresponse $textResponse");
@@ -851,6 +885,9 @@
           }, onDone: () {
               String back = buffer.toString();
               request.response.headers.set("Access-Control-Allow-Origin", "*");
+              request.response.headers.set(
+                  "Cache-Control",
+                  "no-cache, no-store, must-revalidate");

[kustermann, 2013/08/13 07:44:03]

Why do you have this. We never GET anything from the error reporting server
AFAIK (we only POST). Why do you need to set the caching headers here?

[ricow1, 2013/08/14 08:13:51]

Removed here

 
               request.response.done.catchError((error) {
                 DebugLogger.error("Error getting error from browser"
@@ -876,7 +913,6 @@
         String back = buffer.toString();
         request.response.close();
         testDoneCallBack(browserId, back, testId);
-        DebugLogger.info("Done handling request to: ${request.uri.path}");
       }, onError: (error) { print(error); });
   }
 
@@ -917,11 +953,11 @@
     function startTesting() {
       var number_of_tests = 0;
       var current_id;
+      // Describes a state where we are currently fetching the next test
+      // from the server. We use this to never double request tasks.
+      var FETCHING_NEXT_TEST = -1;
       var last_reported_id;
       var testing_window;
-      // We use this to determine if we did actually get back a start event
-      // from the test we just loaded.
-      var did_start = false;
 
       var embedded_iframe = document.getElementById('embedded_iframe');
       var use_iframe = ${useIframe};
@@ -943,8 +979,6 @@
               var split = this.responseText.split('#');

[kustermann, 2013/08/13 07:44:03]

We should probably reverse the entries. The URL could contain '#' after all:
ID#URL

[ricow1, 2013/08/14 08:13:51]

I will not do that in this cl

               var nextTask = split[0];
               current_id = split[1];

[kustermann, 2013/08/13 07:44:03]

I think this is not right. After setting "current_id = split[1]" we assume that
the current task is already running. What if we've a pending "dom-post" message
in the event queue?
Then we'll report it as the result of 'current_id', right?

[ricow1, 2013/08/14 08:13:51]

Changed

[ricow1, 2013/08/14 08:13:51]

Changed

-              reportError('Done getting task : ' + elapsed);
-              did_start = false;
               run(nextTask);
             }
           } else {
@@ -954,8 +988,9 @@
       }
 
       function getNextTask() {
-        var elapsed = new Date() - start;
-        reportError('Getting task at: ' + elapsed);
+        // Until we have the next task we set the current_id to a specific
+        // negative value.
+        current_id = FETCHING_NEXT_TEST;
         var client = new XMLHttpRequest();
         client.onreadystatechange = newTaskHandler;
         client.open('GET', '$nextTestPath/$browserId');
@@ -1000,37 +1035,31 @@
       }
 
       function reportMessage(msg) {
-        if (msg == 'STARTING') {
-          did_start = true;
-          return;
-        }
-        var client = new XMLHttpRequest();
+        // We define this here to capture the value of the posting id.
+        // That way we can make sure to never ask for a new task if the
+        // reported id is not the currently running id. If we just use
+        // current_id and last_reported_id, current_id may have been changed
+        // by the next test.
+        var posting_id =
+            current_id == FETCHING_NEXT_TEST ? last_reported_id : current_id;
         function handleReady() {
           if (this.readyState == this.DONE) {
             if (this.status == 200) {
-              if (last_reported_id != current_id && did_start) {
-                var elapsed = new Date() - start;
-                reportError('Done sending results at: ' + elapsed);
+              if (posting_id == current_id) {
+	        last_reported_id = current_id;
                 getNextTask();
-                last_reported_id = current_id;
               }
             } else {
               reportError('Error sending result to server');
             }
           }
         }
+        var client = new XMLHttpRequest();
         client.onreadystatechange = handleReady;
-        // If did_start is false it means that we did actually set the url on
-        // the testing_window, but this is a report left in the event loop or
-        // a callback because the page did not load yet.
-        // In both cases this is a double report from the last test.
-        var posting_id = did_start ? current_id : last_reported_id;
         client.open('POST', '$reportPath/${browserId}?id=' + posting_id);
         client.setRequestHeader('Content-type',
                                 'application/x-www-form-urlencoded');
         client.send(msg);
-        var elapsed = new Date() - start;
-        reportError('Sending results at: ' + elapsed);
       }
 
       function messageHandler(e) {