Use correct WebView from TestRunner methods called via testRunner bindings.

Use correct WebView from TestRunner methods called via testRunner bindings.

TestRunner provides javascript bindings for test functions that tests
can use for various functionality.  Some of these test functions operate
on global state - for example:
*) testRunner.dumpAsText (test flag affecting test behavior)
*) testRunner.setAllowDisplayOfInsecureContent (test flag affecting product
   behavior)
*) testRunner.setTextSubpixelPositioning (directly interacts with product).
Some of these test functions operate on a specific view - for example:
*) testRunner.capturePixelsAsyncThen
*) testRunner.setPageVisibility
View-specific functions should operate on a view associated with the
view of the frame that the javascript is calling from (rather then on
the main window's view).  To accomplish this the CL splits view-specific
functionality of TestRunner into a separete TestRunnerForSpecificView
class (with one instance per WebTestProxyBase).

This CL is important to enable setting TestRunner::web_view_ only to the
actual main test window (rather than to the first WebView/RenderView
that happens to be created in a given renderer process) - see the
planned https://crrev.com/1896623002.  Without the current CL javascript
calls from a DevTools window would cease to work (they would encounter a
null TestRunner::web_view_ and failsafe, rather than using the DevTools
view from which javascript calls them).  Repro test case would be
running layout tests under inspector/ directory (with or without
--site-per-process flag).

BUG=595089
Committed: https://crrev.com/b2ad050469118ea18e7c4c12112180d3fb8ab725
Cr-Commit-Position: refs/heads/master@{#390088}

[Łukasz Anforowicz, 2016-04-21 21:46:49]

Description was changed from

==========
Use correct WebView from TestRunner methods called via testRunner bindings.

EventSender provides javascript bindings for test functions that tests
can use for various functionality.  Some of these test functions operate
on global state (i.e. ) - these test functions are fine with being implemented
by a global TestRunner object.  Other test functions operate on a view
- these should operate on a view associated with the view of the frame
that the javascript is calling from (rather then on the main window's view).
To accomplish this the CL splits view-specific functionality of TestRunner
into a separete class (with one instance per WebTestProxyBase).

BUG=595089
==========

to

==========
Use correct WebView from TestRunner methods called via testRunner bindings.

EventSender provides javascript bindings for test functions that tests
can use for various functionality.  Some of these test functions operate
on global state (i.e. ) - these test functions are fine with being
implemented by a global TestRunner object.  Other test functions operate
on a view - these should operate on a view associated with the view of
the frame that the javascript is calling from (rather then on the main
window's view).  To accomplish this the CL splits view-specific
functionality of TestRunner into a separete class (with one instance per
WebTestProxyBase).

This CL is important to enable setting TestRunner::web_view_ only to the
actual main test window (rather than to the first WebView/RenderView
that happens to be created in a given renderer process) -
https://crrev.com/1896623002.  Without the current CL javascript calls
from a DevTools window would cease to work (they would encounter a null
TestRunner::web_view_ and failsafe, rather than using the DevTools view
from where javascript calls them).  Repro test case would be running
layout tests under inspector/ directory (with or without
--site-per-process flag).

BUG=595089
==========

[Łukasz Anforowicz, 2016-04-21 23:41:00]

lukasza@chromium.org changed reviewers:
+ jochen@chromium.org

[Łukasz Anforowicz, 2016-04-21 23:41:01]

jochen@ - this CL is not quite ready for a real review and land (I have some
pending self-review tasks) but I wanted to push it out and ask about naming +
logistics of code.  I feel confident that view-specific stuff in TestRunner
should be moved to a separate TestRunnerForSpecificView class (only keeping
truly global stuff in TestRunner).  OTOH, I am not so sure about the names and
where code should live.

Does TestRunnerForSpecificView sound okay for a class name?  I don't like it but
I couldn't think of a better alternative?

Also - in this CL I plan to keep things in test_runner.cc (to help with review,
diff, blame) and only move it to a separate compilation unit in a separate CL.

WDYT?

https://codereview.chromium.org/1908183002/diff/1/components/test_runner/test...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/1908183002/diff/1/components/test_runner/test...
components/test_runner/test_runner.h:656: class TestRunnerForSpecificView {
I don't like the name of the new class, but I couldn't think of a better
alternative... :-(

Notes:
- The bindings from TestRunner + TestRunnerForSpecificView are gathered together
and exposed through a single |window.testRunner| javascript object (i.e. they
are gathered together by TestRunnerBindings::GetObjectTemplateBuilder method).

- TestRunnerForSpecificView is owned by WebTestProxyBase in a similar way to
AccessbilityController, EventSender and TextInputController (all of which
provide javascript bindings that should act on the correct view [associated with
the bindings], not on the main test view).

- Maybe I should also rename TestRunner to something like TestRunnerGlobal? 
Bleh...

[jochen (gone - plz use gerrit), 2016-04-22 15:25:14]

lgtm

[jochen (gone - plz use gerrit), 2016-04-22 15:25:59]

lgtm - as in, it's testing code. If it's green on the bots, you're good to go :)

[jochen (gone - plz use gerrit), 2016-04-22 15:27:33]

On 2016/04/21 at 23:41:01, lukasza wrote:
> jochen@ - this CL is not quite ready for a real review and land (I have some
pending self-review tasks) but I wanted to push it out and ask about naming +
logistics of code.  I feel confident that view-specific stuff in TestRunner
should be moved to a separate TestRunnerForSpecificView class (only keeping
truly global stuff in TestRunner).  OTOH, I am not so sure about the names and
where code should live.
> 
> Does TestRunnerForSpecificView sound okay for a class name?  I don't like it
but I couldn't think of a better alternative?
> 
> Also - in this CL I plan to keep things in test_runner.cc (to help with
review, diff, blame) and only move it to a separate compilation unit in a
separate CL.
> 
> WDYT?
> 
>
https://codereview.chromium.org/1908183002/diff/1/components/test_runner/test...
> File components/test_runner/test_runner.h (right):
> 
>
https://codereview.chromium.org/1908183002/diff/1/components/test_runner/test...
> components/test_runner/test_runner.h:656: class TestRunnerForSpecificView {
> I don't like the name of the new class, but I couldn't think of a better
alternative... :-(

good enough imo

> 
> Notes:
> - The bindings from TestRunner + TestRunnerForSpecificView are gathered
together and exposed through a single |window.testRunner| javascript object
(i.e. they are gathered together by TestRunnerBindings::GetObjectTemplateBuilder
method).

that's fine, as you point out, the bindings object isn't really touched by this.

> 
> - TestRunnerForSpecificView is owned by WebTestProxyBase in a similar way to
AccessbilityController, EventSender and TextInputController (all of which
provide javascript bindings that should act on the correct view [associated with
the bindings], not on the main test view).
> 
> - Maybe I should also rename TestRunner to something like TestRunnerGlobal? 
Bleh...

or just add a comment

[Łukasz Anforowicz, 2016-04-23 00:14:11]

Description was changed from

==========
Use correct WebView from TestRunner methods called via testRunner bindings.

EventSender provides javascript bindings for test functions that tests
can use for various functionality.  Some of these test functions operate
on global state (i.e. ) - these test functions are fine with being
implemented by a global TestRunner object.  Other test functions operate
on a view - these should operate on a view associated with the view of
the frame that the javascript is calling from (rather then on the main
window's view).  To accomplish this the CL splits view-specific
functionality of TestRunner into a separete class (with one instance per
WebTestProxyBase).

This CL is important to enable setting TestRunner::web_view_ only to the
actual main test window (rather than to the first WebView/RenderView
that happens to be created in a given renderer process) -
https://crrev.com/1896623002.  Without the current CL javascript calls
from a DevTools window would cease to work (they would encounter a null
TestRunner::web_view_ and failsafe, rather than using the DevTools view
from where javascript calls them).  Repro test case would be running
layout tests under inspector/ directory (with or without
--site-per-process flag).

BUG=595089
==========

to

==========
Use correct WebView from TestRunner methods called via testRunner bindings.

TestRunner provides javascript bindings for test functions that tests
can use for various functionality.  Some of these test functions operate
on global state - for example:
*) testRunner.dumpAsText (test flag affecting test behavior)
*) testRunner.setAllowDisplayOfInsecureContent (test flag affecting product
   behavior)
*) testRunner.setTextSubpixelPositioning (directly interacts with product).
Some of these test functions operate on a specific view - for example:
*) testRunner.capturePixelsAsyncThen
*) testRunner.setPageVisibility
View-specific functions should operate on a view associated with the
view of the frame that the javascript is calling from (rather then on
the main window's view).  To accomplish this the CL splits view-specific
functionality of TestRunner into a separete TestRunnerForSpecificView
class (with one instance per WebTestProxyBase).

This CL is important to enable setting TestRunner::web_view_ only to the
actual main test window (rather than to the first WebView/RenderView
that happens to be created in a given renderer process) - see the
planned https://crrev.com/1896623002.  Without the current CL javascript
calls from a DevTools window would cease to work (they would encounter a
null TestRunner::web_view_ and failsafe, rather than using the DevTools
view from which javascript calls them).  Repro test case would be
running layout tests under inspector/ directory (with or without
--site-per-process flag).

BUG=595089
==========

[Łukasz Anforowicz, 2016-04-23 00:16:57]

jochen@, could you take another look please?  I've moved a few more functions to
TestRunnerForSpecificView (i.e. once for mouse pointer locking) and moved
Resetting to happen right after giving a WebView to a WebTestProxyBase (I did
this so that TestRunnerForSpecificView::Reset gets run for each view, just as
the web_view_-related parts did for main view via TestRunner::Reset).

[jochen (gone - plz use gerrit), 2016-04-27 11:52:11]

still lgtm

https://codereview.chromium.org/1908183002/diff/60001/components/test_runner/...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/1908183002/diff/60001/components/test_runner/...
components/test_runner/test_runner.h:648:
TestRunnerForSpecificView(WebTestProxyBase* web_test_proxy_base);
explicit

[Łukasz Anforowicz, 2016-04-27 15:05:19]

https://codereview.chromium.org/1908183002/diff/60001/components/test_runner/...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/1908183002/diff/60001/components/test_runner/...
components/test_runner/test_runner.h:648:
TestRunnerForSpecificView(WebTestProxyBase* web_test_proxy_base);
On 2016/04/27 11:52:11, jochen wrote:
> explicit

Done.

[Łukasz Anforowicz, 2016-04-27 15:05:49]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-04-27 15:05:49]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1908183002/#ps80001
(title: "Explicit constructor for TestRunnerForSpecificView.")

[commit-bot: I haz the power, 2016-04-27 15:06:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1908183002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1908183002/80001

[commit-bot: I haz the power, 2016-04-27 15:51:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-30 17:10:13]

Patchset 5 (id:??) landed as
https://crrev.com/b2ad050469118ea18e7c4c12112180d3fb8ab725
Cr-Commit-Position: refs/heads/master@{#390088}

[commit-bot: I haz the power, 2016-04-30 17:50:12]

Description was changed from

==========
Use correct WebView from TestRunner methods called via testRunner bindings.

TestRunner provides javascript bindings for test functions that tests
can use for various functionality.  Some of these test functions operate
on global state - for example:
*) testRunner.dumpAsText (test flag affecting test behavior)
*) testRunner.setAllowDisplayOfInsecureContent (test flag affecting product
   behavior)
*) testRunner.setTextSubpixelPositioning (directly interacts with product).
Some of these test functions operate on a specific view - for example:
*) testRunner.capturePixelsAsyncThen
*) testRunner.setPageVisibility
View-specific functions should operate on a view associated with the
view of the frame that the javascript is calling from (rather then on
the main window's view).  To accomplish this the CL splits view-specific
functionality of TestRunner into a separete TestRunnerForSpecificView
class (with one instance per WebTestProxyBase).

This CL is important to enable setting TestRunner::web_view_ only to the
actual main test window (rather than to the first WebView/RenderView
that happens to be created in a given renderer process) - see the
planned https://crrev.com/1896623002.  Without the current CL javascript
calls from a DevTools window would cease to work (they would encounter a
null TestRunner::web_view_ and failsafe, rather than using the DevTools
view from which javascript calls them).  Repro test case would be
running layout tests under inspector/ directory (with or without
--site-per-process flag).

BUG=595089
==========

to

==========
Use correct WebView from TestRunner methods called via testRunner bindings.

TestRunner provides javascript bindings for test functions that tests
can use for various functionality.  Some of these test functions operate
on global state - for example:
*) testRunner.dumpAsText (test flag affecting test behavior)
*) testRunner.setAllowDisplayOfInsecureContent (test flag affecting product
   behavior)
*) testRunner.setTextSubpixelPositioning (directly interacts with product).
Some of these test functions operate on a specific view - for example:
*) testRunner.capturePixelsAsyncThen
*) testRunner.setPageVisibility
View-specific functions should operate on a view associated with the
view of the frame that the javascript is calling from (rather then on
the main window's view).  To accomplish this the CL splits view-specific
functionality of TestRunner into a separete TestRunnerForSpecificView
class (with one instance per WebTestProxyBase).

This CL is important to enable setting TestRunner::web_view_ only to the
actual main test window (rather than to the first WebView/RenderView
that happens to be created in a given renderer process) - see the
planned https://crrev.com/1896623002.  Without the current CL javascript
calls from a DevTools window would cease to work (they would encounter a
null TestRunner::web_view_ and failsafe, rather than using the DevTools
view from which javascript calls them).  Repro test case would be
running layout tests under inspector/ directory (with or without
--site-per-process flag).

BUG=595089

Committed: https://crrev.com/b2ad050469118ea18e7c4c12112180d3fb8ab725
Cr-Commit-Position: refs/heads/master@{#390088}
==========