Hoist SetNeedsBeginFrame messages up to the RWHostImpl

Hoist SetNeedsBeginFrame messages up to the RWHostImpl

Previously, each RenderWidgetHostView class handled all of the
SetNeedsBeginFrameMessages sent from the renderer.  However, in the case
that the render widget is initialized before the view is set, then the
SetNeedsBeginFrame message is dropped and then that renderer is never
ticked with begin frame messages.

To fix this, have the RenderWidgetHostImpl handle SetNeedsBeginFrame
messages, remember the value, and forward it to the view whenever the
view is set so the message is not lost.

R=sievers@chromium.org
BUG=635476
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/71de85ec0f6f6259824758b10db751b65b005bd2
Cr-Commit-Position: refs/heads/master@{#413533}

[enne (OOO), 2016-08-11 22:40:56]

Description was changed from

==========
Hoist SetNeedsBeginFrame messages up to the RWHostImpl

Previously, each RenderWidgetHostView class handled all of the
SetNeedsBeginFrameMessages sent from the renderer.  However, in the case
that the render widget is initialized before the view is set, then the
SetNeedsBeginFrame message is dropped and then that renderer is never
ticked with begin frame messages.

To fix this, have the RenderWidgetHostImpl handle SetNeedsBeginFrame
messages, remember the value, and forward it to the view whenever the
view is set so the message is not lost.

R=sievers@chromium.org
BUG=635476
==========

to

==========
Hoist SetNeedsBeginFrame messages up to the RWHostImpl

Previously, each RenderWidgetHostView class handled all of the
SetNeedsBeginFrameMessages sent from the renderer.  However, in the case
that the render widget is initialized before the view is set, then the
SetNeedsBeginFrame message is dropped and then that renderer is never
ticked with begin frame messages.

To fix this, have the RenderWidgetHostImpl handle SetNeedsBeginFrame
messages, remember the value, and forward it to the view whenever the
view is set so the message is not lost.

R=sievers@chromium.org
BUG=635476
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[enne (OOO), 2016-08-11 22:43:35]

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

[commit-bot: I haz the power, 2016-08-11 22:44:55]

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-11 22:58:04]

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

[commit-bot: I haz the power, 2016-08-11 22:58:05]

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

[enne (OOO), 2016-08-12 00:07:27]

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

[commit-bot: I haz the power, 2016-08-12 00:08:18]

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-12 00:41:11]

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

[commit-bot: I haz the power, 2016-08-12 00:41:12]

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...)

[enne (OOO), 2016-08-12 18:30:24]

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

[commit-bot: I haz the power, 2016-08-12 18:30:55]

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-12 19:46:53]

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

[commit-bot: I haz the power, 2016-08-12 19:46:54]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2016-08-12 21:54:24]

I fixed the test errors, so this should be ready for review.

[no sievers, 2016-08-15 17:39:13]

https://codereview.chromium.org/2242613003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/2242613003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:255:
RenderWidgetHostViewChildFrame::SetNeedsBeginFrames(needs_begin_frames);
Is this an intentional change in behavior that now both the |platform_view_| and
the base class end up requesting BeginFrames?

https://codereview.chromium.org/2242613003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:332: bool
RenderWidgetHostViewGuest::OnMessageReceived(const IPC::Message& msg) {
Is this unused now?
And can it also be removed from RenderWidgetHostViewBase::OnMessageReceived()
incl. the call site from RWHImpl (view_->OnMessageReceived(msg))?

[enne (OOO), 2016-08-15 20:49:36]

https://codereview.chromium.org/2242613003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/2242613003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:255:
RenderWidgetHostViewChildFrame::SetNeedsBeginFrames(needs_begin_frames);
On 2016/08/15 at 17:39:13, sievers wrote:
> Is this an intentional change in behavior that now both the |platform_view_|
and the base class end up requesting BeginFrames?

That's fair.  Previously this would only go to the platform view.  This doesn't
seem necessary.  Removed.

https://codereview.chromium.org/2242613003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:332: bool
RenderWidgetHostViewGuest::OnMessageReceived(const IPC::Message& msg) {
On 2016/08/15 at 17:39:13, sievers wrote:
> Is this unused now?
> And can it also be removed from RenderWidgetHostViewBase::OnMessageReceived()
incl. the call site from RWHImpl (view_->OnMessageReceived(msg))?

Can platform_view be a RenderWidgetHostViewMac? if so, then it's not unused. 
RenderWidgetHostViewMac and RenderWidgetHostViewAndroid both have other
messages, so it can't be removed from RenderWidgetHostViewBase or
RenderWidgetHostImpl.

[enne (OOO), 2016-08-16 00:00:49]

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

[commit-bot: I haz the power, 2016-08-16 00:01:07]

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-16 00:54:02]

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

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

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2016-08-17 20:01:59]

Ping?

[enne (OOO), 2016-08-17 20:02:18]

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

[commit-bot: I haz the power, 2016-08-17 20:02:46]

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-17 20:55:30]

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

[commit-bot: I haz the power, 2016-08-17 20:55:31]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2016-08-18 00:36:57]

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

[enne (OOO), 2016-08-18 00:37:17]

sievers appears like he'll be out for the rest of the week.

+piman as content OWNER alternate

[enne (OOO), 2016-08-22 17:50:27]

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

[commit-bot: I haz the power, 2016-08-22 17:51:08]

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

[no sievers, 2016-08-22 18:13:56]

lgtm with 1 comment

https://codereview.chromium.org/2242613003/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (left):

https://codereview.chromium.org/2242613003/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:669: bool
needs_begin_frames) {
I don't quite understand when this codepath is ever used - i.e. when this is not
a RWHVGuest which overrides this (through the generic OnMsgReceived forwarding
to the |platform_view_| before your patch, and the SetNeedsBF() override after),
other than in tests maybe.

But your patch doesn't look like it would change the behavior. Might be worth
following up though if we maybe route the SetNeeds...() vs. actual sending of BF
through different views - it's definitely confusing that the logic exists here
and then SetNeedsBF() (and *only* that is overriden in RWHVGuest).

[enne (OOO), 2016-08-22 18:25:15]

Thanks.  I'll look into simplifying this in a followup patch.

[enne (OOO), 2016-08-22 19:00:08]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2016-08-22 19:00:11]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-08-22 19:00:56]

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-22 21:12:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-22 21:13:41]

Description was changed from

==========
Hoist SetNeedsBeginFrame messages up to the RWHostImpl

Previously, each RenderWidgetHostView class handled all of the
SetNeedsBeginFrameMessages sent from the renderer.  However, in the case
that the render widget is initialized before the view is set, then the
SetNeedsBeginFrame message is dropped and then that renderer is never
ticked with begin frame messages.

To fix this, have the RenderWidgetHostImpl handle SetNeedsBeginFrame
messages, remember the value, and forward it to the view whenever the
view is set so the message is not lost.

R=sievers@chromium.org
BUG=635476
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Hoist SetNeedsBeginFrame messages up to the RWHostImpl

Previously, each RenderWidgetHostView class handled all of the
SetNeedsBeginFrameMessages sent from the renderer.  However, in the case
that the render widget is initialized before the view is set, then the
SetNeedsBeginFrame message is dropped and then that renderer is never
ticked with begin frame messages.

To fix this, have the RenderWidgetHostImpl handle SetNeedsBeginFrame
messages, remember the value, and forward it to the view whenever the
view is set so the message is not lost.

R=sievers@chromium.org
BUG=635476
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/71de85ec0f6f6259824758b10db751b65b005bd2
Cr-Commit-Position: refs/heads/master@{#413533}
==========

[commit-bot: I haz the power, 2016-08-22 21:13:42]

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