cc: Make separate interface for BeginFrame ipc from OutputSurface

cc: Make separate interface for BeginFrame ipc from OutputSurface

Decouple BeginFrame message from OutputSurface and creates new interface.

R=danakj@chromium.org, brianderson@chromium.org, skyostil@chromium.org, piman@chromium.org, boliu@chromium.org
BUG=416760
TEST=cc_unittests, content_unittests

Committed: https://crrev.com/a7e3ac41cb5b104e6d96fc31c2a1e5330b601ec7
Cr-Commit-Position: refs/heads/master@{#303715}

[simonhong, 2014-10-01 08:30:52]

simonhong@chromium.org changed reviewers:
+ mithro@mithis.com, piman@chromium.org
- mithro@chromium.org

[simonhong, 2014-10-02 04:52:18]

Patchset #1 (id:1) has been deleted

[simonhong, 2014-10-10 04:59:31]

danakj@, would you please review overall design except for unit test?
I'm working on unittests.
Thanks!

[Sami, 2014-10-10 13:32:33]

I like where this is going.

https://codereview.chromium.org/619843002/diff/300001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/619843002/diff/300001/content/common/view_mes...
content/common/view_messages.h:966:
IPC_MESSAGE_ROUTED1(ViewHostMsg_SetNeedsBeginFrame,
Please still keep this in the "messages sent from the renderer to the browser"
section.

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_external_begin_frame_source.h (right):

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
content/renderer/gpu/compositor_external_begin_frame_source.h:39: virtual void
OnNeedsBeginFramesChange(bool needs_begin_frames) OVERRIDE;
nit: I think we can now write just

  virtual void OnNeedsBeginFramesChange(bool needs_begin_frames) override;

Not sure if the virtual still needs to be there. Eventually it is going away.

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
content/renderer/gpu/compositor_output_surface.cc:40:
ViewMsg_UpdateVSyncParameters::ID,
Do we still need this message in this new world?

[danakj, 2014-10-10 16:03:36]

Ya looks cool

https://codereview.chromium.org/619843002/diff/300001/cc/layers/layer_unittes...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/619843002/diff/300001/cc/layers/layer_unittes...
cc/layers/layer_unittest.cc:46: scoped_ptr<ExternalBeginFrameSource>());
pass nullptr instead

https://codereview.chromium.org/619843002/diff/300001/cc/layers/layer_unittes...
cc/layers/layer_unittest.cc:940: scoped_ptr<ExternalBeginFrameSource>()).Pass();
nullptr here, etc

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:165: // A frame source that delivers
BeginFrame from outside of CC layer.
from outside of cc.

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:166: class CC_EXPORT ExternalBeginFrameSource
: public BeginFrameSourceMixIn {
This should inherit from BeginFrameSource (or why not just add SetClientReady to
BeginFrameSource and use that?)

The MixIn has concrete implementations, it should inherit from an abstract
class, not vice versa. And users of ExternalBFS shouldn't care that the MixIn
class is implementing parts of the interface.

https://codereview.chromium.org/619843002/diff/300001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/619843002/diff/300001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:425: void AsValueInto(base::debug::TracedValue*
value) const {
keep the defn in the .cc

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_external_begin_frame_source.cc (right):

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
content/renderer/gpu/compositor_external_begin_frame_source.cc:49: Send(new
ViewHostMsg_SetNeedsBeginFrame(routing_id_, needs_begin_frames));
What's sending us here on shutdown now? Do we need to get here still?
ThreadProxy used to call SetNeedsBeginFrames(false) (which of course wouldn't
work in a single threaded scheduler world, heh).

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_external_begin_frame_source.h (right):

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
content/renderer/gpu/compositor_external_begin_frame_source.h:29: : public
cc::ExternalBeginFrameSource,
This would inherit from the BFSMixin and also the ExternalBFS.

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
content/renderer/gpu/compositor_external_begin_frame_source.h:39: virtual void
OnNeedsBeginFramesChange(bool needs_begin_frames) OVERRIDE;
On 2014/10/10 13:32:33, Sami wrote:
> nit: I think we can now write just
> 
>   virtual void OnNeedsBeginFramesChange(bool needs_begin_frames) override;

+1

> Not sure if the virtual still needs to be there. Eventually it is going away.

It does for now, clang hasn't rolled yet.

https://codereview.chromium.org/619843002/diff/300001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/619843002/diff/300001/content/renderer/render...
content/renderer/render_thread_impl.cc:912:
scoped_refptr<base::MessageLoopProxy> output_surface_loop;
nit: rename to compositor_impl_side_loop?

[piman, 2014-10-10 20:39:46]

https://codereview.chromium.org/619843002/diff/300001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/619843002/diff/300001/content/renderer/render...
content/renderer/render_thread_impl.cc:924:
AddFilter(begin_frame_source_filter_.get());
I would prefer if there was still a single filter. We found scaling problems
with filters, because every IPC message needs to go through every filter, so a
new filter adds overhead to *all* messages, including unrelated ones, see
crbug.com/340881

Since both CompositorOutputSurface's and CompositorExternalBeginFrameSource's
filters do essentially the same thing (find messages to handle on the compositor
thread and forward to that), we can make a single filter, quickly reject
irrelevant messages, then post onto the compositor thread and dispatch between
CompositorOutputSurface and CompositorExternalBeginFrameSource based on the type
(bonus points if we can fold InputEventFilter into the same logic).

[mithro-old, 2014-10-13 02:56:21]

Great start!

I think we should make sure to keep this just to Android using the new IPC path
and then follow up with making the other platforms also use this.

Other reviewers, how does that sound?

Tim 'mithro' Ansell

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:84:
external_begin_frame_source_internal_(external_begin_frame_source.Pass()),
Can we just store this in the primary_frame_source_internal_ instead?

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:104:
external_begin_frame_source_internal_->SetClientReady();
I'm unsure what the "SetClientReady" function is for?

https://codereview.chromium.org/619843002/diff/300001/cc/test/fake_output_sur...
File cc/test/fake_output_surface_client.h (left):

https://codereview.chromium.org/619843002/diff/300001/cc/test/fake_output_sur...
cc/test/fake_output_surface_client.h:20: swap_count_(0),
Why is swap_count_ being removed here too?

https://codereview.chromium.org/619843002/diff/300001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/619843002/diff/300001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:47: class ExternalBeginFrameSource;
Can we get away with the layer_tree_host not needing to know anything about this
interface?

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
content/renderer/gpu/compositor_output_surface.cc:40:
ViewMsg_UpdateVSyncParameters::ID,
On 2014/10/10 13:32:33, Sami wrote:
> Do we still need this message in this new world?

/Eventually/ we can remove this UpdateVSyncParameters, but we need to make
everything use the BeginFrameArgs API first.

[simonhong, 2014-10-13 04:38:27]

mithro@, I removed Android define because it will be removed in a short
term by next unified BeginFrame patch :)


On Mon, Oct 13, 2014 at 11:56 AM, <mithro@mithis.com> wrote:

> Great start!
>
> I think we should make sure to keep this just to Android using the new IPC
> path
> and then follow up with making the other platforms also use this.
>
> Other reviewers, how does that sound?
>
> Tim 'mithro' Ansell
>
>
> https://codereview.chromium.org/619843002/diff/300001/cc/
> scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
>
> https://codereview.chromium.org/619843002/diff/300001/cc/
> scheduler/scheduler.cc#newcode84
> cc/scheduler/scheduler.cc:84:
> external_begin_frame_source_internal_(external_begin_frame_source.Pass()),
> Can we just store this in the primary_frame_source_internal_ instead?
>
> https://codereview.chromium.org/619843002/diff/300001/cc/
> scheduler/scheduler.cc#newcode104
> cc/scheduler/scheduler.cc:104:
> external_begin_frame_source_internal_->SetClientReady();
> I'm unsure what the "SetClientReady" function is for?
>
> https://codereview.chromium.org/619843002/diff/300001/cc/
> test/fake_output_surface_client.h
> File cc/test/fake_output_surface_client.h (left):
>
> https://codereview.chromium.org/619843002/diff/300001/cc/
> test/fake_output_surface_client.h#oldcode20
> cc/test/fake_output_surface_client.h:20: swap_count_(0),
> Why is swap_count_ being removed here too?
>
> https://codereview.chromium.org/619843002/diff/300001/cc/
> trees/layer_tree_host.h
> File cc/trees/layer_tree_host.h (right):
>
> https://codereview.chromium.org/619843002/diff/300001/cc/
> trees/layer_tree_host.h#newcode47
> cc/trees/layer_tree_host.h:47: class ExternalBeginFrameSource;
> Can we get away with the layer_tree_host not needing to know anything
> about this interface?
>
> https://codereview.chromium.org/619843002/diff/300001/
> content/renderer/gpu/compositor_output_surface.cc
> File content/renderer/gpu/compositor_output_surface.cc (right):
>
> https://codereview.chromium.org/619843002/diff/300001/
> content/renderer/gpu/compositor_output_surface.cc#newcode40
> content/renderer/gpu/compositor_output_surface.cc:40:
> ViewMsg_UpdateVSyncParameters::ID,
> On 2014/10/10 13:32:33, Sami wrote:
>
>> Do we still need this message in this new world?
>>
>
> /Eventually/ we can remove this UpdateVSyncParameters, but we need to
> make everything use the BeginFrameArgs API first.
>
> https://codereview.chromium.org/619843002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2014-10-15 01:04:22]

PTAL again!
Thanks!

https://codereview.chromium.org/619843002/diff/300001/cc/layers/layer_unittes...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/619843002/diff/300001/cc/layers/layer_unittes...
cc/layers/layer_unittest.cc:46: scoped_ptr<ExternalBeginFrameSource>());
On 2014/10/10 16:03:35, danakj wrote:
> pass nullptr instead

Done.

https://codereview.chromium.org/619843002/diff/300001/cc/layers/layer_unittes...
cc/layers/layer_unittest.cc:940: scoped_ptr<ExternalBeginFrameSource>()).Pass();
On 2014/10/10 16:03:35, danakj wrote:
> nullptr here, etc

Done.

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:165: // A frame source that delivers
BeginFrame from outside of CC layer.
On 2014/10/10 16:03:35, danakj wrote:
> from outside of cc.

Done.

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:166: class CC_EXPORT ExternalBeginFrameSource
: public BeginFrameSourceMixIn {
On 2014/10/10 16:03:35, danakj wrote:
> This should inherit from BeginFrameSource (or why not just add SetClientReady
to
> BeginFrameSource and use that?)
> 
> The MixIn has concrete implementations, it should inherit from an abstract
> class, not vice versa. And users of ExternalBFS shouldn't care that the MixIn
> class is implementing parts of the interface.

I think ExternalBeginFrameSource should have a different specs such as
ref-counted and non-thread safe object because it would be used to request
BeginFrame from outside of cc(in browser-side scheduler with unified
BeginFrame).
So, I made it as a concrete class for base class.

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:84:
external_begin_frame_source_internal_(external_begin_frame_source.Pass()),
On 2014/10/13 02:56:20, mithro wrote:
> Can we just store this in the primary_frame_source_internal_ instead?

external_begin_frame_source is changed to ref-counted object.
So, it should be stored in the separate member.

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:104:
external_begin_frame_source_internal_->SetClientReady();
On 2014/10/13 02:56:20, mithro wrote:
> I'm unsure what the "SetClientReady" function is for?

SetClientReady() will be used if BeginFrameSource has two state(Create and Run)
and it is created and used on different thread.
CompositorExternalBeginFrameSource can be in runnable state after it is
delivered to scheduler on compositor thread.

https://codereview.chromium.org/619843002/diff/300001/cc/test/fake_output_sur...
File cc/test/fake_output_surface_client.h (left):

https://codereview.chromium.org/619843002/diff/300001/cc/test/fake_output_sur...
cc/test/fake_output_surface_client.h:20: swap_count_(0),
On 2014/10/13 02:56:20, mithro wrote:
> Why is swap_count_ being removed here too?

Oops. my mistake.

https://codereview.chromium.org/619843002/diff/300001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/619843002/diff/300001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:47: class ExternalBeginFrameSource;
On 2014/10/13 02:56:20, mithro wrote:
> Can we get away with the layer_tree_host not needing to know anything about
this
> interface?

I think ExternalBeginFrameSource has a different spec like ref-counted and
non-thread safe. So I need to export as a base class.

https://codereview.chromium.org/619843002/diff/300001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/619843002/diff/300001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:425: void AsValueInto(base::debug::TracedValue*
value) const {
On 2014/10/10 16:03:35, danakj wrote:
> keep the defn in the .cc

Done.

https://codereview.chromium.org/619843002/diff/300001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/619843002/diff/300001/content/common/view_mes...
content/common/view_messages.h:966:
IPC_MESSAGE_ROUTED1(ViewHostMsg_SetNeedsBeginFrame,
On 2014/10/10 13:32:33, Sami wrote:
> Please still keep this in the "messages sent from the renderer to the browser"
> section.

Done.

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_external_begin_frame_source.cc (right):

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
content/renderer/gpu/compositor_external_begin_frame_source.cc:49: Send(new
ViewHostMsg_SetNeedsBeginFrame(routing_id_, needs_begin_frames));
On 2014/10/10 16:03:35, danakj wrote:
> What's sending us here on shutdown now? Do we need to get here still?
> ThreadProxy used to call SetNeedsBeginFrames(false) (which of course wouldn't
> work in a single threaded scheduler world, heh).

I made cc::Scheduler call SentNeedsBeginFrames(false) in its dtor.

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_external_begin_frame_source.h (right):

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
content/renderer/gpu/compositor_external_begin_frame_source.h:29: : public
cc::ExternalBeginFrameSource,
On 2014/10/10 16:03:36, danakj wrote:
> This would inherit from the BFSMixin and also the ExternalBFS.

ditto.

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
content/renderer/gpu/compositor_external_begin_frame_source.h:39: virtual void
OnNeedsBeginFramesChange(bool needs_begin_frames) OVERRIDE;
On 2014/10/10 13:32:33, Sami wrote:
> nit: I think we can now write just
> 
>   virtual void OnNeedsBeginFramesChange(bool needs_begin_frames) override;
> 
> Not sure if the virtual still needs to be there. Eventually it is going away.

Done.

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/619843002/diff/300001/content/renderer/gpu/co...
content/renderer/gpu/compositor_output_surface.cc:40:
ViewMsg_UpdateVSyncParameters::ID,
On 2014/10/10 13:32:33, Sami wrote:
> Do we still need this message in this new world?

Yes, we should maintain this message in a short term
because browser process in cros still uses threaded compositing mode.
However, Browser initiated BeginFrame will only be adopted by single threaded
mode.

https://codereview.chromium.org/619843002/diff/300001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/619843002/diff/300001/content/renderer/render...
content/renderer/render_thread_impl.cc:912:
scoped_refptr<base::MessageLoopProxy> output_surface_loop;
On 2014/10/10 16:03:36, danakj wrote:
> nit: rename to compositor_impl_side_loop?

Done.

https://codereview.chromium.org/619843002/diff/300001/content/renderer/render...
content/renderer/render_thread_impl.cc:924:
AddFilter(begin_frame_source_filter_.get());
Thanks for pointing out.
CreateCompositorMessageFilter() is created to handle this in
content/renderer/gpu/compositor_message_filter.h

On 2014/10/10 20:39:46, piman (Very slow to review) wrote:
> I would prefer if there was still a single filter. We found scaling problems
> with filters, because every IPC message needs to go through every filter, so a
> new filter adds overhead to *all* messages, including unrelated ones, see
> crbug.com/340881
> 
> Since both CompositorOutputSurface's and CompositorExternalBeginFrameSource's
> filters do essentially the same thing (find messages to handle on the
compositor
> thread and forward to that), we can make a single filter, quickly reject
> irrelevant messages, then post onto the compositor thread and dispatch between
> CompositorOutputSurface and CompositorExternalBeginFrameSource based on the
type
> (bonus points if we can fold InputEventFilter into the same logic).

[piman, 2014-10-15 03:34:50]

https://codereview.chromium.org/619843002/diff/890001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/619843002/diff/890001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:545: widget_->routing_id());
If both the CompositorExternalBeginFrameSource and the CompositorOutputSurface
have the same routing_id, they will stomp on each other (ForwardingMessageFilter
expects a single handler per routing id). Either you need to have a single
handler and have both the CompositorExternalBeginFrameSource and the
CompositorOutputSurface register to it, or change ForwardingMessageFilter to use
a multimap instead of a map.

[simonhong, 2014-10-15 06:43:52]

Patchset #14 (id:910001) has been deleted

[simonhong, 2014-10-15 06:44:09]

Patchset #14 (id:930001) has been deleted

[simonhong, 2014-10-15 06:45:14]

piman@, I addressed your comment.
Thanks for review!

https://codereview.chromium.org/619843002/diff/890001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/619843002/diff/890001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:545: widget_->routing_id());
On 2014/10/15 03:34:50, piman (Very slow to review) wrote:
> If both the CompositorExternalBeginFrameSource and the CompositorOutputSurface
> have the same routing_id, they will stomp on each other
(ForwardingMessageFilter
> expects a single handler per routing id). Either you need to have a single
> handler and have both the CompositorExternalBeginFrameSource and the
> CompositorOutputSurface register to it, or change ForwardingMessageFilter to
use
> a multimap instead of a map.

Many thanks for this.
I didn't know about it.
I modified ForwardingMessagerFilter to use a multimap.

[danakj, 2014-10-15 06:49:50]

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:166: class CC_EXPORT ExternalBeginFrameSource
: public BeginFrameSourceMixIn {
On 2014/10/15 01:04:21, simonhong wrote:
> On 2014/10/10 16:03:35, danakj wrote:
> > This should inherit from BeginFrameSource (or why not just add
SetClientReady
> to
> > BeginFrameSource and use that?)
> > 
> > The MixIn has concrete implementations, it should inherit from an abstract
> > class, not vice versa. And users of ExternalBFS shouldn't care that the
MixIn
> > class is implementing parts of the interface.
> 
> I think ExternalBeginFrameSource should have a different specs such as
> ref-counted and non-thread safe object because it would be used to request
> BeginFrame from outside of cc(in browser-side scheduler with unified
> BeginFrame).
> So, I made it as a concrete class for base class.

So you have a use case for why it would need to be ref counted?

[simonhong, 2014-10-15 06:56:38]

On 2014/10/15 06:49:50, danakj wrote:
>
https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
> File cc/scheduler/begin_frame_source.h (right):
> 
>
https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
> cc/scheduler/begin_frame_source.h:166: class CC_EXPORT
ExternalBeginFrameSource
> : public BeginFrameSourceMixIn {
> On 2014/10/15 01:04:21, simonhong wrote:
> > On 2014/10/10 16:03:35, danakj wrote:
> > > This should inherit from BeginFrameSource (or why not just add
> SetClientReady
> > to
> > > BeginFrameSource and use that?)
> > > 
> > > The MixIn has concrete implementations, it should inherit from an abstract
> > > class, not vice versa. And users of ExternalBFS shouldn't care that the
> MixIn
> > > class is implementing parts of the interface.
> > 
> > I think ExternalBeginFrameSource should have a different specs such as
> > ref-counted and non-thread safe object because it would be used to request
> > BeginFrame from outside of cc(in browser-side scheduler with unified
> > BeginFrame).
> > So, I made it as a concrete class for base class.
> 
> So you have a use case for why it would need to be ref counted?

In renderer-side, only Scheduler owns it.
In browser-side, BeginFrame should be delivered from content layer in Android
and Mac.
So, ExternalBFS should be owned by content and cc.

[danakj, 2014-10-15 06:58:33]

On Oct 15, 2014 2:56 AM, <simonhong@chromium.org> wrote:
>
> On 2014/10/15 06:49:50, danakj wrote:
>
>
https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
>>
>> File cc/scheduler/begin_frame_source.h (right):
>
>
>
>
https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
>>
>> cc/scheduler/begin_frame_source.h:166: class CC_EXPORT
>
> ExternalBeginFrameSource
>>
>> : public BeginFrameSourceMixIn {
>> On 2014/10/15 01:04:21, simonhong wrote:
>> > On 2014/10/10 16:03:35, danakj wrote:
>> > > This should inherit from BeginFrameSource (or why not just add
>> SetClientReady
>> > to
>> > > BeginFrameSource and use that?)
>> > >
>> > > The MixIn has concrete implementations, it should inherit from an
abstract
>> > > class, not vice versa. And users of ExternalBFS shouldn't care that
the
>> MixIn
>> > > class is implementing parts of the interface.
>> >
>> > I think ExternalBeginFrameSource should have a different specs such as
>> > ref-counted and non-thread safe object because it would be used to
request
>> > BeginFrame from outside of cc(in browser-side scheduler with unified
>> > BeginFrame).
>> > So, I made it as a concrete class for base class.
>
>
>> So you have a use case for why it would need to be ref counted?
>
>
> In renderer-side, only Scheduler owns it.
> In browser-side, BeginFrame should be delivered from content layer in
Android
> and Mac.
> So, ExternalBFS should be owned by content and cc.

Does it need to outlive the compositor sometimes but not other times? Why
isn't one of those 2 just holding a raw pointer? The lifetime should be
well defined and controlled here?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-10-15 07:09:18]

Just a FYI, from
http://www.chromium.org/developers/coding-style/important-abstractions-and-da...

scoped_refptr<T> & base::RefCounted & base::RefCountedThreadSafe
> Reference counting is occasionally useful but is more often a sign that
> someone isn't thinking carefully about ownership. Use it when ownership is
> truly shared (for example, multiple tabs sharing the same renderer
> process), not for when lifetime management is difficult to reason about.


Tim

On 15 October 2014 17:58, Dana Jansens <danakj@chromium.org> wrote:

>
> On Oct 15, 2014 2:56 AM, <simonhong@chromium.org> wrote:
> >
> > On 2014/10/15 06:49:50, danakj wrote:
> >
> >
>
https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
> >>
> >> File cc/scheduler/begin_frame_source.h (right):
> >
> >
> >
> >
>
https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
> >>
> >> cc/scheduler/begin_frame_source.h:166: class CC_EXPORT
> >
> > ExternalBeginFrameSource
> >>
> >> : public BeginFrameSourceMixIn {
> >> On 2014/10/15 01:04:21, simonhong wrote:
> >> > On 2014/10/10 16:03:35, danakj wrote:
> >> > > This should inherit from BeginFrameSource (or why not just add
> >> SetClientReady
> >> > to
> >> > > BeginFrameSource and use that?)
> >> > >
> >> > > The MixIn has concrete implementations, it should inherit from an
> abstract
> >> > > class, not vice versa. And users of ExternalBFS shouldn't care that
> the
> >> MixIn
> >> > > class is implementing parts of the interface.
> >> >
> >> > I think ExternalBeginFrameSource should have a different specs such as
> >> > ref-counted and non-thread safe object because it would be used to
> request
> >> > BeginFrame from outside of cc(in browser-side scheduler with unified
> >> > BeginFrame).
> >> > So, I made it as a concrete class for base class.
> >
> >
> >> So you have a use case for why it would need to be ref counted?
> >
> >
> > In renderer-side, only Scheduler owns it.
> > In browser-side, BeginFrame should be delivered from content layer in
> Android
> > and Mac.
> > So, ExternalBFS should be owned by content and cc.
>
> Does it need to outlive the compositor sometimes but not other times? Why
> isn't one of those 2 just holding a raw pointer? The lifetime should be
> well defined and controlled here?
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2014-10-15 07:25:07]

On 2014/10/15 06:58:33, danakj wrote:
> On Oct 15, 2014 2:56 AM, <mailto:simonhong@chromium.org> wrote:
> >
> > On 2014/10/15 06:49:50, danakj wrote:
> >
> >
>
https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
> >>
> >> File cc/scheduler/begin_frame_source.h (right):
> >
> >
> >
> >
>
https://codereview.chromium.org/619843002/diff/300001/cc/scheduler/begin_fram...
> >>
> >> cc/scheduler/begin_frame_source.h:166: class CC_EXPORT
> >
> > ExternalBeginFrameSource
> >>
> >> : public BeginFrameSourceMixIn {
> >> On 2014/10/15 01:04:21, simonhong wrote:
> >> > On 2014/10/10 16:03:35, danakj wrote:
> >> > > This should inherit from BeginFrameSource (or why not just add
> >> SetClientReady
> >> > to
> >> > > BeginFrameSource and use that?)
> >> > >
> >> > > The MixIn has concrete implementations, it should inherit from an
> abstract
> >> > > class, not vice versa. And users of ExternalBFS shouldn't care that
> the
> >> MixIn
> >> > > class is implementing parts of the interface.
> >> >
> >> > I think ExternalBeginFrameSource should have a different specs such as
> >> > ref-counted and non-thread safe object because it would be used to
> request
> >> > BeginFrame from outside of cc(in browser-side scheduler with unified
> >> > BeginFrame).
> >> > So, I made it as a concrete class for base class.
> >
> >
> >> So you have a use case for why it would need to be ref counted?
> >
> >
> > In renderer-side, only Scheduler owns it.
> > In browser-side, BeginFrame should be delivered from content layer in
> Android
> > and Mac.
> > So, ExternalBFS should be owned by content and cc.
> 
> Does it need to outlive the compositor sometimes but not other times?
Yes, it only needs to outlive the compositor in Mac and Android.
Other case(Aura) receives BeginFrame via ipc from GPU process.

 Why
> isn't one of those 2 just holding a raw pointer?
I didn't think about just holding raw pointer in one of those.
I thought refptr looks more better when there are two users of it.
Also, Scheduler can be used on both mode (single thread and threaded).
So I want to make more general by using ref-counted.

The lifetime should be
> well defined and controlled here?
Both of it should be destroyed when LayerTreeHost is destroyed.

I think storing a raw point is OK if this way is more better than using refptr.

> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-10-15 15:02:59]

On Wed, Oct 15, 2014 at 3:25 AM, <simonhong@chromium.org> wrote:

> On 2014/10/15 06:58:33, danakj wrote:
>
>  On Oct 15, 2014 2:56 AM, <mailto:simonhong@chromium.org> wrote:
>> >
>> > On 2014/10/15 06:49:50, danakj wrote:
>> >
>> >
>>
>
> https://codereview.chromium.org/619843002/diff/300001/cc/
> scheduler/begin_frame_source.h
>
>> >>
>> >> File cc/scheduler/begin_frame_source.h (right):
>> >
>> >
>> >
>> >
>>
>
> https://codereview.chromium.org/619843002/diff/300001/cc/
> scheduler/begin_frame_source.h#newcode166
>
>> >>
>> >> cc/scheduler/begin_frame_source.h:166: class CC_EXPORT
>> >
>> > ExternalBeginFrameSource
>> >>
>> >> : public BeginFrameSourceMixIn {
>> >> On 2014/10/15 01:04:21, simonhong wrote:
>> >> > On 2014/10/10 16:03:35, danakj wrote:
>> >> > > This should inherit from BeginFrameSource (or why not just add
>> >> SetClientReady
>> >> > to
>> >> > > BeginFrameSource and use that?)
>> >> > >
>> >> > > The MixIn has concrete implementations, it should inherit from an
>> abstract
>> >> > > class, not vice versa. And users of ExternalBFS shouldn't care that
>> the
>> >> MixIn
>> >> > > class is implementing parts of the interface.
>> >> >
>> >> > I think ExternalBeginFrameSource should have a different specs such
>> as
>> >> > ref-counted and non-thread safe object because it would be used to
>> request
>> >> > BeginFrame from outside of cc(in browser-side scheduler with unified
>> >> > BeginFrame).
>> >> > So, I made it as a concrete class for base class.
>> >
>> >
>> >> So you have a use case for why it would need to be ref counted?
>> >
>> >
>> > In renderer-side, only Scheduler owns it.
>> > In browser-side, BeginFrame should be delivered from content layer in
>> Android
>> > and Mac.
>> > So, ExternalBFS should be owned by content and cc.
>>
>
>  Does it need to outlive the compositor sometimes but not other times?
>>
> Yes, it only needs to outlive the compositor in Mac and Android.
> Other case(Aura) receives BeginFrame via ipc from GPU process.
>
>  Why
>
>> isn't one of those 2 just holding a raw pointer?
>>
> I didn't think about just holding raw pointer in one of those.
> I thought refptr looks more better when there are two users of it.
> Also, Scheduler can be used on both mode (single thread and threaded).
> So I want to make more general by using ref-counted.
>
> The lifetime should be
>
>> well defined and controlled here?
>>
> Both of it should be destroyed when LayerTreeHost is destroyed.
>
> I think storing a raw point is OK if this way is more better than using
> refptr.
>

Yes please. General is the enemy of simple. Reference counting makes it
very hard to tell when something will be destroyed, and we should avoid it
when we can.

If it needs to outlive the compositor sometimes (ie you need it to be
around after LTH is destroyed), then let's own it outside the compositor
and just pass in a raw pointer. If not, giving ownership to cc and
considering it dead when destroying the LTH makes sense also.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2014-10-15 23:33:26]

dana@, thanks for advice.
giving ownership to cc makes sense in this case.
Also, I found that synchronous compositor(webview) needs external
BeginFrameSource as well.

On Thu, Oct 16, 2014 at 12:02 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Oct 15, 2014 at 3:25 AM, <simonhong@chromium.org> wrote:
>
>> On 2014/10/15 06:58:33, danakj wrote:
>>
>>  On Oct 15, 2014 2:56 AM, <mailto:simonhong@chromium.org> wrote:
>>> >
>>> > On 2014/10/15 06:49:50, danakj wrote:
>>> >
>>> >
>>>
>>
>> https://codereview.chromium.org/619843002/diff/300001/cc/
>> scheduler/begin_frame_source.h
>>
>>> >>
>>> >> File cc/scheduler/begin_frame_source.h (right):
>>> >
>>> >
>>> >
>>> >
>>>
>>
>> https://codereview.chromium.org/619843002/diff/300001/cc/
>> scheduler/begin_frame_source.h#newcode166
>>
>>> >>
>>> >> cc/scheduler/begin_frame_source.h:166: class CC_EXPORT
>>> >
>>> > ExternalBeginFrameSource
>>> >>
>>> >> : public BeginFrameSourceMixIn {
>>> >> On 2014/10/15 01:04:21, simonhong wrote:
>>> >> > On 2014/10/10 16:03:35, danakj wrote:
>>> >> > > This should inherit from BeginFrameSource (or why not just add
>>> >> SetClientReady
>>> >> > to
>>> >> > > BeginFrameSource and use that?)
>>> >> > >
>>> >> > > The MixIn has concrete implementations, it should inherit from an
>>> abstract
>>> >> > > class, not vice versa. And users of ExternalBFS shouldn't care
>>> that
>>> the
>>> >> MixIn
>>> >> > > class is implementing parts of the interface.
>>> >> >
>>> >> > I think ExternalBeginFrameSource should have a different specs such
>>> as
>>> >> > ref-counted and non-thread safe object because it would be used to
>>> request
>>> >> > BeginFrame from outside of cc(in browser-side scheduler with unified
>>> >> > BeginFrame).
>>> >> > So, I made it as a concrete class for base class.
>>> >
>>> >
>>> >> So you have a use case for why it would need to be ref counted?
>>> >
>>> >
>>> > In renderer-side, only Scheduler owns it.
>>> > In browser-side, BeginFrame should be delivered from content layer in
>>> Android
>>> > and Mac.
>>> > So, ExternalBFS should be owned by content and cc.
>>>
>>
>>  Does it need to outlive the compositor sometimes but not other times?
>>>
>> Yes, it only needs to outlive the compositor in Mac and Android.
>> Other case(Aura) receives BeginFrame via ipc from GPU process.
>>
>>  Why
>>
>>> isn't one of those 2 just holding a raw pointer?
>>>
>> I didn't think about just holding raw pointer in one of those.
>> I thought refptr looks more better when there are two users of it.
>> Also, Scheduler can be used on both mode (single thread and threaded).
>> So I want to make more general by using ref-counted.
>>
>> The lifetime should be
>>
>>> well defined and controlled here?
>>>
>> Both of it should be destroyed when LayerTreeHost is destroyed.
>>
>> I think storing a raw point is OK if this way is more better than using
>> refptr.
>>
>
> Yes please. General is the enemy of simple. Reference counting makes it
> very hard to tell when something will be destroyed, and we should avoid it
> when we can.
>
> If it needs to outlive the compositor sometimes (ie you need it to be
> around after LTH is destroyed), then let's own it outside the compositor
> and just pass in a raw pointer. If not, giving ownership to cc and
> considering it dead when destroying the LTH makes sense also.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-10-16 02:09:04]

https://codereview.chromium.org/619843002/diff/970001/ipc/ipc_forwarding_mess...
File ipc/ipc_forwarding_message_filter.cc (right):

https://codereview.chromium.org/619843002/diff/970001/ipc/ipc_forwarding_mess...
ipc/ipc_forwarding_message_filter.cc:31: multi_handlers_.erase(routing_id);
I think this is problematic. If you have 2 independent handlers with the same
routing id, both would be removed here, whereas the intent (and current usage
pattern) is only to remove one.

Either we need to pass the handler here (if it makes sense), or we need another
way to identify which handler we're removing.

https://codereview.chromium.org/619843002/diff/970001/ipc/ipc_forwarding_mess...
ipc/ipc_forwarding_message_filter.cc:41: std::multimap<int, Handler>::iterator>
handlers;
nit: If you're going to post the task under the lock, this doesn't need to be
declared outside the lock scope.

If declared at the initialization site, it may be a good use case for auto.

https://codereview.chromium.org/619843002/diff/970001/ipc/ipc_forwarding_mess...
ipc/ipc_forwarding_message_filter.cc:48: for (std::multimap<int,
Handler>::iterator it = handlers.first;
nit: another good use case for auto

https://codereview.chromium.org/619843002/diff/970001/ipc/ipc_forwarding_mess...
ipc/ipc_forwarding_message_filter.cc:50:
target_task_runner_->PostTask(FROM_HERE, base::Bind(it->second, message));
nit: indent

[simonhong, 2014-10-17 05:14:24]

Patchset #19 (id:1040001) has been deleted

[simonhong, 2014-10-17 05:22:46]

Patchset #19 (id:1060001) has been deleted

[simonhong, 2014-10-17 05:46:06]

Patchset #19 (id:1080001) has been deleted

[simonhong, 2014-10-17 07:00:50]

Patchset #19 (id:1090001) has been deleted

[simonhong, 2014-10-17 07:19:09]

Patchset #19 (id:410063) has been deleted

[simonhong, 2014-10-17 07:22:11]

piman@, additional tests are added in ipc_tests for new ForwardingMessageFilter.
danakj@, I refactored SynchronousCompositorOutputSurface, too.
Please take a look again.
Many thanks for reviewing!

https://codereview.chromium.org/619843002/diff/970001/ipc/ipc_forwarding_mess...
File ipc/ipc_forwarding_message_filter.cc (right):

https://codereview.chromium.org/619843002/diff/970001/ipc/ipc_forwarding_mess...
ipc/ipc_forwarding_message_filter.cc:31: multi_handlers_.erase(routing_id);
On 2014/10/16 02:09:04, piman (Very slow to review) wrote:
> I think this is problematic. If you have 2 independent handlers with the same
> routing id, both would be removed here, whereas the intent (and current usage
> pattern) is only to remove one.
> 
> Either we need to pass the handler here (if it makes sense), or we need
another
> way to identify which handler we're removing.

Changed to RemoveRoute(int routing_id, const Handler& handler) to remove
specific handler among 2 handlers.

https://codereview.chromium.org/619843002/diff/970001/ipc/ipc_forwarding_mess...
ipc/ipc_forwarding_message_filter.cc:41: std::multimap<int, Handler>::iterator>
handlers;
On 2014/10/16 02:09:04, piman (Very slow to review) wrote:
> nit: If you're going to post the task under the lock, this doesn't need to be
> declared outside the lock scope.
> 
> If declared at the initialization site, it may be a good use case for auto.

Done.

https://codereview.chromium.org/619843002/diff/970001/ipc/ipc_forwarding_mess...
ipc/ipc_forwarding_message_filter.cc:48: for (std::multimap<int,
Handler>::iterator it = handlers.first;
On 2014/10/16 02:09:04, piman (Very slow to review) wrote:
> nit: another good use case for auto

Done.

https://codereview.chromium.org/619843002/diff/970001/ipc/ipc_forwarding_mess...
ipc/ipc_forwarding_message_filter.cc:50:
target_task_runner_->PostTask(FROM_HERE, base::Bind(it->second, message));
On 2014/10/16 02:09:04, piman (Very slow to review) wrote:
> nit: indent

Done.

[simonhong, 2014-10-17 08:20:57]

Patchset #19 (id:1120001) has been deleted

[simonhong, 2014-10-17 08:21:20]

Patchset #19 (id:1140001) has been deleted

[simonhong, 2014-10-17 10:37:30]

Patchset #19 (id:1160001) has been deleted

[piman, 2014-10-17 21:51:22]

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

[piman, 2014-10-17 21:51:24]

+cpu for OWNERS in ipc/
LGTM for ipc and content/renderer, I'll let others review the rest as they have
so far.

[brianderson, 2014-10-18 00:08:43]

brianderson@chromium.org changed reviewers:
+ boliu@chromium.org

[brianderson, 2014-10-18 00:08:45]

+boliu because of changes to the synchronous compositor.

https://codereview.chromium.org/619843002/diff/1200001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/619843002/diff/1200001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:1441:
EXPECT_TRUE(client.needs_begin_frames());
BeginFramesNotFromClient tests are meant to test configurations where
BeginFrames come from inside the scheduler. The scheduler should not be calling
SetNeedsBeginFrames on the client in those cases.

https://codereview.chromium.org/619843002/diff/1200001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:1539:
EXPECT_TRUE(client.needs_begin_frames());
Ditto for this class of tests.

https://codereview.chromium.org/619843002/diff/1200001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/619843002/diff/1200001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:71: base::TimeDelta::FromMilliseconds(16));
Will this slow down the LayerTree tests? I think there is code somewhere that
makes the intervals much shorter for testing at ToT.

https://codereview.chromium.org/619843002/diff/1200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/619843002/diff/1200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:1984: nullptr);
Why is an external source used in the LayerTree tests, but not here?

https://codereview.chromium.org/619843002/diff/1200001/cc/trees/thread_proxy.h
File cc/trees/thread_proxy.h (right):

https://codereview.chromium.org/619843002/diff/1200001/cc/trees/thread_proxy....
cc/trees/thread_proxy.h:304: scoped_ptr<BeginFrameSource>
external_begin_frame_source_;
Should this go into the CompositorThreadOnly group?

https://codereview.chromium.org/619843002/diff/1200001/content/common/view_me...
File content/common/view_messages.h (right):

https://codereview.chromium.org/619843002/diff/1200001/content/common/view_me...
content/common/view_messages.h:966:
IPC_MESSAGE_ROUTED1(ViewHostMsg_SetNeedsBeginFrame,
Let's rename this to SetNeedsBeginFrames (note the 's' at the end) to match the
naming used by Tim's recent refactor.

[boliu, 2014-10-18 00:15:53]

Just want to make a plug first that the android webview test shell is hardware
accelerated now. Build "android_webview_apk" and install
"out/Debug/apks/AndroidWebView.apk". The instrumentation tests use that shell
and looks like they all blew up.

[boliu, 2014-10-18 00:49:39]

Skimmed the sync compositor bits.

Having OutputSurface and FrameSource talk to each each other through
SynchronousComopsitorImpl is really awkward. Can we have
OutputSurface::WillBeginFrame/DidBeginFrame. Then Comositor can do:

output_surface->WillBeginFrame()
frame_source->BeginFrame()
output_surface->DidBginFrame()

Something like that?

https://codereview.chromium.org/619843002/diff/1200001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/619843002/diff/1200001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:198:
return compositor->GetExternalBeginFrameSource();
This is not thread safe.

Maybe follow the OutputSurface pattern, have the factory create FrameSource, and
then FrameSource can try to find the SynchronousCompositorImpl once it's bound
to the compositor thread?

https://codereview.chromium.org/619843002/diff/1200001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/619843002/diff/1200001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:111: return
output_surface_->invoking_composite();
Can lift invoking_composite out of OutputSurface to this class.

[cpu_(ooo_6.6-7.5), 2014-10-18 01:04:00]

uncomfortable with the multi_handlers idea. Let me ask the inventor of the
filtering  +jam

[cpu_(ooo_6.6-7.5), 2014-10-18 01:04:46]

cpu@chromium.org changed reviewers:
+ jam@chromium.org

[piman, 2014-10-18 01:44:54]

On Fri, Oct 17, 2014 at 6:03 PM, <cpu@chromium.org> wrote:

> uncomfortable with the multi_handlers idea. Let me ask the inventor of the
> filtering  +jam
>

Note: this is localized to ForwardingMessageFilter, which is only used in 2
places, not all MessageFilters.

Antoine


>
> https://codereview.chromium.org/619843002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-10-18 18:02:39]

https://codereview.chromium.org/619843002/diff/1200001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/619843002/diff/1200001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:71: base::TimeDelta::FromMilliseconds(16));
On 2014/10/18 00:08:45, brianderson wrote:
> Will this slow down the LayerTree tests? I think there is code somewhere that
> makes the intervals much shorter for testing at ToT.

https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/layer_tree...

[cpu_(ooo_6.6-7.5), 2014-10-20 20:10:03]

yeah, its only there, still an opinion for jam would be nice. If we don't hear
from him in 48 hours lets assume its a go.

https://codereview.chromium.org/619843002/diff/1200001/ipc/ipc_forwarding_mes...
File ipc/ipc_forwarding_message_filter.h (right):

https://codereview.chromium.org/619843002/diff/1200001/ipc/ipc_forwarding_mes...
ipc/ipc_forwarding_message_filter.h:20: // deliver them to a different task
runner than they would otherwise
comment is stale.

[simonhong, 2014-10-21 06:39:43]

Patchset #21 (id:1220001) has been deleted

[simonhong, 2014-10-21 06:40:20]

Patchset #21 (id:1240001) has been deleted

[simonhong, 2014-10-21 06:45:37]

Patchset #21 (id:1260001) has been deleted

[simonhong, 2014-10-21 11:30:58]

Patchset #21 (id:1280001) has been deleted

[simonhong, 2014-10-21 11:31:22]

Patchset #21 (id:1300001) has been deleted

[simonhong, 2014-10-22 04:35:43]

Patchset #21 (id:1320001) has been deleted

[simonhong, 2014-10-22 04:36:07]

Patchset #21 (id:1340001) has been deleted

[simonhong, 2014-10-22 04:36:27]

Patchset #21 (id:1360001) has been deleted

[simonhong, 2014-10-23 01:03:09]

All comments are addressed.
PTAL again.

boliu@, The purpose of this cl is remove BeginFrame bits from OutputSurface.
So, I modified like this.
In SynchronousCompositorImpl::DemandDrawXX
   call SynchronousCompositorOutputSurface::DemandDrawXX()
   call BeginFrameSource::BeginFrame()
   call SetContinuousInvalidate()
With this scenario, I think that output surface doesn't need to think about
BeginFrame anymore.
But, still many WebView tests are failed.

https://codereview.chromium.org/619843002/diff/1200001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/619843002/diff/1200001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:1441:
EXPECT_TRUE(client.needs_begin_frames());
On 2014/10/18 00:08:45, brianderson wrote:
> BeginFramesNotFromClient tests are meant to test configurations where
> BeginFrames come from inside the scheduler. The scheduler should not be
calling
> SetNeedsBeginFrames on the client in those cases.

All client.needs_begin_frames() in NotFromClient are removed and
DCHECK(!client.ExternalBeginFrameSource()) is added.
ExternalBeginFrameSource should not be instantiated in these NotFromClient
tests.

https://codereview.chromium.org/619843002/diff/1200001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/619843002/diff/1200001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:71: base::TimeDelta::FromMilliseconds(16));
On 2014/10/18 18:02:39, danakj wrote:
> On 2014/10/18 00:08:45, brianderson wrote:
> > Will this slow down the LayerTree tests? I think there is code somewhere
that
> > makes the intervals much shorter for testing at ToT.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/layer_tree...

Modified to use refresh_rate.

https://codereview.chromium.org/619843002/diff/1200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/619843002/diff/1200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:1984: nullptr);
On 2014/10/18 00:08:45, brianderson wrote:
> Why is an external source used in the LayerTree tests, but not here?

These four tests didn't use external BeginFrameSource.
But, all other tests except them overrides begin_frame_scheduling_enabled value
when they needed.

https://codereview.chromium.org/619843002/diff/1200001/cc/trees/thread_proxy.h
File cc/trees/thread_proxy.h (right):

https://codereview.chromium.org/619843002/diff/1200001/cc/trees/thread_proxy....
cc/trees/thread_proxy.h:304: scoped_ptr<BeginFrameSource>
external_begin_frame_source_;
On 2014/10/18 00:08:45, brianderson wrote:
> Should this go into the CompositorThreadOnly group?

Done.

https://codereview.chromium.org/619843002/diff/1200001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/619843002/diff/1200001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:198:
return compositor->GetExternalBeginFrameSource();
On 2014/10/18 00:49:38, boliu wrote:
> This is not thread safe.
> 
> Maybe follow the OutputSurface pattern, have the factory create FrameSource,
and
> then FrameSource can try to find the SynchronousCompositorImpl once it's bound
> to the compositor thread?

Done.

https://codereview.chromium.org/619843002/diff/1200001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/619843002/diff/1200001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:111: return
output_surface_->invoking_composite();
On 2014/10/18 00:49:38, boliu wrote:
> Can lift invoking_composite out of OutputSurface to this class.

Done.

https://codereview.chromium.org/619843002/diff/1200001/content/common/view_me...
File content/common/view_messages.h (right):

https://codereview.chromium.org/619843002/diff/1200001/content/common/view_me...
content/common/view_messages.h:966:
IPC_MESSAGE_ROUTED1(ViewHostMsg_SetNeedsBeginFrame,
On 2014/10/18 00:08:45, brianderson wrote:
> Let's rename this to SetNeedsBeginFrames (note the 's' at the end) to match
the
> naming used by Tim's recent refactor.

Done.

https://codereview.chromium.org/619843002/diff/1200001/ipc/ipc_forwarding_mes...
File ipc/ipc_forwarding_message_filter.h (right):

https://codereview.chromium.org/619843002/diff/1200001/ipc/ipc_forwarding_mes...
ipc/ipc_forwarding_message_filter.h:20: // deliver them to a different task
runner than they would otherwise
On 2014/10/20 20:10:03, cpu wrote:
> comment is stale.

Done.

[boliu, 2014-10-23 01:06:44]

It's failing the DCHECK below. Remember you can test this yourself with a
chromium checkout. Build android_webview_apk.

[FATAL:synchronous_compositor_output_surface.cc(212)] Check failed:
!frame_holder_.get(). 

Stack Trace:
  RELADDR   FUNCTION                                                            
                                                                                
   FILE:LINE
  00347ce3  logging::LogMessage::~LogMessage()+42                               
                                                                                
  
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../base/logging.cc:545
  009008a7 
content::SynchronousCompositorOutputSurface::InvokeComposite(gfx::Transform
const&, gfx::Rect, gfx::Rect, gfx::Rect, gfx::Transform, bool)+74           
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../content/browser/android/in_process/synchronous_compositor_output_surface.cc:212
  00900aeb  content::SynchronousCompositorOutputSurface::DemandDrawHw(gfx::Size,
gfx::Transform const&, gfx::Rect, gfx::Rect, gfx::Rect, gfx::Transform
const&)+258 
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../content/browser/android/in_process/synchronous_compositor_output_surface.cc:170
  008ffe2b  content::SynchronousCompositorImpl::DemandDrawHw(gfx::Size,
gfx::Transform const&, gfx::Rect, gfx::Rect, gfx::Rect, gfx::Transform
const&)+274          
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../content/browser/android/in_process/synchronous_compositor_impl.cc:147
  00244893  android_webview::BrowserViewRenderer::CompositeHw()+308             
                                                                                
  
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../android_webview/browser/browser_view_renderer.cc:281
  00244a0d  android_webview::BrowserViewRenderer::OnDrawHardware(_jobject*)+324 
                                                                                
  
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../android_webview/browser/browser_view_renderer.cc:243
  00261b59  android_webview::AwContents::OnDraw(_JNIEnv*, _jobject*, _jobject*,
unsigned char, int, int, int, int, int, int)+136                                
   
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../android_webview/native/aw_contents.cc:992
  00261be9  android_webview::OnDraw(_JNIEnv*, _jobject*, long long, _jobject*,
unsigned char, int, int, int, int, int, int)+112                                
    
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/gen/android_webview/jni/AwContents_jni.h:112

[simonhong, 2014-10-23 01:20:01]

On 2014/10/23 01:06:44, boliu wrote:
> It's failing the DCHECK below. Remember you can test this yourself with a
> chromium checkout. Build android_webview_apk.
> 
> [FATAL:synchronous_compositor_output_surface.cc(212)] Check failed:
> !frame_holder_.get(). 
> 
> Stack Trace:
>   RELADDR   FUNCTION                                                          
 
>                                                                               
 
>    FILE:LINE
>   00347ce3  logging::LogMessage::~LogMessage()+42                             
 
>                                                                               
 
>   
>
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../base/logging.cc:545
>   009008a7 
> content::SynchronousCompositorOutputSurface::InvokeComposite(gfx::Transform
> const&, gfx::Rect, gfx::Rect, gfx::Rect, gfx::Transform, bool)+74           
>
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../content/browser/android/in_process/synchronous_compositor_output_surface.cc:212
>   00900aeb 
content::SynchronousCompositorOutputSurface::DemandDrawHw(gfx::Size,
> gfx::Transform const&, gfx::Rect, gfx::Rect, gfx::Rect, gfx::Transform
> const&)+258 
>
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../content/browser/android/in_process/synchronous_compositor_output_surface.cc:170
>   008ffe2b  content::SynchronousCompositorImpl::DemandDrawHw(gfx::Size,
> gfx::Transform const&, gfx::Rect, gfx::Rect, gfx::Rect, gfx::Transform
> const&)+274          
>
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../content/browser/android/in_process/synchronous_compositor_impl.cc:147
>   00244893  android_webview::BrowserViewRenderer::CompositeHw()+308           
 
>                                                                               
 
>   
>
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../android_webview/browser/browser_view_renderer.cc:281
>   00244a0d 
android_webview::BrowserViewRenderer::OnDrawHardware(_jobject*)+324 
>                                                                               
 
>   
>
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../android_webview/browser/browser_view_renderer.cc:243
>   00261b59  android_webview::AwContents::OnDraw(_JNIEnv*, _jobject*,
_jobject*,
> unsigned char, int, int, int, int, int, int)+136                              
 
>    
>
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/../../android_webview/native/aw_contents.cc:992
>   00261be9  android_webview::OnDraw(_JNIEnv*, _jobject*, long long, _jobject*,
> unsigned char, int, int, int, int, int, int)+112                              
 
>     
>
/b/build/slave/android_dbg_tests_recipe/build/src/out/Debug/gen/android_webview/jni/AwContents_jni.h:112

Ah.. thanks.
I need to use Debug build.

[brianderson, 2014-10-23 02:12:45]

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:121: virtual void SetClientReady() = 0;
Anyway we can avoid having this method as part of the BeginFrameSource
interface? It's only needed for external sources to setup message routing. Is
there a different hook we can use?

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:145: void SetClientReady() override {}
Delete this line so we do not override SetClientReady() and all subclasses must
implement it.

The other implementations above are expected to be the same for all subclasses,
which is why they aren't virtual.

https://codereview.chromium.org/619843002/diff/1460001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h
(right):

https://codereview.chromium.org/619843002/diff/1460001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h:24:
void SetNeedsBeginFrames(bool needs_begin_frames) override;
Note: The BeginFrameSourceMixIn expects you to implement
OnNeedsBeginFramesChange rather than override SetNeedsBeginFrames.

https://codereview.chromium.org/619843002/diff/1460001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/619843002/diff/1460001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:312:
OnSetNeedsBeginFrame)
OnSetNeedsBeginFrame should have an 's' added too, but there are a lot of places
that need to be updated. Let's not worry about renaming anymore instances in
this patch and do a followup patch.

[cpu_(ooo_6.6-7.5), 2014-10-23 19:31:49]

ipc changes lgtm

[jam, 2014-10-23 22:53:35]

On 2014/10/18 01:04:00, cpu wrote:
> uncomfortable with the multi_handlers idea. Let me ask the inventor of the
> filtering  +jam

just saw this change. i agree that it's a bit confusing to have this concept.

if there are multiple handlers for a routing id, what determines the order?

i dont think this should go into IPC. if there's some usage in cc that needs
this, then put that logic there. then at least it'll be specific to cc without
having to put something in src\ipc that has undefined ordering and sending to
multiple listeners.

[simonhong, 2014-10-23 23:06:11]

jam@, does we need to consider the execution order when new filter is added?
2014. 10. 24. 오전 7:53에 <jam@chromium.org>님이 작성:

> On 2014/10/18 01:04:00, cpu wrote:
>
>> uncomfortable with the multi_handlers idea. Let me ask the inventor of the
>> filtering  +jam
>>
>
> just saw this change. i agree that it's a bit confusing to have this
> concept.
>
> if there are multiple handlers for a routing id, what determines the order?
>
> i dont think this should go into IPC. if there's some usage in cc that
> needs
> this, then put that logic there. then at least it'll be specific to cc
> without
> having to put something in src\ipc that has undefined ordering and sending
> to
> multiple listeners.
>
> https://codereview.chromium.org/619843002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cpu_(ooo_6.6-7.5), 2014-10-24 02:09:08]

retracting my assessment until jam is happy.

[simonhong, 2014-10-24 07:56:45]

On 2014/10/24 02:09:08, cpu wrote:
> retracting my assessment until jam is happy.

piman@ how about creating new class like CompositorMessageFilter into c/r/gpu/
instead of modifying existing one?

[simonhong, 2014-10-24 12:17:59]

Patchset #28 (id:1520001) has been deleted

[simonhong, 2014-10-24 14:30:19]

PTAL again.
Many thanks for review!

piman@, CompositorForwardingMessageFilter is created in c/b/gpu/ instead of
modifing IPC::ForwardingMessageFilter.

boliu@, I made SynchronousCompositorOutputSurface request next BeginFrame to
compositor.
IMO, this is not a bad scenario because BeginFrame is separated from
OutputSurface in this CL.
Currently, both (OutputSurface and BeginFrameSource) are independent each other.
So, I think  OuputSurface requests new BeginFrame to Compositor is natural when
it needs to schedule new BeginFrame.

brianderson@, please check inline comment.

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:121: virtual void SetClientReady() = 0;
On 2014/10/23 02:12:44, brianderson wrote:
> Anyway we can avoid having this method as part of the BeginFrameSource
> interface? It's only needed for external sources to setup message routing. Is
> there a different hook we can use?

At first, I created ExternalBeginFrameSource deriving BeginFrameSourceMixIn.
It has SetClientReady() as virtual abstract method.
But, danakj@ pointed out that BeginFrameSourceMixIn should not be a base class
of abstract class.
Instead, danakj@ suggested adding SetClientReady() into BeginFrameSource.

Other option is adding SetClientReady() into BFSMixIn.
But I think all of Source inherits BFSMixIn so, it is same as current.

In this case, what is the best solution? :)

https://codereview.chromium.org/619843002/diff/1460001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h
(right):

https://codereview.chromium.org/619843002/diff/1460001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h:24:
void SetNeedsBeginFrames(bool needs_begin_frames) override;
On 2014/10/23 02:12:44, brianderson wrote:
> Note: The BeginFrameSourceMixIn expects you to implement
> OnNeedsBeginFramesChange rather than override SetNeedsBeginFrames.

Done.

https://codereview.chromium.org/619843002/diff/1460001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/619843002/diff/1460001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:312:
OnSetNeedsBeginFrame)
On 2014/10/23 02:12:44, brianderson wrote:
> OnSetNeedsBeginFrame should have an 's' added too, but there are a lot of
places
> that need to be updated. Let's not worry about renaming anymore instances in
> this patch and do a followup patch.

Done.

[boliu, 2014-10-24 17:42:41]

Sync compositor call structure is still super awkward. Do not have OutputSurface
call BeginFrameSource through Compositor.

You have two options:

Option 1:
Split OutputSurface::DemandDrawHw/Sw into two parts, a WillBeginFrame, and a
DidBeginFrame. Then compositor can do:

output_surface->WillBeginFrame();
frame_source->BeginFrame();
output_surface->DidBginFrame();

This is quite a bit of refactoring, but there's no awkward "call through"
behavior.

Option 2:
Let OutputSurface hold BeginFrameSource. Then simply replace
client_->BeginFrame(...) with begin_frame_source_->BeginFrame(...).

This would be the easiest change. Not sure if this defeats the purpose of this
CL though.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
(right):

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:22:
SynchronousCompositorImpl::FromRoutingID(routing_id_);
nit suggestions: Factor this out into a GetCompositor helper?

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:38:
if (compositor && !compositor->invoking_composite())
You can do the invoking_composite check in Compositor. No need to expose
invoking_composite.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:103:
begin_frame_source_ = begin_frame_source;
Need keep BeginFrameSource and compositor_client_ state in sync, ie call
compositor_client_->SetContinuousInvalidate(NeedsBeginFrames());

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:137:
DCHECK(begin_frame_source_);
This DCHECK is not guaranteed.

Note we only call DidInitializeCompositor/DidDestroyCompositor while compositor
has an output surface.

Now this condition needs to be having an output surface *and* a begin frame
source.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:149: 
Where is the BeginFrame and SetContinuousInvalidate calls here?

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:153: void
SynchronousCompositorImpl::RequestBeginFrame() {
Super confusing name, should be just BeginFrame. But this shouldn't exist at
all.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:154:
begin_frame_source_->BeginFrame();
DCHECK(begin_frame_source_)

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:155:
SetContinuousInvalidate(begin_frame_source_->NeedsBeginFrames());
Why?

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:175:
begin_frame_source_->BeginFrame();
Why?

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:258:
compositor_client_->SetContinuousInvalidate(needs_begin_frames);
We want to hold off on this if invoking_composite_ is true.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.h:53: void
SetExternalBeginFrameSource(
nit: Group methods called by FrameSource together, and add a comment

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.h:60:
SynchronousCompositorOutputSurface* output_surface() const {
not used, remove

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.h:113: void
BeginFrame() const;
Not called or defined. Remove

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:226:
delegate->RequestBeginFrame();
Calling through to BeginFrameSource is super awkward here. Like I said in the
previous review, don't have OutputSurface call to BeginFrameSource through the
compositor. That's way too awkward and confusing.

[brianderson, 2014-10-24 20:45:55]

On 2014/10/24 17:42:41, boliu wrote:
> Sync compositor call structure is still super awkward. Do not have
OutputSurface
> call BeginFrameSource through Compositor.
> 
> You have two options:
> 
> Option 1:
> Split OutputSurface::DemandDrawHw/Sw into two parts, a WillBeginFrame, and a
> DidBeginFrame. Then compositor can do:
> 
> output_surface->WillBeginFrame();
> frame_source->BeginFrame();
> output_surface->DidBginFrame();
> 
> This is quite a bit of refactoring, but there's no awkward "call through"
> behavior.
> 
> Option 2:
> Let OutputSurface hold BeginFrameSource. Then simply replace
> client_->BeginFrame(...) with begin_frame_source_->BeginFrame(...).
> 
> This would be the easiest change. Not sure if this defeats the purpose of this
> CL though.

How does the lifetime of the OutputSurface compare to the SynchronousCompositor?

Part of the purpose of the refactor is to allow us to get BeginFrames when we've
lost the OutputSurface. That makes no sense in the context of the
SynchronousCompositor though, so option 2 may not be a bad option.

> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> File
>
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
> (right):
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
>
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:22:
> SynchronousCompositorImpl::FromRoutingID(routing_id_);
> nit suggestions: Factor this out into a GetCompositor helper?
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
>
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:38:
> if (compositor && !compositor->invoking_composite())
> You can do the invoking_composite check in Compositor. No need to expose
> invoking_composite.
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> File content/browser/android/in_process/synchronous_compositor_impl.cc
(right):
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> content/browser/android/in_process/synchronous_compositor_impl.cc:103:
> begin_frame_source_ = begin_frame_source;
> Need keep BeginFrameSource and compositor_client_ state in sync, ie call
> compositor_client_->SetContinuousInvalidate(NeedsBeginFrames());
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> content/browser/android/in_process/synchronous_compositor_impl.cc:137:
> DCHECK(begin_frame_source_);
> This DCHECK is not guaranteed.
> 
> Note we only call DidInitializeCompositor/DidDestroyCompositor while
compositor
> has an output surface.
> 
> Now this condition needs to be having an output surface *and* a begin frame
> source.
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> content/browser/android/in_process/synchronous_compositor_impl.cc:149: 
> Where is the BeginFrame and SetContinuousInvalidate calls here?
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> content/browser/android/in_process/synchronous_compositor_impl.cc:153: void
> SynchronousCompositorImpl::RequestBeginFrame() {
> Super confusing name, should be just BeginFrame. But this shouldn't exist at
> all.
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> content/browser/android/in_process/synchronous_compositor_impl.cc:154:
> begin_frame_source_->BeginFrame();
> DCHECK(begin_frame_source_)
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> content/browser/android/in_process/synchronous_compositor_impl.cc:155:
> SetContinuousInvalidate(begin_frame_source_->NeedsBeginFrames());
> Why?
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> content/browser/android/in_process/synchronous_compositor_impl.cc:175:
> begin_frame_source_->BeginFrame();
> Why?
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> content/browser/android/in_process/synchronous_compositor_impl.cc:258:
> compositor_client_->SetContinuousInvalidate(needs_begin_frames);
> We want to hold off on this if invoking_composite_ is true.
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> File content/browser/android/in_process/synchronous_compositor_impl.h (right):
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> content/browser/android/in_process/synchronous_compositor_impl.h:53: void
> SetExternalBeginFrameSource(
> nit: Group methods called by FrameSource together, and add a comment
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> content/browser/android/in_process/synchronous_compositor_impl.h:60:
> SynchronousCompositorOutputSurface* output_surface() const {
> not used, remove
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> content/browser/android/in_process/synchronous_compositor_impl.h:113: void
> BeginFrame() const;
> Not called or defined. Remove
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> File
content/browser/android/in_process/synchronous_compositor_output_surface.cc
> (right):
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
>
content/browser/android/in_process/synchronous_compositor_output_surface.cc:226:
> delegate->RequestBeginFrame();
> Calling through to BeginFrameSource is super awkward here. Like I said in the
> previous review, don't have OutputSurface call to BeginFrameSource through the
> compositor. That's way too awkward and confusing.

[brianderson, 2014-10-24 20:46:58]

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:121: virtual void SetClientReady() = 0;
On 2014/10/24 14:30:18, simonhong wrote:
> On 2014/10/23 02:12:44, brianderson wrote:
> > Anyway we can avoid having this method as part of the BeginFrameSource
> > interface? It's only needed for external sources to setup message routing.
Is
> > there a different hook we can use?
> 
> At first, I created ExternalBeginFrameSource deriving BeginFrameSourceMixIn.
> It has SetClientReady() as virtual abstract method.
> But, danakj@ pointed out that BeginFrameSourceMixIn should not be a base class
> of abstract class.
> Instead, danakj@ suggested adding SetClientReady() into BeginFrameSource.
> 
> Other option is adding SetClientReady() into BFSMixIn.
> But I think all of Source inherits BFSMixIn so, it is same as current.
> 
> In this case, what is the best solution? :)

Ok, what you have now is good then. Any other solution I can think of is going
to add more boilerplate than is really worth it.

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:145: void SetClientReady() override {}
On 2014/10/23 02:12:44, brianderson wrote:
> Delete this line so we do not override SetClientReady() and all subclasses
must
> implement it.
> 
> The other implementations above are expected to be the same for all
subclasses,
> which is why they aren't virtual.

Ignore my comment here. I didn't know that override implies virtual before, but
I do now. Also, inheriting an empty implementation should be fine in this case.

It'll probably be a good idea to make NeedsBeginFrames, SetNeedsBeginFrames,
AddObserver, and RemoveObserver "final" though since MixIn subclasses shouldn't
override them.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
(right):

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:14:
SynchronousCompositorExternalBeginFrameSource(int routing_id)
Why does this need to take in a routing_id rather than just a
SynchronousCompositorImpl*?

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_forwarding_message_filter.cc (right):

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter.cc:74:
target_task_runner_->PostTask(FROM_HERE, base::Bind(it->second, message));
This was probably discussed already, but why do we iterate through the handlers
here (before the post task) instead of within a posted task to the compositor
thread?

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_forwarding_message_filter.h (right):

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter.h:30: // arrived, all
handlers in |multi_handlers_| for routing id will be executed.
are arrived -> have arrived

[piman, 2014-10-24 20:49:06]

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_forwarding_message_filter.cc (right):

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter.cc:14:
CompositorForwardingMessageFilter* CompositorForwardingMessageFilter::Create(
If we're making a specific class for the compositor, I think we can simplify it
significantly rather than just copy&paste from ForwardingMessageFilter... See
below.

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter.cc:64:
message_ids_to_filter_.end())
The message id list is constant. We can have a simple switch statement on the
list of messages, rather than a costly std::set.

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter.cc:74:
target_task_runner_->PostTask(FROM_HERE, base::Bind(it->second, message));
This posts N tasks if we have N handlers, even though only one will handle the
message.
Better would be to post a single task to the target_task_runner, and then
dispatch to the right handler on the compositor thread.

As an added bonus, it means we always access multi_handlers_ on the same thread
(both AddRoute and RemoveRoute are called on the compositor thread), so we can
completely remove the lock (replace with a thread checker maybe).

[cpu_(ooo_6.6-7.5), 2014-10-29 01:23:48]

cpu@chromium.org changed reviewers:
- cpu@chromium.org

[simonhong, 2014-10-29 14:47:14]

PTAL again.
Many thanks!

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:121: virtual void SetClientReady() = 0;
On 2014/10/24 20:46:58, brianderson wrote:
> On 2014/10/24 14:30:18, simonhong wrote:
> > On 2014/10/23 02:12:44, brianderson wrote:
> > > Anyway we can avoid having this method as part of the BeginFrameSource
> > > interface? It's only needed for external sources to setup message routing.
> Is
> > > there a different hook we can use?
> > 
> > At first, I created ExternalBeginFrameSource deriving BeginFrameSourceMixIn.
> > It has SetClientReady() as virtual abstract method.
> > But, danakj@ pointed out that BeginFrameSourceMixIn should not be a base
class
> > of abstract class.
> > Instead, danakj@ suggested adding SetClientReady() into BeginFrameSource.
> > 
> > Other option is adding SetClientReady() into BFSMixIn.
> > But I think all of Source inherits BFSMixIn so, it is same as current.
> > 
> > In this case, what is the best solution? :)
> 
> Ok, what you have now is good then. Any other solution I can think of is going
> to add more boilerplate than is really worth it.

Yep!

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:145: void SetClientReady() override {}
On 2014/10/24 20:46:58, brianderson wrote:
> On 2014/10/23 02:12:44, brianderson wrote:
> > Delete this line so we do not override SetClientReady() and all subclasses
> must
> > implement it.
> > 
> > The other implementations above are expected to be the same for all
> subclasses,
> > which is why they aren't virtual.
> 
> Ignore my comment here. I didn't know that override implies virtual before,
but
> I do now. Also, inheriting an empty implementation should be fine in this
case.
> 
> It'll probably be a good idea to make NeedsBeginFrames, SetNeedsBeginFrames,
> AddObserver, and RemoveObserver "final" though since MixIn subclasses
shouldn't
> override them.

Some of them are still used by its subclass.
AddObserver & RemoveObserver only can be final candidate.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
(right):

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:14:
SynchronousCompositorExternalBeginFrameSource(int routing_id)
On 2014/10/24 20:46:58, brianderson wrote:
> Why does this need to take in a routing_id rather than just a
> SynchronousCompositorImpl*?

I think using GetCompositor() is more better than store compositor pointer
because life-cycle of compositor and BeginFrameSource are different.
When we store compositor pointer, compositor should manage BeginFrameSource,
too.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:22:
SynchronousCompositorImpl::FromRoutingID(routing_id_);
On 2014/10/24 17:42:40, boliu wrote:
> nit suggestions: Factor this out into a GetCompositor helper?

Done.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:38:
if (compositor && !compositor->invoking_composite())
On 2014/10/24 17:42:40, boliu wrote:
> You can do the invoking_composite check in Compositor. No need to expose
> invoking_composite.

Done.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:103:
begin_frame_source_ = begin_frame_source;
On 2014/10/24 17:42:40, boliu wrote:
> Need keep BeginFrameSource and compositor_client_ state in sync, ie call
> compositor_client_->SetContinuousInvalidate(NeedsBeginFrames()); 

DCHECK(compositor_client_) is added.
And, begin_frame_source_ is set to null when
DidDestroySynchronousOutputSurface() is called.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:137:
DCHECK(begin_frame_source_);
On 2014/10/24 17:42:40, boliu wrote:
> This DCHECK is not guaranteed.
> 
> Note we only call DidInitializeCompositor/DidDestroyCompositor while
compositor
> has an output surface.
> 
> Now this condition needs to be having an output surface *and* a begin frame
> source.

Removed.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:149: 
On 2014/10/24 17:42:40, boliu wrote:
> Where is the BeginFrame and SetContinuousInvalidate calls here?

SetContinuousInvalidate() is added.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:153: void
SynchronousCompositorImpl::RequestBeginFrame() {
On 2014/10/24 17:42:40, boliu wrote:
> Super confusing name, should be just BeginFrame. But this shouldn't exist at
> all.

This function is removed.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:175:
begin_frame_source_->BeginFrame();
On 2014/10/24 17:42:40, boliu wrote:
> Why?

Removed.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:258:
compositor_client_->SetContinuousInvalidate(needs_begin_frames);
On 2014/10/24 17:42:40, boliu wrote:
> We want to hold off on this if invoking_composite_ is true.

Made early out if invoking_composite_ is true.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.h:53: void
SetExternalBeginFrameSource(
On 2014/10/24 17:42:41, boliu wrote:
> nit: Group methods called by FrameSource together, and add a comment

Done.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.h:60:
SynchronousCompositorOutputSurface* output_surface() const {
On 2014/10/24 17:42:41, boliu wrote:
> not used, remove

Done.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.h:113: void
BeginFrame() const;
On 2014/10/24 17:42:41, boliu wrote:
> Not called or defined. Remove

Done.

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:226:
delegate->RequestBeginFrame();
On 2014/10/24 17:42:41, boliu wrote:
> Calling through to BeginFrameSource is super awkward here. Like I said in the
> previous review, don't have OutputSurface call to BeginFrameSource through the
> compositor. That's way too awkward and confusing.

Yes, I understood your meaning.
Requesting to compositor again was strange when output surface receives a draw
request from compositor.

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_forwarding_message_filter.cc (right):

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter.cc:14:
CompositorForwardingMessageFilter* CompositorForwardingMessageFilter::Create(
On 2014/10/24 20:49:06, piman (Very slow to review) wrote:
> If we're making a specific class for the compositor, I think we can simplify
it
> significantly rather than just copy&paste from ForwardingMessageFilter... See
> below.

Done.

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter.cc:64:
message_ids_to_filter_.end())
On 2014/10/24 20:49:06, piman (Very slow to review) wrote:
> The message id list is constant. We can have a simple switch statement on the
> list of messages, rather than a costly std::set.

Done.

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter.cc:74:
target_task_runner_->PostTask(FROM_HERE, base::Bind(it->second, message));
On 2014/10/24 20:49:06, piman (Very slow to review) wrote:
> This posts N tasks if we have N handlers, even though only one will handle the
> message.
> Better would be to post a single task to the target_task_runner, and then
> dispatch to the right handler on the compositor thread.
> 
> As an added bonus, it means we always access multi_handlers_ on the same
thread
> (both AddRoute and RemoveRoute are called on the compositor thread), so we can
> completely remove the lock (replace with a thread checker maybe).

Modified to dispatch by posted task on compositor thread.

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_forwarding_message_filter.h (right):

https://codereview.chromium.org/619843002/diff/1580001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter.h:30: // arrived, all
handlers in |multi_handlers_| for routing id will be executed.
On 2014/10/24 20:46:58, brianderson wrote:
> are arrived -> have arrived

Done.

[danakj, 2014-10-29 15:12:25]

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
(right):

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:14:
SynchronousCompositorExternalBeginFrameSource(int routing_id)
On 2014/10/29 14:47:13, simonhong wrote:
> On 2014/10/24 20:46:58, brianderson wrote:
> > Why does this need to take in a routing_id rather than just a
> > SynchronousCompositorImpl*?
> 
> I think using GetCompositor() is more better than store compositor pointer
> because life-cycle of compositor and BeginFrameSource are different.
> When we store compositor pointer, compositor should manage BeginFrameSource,
> too.

If an owner of BeginFrameSource will guarantee the compositor outlives the BFS
or tells it when the compositor is destroyed, then holding a pointer is fine,
without involving the compositor. You don't need to make every pointer
refcounted.

[boliu, 2014-10-29 15:41:58]

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
(right):

https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:14:
SynchronousCompositorExternalBeginFrameSource(int routing_id)
On 2014/10/29 15:12:24, danakj wrote:
> On 2014/10/29 14:47:13, simonhong wrote:
> > On 2014/10/24 20:46:58, brianderson wrote:
> > > Why does this need to take in a routing_id rather than just a
> > > SynchronousCompositorImpl*?

Answering Brian's original question:

I requested this. It's not thread safe to grab a SynchronousCompositorImpl here.

The only way for BFS (a renderer side object) to get a SynchronousCompositorImpl
(a browser side object) is through
SynchronousCompositorImpl::FromID/FromRoutingID. And these are only safe to call
on the UI/compositor thread. BFS is created on the blink main thread so it's not
safe to call it here.

This is the same pattern that SynchronousCompositorOutputSurface uses: get
routing_id on creation, and get the compositor when bound on compositor thread.

> > 
> > I think using GetCompositor() is more better than store compositor pointer
> > because life-cycle of compositor and BeginFrameSource are different.
> > When we store compositor pointer, compositor should manage BeginFrameSource,
> > too.
> 
> If an owner of BeginFrameSource will guarantee the compositor outlives the BFS
> or tells it when the compositor is destroyed, then holding a pointer is fine,
> without involving the compositor. You don't need to make every pointer
> refcounted.

The link between browser side and renderer side objects is weak; either one can
be destroyed first. So need to do the latter, ie when a objects is destroyed, it
needs to inform the other side. And both sides need to handle things correctly
after the other side is gone.

But yeah, holding a SynchronousCompositorImpl pointer in SetClientReady is fine,
as long as it's only used on UI/compositor thread.

[piman, 2014-10-29 17:07:57]

content/renderer lgtm

[simonhong, 2014-10-30 00:21:33]

On 2014/10/29 15:41:58, boliu wrote:
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
> File
>
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
> (right):
> 
>
https://codereview.chromium.org/619843002/diff/1580001/content/browser/androi...
>
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:14:
> SynchronousCompositorExternalBeginFrameSource(int routing_id)
> On 2014/10/29 15:12:24, danakj wrote:
> > On 2014/10/29 14:47:13, simonhong wrote:
> > > On 2014/10/24 20:46:58, brianderson wrote:
> > > > Why does this need to take in a routing_id rather than just a
> > > > SynchronousCompositorImpl*?
> 
> Answering Brian's original question:
> 
> I requested this. It's not thread safe to grab a SynchronousCompositorImpl
here.
> 
> The only way for BFS (a renderer side object) to get a
SynchronousCompositorImpl
> (a browser side object) is through
> SynchronousCompositorImpl::FromID/FromRoutingID. And these are only safe to
call
> on the UI/compositor thread. BFS is created on the blink main thread so it's
not
> safe to call it here.
> 
> This is the same pattern that SynchronousCompositorOutputSurface uses: get
> routing_id on creation, and get the compositor when bound on compositor
thread.
> 
> > > 
> > > I think using GetCompositor() is more better than store compositor pointer
> > > because life-cycle of compositor and BeginFrameSource are different.
> > > When we store compositor pointer, compositor should manage
BeginFrameSource,
> > > too.
> > 
> > If an owner of BeginFrameSource will guarantee the compositor outlives the
BFS
> > or tells it when the compositor is destroyed, then holding a pointer is
fine,
> > without involving the compositor. You don't need to make every pointer
> > refcounted.
> 
> The link between browser side and renderer side objects is weak; either one
can
> be destroyed first. So need to do the latter, ie when a objects is destroyed,
it
> needs to inform the other side. And both sides need to handle things correctly
> after the other side is gone.
> 
> But yeah, holding a SynchronousCompositorImpl pointer in SetClientReady is
fine,
> as long as it's only used on UI/compositor thread.

BFS is modified to grab compositor pointer when SetClientReady() is called.
Also, one of both(compositor, BFS) is destroyed first, other will invalidate it.
Does this way make all happy?

[boliu, 2014-10-30 16:21:33]

sync compositor bits getting close

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
(right):

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:48:
SynchronousCompositorImpl* compositor = GetCompositor();
Just put the code for GetCompositor here, since this is the only callsite. It's
super confusing to have both compositor_ and GetCompositor that does *not*
return compositor_

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:51:
SetCompositor(compositor);
nit: I think it's awkward SetCompositor is called both here and from
SyncComopsitorImpl. Can you just have it only be called from
SetExternalBeginFrameSource? Kinda sucks to re-enter into this class, but imo
it's more consistent than this.

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:106:
DCHECK(compositor_client_);
Why? I don't think this is guaranteed in general. Even if it is, it's a bad
assumption.

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:107:
begin_frame_source_ = begin_frame_source;
You are assuming here that SetExternalBeginFrameSource is called before
DidBindOutputSurface, which is also a bad assumption.

In general, don't assume (SetExternalBeginFrameSource, DidBindOutputSurface,
SetClient) are called in any particular order.

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:156: if
(begin_frame_source_)
This should be a DCHECK like output_surface_.

compositor_client_ will only call DemandDrawHw/Sw between
DidInitializeCompositor and DidDestroyCompositor. So don't call
DidInitializeCompositor until both OS and BFS are ready, and call
DidDestroyCompositor as soon as one of them goes away.

Then you can make this a DCHECK.

[simonhong, 2014-10-31 00:22:08]

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
(right):

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:48:
SynchronousCompositorImpl* compositor = GetCompositor();
On 2014/10/30 16:21:32, boliu wrote:
> Just put the code for GetCompositor here, since this is the only callsite.
It's
> super confusing to have both compositor_ and GetCompositor that does *not*
> return compositor_

Done.

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:51:
SetCompositor(compositor);
On 2014/10/30 16:21:32, boliu wrote:
> nit: I think it's awkward SetCompositor is called both here and from
> SyncComopsitorImpl. Can you just have it only be called from
> SetExternalBeginFrameSource? Kinda sucks to re-enter into this class, but imo
> it's more consistent than this.

Done.

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:106:
DCHECK(compositor_client_);
On 2014/10/30 16:21:32, boliu wrote:
> Why? I don't think this is guaranteed in general. Even if it is, it's a bad
> assumption.

Done.

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:107:
begin_frame_source_ = begin_frame_source;
On 2014/10/30 16:21:33, boliu wrote:
> You are assuming here that SetExternalBeginFrameSource is called before
> DidBindOutputSurface, which is also a bad assumption.
> 
> In general, don't assume (SetExternalBeginFrameSource, DidBindOutputSurface,
> SetClient) are called in any particular order.

Removed that assumption related code in DidBindOutputSurface.
(removed DCHECK(begin_frame_source_))

Btw, why that assumption is bad?
In current implementation, DidBindOutputSurface is always called after
SetClient().
Also, if SetClient() is not called first than DidBindOutputSurface(),
we can't generate next frame in current code base.

https://codereview.chromium.org/619843002/diff/1720001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:156: if
(begin_frame_source_)
On 2014/10/30 16:21:32, boliu wrote:
> This should be a DCHECK like output_surface_.
> 
> compositor_client_ will only call DemandDrawHw/Sw between
> DidInitializeCompositor and DidDestroyCompositor. So don't call
> DidInitializeCompositor until both OS and BFS are ready, and call
> DidDestroyCompositor as soon as one of them goes away.
> 
> Then you can make this a DCHECK.

yep, right. It should be always not null.

[boliu, 2014-10-31 01:14:01]

You are still implicitly assuming SetExternalBeginFrameSource happens before
DidBindOutputSurface. And I see it's actually going to make the code more
complicated to handle the reverse. So question to compositor owners
danakj/brianderson:

Is it ok to assume BFS is initialized on compositor before output surface is
bound?

If yes, I'm mostly happy with the current sync compositor code, and will only
suggest adding some DCHECKs.

The reason I thought this is a bad assumption is there's nothing in the design
of the API that suggests these things are ordered, so safer to assume they are
not. Eg in the fugure, cc could be refactored to have output surface be bound
first, but only sometimes. Then that would be a hard-ish bug to catch.

[danakj, 2014-10-31 14:09:53]

On Thu, Oct 30, 2014 at 9:14 PM, <boliu@chromium.org> wrote:

> You are still implicitly assuming SetExternalBeginFrameSource happens
> before
> DidBindOutputSurface. And I see it's actually going to make the code more
> complicated to handle the reverse. So question to compositor owners
> danakj/brianderson:
>
> Is it ok to assume BFS is initialized on compositor before output surface
> is
> bound?
>
> If yes, I'm mostly happy with the current sync compositor code, and will
> only
> suggest adding some DCHECKs.
>
> The reason I thought this is a bad assumption is there's nothing in the
> design
> of the API that suggests these things are ordered, so safer to assume they
> are
> not. Eg in the fugure, cc could be refactored to have output surface be
> bound
> first, but only sometimes. Then that would be a hard-ish bug to catch.
>

I think it's fine. Add comments to the interface that describe the ordering
requirements, and DCHECKs to verify it, and a cc unit test that shows we do
it.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[boliu, 2014-10-31 16:48:21]

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
(right):

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:51:
compositor->SetExternalBeginFrameSource(this);
compositor->SetContinuousInvalidate(...)

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:105:
begin_frame_source_ = begin_frame_source;
It's a bit awkard to DCHECK this, and so let's split this out into
DidInitializeBeginFrameSource and DidDestroyBFS:

DidInititalize(BFS* bfs) {
  DCHECK(!begin_frame_source_);
  DCHECK(!output_surface_);
  DCHECK(bfs);
  begin_frame_source_ = bfs;
  begin_frame_source_->SetCompositor(this);
}

DidDestroy(BFS* bfs) {
  DCHECK(!begin_frame_source_);
  DCHECK_EQ(begin_frame_source_, bfs);
  bfs->SetCompositor(nullptr);
  begin_frame_source_ = nullptr;

  if (output_surface_)
     output_surface_->SetBeginFrameSource(nullptr);
  
  // These 3 lines should probably be a helper function.
  if (compositor_client_)
    compositor_client_->DidDestroyCompositor(this);
  compositor_client_ = nullptr;
}

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:211:
DCHECK(CalledOnValidThread());
You actually do want DCHECK(begin_frame_source_); then :)
Also should add a DCHECK(output_surface); that should have been here already.

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:226:
output_surface_ = NULL;
output_surface_->SetBeginFrameSource(nullptr);

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:231:
DCHECK(begin_frame_source_);
Don't assume particular destruction order either. You can just remove this new
block I think.

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:266: void
SynchronousCompositorImpl::SetContinuousInvalidate(
Prefer only have one way to get "needs_begin_frames". So make this
"NeedsBeginFrameChanged()" with the parameter removed, and then just ask BFS for
the value below.

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:228:
if (begin_frame_source_)
This can be a DCHECK

[brianderson, 2014-11-01 01:08:32]

cc lgtm

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/619843002/diff/1460001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:145: void SetClientReady() override {}
On 2014/10/29 14:47:12, simonhong wrote:
> On 2014/10/24 20:46:58, brianderson wrote:
> > On 2014/10/23 02:12:44, brianderson wrote:
> > > Delete this line so we do not override SetClientReady() and all subclasses
> > must
> > > implement it.
> > > 
> > > The other implementations above are expected to be the same for all
> > subclasses,
> > > which is why they aren't virtual.
> > 
> > Ignore my comment here. I didn't know that override implies virtual before,
> but
> > I do now. Also, inheriting an empty implementation should be fine in this
> case.
> > 
> > It'll probably be a good idea to make NeedsBeginFrames, SetNeedsBeginFrames,
> > AddObserver, and RemoveObserver "final" though since MixIn subclasses
> shouldn't
> > override them.
> 
> Some of them are still used by its subclass.
> AddObserver & RemoveObserver only can be final candidate. 

Hmm, NeedsBeginFrames and SetNeedsBeginFrames should also be able to be final
since BeginFrameSourceMixIn has OnNeedsBeginFramesChange that it expects
subclasses to implement. Don't worry about cleaning it up in this patch though,
I've opened crbug.com/429391 for followup.

[simonhong, 2014-11-01 02:13:06]

boliu@, your comments are addressed.
danakj@, check routine that ensure BFS is initialized when OutputSurface is
created is added in the LayerTreeTest::CreateOutputSurface().
briandersion@, BeginFrameSourceMixIn::SetNeedsBeginFrames() is slightly changed.

Please check again and many thanks for kind review!

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
(right):

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:51:
compositor->SetExternalBeginFrameSource(this);
On 2014/10/31 16:48:21, boliu wrote:
> compositor->SetContinuousInvalidate(...)

Added.

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:105:
begin_frame_source_ = begin_frame_source;
On 2014/10/31 16:48:21, boliu wrote:
> It's a bit awkard to DCHECK this, and so let's split this out into
> DidInitializeBeginFrameSource and DidDestroyBFS:
> 
> DidInititalize(BFS* bfs) {
>   DCHECK(!begin_frame_source_);
>   DCHECK(!output_surface_);
>   DCHECK(bfs);
>   begin_frame_source_ = bfs;
>   begin_frame_source_->SetCompositor(this);
> }
> 
> DidDestroy(BFS* bfs) {
>   DCHECK(!begin_frame_source_);
>   DCHECK_EQ(begin_frame_source_, bfs);
>   bfs->SetCompositor(nullptr);
>   begin_frame_source_ = nullptr;
> 
>   if (output_surface_)
>      output_surface_->SetBeginFrameSource(nullptr);
>   
>   // These 3 lines should probably be a helper function.
>   if (compositor_client_)
>     compositor_client_->DidDestroyCompositor(this);
>   compositor_client_ = nullptr;
> }

Done.

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:211:
DCHECK(CalledOnValidThread());
On 2014/10/31 16:48:21, boliu wrote:
> You actually do want DCHECK(begin_frame_source_); then :)
> Also should add a DCHECK(output_surface); that should have been here already.

Done.

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:226:
output_surface_ = NULL;
On 2014/10/31 16:48:21, boliu wrote:
> output_surface_->SetBeginFrameSource(nullptr);

Done.

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:231:
DCHECK(begin_frame_source_);
On 2014/10/31 16:48:21, boliu wrote:
> Don't assume particular destruction order either. You can just remove this new
> block I think.

Done.

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:266: void
SynchronousCompositorImpl::SetContinuousInvalidate(
On 2014/10/31 16:48:21, boliu wrote:
> Prefer only have one way to get "needs_begin_frames". So make this
> "NeedsBeginFrameChanged()" with the parameter removed, and then just ask BFS
for
> the value below.

Done.

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_output_surface.cc
(right):

https://codereview.chromium.org/619843002/diff/1740001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_output_surface.cc:228:
if (begin_frame_source_)
On 2014/10/31 16:48:21, boliu wrote:
> This can be a DCHECK

Done.

[simonhong, 2014-11-01 03:35:52]

Patchset #40 (id:1780001) has been deleted

[simonhong, 2014-11-01 03:38:12]

Patchset #39 (id:1760001) has been deleted

[boliu, 2014-11-03 19:16:57]

Thanks for bearing through this :)

content/browser/android/in_process lgtm, although I don't have owner powers
there
android_webview lgtm

[boliu, 2014-11-03 19:17:45]

Oops, one last small comment

https://codereview.chromium.org/619843002/diff/1800001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/619843002/diff/1800001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:115:
begin_frame_source_->SetCompositor(this);
SetCompositor(nullptr);

[simonhong, 2014-11-03 21:26:21]

simonhong@chromium.org changed reviewers:
+ dcheng@chromium.org, sievers@chromium.org, sky@chromium.org
- jam@chromium.org

[simonhong, 2014-11-03 21:26:24]

boliu@, many many thanks for review!

Please review below files
sievers@, content/browser/renderer_host/*
piman@, content/*.gypi,
dcheng@, content/common/*
sky@, content/test/*, ui/compositor/*
jamesr@, mojo/services/*

https://codereview.chromium.org/619843002/diff/1800001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/619843002/diff/1800001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_impl.cc:115:
begin_frame_source_->SetCompositor(this);
On 2014/11/03 19:17:45, boliu wrote:
> SetCompositor(nullptr);

Done.

[jamesr, 2014-11-03 21:44:09]

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

[jamesr, 2014-11-03 21:44:11]

mojo/services/html_viewer/ lgtm, although is the begin frame source supposed to
be nullptr there or are we supposed to inject something else?

[simonhong, 2014-11-03 21:47:36]

On 2014/11/03 21:44:11, jamesr wrote:
> mojo/services/html_viewer/ lgtm, although is the begin frame source supposed
to
> be nullptr there or are we supposed to inject something else?

If it is needed, it should not be null here.

[jamesr, 2014-11-03 21:50:13]

I'm asking you if it's needed.  Is it?

[simonhong, 2014-11-03 21:54:37]

It is dependent on settings.begin_frame_scheduling_enabled.
If that flag is true, external begin frame source is needed and passed to
LTH in here.
I think WebLayerTreeViewImpl doesn't need it.
So, passing nullptr is ok in here.


On Tue, Nov 4, 2014 at 6:50 AM, <jamesr@chromium.org> wrote:

> I'm asking you if it's needed.  Is it?
>
> https://codereview.chromium.org/619843002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[no sievers, 2014-11-03 22:33:34]

On 2014/11/03 21:26:24, simonhong wrote:
> sievers@, content/browser/renderer_host/*

content/browser/renderer_host/* lgtm

[piman, 2014-11-03 23:51:15]

content/*.gypi lgtm

[simonhong, 2014-11-04 20:10:23]

Kindly ping to dcheng@ and sky@.

dcheng@, content/common/view_messages.h
sky@, content/test/*, ui/compositor/*

Thanks!

[dcheng, 2014-11-04 20:45:49]

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

[dcheng, 2014-11-04 20:45:51]

The IPC changes are probably fine, but I'm at BlinkOn right now and don't have
the cycles to examine the moves in more in detail. If you need a reply before
Thursday, I'd suggest finding another IPC reviewer. Sorry.

[dcheng, 2014-11-04 20:46:48]

dcheng@chromium.org changed reviewers:
- cpu@chromium.org

[dcheng, 2014-11-04 20:46:51]

(Um, does anyone know why Rietveld added cpu as a reviewer? I don't think I
added him... -cpu)

[sky, 2014-11-04 21:08:30]

LGTM

https://codereview.chromium.org/619843002/diff/1820001/content/test/web_layer...
File content/test/web_layer_tree_view_impl_for_testing.cc (right):

https://codereview.chromium.org/619843002/diff/1820001/content/test/web_layer...
content/test/web_layer_tree_view_impl_for_testing.cc:49: NULL,
Since these are on the same line you should convert to nullptr.

[simonhong, 2014-11-04 21:08:44]

On 2014/11/04 20:46:51, dcheng wrote:
> (Um, does anyone know why Rietveld added cpu as a reviewer? I don't think I
> added him... -cpu)

Ah.. BlinkOn is On in this week!
No problem for waiting.

cpu was added for reviewing for ./ipc.
IPC related modification is removed but, I can't remove cpu in the reviewer
list.
I think it's a Rietveld bug.

[simonhong, 2014-11-04 21:18:06]

https://codereview.chromium.org/619843002/diff/1820001/content/test/web_layer...
File content/test/web_layer_tree_view_impl_for_testing.cc (right):

https://codereview.chromium.org/619843002/diff/1820001/content/test/web_layer...
content/test/web_layer_tree_view_impl_for_testing.cc:49: NULL,
On 2014/11/04 21:08:30, sky wrote:
> Since these are on the same line you should convert to nullptr.

Done.

[simonhong, 2014-11-06 21:38:54]

Remind to dcheng@ and skyostil@ because BlinkOn is over :)
skyostil@, I need owner's stamp on content/browser/android/
dcheng@, FYI BeginFrame messages are out from android define. It will be used on
all other platforms, too.

[simonhong, 2014-11-10 13:19:54]

On 2014/11/06 21:38:54, simonhong wrote:
> Remind to dcheng@ and skyostil@ because BlinkOn is over :)
> skyostil@, I need owner's stamp on content/browser/android/
> dcheng@, FYI BeginFrame messages are out from android define. It will be used
on
> all other platforms, too.

Kindly ping to below owners.
skyostil@ - content/browser/android/*
dcheng@, content/common/view_messages.h

[Sami, 2014-11-10 16:09:44]

> Kindly ping to below owners.
> skyostil@ - content/browser/android/*

Apologies for the delay. I also blame BlinkOn :) Overall content/browser/android
looks but I have a suggested simplification.

https://codereview.chromium.org/619843002/diff/1840001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
(right):

https://codereview.chromium.org/619843002/diff/1840001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:51:
compositor->DidInitializeExternalBeginFrameSource(this);
Calling into the compositor and relying on it to call back into SetCompositor()
seems a little roundabout and brittle. How about we set compositor_ =
SynchronousCompositorImpl::FromRoutingID(routing_id_) here and change
SetCompositor into ClearCompositor so the pointer can be nulled at the
appropriate time. Would that work?

https://codereview.chromium.org/619843002/diff/1840001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h
(right):

https://codereview.chromium.org/619843002/diff/1840001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h:15:
// Make sure that this is initialized and set to compositor befere output
s/befere/before/

https://codereview.chromium.org/619843002/diff/1840001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_output_surface.h
(right):

https://codereview.chromium.org/619843002/diff/1840001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_output_surface.h:88:
void SetExternalBeginFrameSource(
This should be written in hacker_style if you want to inline it like this:

    set_external_begin_frame_source

Otherwise you could move the implementation to the .cc.

[boliu, 2014-11-10 16:19:34]

https://codereview.chromium.org/619843002/diff/1840001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
(right):

https://codereview.chromium.org/619843002/diff/1840001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:51:
compositor->DidInitializeExternalBeginFrameSource(this);
On 2014/11/10 16:09:43, Sami wrote:
> Calling into the compositor and relying on it to call back into
SetCompositor()
> seems a little roundabout and brittle. How about we set compositor_ =
> SynchronousCompositorImpl::FromRoutingID(routing_id_) here and change
> SetCompositor into ClearCompositor so the pointer can be nulled at the
> appropriate time. Would that work?

I asked for this current pattern and would like to keep it. It's to keep setting
|compositor_| in one place. It's set to null when in compositor destructor.

[Sami, 2014-11-10 17:52:44]

On 2014/11/10 16:19:34, boliu wrote:
> I asked for this current pattern and would like to keep it. It's to keep
setting
> |compositor_| in one place. It's set to null when in compositor destructor.

Ok, sorry I missed the discussion on that. I just thought it makes it a little
difficult to see which compositor the BFS is talking to since on one hand it
pulls one from a global registry and on other hand someone magically plugs in
the instance. It almost makes it feel like resolving the routing id should be
done somewhere else.

However, with a reasonable amount of thought the current model can be deciphered
too, so content/browser/android/ lgtm (with the other two nits I pointed out
above).

[simonhong, 2014-11-10 20:40:28]

All done!
Thanks skyostil@.

Only dcheng@'s stamp is needed to land :)

https://codereview.chromium.org/619843002/diff/1840001/content/browser/androi...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h
(right):

https://codereview.chromium.org/619843002/diff/1840001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h:15:
// Make sure that this is initialized and set to compositor befere output
On 2014/11/10 16:09:43, Sami wrote:
> s/befere/before/

Done.

https://codereview.chromium.org/619843002/diff/1840001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_output_surface.h
(right):

https://codereview.chromium.org/619843002/diff/1840001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_output_surface.h:88:
void SetExternalBeginFrameSource(
On 2014/11/10 16:09:43, Sami wrote:
> This should be written in hacker_style if you want to inline it like this:
> 
>     set_external_begin_frame_source
> 
> Otherwise you could move the implementation to the .cc.

Modified to set_external_begin_frame_source.

[dcheng, 2014-11-10 22:56:11]

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

[dcheng, 2014-11-10 22:56:15]

Sorry for the delay in reviewing this. Even though the IPC changes appear fairly
trivial, I generally trace through to make sure I understand where things are
going. The IPC bits look fine, but I have a few suggestions related to non-IPC
portions.

https://codereview.chromium.org/619843002/diff/1860001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:105: this, this, NULL, NULL,
settings, NULL, nullptr);
The inconsistency between NULL and nullptr here is rather jarring.

https://codereview.chromium.org/619843002/diff/1860001/cc/layers/layer_unitte...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:941: nullptr).Pass();
Pass() is redundant here.

https://codereview.chromium.org/619843002/diff/1860001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:952: nullptr).Pass();
Ditto. One day, I hope to be able to automatically find and fix these, but that
day is not here yet =)

https://codereview.chromium.org/619843002/diff/1860001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:37:
DCHECK(scheduler->primary_frame_source_internal_.get())
.get() is superfluous here--scoped_ptr supports boolean tests.

https://codereview.chromium.org/619843002/diff/1860001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:157: FakeExternalBeginFrameSource*
ExternalBeginFrameSource() {
Nit: since this isn't a virtual anymore, this should probably be:
FakeExternalBeginFrameSource* fake_external_begin_frame_source() const { return
fake_external_begin_frame_source_; }

or something, since it's just a simple getter.

https://codereview.chromium.org/619843002/diff/1860001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_no_message_loop.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_no_message_loop.cc:103: this, this, NULL,
NULL, settings, NULL, nullptr);
Same comment--seems weird to mix NULL and nullptr here.

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_external_begin_frame_source.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_external_begin_frame_source.cc:48:
begin_frame_source_proxy_);
Indents on this and the next 4 lines looks funny. Is this from git cl format--if
so, I need to file a bug against clang-format =)

(Note that clang-format currently mangles message map macros so I wouldn't
advocate running across the entire patch anyway, unless you take care to fix the
message maps manually =/)

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_external_begin_frame_source.cc:59:
IPC_MESSAGE_HANDLER(ViewMsg_BeginFrame, OnBeginFrame);
I don't think a ; is needed, and most people usually leave it out. We should do
the same here for consistency. IPC message map macros are weird =/

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_forwarding_message_filter.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter.cc:72: if
(handlers.first == handlers.second)
I don't think there's any real reason to have this special case. Just use a
range-based for loop which will loop 0 times if the range is empty.

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter.cc:75: for (auto it =
handlers.first; it != handlers.second; ++it)
Pretty sure you can use a range-based for loop here and other places where you
iterate through multimaps.

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_forwarding_message_filter_unittest.cc
(right):

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter_unittest.cc:31: void
reset_count() { count_ = 0; }
Not a simple getter/setter, so this should be ResetCount() or just name it
set_count() and call it with 0.

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter_unittest.cc:43:
base::TestSimpleTaskRunner* task_runner = new base::TestSimpleTaskRunner;
This is leaked. Just allocate it on the stack.

[piman, 2014-11-10 23:10:23]

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_forwarding_message_filter.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter.cc:75: for (auto it =
handlers.first; it != handlers.second; ++it)
On 2014/11/10 22:56:15, dcheng wrote:
> Pretty sure you can use a range-based for loop here and other places where you
> iterate through multimaps.

The range is based on the std::pair's first/second, not on the container's
begin()/end(). Is there an adapter that converts one to the other?

[dcheng, 2014-11-10 23:13:07]

On 2014/11/10 at 23:10:23, piman wrote:
>
https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
> File content/renderer/gpu/compositor_forwarding_message_filter.cc (right):
> 
>
https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
> content/renderer/gpu/compositor_forwarding_message_filter.cc:75: for (auto it
= handlers.first; it != handlers.second; ++it)
> On 2014/11/10 22:56:15, dcheng wrote:
> > Pretty sure you can use a range-based for loop here and other places where
you
> > iterate through multimaps.
> 
> The range is based on the std::pair's first/second, not on the container's
begin()/end(). Is there an adapter that converts one to the other?

Ah, you're right. I think at some point the draft had begin()/end() for pair,
but that's no longer true.

[simonhong, 2014-11-11 12:54:15]

Thanks dcheng@ for overall review.
Your comments are addressed.

https://codereview.chromium.org/619843002/diff/1860001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:105: this, this, NULL, NULL,
settings, NULL, nullptr);
On 2014/11/10 22:56:15, dcheng wrote:
> The inconsistency between NULL and nullptr here is rather jarring.

Done.

https://codereview.chromium.org/619843002/diff/1860001/cc/layers/layer_unitte...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:941: nullptr).Pass();
On 2014/11/10 22:56:15, dcheng wrote:
> Pass() is redundant here.

Done.

https://codereview.chromium.org/619843002/diff/1860001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:952: nullptr).Pass();
On 2014/11/10 22:56:15, dcheng wrote:
> Ditto. One day, I hope to be able to automatically find and fix these, but
that
> day is not here yet =)

Done.

https://codereview.chromium.org/619843002/diff/1860001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:37:
DCHECK(scheduler->primary_frame_source_internal_.get())
On 2014/11/10 22:56:15, dcheng wrote:
> .get() is superfluous here--scoped_ptr supports boolean tests.

Done.

https://codereview.chromium.org/619843002/diff/1860001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:157: FakeExternalBeginFrameSource*
ExternalBeginFrameSource() {
On 2014/11/10 22:56:15, dcheng wrote:
> Nit: since this isn't a virtual anymore, this should probably be:
> FakeExternalBeginFrameSource* fake_external_begin_frame_source() const {
return
> fake_external_begin_frame_source_; }
> 
> or something, since it's just a simple getter.

made this as a const.

https://codereview.chromium.org/619843002/diff/1860001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_no_message_loop.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_no_message_loop.cc:103: this, this, NULL,
NULL, settings, NULL, nullptr);
On 2014/11/10 22:56:15, dcheng wrote:
> Same comment--seems weird to mix NULL and nullptr here.

unified to use nullptr.

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_external_begin_frame_source.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_external_begin_frame_source.cc:48:
begin_frame_source_proxy_);
On 2014/11/10 22:56:15, dcheng wrote:
> Indents on this and the next 4 lines looks funny. Is this from git cl
format--if
> so, I need to file a bug against clang-format =)
> 
> (Note that clang-format currently mangles message map macros so I wouldn't
> advocate running across the entire patch anyway, unless you take care to fix
the
> message maps manually =/)

changed to less funny :)
git cl format didn't complain about this.

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_external_begin_frame_source.cc:59:
IPC_MESSAGE_HANDLER(ViewMsg_BeginFrame, OnBeginFrame);
On 2014/11/10 22:56:15, dcheng wrote:
> I don't think a ; is needed, and most people usually leave it out. We should
do
> the same here for consistency. IPC message map macros are weird =/

Done.

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_forwarding_message_filter_unittest.cc
(right):

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter_unittest.cc:31: void
reset_count() { count_ = 0; }
On 2014/11/10 22:56:15, dcheng wrote:
> Not a simple getter/setter, so this should be ResetCount() or just name it
> set_count() and call it with 0.

Done.

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter_unittest.cc:43:
base::TestSimpleTaskRunner* task_runner = new base::TestSimpleTaskRunner;
On 2014/11/10 22:56:15, dcheng wrote:
> This is leaked. Just allocate it on the stack.

Done.

[dcheng, 2014-11-11 18:59:01]

LGTM with nits and compile fix addressed.

https://codereview.chromium.org/619843002/diff/1860001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:157: FakeExternalBeginFrameSource*
ExternalBeginFrameSource() {
On 2014/11/11 12:54:15, simonhong wrote:
> On 2014/11/10 22:56:15, dcheng wrote:
> > Nit: since this isn't a virtual anymore, this should probably be:
> > FakeExternalBeginFrameSource* fake_external_begin_frame_source() const {
> return
> > fake_external_begin_frame_source_; }
> > 
> > or something, since it's just a simple getter.
> 
> made this as a const.

My main point is this should be named_with_accessor_style()

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_forwarding_message_filter_unittest.cc
(right):

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter_unittest.cc:43:
base::TestSimpleTaskRunner* task_runner = new base::TestSimpleTaskRunner;
On 2014/11/11 12:54:15, simonhong wrote:
> On 2014/11/10 22:56:15, dcheng wrote:
> > This is leaked. Just allocate it on the stack.
> 
> Done.

Oops, this needs to be held in a scoped_refptr, since task runners are
refcounted.

[simonhong, 2014-11-11 19:30:30]

Patchset #44 (id:1900001) has been deleted

[simonhong, 2014-11-11 19:33:07]

Thanks dcheng!
All done.

https://codereview.chromium.org/619843002/diff/1860001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/619843002/diff/1860001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:157: FakeExternalBeginFrameSource*
ExternalBeginFrameSource() {
On 2014/11/11 18:59:01, dcheng wrote:
> On 2014/11/11 12:54:15, simonhong wrote:
> > On 2014/11/10 22:56:15, dcheng wrote:
> > > Nit: since this isn't a virtual anymore, this should probably be:
> > > FakeExternalBeginFrameSource* fake_external_begin_frame_source() const {
> > return
> > > fake_external_begin_frame_source_; }
> > > 
> > > or something, since it's just a simple getter.
> > 
> > made this as a const.
> 
> My main point is this should be named_with_accessor_style()

Ah! changed to fake_external_begin_frame_source().

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_forwarding_message_filter_unittest.cc
(right):

https://codereview.chromium.org/619843002/diff/1860001/content/renderer/gpu/c...
content/renderer/gpu/compositor_forwarding_message_filter_unittest.cc:43:
base::TestSimpleTaskRunner* task_runner = new base::TestSimpleTaskRunner;
On 2014/11/11 18:59:01, dcheng wrote:
> On 2014/11/11 12:54:15, simonhong wrote:
> > On 2014/11/10 22:56:15, dcheng wrote:
> > > This is leaked. Just allocate it on the stack.
> > 
> > Done.
> 
> Oops, this needs to be held in a scoped_refptr, since task runners are
> refcounted.

Ah.. sorry my mistake.
Done.

[simonhong, 2014-11-11 19:33:30]

The CQ bit was checked by simonhong@chromium.org

[commit-bot: I haz the power, 2014-11-11 19:34:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/619843002/1920001

[commit-bot: I haz the power, 2014-11-11 20:50:42]

Committed patchset #44 (id:1920001)

[commit-bot: I haz the power, 2014-11-11 20:51:28]

Patchset 44 (id:??) landed as
https://crrev.com/a7e3ac41cb5b104e6d96fc31c2a1e5330b601ec7
Cr-Commit-Position: refs/heads/master@{#303715}

[Stephen White, 2014-11-12 04:09:48]

I suspect this change may be causing failures of content_browsertests on
ChromeOS and Android Debug, e.g.:

failures:
CrossProcessFrameTreeBrowserTest.CreateCrossProcessSubframeProxies
SitePerProcessBrowserTest.NavigateRemoteFrame
SitePerProcessBrowserTest.ProxyCreationSkipsSubtree
SitePerProcessBrowserTest.CrossSiteIframe

http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg...

I'm inclined to try a speculative revert, since this is blocking the Blink roll
and we're getting behind.

[Stephen White, 2014-11-12 04:40:22]

Never mind -- it can't be this change, since this is a Chrome change, and the
main waterfall bots are fine.