Chrome extension to get console messages upon test failures. Unlike the

Chrome extension to get console messages upon test failures. Unlike the
prior one which used chrome.experimental.console APIs, which aren't accessible
to non-devtool extensions, this one uses remote debugging APIs, which are.
Committed: https://code.google.com/p/dart/source/detail?r=13442

[eub, 2012-10-09 00:17:48]

Hi, am I here for Python style?  We've been pretty permissive on that but here
are my opinions FWTW.

https://codereview.chromium.org/11092015/diff/2001/tools/testing/run_selenium.py
File tools/testing/run_selenium.py (right):

https://codereview.chromium.org/11092015/diff/2001/tools/testing/run_selenium...
tools/testing/run_selenium.py:146: 'ConsoleCollector.crx')
We're not consistent but I think we more often indent this to the open-paren.

https://codereview.chromium.org/11092015/diff/2001/tools/testing/run_selenium...
tools/testing/run_selenium.py:147: options =
selenium.webdriver.chrome.options.Options();
No need for semicolon here and following.

https://codereview.chromium.org/11092015/diff/2001/tools/testing/run_selenium...
tools/testing/run_selenium.py:158: 'ConsoleCollector.crx')
+ both of the above comments again, here and below.

[gram, 2012-10-09 00:19:57]

Adding Emily

[ahe, 2012-10-09 06:01:04]

Does the time out imply that tests will wait one second before completing?

https://codereview.chromium.org/11092015/diff/2002/pkg/unittest/test_controll...
File pkg/unittest/test_controller.js (right):

https://codereview.chromium.org/11092015/diff/2002/pkg/unittest/test_controll...
pkg/unittest/test_controller.js:20: var _callback;
Could you add a comment to explain what is going on here?

https://codereview.chromium.org/11092015/diff/2002/tools/testing/extensions/c...
File tools/testing/extensions/chrome/ConsoleCollector/background.js (right):

https://codereview.chromium.org/11092015/diff/2002/tools/testing/extensions/c...
tools/testing/extensions/chrome/ConsoleCollector/background.js:36: //    
protocol/1.0/console#type-ConsoleMessage
I don't think you need to wrap URIs.

[Emily Fortuna, 2012-10-09 18:15:21]

https://codereview.chromium.org/11092015/diff/2002/pkg/unittest/test_controll...
File pkg/unittest/test_controller.js (right):

https://codereview.chromium.org/11092015/diff/2002/pkg/unittest/test_controll...
pkg/unittest/test_controller.js:20: var _callback;
On 2012/10/09 06:01:04, ahe wrote:
> Could you add a comment to explain what is going on here?

+1

https://codereview.chromium.org/11092015/diff/2002/tools/testing/extensions/c...
File tools/testing/extensions/chrome/ConsoleCollector/background.js (right):

https://codereview.chromium.org/11092015/diff/2002/tools/testing/extensions/c...
tools/testing/extensions/chrome/ConsoleCollector/background.js:23:
callback(messages); // Send back the messages we obtained.
do we want to do the google style javascript commenting (closure-style)?

https://codereview.chromium.org/11092015/diff/2002/tools/testing/extensions/c...
tools/testing/extensions/chrome/ConsoleCollector/background.js:25:
chrome.debugger.sendCommand(debuggeeId, "Console.disable", {}, function() { });
80 char

https://codereview.chromium.org/11092015/diff/2002/tools/testing/extensions/c...
tools/testing/extensions/chrome/ConsoleCollector/background.js:73: 
nits: extra newlines at the bottoms of this file and others like content.js

https://codereview.chromium.org/11092015/diff/2002/tools/testing/run_selenium.py
File tools/testing/run_selenium.py (right):

https://codereview.chromium.org/11092015/diff/2002/tools/testing/run_selenium...
tools/testing/run_selenium.py:147: options =
selenium.webdriver.chrome.options.Options();
delete the semicolons through here

https://codereview.chromium.org/11092015/diff/2002/tools/testing/run_selenium...
tools/testing/run_selenium.py:160: options.add_extension(crx);
delete the semicolon...

[gram, 2012-10-09 18:57:14]

PTAL. I managed to eliminate the need for a timer by logging a sentinel message
to mark the end of the message list.

https://codereview.chromium.org/11092015/diff/2001/tools/testing/run_selenium.py
File tools/testing/run_selenium.py (right):

https://codereview.chromium.org/11092015/diff/2001/tools/testing/run_selenium...
tools/testing/run_selenium.py:146: 'ConsoleCollector.crx')
On 2012/10/09 00:17:48, eub wrote:
> We're not consistent but I think we more often indent this to the open-paren.

Done.

https://codereview.chromium.org/11092015/diff/2001/tools/testing/run_selenium...
tools/testing/run_selenium.py:147: options =
selenium.webdriver.chrome.options.Options();
On 2012/10/09 00:17:48, eub wrote:
> No need for semicolon here and following.

Done.

https://codereview.chromium.org/11092015/diff/2001/tools/testing/run_selenium...
tools/testing/run_selenium.py:158: 'ConsoleCollector.crx')
On 2012/10/09 00:17:48, eub wrote:
> + both of the above comments again, here and below.

Done.

https://codereview.chromium.org/11092015/diff/2002/pkg/unittest/test_controll...
File pkg/unittest/test_controller.js (right):

https://codereview.chromium.org/11092015/diff/2002/pkg/unittest/test_controll...
pkg/unittest/test_controller.js:20: var _callback;
On 2012/10/09 18:15:22, Emily Fortuna wrote:
> On 2012/10/09 06:01:04, ahe wrote:
> > Could you add a comment to explain what is going on here?
> 
> +1

Done.

https://codereview.chromium.org/11092015/diff/2002/tools/testing/extensions/c...
File tools/testing/extensions/chrome/ConsoleCollector/background.js (right):

https://codereview.chromium.org/11092015/diff/2002/tools/testing/extensions/c...
tools/testing/extensions/chrome/ConsoleCollector/background.js:25:
chrome.debugger.sendCommand(debuggeeId, "Console.disable", {}, function() { });
On 2012/10/09 18:15:22, Emily Fortuna wrote:
> 80 char

Done.

https://codereview.chromium.org/11092015/diff/2002/tools/testing/extensions/c...
tools/testing/extensions/chrome/ConsoleCollector/background.js:36: //    
protocol/1.0/console#type-ConsoleMessage
On 2012/10/09 06:01:04, ahe wrote:
> I don't think you need to wrap URIs.

Done.

https://codereview.chromium.org/11092015/diff/2002/tools/testing/extensions/c...
tools/testing/extensions/chrome/ConsoleCollector/background.js:73: 
On 2012/10/09 18:15:22, Emily Fortuna wrote:
> nits: extra newlines at the bottoms of this file and others like content.js

Done.

https://codereview.chromium.org/11092015/diff/2002/tools/testing/run_selenium.py
File tools/testing/run_selenium.py (right):

https://codereview.chromium.org/11092015/diff/2002/tools/testing/run_selenium...
tools/testing/run_selenium.py:147: options =
selenium.webdriver.chrome.options.Options();
On 2012/10/09 18:15:22, Emily Fortuna wrote:
> delete the semicolons through here

Done.

https://codereview.chromium.org/11092015/diff/2002/tools/testing/run_selenium...
tools/testing/run_selenium.py:160: options.add_extension(crx);
On 2012/10/09 18:15:22, Emily Fortuna wrote:
> delete the semicolon...

Done.

[Emily Fortuna, 2012-10-09 20:27:24]

lgtm with one more formatting adjustment.

https://codereview.chromium.org/11092015/diff/10003/tools/testing/extensions/...
File tools/testing/extensions/chrome/ConsoleCollector/content.js (right):

https://codereview.chromium.org/11092015/diff/10003/tools/testing/extensions/...
tools/testing/extensions/chrome/ConsoleCollector/content.js:5: // This is the
content page script. This runs in the context of the browser
nit: but in some files you use the /** */ style notation but in others just //
can we have this consistent through the files?