Use observer pattern instead of sniffing SwapCompositorFrame IPC

Use observer pattern instead of sniffing SwapCompositorFrame IPC

ViewHostMsg_SwapCompositorFrame will be gone once we switch to mojo, so
we need a different mechanism for observing arrival of CompositorFrames
in classes other than RenderWidgetHostImpl. Now RenderWidgetHostImpl
notifies WebContentsImpl on arrival of a CompositorFrame and other
classes can become a WebContentsObserver to get notified.

TBR=fsamuel@chromium.org

Review-Url: https://codereview.chromium.org/2780373002
Cr-Commit-Position: refs/heads/master@{#461869}
Committed: https://chromium.googlesource.com/chromium/src/+/90fb8163ea87ebee81746205d0f6b80fe508a70f

[Saman Sami, 2017-03-30 01:31:40]

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

[commit-bot: I haz the power, 2017-03-30 01:32:52]

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

[Saman Sami, 2017-03-30 01:51:25]

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

[commit-bot: I haz the power, 2017-03-30 01:52:41]

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, 2017-03-30 04:35:39]

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

[commit-bot: I haz the power, 2017-03-30 04:35:40]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-30 15:56:18]

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

[commit-bot: I haz the power, 2017-03-30 15:57:00]

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

[Saman Sami, 2017-03-30 16:28:31]

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

[commit-bot: I haz the power, 2017-03-30 16:28:51]

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

[Saman Sami, 2017-03-30 16:44:49]

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

[commit-bot: I haz the power, 2017-03-30 16:45:16]

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

[Saman Sami, 2017-03-30 16:57:01]

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

[Saman Sami, 2017-03-30 16:57:27]

Description was changed from

==========
Test
==========

to

==========
Use observer pattern instead of sniffing SwapCompositorFrame IPC
==========

[commit-bot: I haz the power, 2017-03-30 16:57:28]

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

[Saman Sami, 2017-03-30 17:04:04]

Description was changed from

==========
Use observer pattern instead of sniffing SwapCompositorFrame IPC
==========

to

==========
Use observer pattern instead of sniffing SwapCompositorFrame IPC

ViewHostMsg_SwapCompositorFrame will be gone once we switch to mojo, so
we need a different mechanism for observing arrival of CompositorFrames
in classes other than RenderWidgetHostImpl. Now RenderWidgetHostImpl
notifies WebContentsImpl on arrival of a CompositorFrame and other
classes can become a WebContentsObserver to get notified.
==========

[Saman Sami, 2017-03-30 17:09:56]

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

[commit-bot: I haz the power, 2017-03-30 17:10:13]

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

[Saman Sami, 2017-03-30 17:10:19]

samans@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Saman Sami, 2017-03-30 17:10:46]

samans@chromium.org changed reviewers:
- fsamuel@chromium.org

[Saman Sami, 2017-03-30 17:13:43]

samans@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Saman Sami, 2017-03-30 17:14:06]

PTAL

[Saman Sami, 2017-03-30 17:15:07]

I'm still not entirely sure that all the bots will be green but it might be
worth a look.

[Saman Sami, 2017-03-30 17:38:10]

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

[Saman Sami, 2017-03-30 17:38:11]

OK looks like the failing test passed
piman PTAL

[Fady Samuel, 2017-03-30 17:42:05]

I like that this decouples us further from Chrome IPC, but I'm not sure how I
feel about CompositorFrameMetadata being exposed out of the content API. I will
defer to Antoine.

[Saman Sami, 2017-03-30 17:46:05]

On 2017/03/30 17:42:05, Fady Samuel wrote:
> I like that this decouples us further from Chrome IPC, but I'm not sure how I
> feel about CompositorFrameMetadata being exposed out of the content API. I
will
> defer to Antoine.

Is there a non-public WebContentsObserver? I would also prefer that. The
FrameWatcher thing is already in public though...

[commit-bot: I haz the power, 2017-03-30 18:34:06]

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

[commit-bot: I haz the power, 2017-03-30 18:34:07]

Dry run: This issue passed the CQ dry run.

[piman, 2017-03-30 22:46:35]

On 2017/03/30 17:46:05, Saman Sami wrote:
> On 2017/03/30 17:42:05, Fady Samuel wrote:
> > I like that this decouples us further from Chrome IPC, but I'm not sure how
I
> > feel about CompositorFrameMetadata being exposed out of the content API. I
> will
> > defer to Antoine.
> 
> Is there a non-public WebContentsObserver? I would also prefer that. The
> FrameWatcher thing is already in public though...

This is a great cleanup, but as Fadi mentions, I would prefer if we didn't
expose cc internals to the public content API.

I think FrameWatcher is only used for tests, and in those, the only thing used
from the meta data is the root scroll offset - which is a concept that I think
makes sense to expose at the content API level.
So maybe we can make ReceivedCompositorFrame pass the scroll offset instead of
the whole CompositorFrameMetadata, wdyt?

[Saman Sami, 2017-03-30 23:31:10]

On 2017/03/30 22:46:35, piman wrote:
> On 2017/03/30 17:46:05, Saman Sami wrote:
> > On 2017/03/30 17:42:05, Fady Samuel wrote:
> > > I like that this decouples us further from Chrome IPC, but I'm not sure
how
> I
> > > feel about CompositorFrameMetadata being exposed out of the content API. I
> > will
> > > defer to Antoine.
> > 
> > Is there a non-public WebContentsObserver? I would also prefer that. The
> > FrameWatcher thing is already in public though...
> 
> This is a great cleanup, but as Fadi mentions, I would prefer if we didn't
> expose cc internals to the public content API.
> 
> I think FrameWatcher is only used for tests, and in those, the only thing used
> from the meta data is the root scroll offset - which is a concept that I think
> makes sense to expose at the content API level.
> So maybe we can make ReceivedCompositorFrame pass the scroll offset instead of
> the whole CompositorFrameMetadata, wdyt?

Thanks, but what about dev tools? We need more than just scroll offset there. I
would rather have the same observing system for both dev tools and tests.

[piman, 2017-03-30 23:54:13]

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

[piman, 2017-03-30 23:54:14]

Mmh, devtools is inside of content, so it shouldn't have to go through the
public API. dgozman: thoughts?

(note, this is yet another reason why I think dumping the kitchen sink into
CompositorFrameMetadata is bad practice because of coupling)

[Saman Sami, 2017-03-31 00:29:37]

On 2017/03/30 23:54:14, piman wrote:
> Mmh, devtools is inside of content, so it shouldn't have to go through the
> public API. dgozman: thoughts?
> 
> (note, this is yet another reason why I think dumping the kitchen sink into
> CompositorFrameMetadata is bad practice because of coupling)

This is somewhat hacky, but one way to get around this is calling
ReceivedCompositorFrame without metadata and have a separate method in
WebContentsImpl that the observers inside content/ can call to get the latest
metadata... The thing is, I don't want to pass only scroll offset to the tests;
that could be inflexible for future tests. Also, I don't want to have two
separate systems, one for tests and one for dev tools. The WebContentsObserver
thing is a public class and I don't think there is an equivalent content-only
feature?

[dgozman, 2017-03-31 01:07:32]

On 2017/03/30 23:54:14, piman wrote:
> Mmh, devtools is inside of content, so it shouldn't have to go through the
> public API. dgozman: thoughts?
> 
> (note, this is yet another reason why I think dumping the kitchen sink into
> CompositorFrameMetadata is bad practice because of coupling)

I'm fine with any inside-content API for devtools. Look at
RenderFrameDevToolsAgentHost::SignalSynchronousSwapCompositorFrame for example.

[Saman Sami, 2017-04-04 14:21:16]

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

[commit-bot: I haz the power, 2017-04-04 14:21:37]

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

[Saman Sami, 2017-04-04 15:31:58]

PTAL
We no longer send the metadata as a parameter. RenderWidgetHostImpl keeps the
last metadata that classes inside content/ can access.

[commit-bot: I haz the power, 2017-04-04 15:42:29]

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

[commit-bot: I haz the power, 2017-04-04 15:42:30]

Dry run: This issue passed the CQ dry run.

[piman, 2017-04-04 17:34:46]

I like the approach. Couple minor things.

https://codereview.chromium.org/2780373002/diff/130001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2780373002/diff/130001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2612:
last_frame_metadata_ = frame.metadata.Clone();
Could we move this to after l.2636? 2 reasons:
1- we don't want to update the metadata if we reject the bad message
2- we modify frame.metadata below, I think we want last_frame_metadata_ to be
consistent with that.

https://codereview.chromium.org/2780373002/diff/130001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2614:
delegate_->DidReceiveCompositorFrame();
Should this move to the end of the function, or at least after l.2658? For
consistent state it may be better to signal the observers after the view was
updated.

https://codereview.chromium.org/2780373002/diff/130001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2780373002/diff/130001/content/public/test/br...
content/public/test/browser_test_utils.h:543: FrameWatcher(WebContents*
web_contents);
nit: explicit

[Saman Sami, 2017-04-04 17:48:08]

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

[commit-bot: I haz the power, 2017-04-04 17:48:41]

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

[Saman Sami, 2017-04-04 17:49:54]

PTAL

https://codereview.chromium.org/2780373002/diff/130001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2780373002/diff/130001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2612:
last_frame_metadata_ = frame.metadata.Clone();
On 2017/04/04 17:34:46, piman wrote:
> Could we move this to after l.2636? 2 reasons:
> 1- we don't want to update the metadata if we reject the bad message
> 2- we modify frame.metadata below, I think we want last_frame_metadata_ to be
> consistent with that.
> 

Good point.

https://codereview.chromium.org/2780373002/diff/130001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:2614:
delegate_->DidReceiveCompositorFrame();
On 2017/04/04 17:34:46, piman wrote:
> Should this move to the end of the function, or at least after l.2658? For
> consistent state it may be better to signal the observers after the view was
> updated.

Done.

https://codereview.chromium.org/2780373002/diff/130001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2780373002/diff/130001/content/public/test/br...
content/public/test/browser_test_utils.h:543: FrameWatcher(WebContents*
web_contents);
On 2017/04/04 17:34:46, piman wrote:
> nit: explicit

Done.

[commit-bot: I haz the power, 2017-04-04 18:51:53]

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

[commit-bot: I haz the power, 2017-04-04 18:51:54]

Dry run: This issue passed the CQ dry run.

[piman, 2017-04-04 19:59:43]

lgtm

[Saman Sami, 2017-04-04 21:14:46]

Description was changed from

==========
Use observer pattern instead of sniffing SwapCompositorFrame IPC

ViewHostMsg_SwapCompositorFrame will be gone once we switch to mojo, so
we need a different mechanism for observing arrival of CompositorFrames
in classes other than RenderWidgetHostImpl. Now RenderWidgetHostImpl
notifies WebContentsImpl on arrival of a CompositorFrame and other
classes can become a WebContentsObserver to get notified.
==========

to

==========
Use observer pattern instead of sniffing SwapCompositorFrame IPC

ViewHostMsg_SwapCompositorFrame will be gone once we switch to mojo, so
we need a different mechanism for observing arrival of CompositorFrames
in classes other than RenderWidgetHostImpl. Now RenderWidgetHostImpl
notifies WebContentsImpl on arrival of a CompositorFrame and other
classes can become a WebContentsObserver to get notified.

TBR=fsamuel@chromium.org
==========

[Saman Sami, 2017-04-04 22:14:58]

The CQ bit was checked by samans@chromium.org

[commit-bot: I haz the power, 2017-04-04 22:16:18]

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

[Fady Samuel, 2017-04-04 22:25:15]

lgtm

[commit-bot: I haz the power, 2017-04-04 22:30:54]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1491344098266370,
"parent_rev": "2f3a22ee575562005e7aa02a11010c3896ec35f3", "commit_rev":
"90fb8163ea87ebee81746205d0f6b80fe508a70f"}

[commit-bot: I haz the power, 2017-04-04 22:31:45]

Description was changed from

==========
Use observer pattern instead of sniffing SwapCompositorFrame IPC

ViewHostMsg_SwapCompositorFrame will be gone once we switch to mojo, so
we need a different mechanism for observing arrival of CompositorFrames
in classes other than RenderWidgetHostImpl. Now RenderWidgetHostImpl
notifies WebContentsImpl on arrival of a CompositorFrame and other
classes can become a WebContentsObserver to get notified.

TBR=fsamuel@chromium.org
==========

to

==========
Use observer pattern instead of sniffing SwapCompositorFrame IPC

ViewHostMsg_SwapCompositorFrame will be gone once we switch to mojo, so
we need a different mechanism for observing arrival of CompositorFrames
in classes other than RenderWidgetHostImpl. Now RenderWidgetHostImpl
notifies WebContentsImpl on arrival of a CompositorFrame and other
classes can become a WebContentsObserver to get notified.

TBR=fsamuel@chromium.org

Review-Url: https://codereview.chromium.org/2780373002
Cr-Commit-Position: refs/heads/master@{#461869}
Committed:
https://chromium.googlesource.com/chromium/src/+/90fb8163ea87ebee81746205d0f6...
==========

[commit-bot: I haz the power, 2017-04-04 22:31:46]

Committed patchset #9 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/90fb8163ea87ebee81746205d0f6...