Add support for ie in the browser controller

Add support for ie in the browser controller

Also, add no caching headers (since IE will cache the task that it gets)

R=kustermann@google.com

Committed: https://code.google.com/p/dart/source/detail?r=26112

[ricow1, 2013-08-06 13:27:22]

https://codereview.chromium.org/22420002/diff/1/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/22420002/diff/1/tools/test.dart#newcode155
tools/test.dart:155: if (conf['runtime'].startsWith('ie') &&
!conf["use_browser_controller"]) {
I don't see any issues with multiple simultaneously running ie processes in the
browser controller

[kustermann, 2013-08-06 13:58:45]

lgtm

https://codereview.chromium.org/22420002/diff/2003/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/22420002/diff/2003/tools/test.dart#newcode156
tools/test.dart:156: maxBrowserProcesses = 1;
Does that mean we can now use multiple versions of IE in parallel?
Hurah :)

https://codereview.chromium.org/22420002/diff/2003/tools/testing/dart/browser...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/22420002/diff/2003/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:69: 
Please add 'ie9' and/or 'ie10' to the list depending on what works.

https://codereview.chromium.org/22420002/diff/2003/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:355: "c:\\Program Files\\Internet
Explorer\\iexplore.exe";
Is that path the same on our bots?

https://codereview.chromium.org/22420002/diff/2003/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:368: var index =
result.stdout.indexOf("REG_SZ");
use 'findString' instead of "REG_SZ"

https://codereview.chromium.org/22420002/diff/2003/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:867:
request.response.headers.set("Pragma", "no-cache");
Why do we need Pragma,Expires in addition to Cache-Control?

[ricow1, 2013-08-13 06:42:43]

PTAL
I changed this a bit:
  Since there was a race condition in the java script runner, which was actually
materializing due to our retry functionality, I have changed the logic to not
rely on callbacks from the test controller. This means, that as soon as we have
called getNextTask we will set the current id to a negative value. By doing this
we will always just double report when an extra report comes in, but will never
get into synchronization issues with the task management. Basically what we
expect now is:
  A test may report results 0 (timeout), 1 (correct), >1 (double reporting)
  Fetching of the next test is independent of this and will happen exactly when
1 report has been send to the server (the single threaded nature of javascript
guarantees this)
  In case of a timeout the javascript runner has no notion of this, we still
just kill the browser.

Also, I removed most of our debug logging

https://codereview.chromium.org/22420002/diff/2003/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/22420002/diff/2003/tools/test.dart#newcode156
tools/test.dart:156: maxBrowserProcesses = 1;
On 2013/08/06 13:58:45, kustermann wrote:
> Does that mean we can now use multiple versions of IE in parallel?
> Hurah :)
Yes :-)

https://codereview.chromium.org/22420002/diff/2003/tools/testing/dart/browser...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/22420002/diff/2003/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:69: 
On 2013/08/06 13:58:45, kustermann wrote:
> Please add 'ie9' and/or 'ie10' to the list depending on what works.

Done.

https://codereview.chromium.org/22420002/diff/2003/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:355: "c:\\Program Files\\Internet
Explorer\\iexplore.exe";
On 2013/08/06 13:58:45, kustermann wrote:
> Is that path the same on our bots?
Yes, and filed: dartbug.com/12409

https://codereview.chromium.org/22420002/diff/2003/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:368: var index =
result.stdout.indexOf("REG_SZ");
On 2013/08/06 13:58:45, kustermann wrote:
> use 'findString' instead of "REG_SZ"

Done.

https://codereview.chromium.org/22420002/diff/2003/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:867:
request.response.headers.set("Pragma", "no-cache");
On 2013/08/06 13:58:45, kustermann wrote:
> Why do we need Pragma,Expires in addition to Cache-Control?

That is only if we want to support 1.0 clients. I don't expect us to do that so
I removed the headers again

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

+1 for removing most of the debugging info.

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:149: doneCompleter.complete(exitCode
== 0);
Since we always kill the browser (sooner or later) exitCode will never be zero,
right?

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:355: "c:\\Program Files\\Internet
Explorer\\iexplore.exe";
Is this the same path for IE9 and IE10?

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:370: return
result.stdout.substring(index).trim();
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 ...)

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:380: // Get the version and log that.
That comment is not really necessary, the code is self-explaining.

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:890: "no-cache, no-store,
must-revalidate");
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?

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:976: if (nextTask != undefined)
alert('This is really bad');
This can be removed, right? The 'nextTask' variable is defined only once: three
lines below. So it will always be undefined.

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:979: var split =
this.responseText.split('#');
We should probably reverse the entries. The URL could contain '#' after all:
ID#URL

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:981: current_id = split[1];
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]

As discussed offline we actually do need the explicit start message (to not
wrongly pass back double reports using the next test id).

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:149: doneCompleter.complete(exitCode
== 0);
On 2013/08/13 07:44:03, kustermann wrote:
> Since we always kill the browser (sooner or later) exitCode will never be
zero,
> right?
That is true, change and added error handling

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:355: "c:\\Program Files\\Internet
Explorer\\iexplore.exe";
On 2013/08/13 07:44:03, kustermann wrote:
> Is this the same path for IE9 and IE10?

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

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:370: return
result.stdout.substring(index).trim();
On 2013/08/13 07:44:03, kustermann wrote:
> 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 ...)

Changed to not add the length until inside the if

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:380: // Get the version and log that.
On 2013/08/13 07:44:03, kustermann wrote:
> That comment is not really necessary, the code is self-explaining.

Done.

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:890: "no-cache, no-store,
must-revalidate");
On 2013/08/13 07:44:03, kustermann wrote:
> 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?
Removed here

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:976: if (nextTask != undefined)
alert('This is really bad');
On 2013/08/13 07:44:03, kustermann wrote:
> This can be removed, right? The 'nextTask' variable is defined only once:
three
> lines below. So it will always be undefined.

Yes that is a left over artifact, removed

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:979: var split =
this.responseText.split('#');
On 2013/08/13 07:44:03, kustermann wrote:
> We should probably reverse the entries. The URL could contain '#' after all:
> ID#URL

I will not do that in this cl

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:981: current_id = split[1];
On 2013/08/13 07:44:03, kustermann wrote:
> 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?
Changed

https://codereview.chromium.org/22420002/diff/17001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:981: current_id = split[1];
On 2013/08/13 07:44:03, kustermann wrote:
> 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?

Changed

[kustermann, 2013-08-14 11:06:16]

LGTM with two suggestions

https://codereview.chromium.org/22420002/diff/22001/tools/testing/dart/browse...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/22420002/diff/22001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:357: "c:\\Program Files\\Internet
Explorer\\iexplore.exe";
Check if the path is the same for ie9/ie10.

https://codereview.chromium.org/22420002/diff/22001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:378: 
extra line

https://codereview.chromium.org/22420002/diff/22001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:956: var instantiating_next_test =
false;
I'd rename this to 'var old_test_finished', set it to false when we get
'STARTING' and set it to true after we've received the DOM the first time.

https://codereview.chromium.org/22420002/diff/22001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:1044: instantiating_next_test ?
last_reported_id : current_id;
I'd add a variable here (so it's captured by the closure):
var isDoubleReport = ...;

and 5 lines down, I'd use that variable int he condition whether we call
'getNextTask()' or not.

[ricow1, 2013-08-14 11:29:48]

https://codereview.chromium.org/22420002/diff/22001/tools/testing/dart/browse...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/22420002/diff/22001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:357: "c:\\Program Files\\Internet
Explorer\\iexplore.exe";
On 2013/08/14 11:06:16, kustermann wrote:
> Check if the path is the same for ie9/ie10.

It is

https://codereview.chromium.org/22420002/diff/22001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:378: 
On 2013/08/14 11:06:16, kustermann wrote:
> extra line

done

https://codereview.chromium.org/22420002/diff/22001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:1044: instantiating_next_test ?
last_reported_id : current_id;
On 2013/08/14 11:06:16, kustermann wrote:
> I'd add a variable here (so it's captured by the closure):
> var isDoubleReport = ...;
> 
> and 5 lines down, I'd use that variable int he condition whether we call
> 'getNextTask()' or not.

Changed it _again_ :-)

[kustermann, 2013-08-14 11:30:20]

Awesome LGTM this time.

[ricow1, 2013-08-14 11:30:55]

Committed patchset #9 manually as r26112 (presubmit successful).