Return the WebViewFrameWidget in RenderWidget::webwidget() if there

Return the WebViewFrameWidget in RenderWidget::webwidget() if there
is one.

BUG=419087
Committed: https://crrev.com/8ff3391e7f1a6cabb73807bf781cf949326e0081
Cr-Commit-Position: refs/heads/master@{#418365}

[lfg, 2016-08-29 16:44:39]

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

[lfg, 2016-08-29 16:47:20]

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

[lfg, 2016-08-29 16:47:21]

Hey Daniel, can you take a look?

This patch makes RenderWidget::webwidget() return the WebViewFrameWidget instead
of the WebViewImpl. The idea is that by doing this we can implement common
things in WebFrameWidgetBase and avoid implementing new things in WebViewImpl
that will eventually have to be moved to WebFrameWidget.

[commit-bot: I haz the power, 2016-08-29 16:53:02]

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-30 03:41:43]

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

[commit-bot: I haz the power, 2016-08-30 03:41:44]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-08-31 18:00:33]

https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2634: if (frame_widget_)
Should't this always be non-null while we're calling this function?

[lfg, 2016-08-31 18:04:45]

On 2016/08/31 18:00:33, dcheng wrote:
>
https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_vie...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_vie...
> content/renderer/render_view_impl.cc:2634: if (frame_widget_)
> Should't this always be non-null while we're calling this function?

No, if the main frame is remote, then frame_widget_ is null when calling this
function.

[dcheng, 2016-08-31 23:52:55]

On 2016/08/31 18:04:45, lfg wrote:
> On 2016/08/31 18:00:33, dcheng wrote:
> >
>
https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_vie...
> > File content/renderer/render_view_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_vie...
> > content/renderer/render_view_impl.cc:2634: if (frame_widget_)
> > Should't this always be non-null while we're calling this function?
> 
> No, if the main frame is remote, then frame_widget_ is null when calling this
> function.

Why would we want to return a widget in that case then?

[dcheng, 2016-08-31 23:58:55]

Meh. LGTM, I guess. We should probably add a TODO: it looks like this is exposed
in RenderViewObserver, which we'd like to get rid of anyway...

Hopefully once onion soup is a bit further along, we can figure out a better way
to plumb this for autofill, since it's literally the only RenderViewObserver
that cares about this.

[esprehn, 2016-09-01 00:37:58]

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

[esprehn, 2016-09-01 00:37:58]

https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2633: blink::WebWidget*
RenderViewImpl::webwidget() const {
webWidget ? or widget() ?

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

https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_wid...
content/renderer/render_widget.h:142: virtual blink::WebWidget* webwidget()
const;
This doesn't match the chromium style that lowercase methods are supposed to be
cheap and non-virtual.

GetWebWidget() ?

[lfg, 2016-09-01 16:03:42]

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

[commit-bot: I haz the power, 2016-09-01 16:03:58]

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-09-01 16:06:02]

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

[commit-bot: I haz the power, 2016-09-01 16:06:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2016-09-01 16:50:21]

On 2016/08/31 23:58:55, dcheng wrote:
> Meh. LGTM, I guess. We should probably add a TODO: it looks like this is
exposed
> in RenderViewObserver, which we'd like to get rid of anyway...
> 
> Hopefully once onion soup is a bit further along, we can figure out a better
way
> to plumb this for autofill, since it's literally the only RenderViewObserver
> that cares about this.

Oops, I meant to CC (and reply) on https://codereview.chromium.org/2290523004/
instead. Please ignore this LGTM (my previous question still stands)

[dcheng, 2016-09-01 16:52:07]

On 2016/09/01 00:37:58, esprehn wrote:
>
https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_vie...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_vie...
> content/renderer/render_view_impl.cc:2633: blink::WebWidget*
> RenderViewImpl::webwidget() const {
> webWidget ? or widget() ?
> 
>
https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_wid...
> File content/renderer/render_widget.h (right):
> 
>
https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_wid...
> content/renderer/render_widget.h:142: virtual blink::WebWidget* webwidget()
> const;
> This doesn't match the chromium style that lowercase methods are supposed to
be
> cheap and non-virtual.
> 
> GetWebWidget() ?

This does violate the Chromium naming convention, but I expect that once the
split is done, we'll be reverting it to a non-virtual cheap accessor. So I'm not
sure it's worth the effort to rename.

[lfg, 2016-09-01 17:03:10]

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

[commit-bot: I haz the power, 2016-09-01 17:03:42]

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-09-01 17:14:44]

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

[commit-bot: I haz the power, 2016-09-01 17:14:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lfg, 2016-09-01 17:15:40]

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

[commit-bot: I haz the power, 2016-09-01 17:16:15]

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-09-01 17:35:58]

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

[commit-bot: I haz the power, 2016-09-01 17:35:59]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[lfg, 2016-09-01 17:57:55]

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

[commit-bot: I haz the power, 2016-09-01 17:58:31]

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

[lfg, 2016-09-01 18:06:08]

On 2016/08/31 23:52:55, dcheng wrote:
> On 2016/08/31 18:04:45, lfg wrote:
> > No, if the main frame is remote, then frame_widget_ is null when calling
this
> > function.
> 
> Why would we want to return a widget in that case then?

We don't want to return it, but we are not there yet -- i.e. it crashes if we
don't (the particular crash I saw is calling OnResize() on the RenderWidget with
a proxy main frame).

We should be fixing those cases as we continue to work on splitting
WebView/WebWidget, but the idea of this change is to just prevent new problems
from creeping in (like the problem I fixed between PS4 and PS5). It should also
make developers think twice about where to place new code (WebViewImpl,
WebFrameWidgetImpl, WebFrameWidgetBase).

On 2016/09/01 16:52:07, dcheng wrote:
> On 2016/09/01 00:37:58, esprehn wrote:
https://codereview.chromium.org/2288313002/diff/1/content/renderer/render_wid...
> > content/renderer/render_widget.h:142: virtual blink::WebWidget* webwidget()
> > const;
> > This doesn't match the chromium style that lowercase methods are supposed to
> be
> > cheap and non-virtual.
> > 
> > GetWebWidget() ?
> 
> This does violate the Chromium naming convention, but I expect that once the
> split is done, we'll be reverting it to a non-virtual cheap accessor. So I'm
not
> sure it's worth the effort to rename.

I think this is worth fixing (and fixing again when we go back). It's also not
that much code.  I've also switched usages of RenderWidget::webwidget_ to
RenderWidget::GetWebWidget().

[commit-bot: I haz the power, 2016-09-01 21:23:45]

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

[commit-bot: I haz the power, 2016-09-01 21:23:46]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dcheng, 2016-09-08 19:42:16]

I still don't think we should do the rename: it /is/ going to be a simple getter
again, sooner rather than later. In addition, we're adding a lot more calls to
this, so it's going to be even more work to unwind it once we're done with it.

https://codereview.chromium.org/2288313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewFrameWidget.h (right):

https://codereview.chromium.org/2288313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewFrameWidget.h:91: void
applyReplacementRange(int start, int length) override;
How come these lines are in this patch?

[lfg, 2016-09-08 19:58:24]

On 2016/09/08 19:42:16, dcheng wrote:
> I still don't think we should do the rename: it /is/ going to be a simple
getter
> again, sooner rather than later. In addition, we're adding a lot more calls to
> this, so it's going to be even more work to unwind it once we're done with it.

We're at least a few months of it being back to be a simple getter, while I do
prefer GetWebWidget, I'm not strong about it.

esprehn@, thoughts?

https://codereview.chromium.org/2288313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewFrameWidget.h (right):

https://codereview.chromium.org/2288313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewFrameWidget.h:91: void
applyReplacementRange(int start, int length) override;
On 2016/09/08 19:42:16, dcheng wrote:
> How come these lines are in this patch?

When they were added, they were added only to WebViewImpl, without being in
WebViewFrameWidget. Since with this patch we are now returning a
WebViewFrameWidget, these calls became a no-op unless we forward them.

BTW, this is exactly what this change tries to prevent.

[dcheng, 2016-09-08 20:00:31]

On 2016/09/08 19:58:24, lfg wrote:
> On 2016/09/08 19:42:16, dcheng wrote:
> > I still don't think we should do the rename: it /is/ going to be a simple
> getter
> > again, sooner rather than later. In addition, we're adding a lot more calls
to
> > this, so it's going to be even more work to unwind it once we're done with
it.
> 
> We're at least a few months of it being back to be a simple getter, while I do
> prefer GetWebWidget, I'm not strong about it.
> 
> esprehn@, thoughts?
> 
>
https://codereview.chromium.org/2288313002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebViewFrameWidget.h (right):
> 
>
https://codereview.chromium.org/2288313002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebViewFrameWidget.h:91: void
> applyReplacementRange(int start, int length) override;
> On 2016/09/08 19:42:16, dcheng wrote:
> > How come these lines are in this patch?
> 
> When they were added, they were added only to WebViewImpl, without being in
> WebViewFrameWidget. Since with this patch we are now returning a
> WebViewFrameWidget, these calls became a no-op unless we forward them.
> 
> BTW, this is exactly what this change tries to prevent.

Can we make the ones implemented on WebView private and move it into a
non-virtual, so they're only callable via a WebWidget? I'm not sure if that even
helps that much... but maybe?

[esprehn, 2016-09-08 20:01:12]

I'd prefer we do the rename. Either Chromium style is important to follow,
in which case we should follow it, or it isn't important in which case why
are we spending so much resources reformatting all of blink? :P

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2016-09-08 20:03:06]

Less snarky, but my experience is these projects always take longer than
anticipated so it's best to follow the rules as you go. :)

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[lfg, 2016-09-08 22:17:47]

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

[commit-bot: I haz the power, 2016-09-08 22:18:27]

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

[lfg, 2016-09-08 22:18:43]

Please, take another look.

On 2016/09/08 20:00:31, dcheng wrote:
> On 2016/09/08 19:58:24, lfg wrote:
> > On 2016/09/08 19:42:16, dcheng wrote:
>
https://codereview.chromium.org/2288313002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/web/WebViewFrameWidget.h:91: void
> > applyReplacementRange(int start, int length) override;
> > On 2016/09/08 19:42:16, dcheng wrote:
> > > How come these lines are in this patch?
> > 
> > When they were added, they were added only to WebViewImpl, without being in
> > WebViewFrameWidget. Since with this patch we are now returning a
> > WebViewFrameWidget, these calls became a no-op unless we forward them.
> > 
> > BTW, this is exactly what this change tries to prevent.
> 
> Can we make the ones implemented on WebView private and move it into a
> non-virtual, so they're only callable via a WebWidget? I'm not sure if that
even
> helps that much... but maybe?

I don't think it helps that much, but it doesn't hurt either. I made it private.

[esprehn, 2016-09-08 22:44:02]

lgtm

https://codereview.chromium.org/2288313002/diff/120001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2288313002/diff/120001/content/renderer/rende...
content/renderer/render_widget.cc:1145: if (GetWebWidget())
In blink we'd do:

if (WebWidget* widget = GetWebWidget())
  widget->...()

to avoid the duplicate virtual calls.

[commit-bot: I haz the power, 2016-09-09 00:21:50]

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

[commit-bot: I haz the power, 2016-09-09 00:21:51]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-09-09 04:32:40]

On 2016/09/08 22:18:43, lfg wrote:
> Please, take another look.
> 
> On 2016/09/08 20:00:31, dcheng wrote:
> > On 2016/09/08 19:58:24, lfg wrote:
> > > On 2016/09/08 19:42:16, dcheng wrote:
> >
>
https://codereview.chromium.org/2288313002/diff/80001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/web/WebViewFrameWidget.h:91: void
> > > applyReplacementRange(int start, int length) override;
> > > On 2016/09/08 19:42:16, dcheng wrote:
> > > > How come these lines are in this patch?
> > > 
> > > When they were added, they were added only to WebViewImpl, without being
in
> > > WebViewFrameWidget. Since with this patch we are now returning a
> > > WebViewFrameWidget, these calls became a no-op unless we forward them.
> > > 
> > > BTW, this is exactly what this change tries to prevent.
> > 
> > Can we make the ones implemented on WebView private and move it into a
> > non-virtual, so they're only callable via a WebWidget? I'm not sure if that
> even
> > helps that much... but maybe?
> 
> I don't think it helps that much, but it doesn't hurt either. I made it
private.

I don't mean just making it private; we should make the overrides asserts, and
then change the private implementation to be a non-virtual helper method.

[dcheng, 2016-09-09 05:24:44]

On 2016/09/09 04:32:40, dcheng wrote:
> On 2016/09/08 22:18:43, lfg wrote:
> > Please, take another look.
> > 
> > On 2016/09/08 20:00:31, dcheng wrote:
> > > On 2016/09/08 19:58:24, lfg wrote:
> > > > On 2016/09/08 19:42:16, dcheng wrote:
> > >
> >
>
https://codereview.chromium.org/2288313002/diff/80001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/web/WebViewFrameWidget.h:91: void
> > > > applyReplacementRange(int start, int length) override;
> > > > On 2016/09/08 19:42:16, dcheng wrote:
> > > > > How come these lines are in this patch?
> > > > 
> > > > When they were added, they were added only to WebViewImpl, without being
> in
> > > > WebViewFrameWidget. Since with this patch we are now returning a
> > > > WebViewFrameWidget, these calls became a no-op unless we forward them.
> > > > 
> > > > BTW, this is exactly what this change tries to prevent.
> > > 
> > > Can we make the ones implemented on WebView private and move it into a
> > > non-virtual, so they're only callable via a WebWidget? I'm not sure if
that
> > even
> > > helps that much... but maybe?
> > 
> > I don't think it helps that much, but it doesn't hurt either. I made it
> private.
> 
> I don't mean just making it private; we should make the overrides asserts, and
> then change the private implementation to be a non-virtual helper method.

(If you don't think people accidentally calling this via WebView will be an
issue in the near future though, then this patch LGTM)

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

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

[commit-bot: I haz the power, 2016-09-09 14:51:23]

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

[lfg, 2016-09-09 14:53:54]

On 2016/09/09 05:24:44, dcheng wrote:
> > I don't mean just making it private; we should make the overrides asserts,
and
> > then change the private implementation to be a non-virtual helper method.
> 
> (If you don't think people accidentally calling this via WebView will be an
> issue in the near future though, then this patch LGTM)

I don't think this is necessary. With the function being private in WebViewImpl,
no one can call it without getting the widget from the WebView.

https://codereview.chromium.org/2288313002/diff/120001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2288313002/diff/120001/content/renderer/rende...
content/renderer/render_widget.cc:1145: if (GetWebWidget())
On 2016/09/08 22:44:02, esprehn wrote:
> In blink we'd do:
> 
> if (WebWidget* widget = GetWebWidget())
>   widget->...()
> 
> to avoid the duplicate virtual calls.

Done.

[lfg, 2016-09-09 14:59:13]

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

[lfg, 2016-09-09 14:59:14]

+piman for content/ owners review.

[commit-bot: I haz the power, 2016-09-09 16:35:07]

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

[commit-bot: I haz the power, 2016-09-09 16:35:08]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-09-09 19:00:04]

On 2016/09/09 14:53:54, lfg wrote:
> On 2016/09/09 05:24:44, dcheng wrote:
> > > I don't mean just making it private; we should make the overrides asserts,
> and
> > > then change the private implementation to be a non-virtual helper method.
> > 
> > (If you don't think people accidentally calling this via WebView will be an
> > issue in the near future though, then this patch LGTM)
> 
> I don't think this is necessary. With the function being private in
WebViewImpl,
> no one can call it without getting the widget from the WebView.
> 

That's not true though:

WebWidget* view = GetView();
view->applyReplacementRange(...);  // Doesn't work

WebWidget* widget = GetView();
widget->applyReplacementRange(...);  // Works!

>
https://codereview.chromium.org/2288313002/diff/120001/content/renderer/rende...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/2288313002/diff/120001/content/renderer/rende...
> content/renderer/render_widget.cc:1145: if (GetWebWidget())
> On 2016/09/08 22:44:02, esprehn wrote:
> > In blink we'd do:
> > 
> > if (WebWidget* widget = GetWebWidget())
> >   widget->...()
> > 
> > to avoid the duplicate virtual calls.
> 
> Done.

[dcheng, 2016-09-09 19:01:51]

On 2016/09/09 19:00:04, dcheng wrote:
> On 2016/09/09 14:53:54, lfg wrote:
> > On 2016/09/09 05:24:44, dcheng wrote:
> > > > I don't mean just making it private; we should make the overrides
asserts,
> > and
> > > > then change the private implementation to be a non-virtual helper
method.
> > > 
> > > (If you don't think people accidentally calling this via WebView will be
an
> > > issue in the near future though, then this patch LGTM)
> > 
> > I don't think this is necessary. With the function being private in
> WebViewImpl,
> > no one can call it without getting the widget from the WebView.
> > 
> 
> That's not true though:
> 
> WebWidget* view = GetView();
> view->applyReplacementRange(...);  // Doesn't work

Sorry, this should be WebView* view = GetView().

> 
> WebWidget* widget = GetView();
> widget->applyReplacementRange(...);  // Works!
> 
> >
>
https://codereview.chromium.org/2288313002/diff/120001/content/renderer/rende...
> > File content/renderer/render_widget.cc (right):
> > 
> >
>
https://codereview.chromium.org/2288313002/diff/120001/content/renderer/rende...
> > content/renderer/render_widget.cc:1145: if (GetWebWidget())
> > On 2016/09/08 22:44:02, esprehn wrote:
> > > In blink we'd do:
> > > 
> > > if (WebWidget* widget = GetWebWidget())
> > >   widget->...()
> > > 
> > > to avoid the duplicate virtual calls.
> > 
> > Done.

[lfg, 2016-09-09 19:02:31]

On 2016/09/09 19:00:04, dcheng wrote:
> > 
> > I don't think this is necessary. With the function being private in
> WebViewImpl,
> > no one can call it without getting the widget from the WebView.
> > 
> 
> That's not true though:
> 
> WebWidget* view = GetView();
> view->applyReplacementRange(...);  // Doesn't work
> 
> WebWidget* widget = GetView();
> widget->applyReplacementRange(...);  // Works!

Since the inheritance is now protected, that cast doesn't compile anymore. You'd
need to explicitly do view->widget()->applyReplacementRange(), which would make
you reconsider, especially since the widget() method has a big TODO that it will
be removed.

[dcheng, 2016-09-09 19:05:09]

On 2016/09/09 19:02:31, lfg wrote:
> On 2016/09/09 19:00:04, dcheng wrote:
> > > 
> > > I don't think this is necessary. With the function being private in
> > WebViewImpl,
> > > no one can call it without getting the widget from the WebView.
> > > 
> > 
> > That's not true though:
> > 
> > WebWidget* view = GetView();
> > view->applyReplacementRange(...);  // Doesn't work
> > 
> > WebWidget* widget = GetView();
> > widget->applyReplacementRange(...);  // Works!
> 
> Since the inheritance is now protected, that cast doesn't compile anymore.
You'd
> need to explicitly do view->widget()->applyReplacementRange(), which would
make
> you reconsider, especially since the widget() method has a big TODO that it
will
> be removed.

OK, thanks, I forgot we made it protected inheritance. Thanks for clarifying.

[lfg, 2016-09-09 19:05:38]

On 2016/09/09 19:02:31, lfg wrote:
> On 2016/09/09 19:00:04, dcheng wrote:
> > > 
> > > I don't think this is necessary. With the function being private in
> > WebViewImpl,
> > > no one can call it without getting the widget from the WebView.
> > > 
> > 
> > That's not true though:
> > 
> > WebWidget* view = GetView();
> > view->applyReplacementRange(...);  // Doesn't work
> > 
> > WebWidget* widget = GetView();
> > widget->applyReplacementRange(...);  // Works!
> 
> Since the inheritance is now protected, that cast doesn't compile anymore.
You'd
> need to explicitly do view->widget()->applyReplacementRange(), which would
make
> you reconsider, especially since the widget() method has a big TODO that it
will
> be removed.

By "that cast", I mean the implicit cast in WebWidget* widget = ((WebView*)
something).

[piman, 2016-09-13 00:27:06]

https://codereview.chromium.org/2288313002/diff/140001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2288313002/diff/140001/content/renderer/rende...
content/renderer/render_widget.cc:1072:
GetWebWidget()->resizeVisualViewport(visual_viewport_size);
Soo... why do we use GetWebWidget() sometimes, and webwidget_ some other times?
They will be different, I guess. What is the rule? Can we document it? Can we
test it?

[lfg, 2016-09-13 19:00:21]

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

[commit-bot: I haz the power, 2016-09-13 19:00:44]

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-09-13 19:13:47]

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

[commit-bot: I haz the power, 2016-09-13 19:13:48]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[lfg, 2016-09-13 19:33:28]

Patchset #10 (id:180001) has been deleted

[lfg, 2016-09-13 19:34:07]

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

[commit-bot: I haz the power, 2016-09-13 19:34:47]

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

[lfg, 2016-09-13 19:37:57]

piman, please, take another look.

https://codereview.chromium.org/2288313002/diff/140001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2288313002/diff/140001/content/renderer/rende...
content/renderer/render_widget.cc:1072:
GetWebWidget()->resizeVisualViewport(visual_viewport_size);
On 2016/09/13 00:27:05, piman OOO back 2016-09-12 wrote:
> Soo... why do we use GetWebWidget() sometimes, and webwidget_ some other
times?
> They will be different, I guess. What is the rule? Can we document it? Can we
> test it?

Basically we should always prefer GetWebWidget() over webwidget_.

I thought I had switched every usage of webwidget_ to GetWebWidget(), but it
seems I forgot a few cases. I've renamed webwidget_ to webwidget_internal_ so
that nothing will be missed and added a comment to render_widget.h to avoid
using webwidget_internal_. All use cases of webwidget_ should be now using
GetWebWidget().

The only exception is the initialization and destruction, which currently
handles the lifetime of the WebView and can't be changed until we can finally
split RenderView and RenderWidget. That should happen as we continue the
refactor, but it'll still take at least a few months.

[piman, 2016-09-13 19:41:51]

lgtm

[lfg, 2016-09-13 20:43:08]

The CQ bit was unchecked by lfg@chromium.org

[lfg, 2016-09-13 20:43:11]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-09-13 20:43:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2288313002/#ps200001
(title: "rename webwidget_ -> webwidget_internal_")

[commit-bot: I haz the power, 2016-09-13 20:43:35]

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-09-13 20:59:44]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-09-13 21:05:32]

Description was changed from

==========
Return the WebViewFrameWidget in RenderWidget::webwidget() if there
is one.

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

to

==========
Return the WebViewFrameWidget in RenderWidget::webwidget() if there
is one.

BUG=419087

Committed: https://crrev.com/8ff3391e7f1a6cabb73807bf781cf949326e0081
Cr-Commit-Position: refs/heads/master@{#418365}
==========

[commit-bot: I haz the power, 2016-09-13 21:05:34]

Patchset 10 (id:??) landed as
https://crrev.com/8ff3391e7f1a6cabb73807bf781cf949326e0081
Cr-Commit-Position: refs/heads/master@{#418365}