Add WebWidgetTestProxy.

Add WebWidgetTestProxy.

This patch adds a WebWidgetTestProxy interface that is instantiated when running
LayoutTests and wraps RenderWidget in order to execute test-specific code.

BUG=583347
Committed: https://crrev.com/1a99de092f8ad1974bb62af1a3f6f4f842e6db76
Cr-Commit-Position: refs/heads/master@{#409539}

[lfg, 2016-07-22 21:08:36]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 21:09:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2016-07-22 21:37:00]

Description was changed from

==========
WIP
==========

to

==========
Add WebWidgetTestProxy.

This patch adds a WebWidgetTestProxy interface that is instantiated when running
LayoutTests and wraps RenderWidget in order to execute test-specific code.

BUG=583347
==========

[lfg, 2016-07-22 22:02:53]

lfg@chromium.org changed reviewers:
+ dcheng@chromium.org

[lfg, 2016-07-22 22:02:53]

Daniel, can you take a look? In particular, the blink public API changes.

[commit-bot: I haz the power, 2016-07-22 22:48:57]

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

[commit-bot: I haz the power, 2016-07-22 22:48:58]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-07-23 08:23:19]

Description was changed from

==========
Add WebWidgetTestProxy.

This patch adds a WebWidgetTestProxy interface that is instantiated when running
LayoutTests and wraps RenderWidget in order to execute test-specific code.

BUG=583347
==========

to

==========
Add WebWidgetTestProxy.

This patch adds a WebWidgetTestProxy interface that is instantiated when running
LayoutTests and wraps RenderWidget in order to execute test-specific code.

BUG=583347
==========

[esprehn, 2016-07-23 08:24:50]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org, jochen@chromium.org

[esprehn, 2016-07-23 08:24:51]

Can we just merge the test_runner component back into content/renderer? This
template hackery is makes the code so complicated and confusing. We don't have
components/html_viewer anymore that used it, so this component doesn't seem to
make any sense. I'd really like to see us remove the templates. :)

[jochen (gone - plz use gerrit), 2016-07-23 13:14:59]

On 2016/07/23 at 08:24:51, esprehn wrote:
> Can we just merge the test_runner component back into content/renderer? This
template hackery is makes the code so complicated and confusing. We don't have
components/html_viewer anymore that used it, so this component doesn't seem to
make any sense. I'd really like to see us remove the templates. :)

test_runner never was part of content/renderer, so we'll always need this
template magic :(

[esprehn, 2016-07-23 16:07:42]

On 2016/07/23 at 13:14:59, jochen wrote:
> On 2016/07/23 at 08:24:51, esprehn wrote:
> > Can we just merge the test_runner component back into content/renderer? This
template hackery is makes the code so complicated and confusing. We don't have
components/html_viewer anymore that used it, so this component doesn't seem to
make any sense. I'd really like to see us remove the templates. :)
> 
> test_runner never was part of content/renderer, so we'll always need this
template magic :(

Not sure I understand, why can't we merge it into content (I guess
shell/renderer)? Having the methods repeated and split between
content/shell/renderer/layout_test and components is a maintenance issue. I'd
really like to merge those and move as much of this as possible into blink.

[jochen (gone - plz use gerrit), 2016-07-23 18:18:11]

On 2016/07/23 at 16:07:42, esprehn wrote:
> On 2016/07/23 at 13:14:59, jochen wrote:
> > On 2016/07/23 at 08:24:51, esprehn wrote:
> > > Can we just merge the test_runner component back into content/renderer?
This template hackery is makes the code so complicated and confusing. We don't
have components/html_viewer anymore that used it, so this component doesn't seem
to make any sense. I'd really like to see us remove the templates. :)
> > 
> > test_runner never was part of content/renderer, so we'll always need this
template magic :(
> 
> Not sure I understand, why can't we merge it into content (I guess
shell/renderer)? Having the methods repeated and split between
content/shell/renderer/layout_test and components is a maintenance issue. I'd
really like to merge those and move as much of this as possible into blink.

content and content shell are different beasts - content shell is a content
embedder and as such can't access e.g. RenderViewImpl, just RenderView.

Moving the code from components/test_runner to content/shell/*/layout_tests
and/or blink makes sense.

[lfg, 2016-07-24 14:24:51]

On 2016/07/23 18:18:11, jochen wrote:
> On 2016/07/23 at 16:07:42, esprehn wrote:
> > On 2016/07/23 at 13:14:59, jochen wrote:
> > > On 2016/07/23 at 08:24:51, esprehn wrote:
> > > > Can we just merge the test_runner component back into content/renderer?
> This template hackery is makes the code so complicated and confusing. We don't
> have components/html_viewer anymore that used it, so this component doesn't
seem
> to make any sense. I'd really like to see us remove the templates. :)
> > > 
> > > test_runner never was part of content/renderer, so we'll always need this
> template magic :(
> > 
> > Not sure I understand, why can't we merge it into content (I guess
> shell/renderer)? Having the methods repeated and split between
> content/shell/renderer/layout_test and components is a maintenance issue. I'd
> really like to merge those and move as much of this as possible into blink.
> 
> content and content shell are different beasts - content shell is a content
> embedder and as such can't access e.g. RenderViewImpl, just RenderView.
> 
> Moving the code from components/test_runner to content/shell/*/layout_tests
> and/or blink makes sense.

While that's doable, it won't provide any benefit in terms of code readability
and simplicity, since we won't be able to get rid of the templated inheritance.

I think we could move the Proxy into content/test, and instead of the ProxyBase
we could have a pure virtual client implemented by content_shell.

[jochen (gone - plz use gerrit), 2016-07-25 16:20:54]

lgtm, but please wait for the other reviewers to chime in

[dcheng, 2016-07-25 16:25:14]

Is the new function in the public API only for a CHECK() in test runner? Seems a
bit unusual to plumb through a public API method just for this.

[lfg, 2016-07-25 16:49:25]

On 2016/07/25 16:25:14, dcheng wrote:
> Is the new function in the public API only for a CHECK() in test runner? Seems
a
> bit unusual to plumb through a public API method just for this.

Yes, it's used for casting the WebWidget down to a WebFrameWidget, i.e. poor
man's RTTI. I could remove it, but the bad cast can lead to very subtle bugs
that are hard to catch.

[lfg, 2016-08-02 19:40:34]

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

[lfg, 2016-08-02 19:40:35]

+lukasza, can you do a review on this change?

[esprehn, 2016-08-02 19:59:24]

lgtm, I'd really like to see us simplify this stuff though. This doesn't seem
like it simplifies the system :/

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
File components/test_runner/web_widget_test_proxy.h (right):

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
components/test_runner/web_widget_test_proxy.h:89: bool requestPointerLock()
override {
why pointerLock but none of the other random features? having this template
super class in a different component far away from the code is really burdensome
for maintenance on the test runner. :(

[Łukasz Anforowicz, 2016-08-02 20:38:13]

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
File components/test_runner/web_view_test_proxy.h (right):

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
components/test_runner/web_view_test_proxy.h:6: #define
COMPONENTS_TEST_RUNNER_WEB_TEST_VIEW_PROXY_H_
Ooops - must have missed this in the previous review, but it seems that the
macro doesn't match the file name - shouldn't it be spelled
...WEB_VIEW_TEST_PROXY_H_?

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
File components/test_runner/web_widget_test_client.h (right):

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
components/test_runner/web_widget_test_client.h:40: }
This doesn't block this CL, but it's a pity that we still need the
web_view_test_proxy_base_ field (i.e. we need a sort of a widget -> view
dependency on the test side).  I think web_view_test_proxy_base_ field is only
used to 1) get TestRunner and TestInterfaces (which could be passed directly to
the WebWidgetTestClient's constructor and 2) to reach to view_test_runner to
interact with pointer lock API (which I have no idea what to do about - would
this belong on a widget in the long-term?).

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
File components/test_runner/web_widget_test_proxy.h (right):

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
components/test_runner/web_widget_test_proxy.h:10: #include "base/macros.h"
Do we also need base/logging.h for DCHECK macros?

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
components/test_runner/web_widget_test_proxy.h:89: bool requestPointerLock()
override {
On 2016/08/02 19:59:24, esprehn wrote:
> why pointerLock but none of the other random features?

I am guessing that all the other features can be directly handled by the product
code's implementation in RenderWidget (i.e. there was no need so far to monitor
or change behavior of the other features from test code).

> having this template
> super class in a different component far away from the code is really
burdensome
> for maintenance on the test runner. :(

Ack :-/

https://codereview.chromium.org/2174663005/diff/1/content/public/test/layoutt...
File content/public/test/layouttest_support.h (right):

https://codereview.chromium.org/2174663005/diff/1/content/public/test/layoutt...
content/public/test/layouttest_support.h:72: // WebWidgets and RenderWidgets and
WebFrameTestProxy between WebFrames and
s/WebWidgets and RenderWidgets/WebWidgetTestProxy between WebWidgets and
RenderWidgets/
?

I think this would be accurate even today, with the existing inheritance
relationship between RenderWidget and RenderView, right?

https://codereview.chromium.org/2174663005/diff/1/content/renderer/render_wid...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/2174663005/diff/1/content/renderer/render_wid...
content/renderer/render_widget.h:138: CreateRenderWidgetFunction
create_render_widget);
Perhaps we might want to indicate (with a comment?) that this method is meant to
be used only from test code.  Maybe we can use something similar to a comment
present in render_view_impl.h: // Used by content_layouttest_support to hook
into the creation of RenderViewImpls^H^H^HRenderWidgets.

https://codereview.chromium.org/2174663005/diff/1/content/shell/renderer/layo...
File content/shell/renderer/layout_test/layout_test_content_renderer_client.cc
(right):

https://codereview.chromium.org/2174663005/diff/1/content/shell/renderer/layo...
content/shell/renderer/layout_test/layout_test_content_renderer_client.cc:80:
proxy->set_widget_test_client(std::move(widget_test_client));
For a moment I was confused seeing 1) the call above and 2) seeing removal of
WebViewTestProxyBase::set_widget_test_client method, before realizing that the
CL also 3) adds a WebWidgetTestProxyBase::set_widget_test_client method (which
gets inherited by WebViewTestProxyBase).

I am guessing this call will go away once RenderView does not inherit from
RenderWidget anymore.

https://codereview.chromium.org/2174663005/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/2174663005/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebWidget.h:213: virtual bool isWebFrameWidget()
const { return false; }
I think I am okay with having a new method to be used only from tests / only for
CHECK.  I am worried that the new method looks to be part of the generic API
surface of WebWidget and in the future product code can start to make decisions
based on the new API (or would that be okay?).

2 options I can think of:
- Maybe add ForTesting suffix to the name? (I don't like this option)
- Maybe remove isWebFrameWidget() method and replace it with
assertIsWebFrameWidget() method - this way the method is less likely to be
abused in product code's conditions / decision making.

[lfg, 2016-08-02 22:27:21]

Thanks for the comments, please take another look.

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
File components/test_runner/web_view_test_proxy.h (right):

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
components/test_runner/web_view_test_proxy.h:6: #define
COMPONENTS_TEST_RUNNER_WEB_TEST_VIEW_PROXY_H_
On 2016/08/02 20:38:12, Łukasz Anforowicz wrote:
> Ooops - must have missed this in the previous review, but it seems that the
> macro doesn't match the file name - shouldn't it be spelled
> ...WEB_VIEW_TEST_PROXY_H_?

Oops! Fixed.

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
File components/test_runner/web_widget_test_client.h (right):

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
components/test_runner/web_widget_test_client.h:40: }
On 2016/08/02 20:38:12, Łukasz Anforowicz wrote:
> This doesn't block this CL, but it's a pity that we still need the
> web_view_test_proxy_base_ field (i.e. we need a sort of a widget -> view
> dependency on the test side).  I think web_view_test_proxy_base_ field is only
> used to 1) get TestRunner and TestInterfaces (which could be passed directly
to
> the WebWidgetTestClient's constructor and 2) to reach to view_test_runner to
> interact with pointer lock API (which I have no idea what to do about - would
> this belong on a widget in the long-term?).

Also related to 1, it's also being used to get the WebTestDelegate, which can
also be passed directly during/after construction.

However, for 2, unfortunately this does not belong to widget long-term. In
chrome the pointer lock API is handled by MouseLockController, which lives in
chrome/browser/. We could do some refactor to remove the pointer lock stuff from
TestRunnerForSpecificView, but the code needs to live in an object that's only
instantiated once, which is different from the lifetime of the widget. Since no
such place currently exists, I opted to leave the code in
TestRunnerForSpecifivView.

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
File components/test_runner/web_widget_test_proxy.h (right):

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
components/test_runner/web_widget_test_proxy.h:10: #include "base/macros.h"
On 2016/08/02 20:38:12, Łukasz Anforowicz wrote:
> Do we also need base/logging.h for DCHECK macros?

Done.

https://codereview.chromium.org/2174663005/diff/1/components/test_runner/web_...
components/test_runner/web_widget_test_proxy.h:89: bool requestPointerLock()
override {
On 2016/08/02 20:38:12, Łukasz Anforowicz wrote:
> On 2016/08/02 19:59:24, esprehn wrote:
> > why pointerLock but none of the other random features?
> 
> I am guessing that all the other features can be directly handled by the
product
> code's implementation in RenderWidget (i.e. there was no need so far to
monitor
> or change behavior of the other features from test code).

Thta's right, in the case of pointerlock, we don't really lock the pointer, we
just fake the responses from the test client so that the layout tests can pass.
That's why it needs its own implementation, instead of using the RenderWidget
code.

https://codereview.chromium.org/2174663005/diff/1/content/public/test/layoutt...
File content/public/test/layouttest_support.h (right):

https://codereview.chromium.org/2174663005/diff/1/content/public/test/layoutt...
content/public/test/layouttest_support.h:72: // WebWidgets and RenderWidgets and
WebFrameTestProxy between WebFrames and
On 2016/08/02 20:38:12, Łukasz Anforowicz wrote:
> s/WebWidgets and RenderWidgets/WebWidgetTestProxy between WebWidgets and
> RenderWidgets/
> ?

Done.

> I think this would be accurate even today, with the existing inheritance
> relationship between RenderWidget and RenderView, right?

Well, I guess it depends what do you mean by correct. The RenderViews will get
WebViewTestProxies, but not WebWidgetTestProxies directly. That's why I had to
make WebViewTestProxyBase inheriting from WebWidgetTestProxyBase.

https://codereview.chromium.org/2174663005/diff/1/content/renderer/render_wid...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/2174663005/diff/1/content/renderer/render_wid...
content/renderer/render_widget.h:138: CreateRenderWidgetFunction
create_render_widget);
On 2016/08/02 20:38:12, Łukasz Anforowicz wrote:
> Perhaps we might want to indicate (with a comment?) that this method is meant
to
> be used only from test code.  Maybe we can use something similar to a comment
> present in render_view_impl.h: // Used by content_layouttest_support to hook
> into the creation of RenderViewImpls^H^H^HRenderWidgets.

Done.

https://codereview.chromium.org/2174663005/diff/1/content/shell/renderer/layo...
File content/shell/renderer/layout_test/layout_test_content_renderer_client.cc
(right):

https://codereview.chromium.org/2174663005/diff/1/content/shell/renderer/layo...
content/shell/renderer/layout_test/layout_test_content_renderer_client.cc:80:
proxy->set_widget_test_client(std::move(widget_test_client));
On 2016/08/02 20:38:13, Łukasz Anforowicz wrote:
> For a moment I was confused seeing 1) the call above and 2) seeing removal of
> WebViewTestProxyBase::set_widget_test_client method, before realizing that the
> CL also 3) adds a WebWidgetTestProxyBase::set_widget_test_client method (which
> gets inherited by WebViewTestProxyBase).
> 
> I am guessing this call will go away once RenderView does not inherit from
> RenderWidget anymore.

That's right, yes, see my other comment, but basically it's because we want to
have a WebWidgetTestProxy in a RenderWidget that also happens to be a
RenderView. Hopefully it won't be long until we can remove this.

https://codereview.chromium.org/2174663005/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/2174663005/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebWidget.h:213: virtual bool isWebFrameWidget()
const { return false; }
On 2016/08/02 20:38:13, Łukasz Anforowicz wrote:
> I think I am okay with having a new method to be used only from tests / only
for
> CHECK.  I am worried that the new method looks to be part of the generic API
> surface of WebWidget and in the future product code can start to make
decisions
> based on the new API (or would that be okay?).

I'm not convinced that this would be bad.

> 2 options I can think of:
> - Maybe add ForTesting suffix to the name? (I don't like this option)

I also don't like this.

> - Maybe remove isWebFrameWidget() method and replace it with
> assertIsWebFrameWidget() method - this way the method is less likely to be
> abused in product code's conditions / decision making.

I'm ok with this, but I prefer to leave isWebFrameWidget() in the API, since I'm
not convinced anything bad can come out of it's usage. It's there mostly for
safety, and the performance impact should be small.

[Łukasz Anforowicz, 2016-08-02 22:35:23]

Thanks - lgtm.

https://codereview.chromium.org/2174663005/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/2174663005/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebWidget.h:213: virtual bool isWebFrameWidget()
const { return false; }
On 2016/08/02 22:27:21, lfg wrote:
> On 2016/08/02 20:38:13, Łukasz Anforowicz wrote:
> > I think I am okay with having a new method to be used only from tests / only
> for
> > CHECK.  I am worried that the new method looks to be part of the generic API
> > surface of WebWidget and in the future product code can start to make
> decisions
> > based on the new API (or would that be okay?).
> 
> I'm not convinced that this would be bad.
> 
> > 2 options I can think of:
> > - Maybe add ForTesting suffix to the name? (I don't like this option)
> 
> I also don't like this.
> 
> > - Maybe remove isWebFrameWidget() method and replace it with
> > assertIsWebFrameWidget() method - this way the method is less likely to be
> > abused in product code's conditions / decision making.
> 
> I'm ok with this, but I prefer to leave isWebFrameWidget() in the API, since
I'm
> not convinced anything bad can come out of it's usage. It's there mostly for
> safety, and the performance impact should be small.

Okay - sgtm, if Blink owners approve.

[esprehn, 2016-08-02 22:49:55]

lgtm

https://codereview.chromium.org/2174663005/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameWidget.h (right):

https://codereview.chromium.org/2174663005/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameWidget.h:77: bool isWebFrameWidget() const
override { return true; }
final

[lfg, 2016-08-02 23:22:50]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 23:23:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 01:21:43]

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

[commit-bot: I haz the power, 2016-08-03 01:21:44]

Dry run: This issue passed the CQ dry run.

[lfg, 2016-08-03 16:44:00]

https://codereview.chromium.org/2174663005/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameWidget.h (right):

https://codereview.chromium.org/2174663005/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameWidget.h:77: bool isWebFrameWidget() const
override { return true; }
On 2016/08/02 22:49:55, esprehn wrote:
> final

Done.

[lfg, 2016-08-03 16:44:05]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-08-03 16:44:06]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
lukasza@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2174663005/#ps60001
(title: "addressing comment")

[commit-bot: I haz the power, 2016-08-03 16:44:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 16:50:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-03 16:54:16]

Description was changed from

==========
Add WebWidgetTestProxy.

This patch adds a WebWidgetTestProxy interface that is instantiated when running
LayoutTests and wraps RenderWidget in order to execute test-specific code.

BUG=583347
==========

to

==========
Add WebWidgetTestProxy.

This patch adds a WebWidgetTestProxy interface that is instantiated when running
LayoutTests and wraps RenderWidget in order to execute test-specific code.

BUG=583347

Committed: https://crrev.com/1a99de092f8ad1974bb62af1a3f6f4f842e6db76
Cr-Commit-Position: refs/heads/master@{#409539}
==========

[commit-bot: I haz the power, 2016-08-03 16:54:17]

Patchset 4 (id:??) landed as
https://crrev.com/1a99de092f8ad1974bb62af1a3f6f4f842e6db76
Cr-Commit-Position: refs/heads/master@{#409539}

[lfg, 2016-08-04 00:13:01]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2213653002/ by lfg@chromium.org.

The reason for reverting is: Causing layouttests to fail on the site isolation
bot..