Switch the inheritance of WebView from WebWidget to protected.

Switch the inheritance of WebView from WebWidget to protected.

This change also fixes all the implicit casts from WebView to WebWidget.

This is part of a long-term refactor to split WebView and WebWidget.

BUG=419087
Committed: https://crrev.com/4fa48dad715b0ba02f91cdd6f63ee5b512196297
Cr-Commit-Position: refs/heads/master@{#392370}

[lfg, 2016-04-26 21:02:19]

Description was changed from

==========
Switch the inheritance of WebView from WebWidget to protected.

This change also fixes all the implicit casts from WebView to WebWidget by
adding a new method to retrieve the WebWidget from WebView.

This is part of a long-term refactor to split WebView and WebWidget.

BUG=
==========

to

==========
Switch the inheritance of WebView from WebWidget to protected.

This change also fixes all the implicit casts from WebView to WebWidget.

This is part of a long-term refactor to split WebView and WebWidget.

BUG=
==========

[lfg, 2016-04-26 22:27:08]

Description was changed from

==========
Switch the inheritance of WebView from WebWidget to protected.

This change also fixes all the implicit casts from WebView to WebWidget.

This is part of a long-term refactor to split WebView and WebWidget.

BUG=
==========

to

==========
Switch the inheritance of WebView from WebWidget to protected.

This change also fixes all the implicit casts from WebView to WebWidget.

This is part of a long-term refactor to split WebView and WebWidget.

BUG=419087
==========

[lfg, 2016-04-28 20:10:34]

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

[lfg, 2016-04-28 20:10:35]

Daniel, please take a look.

[lfg, 2016-04-29 17:20:25]

I've looked into the failed layout tests in the site isolation bot, and it's a
renderer crash in WebViewTestClient::AnimateNow because the web_widget is null.
I'm not sure if it's better to disable those tests or just set the widget to the
WebView. I think  the proper fix is to actually animate the local roots, but it
may not be an easy fix to go with this patch.

[lfg, 2016-05-02 20:52:10]

All tests are passing now -- the last few tests were failing because on a remote
to local navigation we weren't updating the webwidget in the WebViewTestProxy. I
switched that update to happen on RenderFrameCreated instead of
RenderViewCreated.

[dcheng, 2016-05-03 09:27:13]

https://codereview.chromium.org/1918183004/diff/160001/components/test_runner...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1918183004/diff/160001/components/test_runner...
components/test_runner/test_runner.cc:3115:
web_view()->mainFrame()->toWebLocalFrame()->frameWidget(),
I think TestRunnerForSpecificView should actually be TestRunnerForLocalFrame:
then it can just use the associated local frame directly (and will be correct if
called in a cross-process subframe). WDYT of adding a TODO here?

https://codereview.chromium.org/1918183004/diff/160001/content/shell/renderer...
File content/shell/renderer/layout_test/layout_test_content_renderer_client.cc
(right):

https://codereview.chromium.org/1918183004/diff/160001/content/shell/renderer...
content/shell/renderer/layout_test/layout_test_content_renderer_client.cc:106:
test_runner::WebTestProxyBase* proxy =
The ultimate goal here is to associate the widget with the test proxy for the
RenderFrame, right? Do you have a sense of how involved that would be? Would it
make sense to group that into this CL?

https://codereview.chromium.org/1918183004/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1918183004/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:84: void close() override {}
I didn't think of this for the WebWidgetClient/WebViewClient split, but can we
just:

using WebWidget::close;
using WebWidget size;

etc?

Instead of having to duplicate the default definitions.

[Łukasz Anforowicz, 2016-05-03 16:09:54]

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

[Łukasz Anforowicz, 2016-05-03 16:09:55]

Some drive-by comments...

https://codereview.chromium.org/1918183004/diff/160001/components/test_runner...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1918183004/diff/160001/components/test_runner...
components/test_runner/test_runner.cc:3115:
web_view()->mainFrame()->toWebLocalFrame()->frameWidget(),
On 2016/05/03 09:27:13, dcheng wrote:
> I think TestRunnerForSpecificView should actually be TestRunnerForLocalFrame:
> then it can just use the associated local frame directly (and will be correct
if
> called in a cross-process subframe). WDYT of adding a TODO here?

TestRunnerForSpecificView has some view-specific things - I think we cannot
transition all of its bindings to a TestRunnerForLocalFrame.  Examples of
view-focused bindings:
- SetPageVisibility
- DumpPageImportanceSignals
- Add/RemovePageOverlay
- SetWindowIsKey

OTOH, I agree that it makes sense for a testRunner.layoutAndPaintAsync and
testRunner.layoutAndPaintAsyncThen to target the widget associated with the
frame the javascript bindings are called from.  I am not sure what is the best
way to achieve this:
- Option #1: Store WebLocalFrame* in TestRunnerBindings and pass it to
TestRunnerForSpecificView::LayoutAndPaintAsync method.
- Option #2: Introduce TestRunnerForLocalFrame - owned by WebFrameTestProxyBase.

Option #2 feels more consistent with how other things work (i.e. nothing
currently uses Option #1).

Disclaimer: I have no idea how widget's relationships with frames/views should
look like in the long-term.  Maybe they can be extracted from
WebView/WebTestProxyBase and therefore the current code structure (i.e.
TestRunnerForSpecificView) is okay for the long-term.

https://codereview.chromium.org/1918183004/diff/160001/content/shell/renderer...
File content/shell/renderer/layout_test/layout_test_content_renderer_client.cc
(right):

https://codereview.chromium.org/1918183004/diff/160001/content/shell/renderer...
content/shell/renderer/layout_test/layout_test_content_renderer_client.cc:106:
test_runner::WebTestProxyBase* proxy =
On 2016/05/03 09:27:13, dcheng wrote:
> The ultimate goal here is to associate the widget with the test proxy for the
> RenderFrame, right? Do you have a sense of how involved that would be? Would
it
> make sense to group that into this CL?

Would it make sense to pass web_local_frame->frameWidget() to
WebFrameTestProxyBase and store it there?  (because WebFrameTestProxyBase is for
frame-level things, and WebTestProxyBase is for view-level things [I need to
rename it to WebViewTestProxyBase one day]).

[lfg, 2016-05-03 21:19:43]

https://codereview.chromium.org/1918183004/diff/160001/components/test_runner...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1918183004/diff/160001/components/test_runner...
components/test_runner/test_runner.cc:3115:
web_view()->mainFrame()->toWebLocalFrame()->frameWidget(),
On 2016/05/03 16:09:55, Łukasz Anforowicz wrote:
> On 2016/05/03 09:27:13, dcheng wrote:
> > I think TestRunnerForSpecificView should actually be
TestRunnerForLocalFrame:
> > then it can just use the associated local frame directly (and will be
correct
> if
> > called in a cross-process subframe). WDYT of adding a TODO here?
> 
> TestRunnerForSpecificView has some view-specific things - I think we cannot
> transition all of its bindings to a TestRunnerForLocalFrame.  Examples of
> view-focused bindings:
> - SetPageVisibility
> - DumpPageImportanceSignals
> - Add/RemovePageOverlay
> - SetWindowIsKey
> 
> OTOH, I agree that it makes sense for a testRunner.layoutAndPaintAsync and
> testRunner.layoutAndPaintAsyncThen to target the widget associated with the
> frame the javascript bindings are called from.  I am not sure what is the best
> way to achieve this:
> - Option #1: Store WebLocalFrame* in TestRunnerBindings and pass it to
> TestRunnerForSpecificView::LayoutAndPaintAsync method.
> - Option #2: Introduce TestRunnerForLocalFrame - owned by
WebFrameTestProxyBase.
> 
> Option #2 feels more consistent with how other things work (i.e. nothing
> currently uses Option #1).

I think option 1 is similar to how the TestRunnerForSpecificView handles the
animations with an array of WebWidgets that needs to schedule animations.

I agree that option 2 feels more consistent with other things though, but we
would only want to call LayoutAndPaint methods for local roots, so it's not
really something that should exist for every frame.

I think we should really group up all the local frames clustered into a single
local root, and have a runner for that. Not sure how feasible that is, or if
it's worth doing at all.

I'll add a TODO to investigate this issue.

https://codereview.chromium.org/1918183004/diff/160001/content/shell/renderer...
File content/shell/renderer/layout_test/layout_test_content_renderer_client.cc
(right):

https://codereview.chromium.org/1918183004/diff/160001/content/shell/renderer...
content/shell/renderer/layout_test/layout_test_content_renderer_client.cc:106:
test_runner::WebTestProxyBase* proxy =
On 2016/05/03 16:09:55, Łukasz Anforowicz wrote:
> On 2016/05/03 09:27:13, dcheng wrote:
> > The ultimate goal here is to associate the widget with the test proxy for
the
> > RenderFrame, right? Do you have a sense of how involved that would be? Would
> it
> > make sense to group that into this CL?
> 
> Would it make sense to pass web_local_frame->frameWidget() to
> WebFrameTestProxyBase and store it there?  (because WebFrameTestProxyBase is
for
> frame-level things, and WebTestProxyBase is for view-level things [I need to
> rename it to WebViewTestProxyBase one day]).

I think that does make sense, but there's also a lot more involved in fixing
this. All over the test runner there are assumptions of a single widget (and
that being the WebView).

I think we need to fix the test runner to make it aware of local roots and
WebFrameWidgets, similarly to what we have with the WebFrameTestClient and
WebViewTestClient, we should look into adding a WebWidgetTestClient that
forwards things to RenderWidget when a local root is not the WebView.

Overall, I think this should be done separately.

https://codereview.chromium.org/1918183004/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1918183004/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:84: void close() override {}
On 2016/05/03 09:27:13, dcheng wrote:
> I didn't think of this for the WebWidgetClient/WebViewClient split, but can we
> just:
> 
> using WebWidget::close;
> using WebWidget size;
> 
> etc?
> 
> Instead of having to duplicate the default definitions.

That does work, but it has the disadvantage of not being able to specify
specific functions in the case of multiple overrides, and the compiler also
silently ignores the using keyword if the function is redefined. I'm ok with it
either way.

[dcheng, 2016-05-04 04:55:30]

lgtm

https://codereview.chromium.org/1918183004/diff/160001/content/shell/renderer...
File content/shell/renderer/layout_test/layout_test_content_renderer_client.cc
(right):

https://codereview.chromium.org/1918183004/diff/160001/content/shell/renderer...
content/shell/renderer/layout_test/layout_test_content_renderer_client.cc:106:
test_runner::WebTestProxyBase* proxy =
On 2016/05/03 at 21:19:43, lfg wrote:
> On 2016/05/03 16:09:55, Łukasz Anforowicz wrote:
> > On 2016/05/03 09:27:13, dcheng wrote:
> > > The ultimate goal here is to associate the widget with the test proxy for
the
> > > RenderFrame, right? Do you have a sense of how involved that would be?
Would
> > it
> > > make sense to group that into this CL?
> > 
> > Would it make sense to pass web_local_frame->frameWidget() to
> > WebFrameTestProxyBase and store it there?  (because WebFrameTestProxyBase is
for
> > frame-level things, and WebTestProxyBase is for view-level things [I need to
> > rename it to WebViewTestProxyBase one day]).
> 
> I think that does make sense, but there's also a lot more involved in fixing
this. All over the test runner there are assumptions of a single widget (and
that being the WebView).
> 
> I think we need to fix the test runner to make it aware of local roots and
WebFrameWidgets, similarly to what we have with the WebFrameTestClient and
WebViewTestClient, we should look into adding a WebWidgetTestClient that
forwards things to RenderWidget when a local root is not the WebView.
> 
> Overall, I think this should be done separately.

OK. Might be worth adding a TODO, but I leave that to you.

https://codereview.chromium.org/1918183004/diff/160001/content/shell/renderer...
content/shell/renderer/layout_test/layout_test_content_renderer_client.cc:106:
test_runner::WebTestProxyBase* proxy =
On 2016/05/03 at 21:19:43, lfg wrote:
> On 2016/05/03 16:09:55, Łukasz Anforowicz wrote:
> > On 2016/05/03 09:27:13, dcheng wrote:
> > > The ultimate goal here is to associate the widget with the test proxy for
the
> > > RenderFrame, right? Do you have a sense of how involved that would be?
Would
> > it
> > > make sense to group that into this CL?
> > 
> > Would it make sense to pass web_local_frame->frameWidget() to
> > WebFrameTestProxyBase and store it there?  (because WebFrameTestProxyBase is
for
> > frame-level things, and WebTestProxyBase is for view-level things [I need to
> > rename it to WebViewTestProxyBase one day]).
> 
> I think that does make sense, but there's also a lot more involved in fixing
this. All over the test runner there are assumptions of a single widget (and
that being the WebView).
> 
> I think we need to fix the test runner to make it aware of local roots and
WebFrameWidgets, similarly to what we have with the WebFrameTestClient and
WebViewTestClient, we should look into adding a WebWidgetTestClient that
forwards things to RenderWidget when a local root is not the WebView.
> 
> Overall, I think this should be done separately.

OK. Might be worth adding a TODO, but I leave that to you.

https://codereview.chromium.org/1918183004/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1918183004/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:84: void close() override {}
On 2016/05/03 at 21:19:43, lfg wrote:
> On 2016/05/03 09:27:13, dcheng wrote:
> > I didn't think of this for the WebWidgetClient/WebViewClient split, but can
we
> > just:
> > 
> > using WebWidget::close;
> > using WebWidget size;
> > 
> > etc?
> > 
> > Instead of having to duplicate the default definitions.
> 
> That does work, but it has the disadvantage of not being able to specify
specific functions in the case of multiple overrides, and the compiler also
silently ignores the using keyword if the function is redefined. I'm ok with it
either way.

I'm OK with it either way. I don't think it's a big deal if there's multiple
overrides, since we should probably convert them together to minimize the
confusion anyway.

https://codereview.chromium.org/1918183004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.h (right):

https://codereview.chromium.org/1918183004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.h:246: WebFrameWidget*
frameWidget() const override;
Nit: let's call this widget (frame should be implied from the context).

[lfg, 2016-05-04 19:37:33]

https://codereview.chromium.org/1918183004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.h (right):

https://codereview.chromium.org/1918183004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.h:246: WebFrameWidget*
frameWidget() const override;
On 2016/05/04 04:55:30, dcheng wrote:
> Nit: let's call this widget (frame should be implied from the context).

I'll do this in a separate patch, since there are >20 calls that already exist
and uses frameWidget.

[lfg, 2016-05-04 19:38:12]

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

[lfg, 2016-05-04 19:38:13]

+jochen for content/ and components/test_runner/

[jochen (gone - plz use gerrit), 2016-05-06 10:34:02]

if the bots were greener, I'd be more confident to approve :)

[lfg, 2016-05-06 20:40:39]

On 2016/05/06 10:34:02, jochen wrote:
> if the bots were greener, I'd be more confident to approve :)

Oops, there was a conflict with
https://crrev.com/7fc9d87e41848601cc8198c7741abfd75089d697, rebased and renamed
the conflicting variable.

[jochen (gone - plz use gerrit), 2016-05-09 15:24:58]

lgtm

[lfg, 2016-05-09 17:06:17]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-05-09 17:06:18]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1918183004/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-09 17:06:40]

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

[commit-bot: I haz the power, 2016-05-09 18:25:23]

Description was changed from

==========
Switch the inheritance of WebView from WebWidget to protected.

This change also fixes all the implicit casts from WebView to WebWidget.

This is part of a long-term refactor to split WebView and WebWidget.

BUG=419087
==========

to

==========
Switch the inheritance of WebView from WebWidget to protected.

This change also fixes all the implicit casts from WebView to WebWidget.

This is part of a long-term refactor to split WebView and WebWidget.

BUG=419087
==========

[commit-bot: I haz the power, 2016-05-09 18:25:25]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-05-09 18:26:36]

Description was changed from

==========
Switch the inheritance of WebView from WebWidget to protected.

This change also fixes all the implicit casts from WebView to WebWidget.

This is part of a long-term refactor to split WebView and WebWidget.

BUG=419087
==========

to

==========
Switch the inheritance of WebView from WebWidget to protected.

This change also fixes all the implicit casts from WebView to WebWidget.

This is part of a long-term refactor to split WebView and WebWidget.

BUG=419087

Committed: https://crrev.com/4fa48dad715b0ba02f91cdd6f63ee5b512196297
Cr-Commit-Position: refs/heads/master@{#392370}
==========

[commit-bot: I haz the power, 2016-05-09 18:26:38]

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

[Łukasz Anforowicz, 2016-05-09 23:14:02]

On 2016/05/09 18:26:38, commit-bot: I haz the power wrote:
> Patchset 13 (id:??) landed as
> https://crrev.com/4fa48dad715b0ba02f91cdd6f63ee5b512196297
> Cr-Commit-Position: refs/heads/master@{#392370}

AFAICT this CL has caused UaF of WebWidget when running some layout tests -
please see https://crbug.com/610513