Move TestRunner from CppVariable to gin::Wrappable

Move TestRunner from CppVariable to gin::Wrappable

BUG=297480, 331301
TEST=all layout tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257079

[hajimehoshi, 2014-03-06 10:33:53]

PTAL

The current situation:

Out of 31033 tests, 
Expected to timeout, but passed: (1)
Expected to fail, but passed: (4)
Unexpected flakiness: image and text failures (2)
Unexpected flakiness: audio failures (3)
Unexpected flakiness: crashes (7)
Unexpected flakiness: timeouts (14)
Unexpected flakiness: text-only failures (75)

[kouhei (in TOK), 2014-03-06 11:49:36]

https://codereview.chromium.org/185263006/diff/100001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_runner.h (right):

https://codereview.chromium.org/185263006/diff/100001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.h:686:
scoped_ptr<::WebTestRunner::WebPermissions> web_permissions_;
scoped_ptr< ::WebT
^^^^^you need a space

[hajimehoshi, 2014-03-06 13:01:10]

https://codereview.chromium.org/185263006/diff/100001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_runner.h (right):

https://codereview.chromium.org/185263006/diff/100001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.h:686:
scoped_ptr<::WebTestRunner::WebPermissions> web_permissions_;
On 2014/03/06 11:49:36, kouhei wrote:
> scoped_ptr< ::WebT
> ^^^^^you need a space 

Out of curiosity, I'd like to know the reason.

[hajimehoshi, 2014-03-06 13:49:46]

https://codereview.chromium.org/185263006/diff/100001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_runner.h (right):

https://codereview.chromium.org/185263006/diff/100001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.h:686:
scoped_ptr<::WebTestRunner::WebPermissions> web_permissions_;
On 2014/03/06 13:01:11, hajimehoshi wrote:
> On 2014/03/06 11:49:36, kouhei wrote:
> > scoped_ptr< ::WebT
> > ^^^^^you need a space 
> 
> Out of curiosity, I'd like to know the reason.

I understood it by reading the compile errors. Thanks.

[hajimehoshi, 2014-03-07 10:42:09]

Remaining failures in all layout tests on my local machine:

http/tests/misc/execute-and-return-value.html failed unexpectedly (renderer
crashed [pid=27283])
editing/surrounding-text/surrounding-text.html failed unexpectedly (text diff)  
                                        
fast/dom/Window/property-access-on-cached-properties-after-frame-removed.html
failed unexpectedly (text diff)
fast/dom/Window/property-access-on-cached-window-after-frame-removed.html failed
unexpectedly (text diff)
fast/dom/Window/property-access-on-cached-properties-after-frame-navigated.html
failed unexpectedly (text diff)
fast/dom/Window/property-access-on-cached-window-after-frame-navigated.html
failed unexpectedly (text diff)
fast/dom/custom/isolated-world.html failed unexpectedly (renderer crashed
[pid=27445])
fast/js/JSON-stringify.html failed unexpectedly (text diff)
svg/W3C-SVG-1.1/animate-elem-41-t.svg failed unexpectedly (image diff)          
    
svg/animations/reinserting-svg-into-document.html failed unexpectedly (text
diff)

BTW, I wonder why the applying this patch fails on the build bots.

[hajimehoshi, 2014-03-10 05:31:02]

Now almost all failures are fixed. The remaining failures
(fast/dom/Window/property-access-on-cached-*) were expected and they required
rebaselines.

[kouhei (in TOK), 2014-03-11 02:18:13]

LGTM with nits.

https://codereview.chromium.org/185263006/diff/200001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_runner.cc (right):

https://codereview.chromium.org/185263006/diff/200001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:604: if (runner_) {
Nit: Use early return.

https://codereview.chromium.org/185263006/diff/200001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:650: if (runner_) {
Ditto

[hajimehoshi, 2014-03-11 03:33:17]

Thanks!

https://codereview.chromium.org/185263006/diff/200001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_runner.cc (right):

https://codereview.chromium.org/185263006/diff/200001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:604: if (runner_) {
On 2014/03/11 02:18:14, kouhei wrote:
> Nit: Use early return.

Done.

https://codereview.chromium.org/185263006/diff/200001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:650: if (runner_) {
On 2014/03/11 02:18:14, kouhei wrote:
> Ditto

Done.

[hajimehoshi, 2014-03-11 03:33:17]

Thanks!

https://codereview.chromium.org/185263006/diff/200001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_runner.cc (right):

https://codereview.chromium.org/185263006/diff/200001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:604: if (runner_) {
On 2014/03/11 02:18:14, kouhei wrote:
> Nit: Use early return.

Done.

https://codereview.chromium.org/185263006/diff/200001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:650: if (runner_) {
On 2014/03/11 02:18:14, kouhei wrote:
> Ditto

Done.

[jochen (gone - plz use gerrit), 2014-03-11 14:36:38]

any idea about the failing inspector layout test?

otherwise looks great!

[hajimehoshi, 2014-03-12 05:14:21]

On 2014/03/11 14:36:38, jochen wrote:
> any idea about the failing inspector layout test?
> 
> otherwise looks great!

Thank you! Apparently, the failing inspector layout test is caused by r168885,
and it was reverted by r168911.

[hajimehoshi, 2014-03-12 05:15:04]

On 2014/03/12 05:14:21, hajimehoshi wrote:
> On 2014/03/11 14:36:38, jochen wrote:
> > any idea about the failing inspector layout test?
> > 
> > otherwise looks great!
> 
> Thank you! Apparently, the failing inspector layout test is caused by r168885,
> and it was reverted by r168911.

I meant they were the revisions of blink.
http://src.chromium.org/viewvc/blink?revision=168911&view=revision

[jochen (gone - plz use gerrit), 2014-03-12 12:38:25]

https://codereview.chromium.org/185263006/diff/280001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_runner.cc (right):

https://codereview.chromium.org/185263006/diff/280001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:800:
args->GetNext(&has_acceleration_x);
why not just increase the number of supported arguments in gin?

https://codereview.chromium.org/185263006/diff/280001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:1179: bool permission_granted
= true;
i think this is never invoked with a second argument

https://codereview.chromium.org/185263006/diff/280001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:1483:
global_flag_.Reset(isolate, v8::Boolean::New(isolate, false));
i think global flag is really just a bool, so you don't need to store a
persistent

[jochen (gone - plz use gerrit), 2014-03-12 19:27:00]

btw, there's a change to testrunner in the CQ right now:
https://codereview.chromium.org/184313005/

[hajimehoshi, 2014-03-13 04:53:07]

Thank you!

https://codereview.chromium.org/185263006/diff/280001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_runner.cc (right):

https://codereview.chromium.org/185263006/diff/280001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:800:
args->GetNext(&has_acceleration_x);
On 2014/03/12 12:38:26, jochen wrote:
> why not just increase the number of supported arguments in gin?

IIUC, your suggestion will require fixing base::Bind, so that demands caution.

I'm afraid I don't think it is the good idea to accept no less than 19 arguments
for a JavaScript function in the first place. Accepting an JavaScript object
instead of many arguments sounds better to me, but it will require modifying the
callers in the tests.

I'd like to leave this as it is this time.

https://codereview.chromium.org/185263006/diff/280001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:838: void
TestRunnerBindings::SetMockDeviceOrientation(gin::Arguments* args) {
Ditto.

https://codereview.chromium.org/185263006/diff/280001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:1179: bool permission_granted
= true;
On 2014/03/12 12:38:26, jochen wrote:
> i think this is never invoked with a second argument

This function has been changed very recently:
https://codereview.chromium.org/183663021

https://codereview.chromium.org/185263006/diff/280001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.cc:1483:
global_flag_.Reset(isolate, v8::Boolean::New(isolate, false));
On 2014/03/12 12:38:26, jochen wrote:
> i think global flag is really just a bool, so you don't need to store a
> persistent

There is only one exception:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
, but I agree with you. Done.

[hajimehoshi, 2014-03-13 05:25:03]

On 2014/03/12 19:27:00, jochen wrote:
> btw, there's a change to testrunner in the CQ right now:
> https://codereview.chromium.org/184313005/

Thanks! Solved the conflicting.

[jochen (gone - plz use gerrit), 2014-03-13 05:58:19]

On 2014/03/13 05:25:03, hajimehoshi wrote:
> On 2014/03/12 19:27:00, jochen wrote:
> > btw, there's a change to testrunner in the CQ right now:
> > https://codereview.chromium.org/184313005/
> 
> Thanks! Solved the conflicting.

lgtm, impressive cl! Thanks!

[hajimehoshi, 2014-03-13 06:10:49]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-03-13 06:11:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/185263006/320001

[commit-bot: I haz the power, 2014-03-13 06:33:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 06:33:45]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[hajimehoshi, 2014-03-13 07:22:05]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-03-13 07:22:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/185263006/320001

[commit-bot: I haz the power, 2014-03-13 08:07:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 08:07:35]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[hajimehoshi, 2014-03-13 08:26:32]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-03-13 08:26:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/185263006/320001

[commit-bot: I haz the power, 2014-03-13 09:16:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 09:16:49]

Retried try job too often on win_x64_rel for step(s) base_unittests,
chrome_elf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[hajimehoshi, 2014-03-13 15:05:33]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-03-13 15:07:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/185263006/320001

[commit-bot: I haz the power, 2014-03-13 15:53:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 15:53:32]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[hajimehoshi, 2014-03-14 02:28:08]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-03-14 02:29:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/185263006/340001

[commit-bot: I haz the power, 2014-03-14 03:15:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-14 03:15:21]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[hajimehoshi, 2014-03-14 03:22:05]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-03-14 03:23:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/185263006/360001

[commit-bot: I haz the power, 2014-03-14 04:32:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-14 04:32:27]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[hajimehoshi, 2014-03-14 04:55:12]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-03-14 04:55:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/185263006/360001

[commit-bot: I haz the power, 2014-03-14 06:18:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-14 06:18:08]

Retried try job too often on linux_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[jochen (gone - plz use gerrit), 2014-03-14 07:50:26]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2014-03-14 07:50:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/185263006/360001

[commit-bot: I haz the power, 2014-03-14 11:50:36]

Change committed as 257079