Issue 2362073002: cc/blimp: Add a LayerTreeHostRemote implementation.

Khushal

Description was changed from ========== cc/blimp: Add a LayerTreeHostRemote implementation. This sets up the framework ...

4 years, 2 months ago (2016-09-22 23:27:57 UTC) #1

Khushal

khushalsagar@chromium.org changed reviewers: + dtrainor@chromium.org, wez@chromium.org

4 years, 2 months ago (2016-09-22 23:29:22 UTC) #2

Khushal

I still have to add tests for this, but wanted to get a review on ...

4 years, 2 months ago (2016-09-22 23:29:22 UTC) #3

danakj

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

4 years, 2 months ago (2016-09-22 23:52:29 UTC) #4

danakj

https://codereview.chromium.org/2362073002/diff/1/cc/BUILD.gn File cc/BUILD.gn (right): https://codereview.chromium.org/2362073002/diff/1/cc/BUILD.gn#newcode46 cc/BUILD.gn:46: "blimp/compositor_proto_state.cc", I don't think LTHRemote (or maybe any of ...

4 years, 2 months ago (2016-09-22 23:52:30 UTC) #5

Khushal

https://codereview.chromium.org/2362073002/diff/1/cc/BUILD.gn File cc/BUILD.gn (right): https://codereview.chromium.org/2362073002/diff/1/cc/BUILD.gn#newcode46 cc/BUILD.gn:46: "blimp/compositor_proto_state.cc", On 2016/09/22 23:52:29, danakj wrote: > I don't ...

4 years, 2 months ago (2016-09-23 00:05:42 UTC) #6

Khushal

Added tests too. Dana, could you do a cc review for this?

4 years, 2 months ago (2016-09-23 21:25:32 UTC) #7

danakj

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_proto_state_sink_client.h File cc/blimp/compositor_proto_state_sink_client.h (right): https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_proto_state_sink_client.h#newcode29 cc/blimp/compositor_proto_state_sink_client.h:29: virtual base::SingleThreadTaskRunner* GetMainTaskRunner() = 0; Why do you need ...

4 years, 2 months ago (2016-09-26 21:36:54 UTC) #8

Khushal

Thanks Dana. https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_proto_state_sink_client.h File cc/blimp/compositor_proto_state_sink_client.h (right): https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_proto_state_sink_client.h#newcode29 cc/blimp/compositor_proto_state_sink_client.h:29: virtual base::SingleThreadTaskRunner* GetMainTaskRunner() = 0; On 2016/09/26 ...

4 years, 2 months ago (2016-09-27 01:07:50 UTC) #9

Thanks Dana.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_pro...
File cc/blimp/compositor_proto_state_sink_client.h (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_pro...
cc/blimp/compositor_proto_state_sink_client.h:29: virtual
base::SingleThreadTaskRunner* GetMainTaskRunner() = 0;
On 2016/09/26 21:36:52, danakj wrote:
> Why do you need this?

My understanding was that the task runner that is given to the compositor for
the main thread is created using the renderer scheduler. I was initially using
ThreadTaskRunnerHandler::Get but that would be for the render thread's message
loop right?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:54:
ui_resource_manager_(base::MakeUnique<UIResourceManager>()),
On 2016/09/26 21:36:53, danakj wrote:
> why is it a pointer?

I just did that because LTHInProcess also had a ptr. Changed.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:83: LOG(ERROR) << "UIResourceManager
requested.";
On 2016/09/26 21:36:52, danakj wrote:
> How about notreached and return null then?

This is used by Painted scrollbars only, which blimp never uses. But I'm not
sure if there is a layer type added that ends up relying on this, then if it
should result in a crash.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:96: // We don't need to care about
SurfaceLayers. The Surfaces system is
On 2016/09/26 21:36:52, danakj wrote:
> Can we NOTREACHED?

The renderer will always call this still, even if no SurfaceLayers are attached
to the tree.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:102: // Compositor-worker not supported.
On 2016/09/26 21:36:52, danakj wrote:
> Can we NOTREACHED?

I think this is all setup code which will still run, even if the page doesn't
use compositor worker.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:115: // TODO(khushalsagar) : Take care of Gpu
raster.
On 2016/09/26 21:36:52, danakj wrote:
> Can you point at a bug?

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:120: // true and changes only during page
transitions.
On 2016/09/26 21:36:52, danakj wrote:
> What do you mean by it should always be true? Calls to this with false should
> never happen? We start not visible though. What can we DCHECK?

I was just adding this comment to point out why we don't care about sending any
kind of visibility updates to the client. During page transitions, blink makes
the compositor invisible but other than that, in blimp mode the renderer is
never made invisible.

Either way, probably unnecessary to have this comment in cc. Updated it.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:130: NOTREACHED() << "We never ask the client
for a CompositorFrameSink";
On 2016/09/26 21:36:53, danakj wrote:
> "We" is a bit ambiguous here. "The LayerTreeHostClient is never asked for a.."
?

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:135: // Since we never have a
CompositorFrameSink, this is always a no-op.
On 2016/09/26 21:36:52, danakj wrote:
> Can we NOTREACHED?

Well, the API says can be called safely anytime, so I did not want to fail here.

> 
> ps anything that is NOTREACHED in here maybe should not be part of the LTH
> interface, and should be only part of the inprocess thing.

The renderer still has to deal with a common interface. Its similar to how we
have the Composite method which is effectively a NOTREACHED in
LayerTreeHostInProcess for threaded mode.

The other option to keep it only on the InProcess thing would be to static cast
in RenderWidgetCompositor and have it be aware of InProcess vs remote use I
guess. Is that better?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:165: NOTREACHED() << "Only supported in
single-threaded mode";
On 2016/09/26 21:36:52, danakj wrote:
> ".. and this class does not support single-thread since it is out of process"
?

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:169: NOTREACHED() << "Only supported in
single-threaded mode";
On 2016/09/26 21:36:52, danakj wrote:
> ditto

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:173: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/26 21:36:53, danakj wrote:
> I don't know about this. This causes the compositor to produce a frame.
> RenderWidget pokes compositor to do this at certain times. I feel like they
may
> be redundant but it has nothing to do with in/out of process.

Its redundant in the blimp case. The RenderWidget would poke the compositor
about it when, for instance, the browser drops the delegated frame and it needs
to ask the compositor to produce another one. All the delegated frame management
and such calls will be relevant on the client only.

I'm not sure what we should be doing differently here?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:183: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/26 21:36:53, danakj wrote:
> This is for testing, to ensure that it completes a draw so that we can look at
> what is drawn. This is unrelated to in/out of process.

Still no need to send anything to the client though?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:188: // We don't throttle commits right now
so this is not relevant.
On 2016/09/26 21:36:52, danakj wrote:
> NOTREACHED?

NOTIMPLEMENTED is better I think. The engine will wait for an Ack from the
client before sending another frame (which does throttle commits in a way) so
it's possible to get into this state.

What I was commenting on was that there is no throttling at the moment where we
don't send updates, even if there are no pending frames, to restrict data use.
But not relevant to add a comment about that here.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:201: return input_handler_weak_ptr_;
On 2016/09/26 21:36:53, danakj wrote:
> This is kinda awk, maybe follow up to make this return a non-reference so you
> can just return nullptr?

Sounds good. I'll clean up in the next patch.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:210: debug_state_ = debug_state;
On 2016/09/26 21:36:53, danakj wrote:
> How does this get to the client? Should it not SetNeedsCommit or something?
Why
> does this not do the Unite thing that LTHInProc does?

If there is a need to see debug info, then that call should need to be made on
the LayerTreeHostInProcess directly on the client. I did not want to add this to
the protocol since this is used for debugging only.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:221: NOTIMPLEMENTED();
On 2016/09/26 21:36:53, danakj wrote:
> Or NOTREACHED?

I don't think we want to say that calling this method in this mode is an error.
It's something that I'll need to look into to understand how this should work in
blimp mode so I don't want to make things crash on this in the meanwhile.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:228: NOTIMPLEMENTED();
On 2016/09/26 21:36:53, danakj wrote:
> Or NOTREACHED?

Same.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:233: return &surface_sequence_generator_;
On 2016/09/26 21:36:53, danakj wrote:
> This is for surfacelayer also like below. So why supported? Are you trying to
> avoid branching in blink or something? But if you tried to use a surface it
> wouldn't work if it's not supported..

Yeah, it won't work. I still wasn't sure if I should be making things crash in
that case. Parts of the page wouldn't render correctly but we don't need to
crash.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:242: void
LayerTreeHostRemote::ResetGpuRasterizationTracking() {}
On 2016/09/26 21:36:52, danakj wrote:
> TODO? Is there a bug to link to?

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:280: // If a main frame request is already
pending with the CompositorFrameSink,
On 2016/09/26 21:36:53, danakj wrote:
> I don't think you mean CompositorFrameSink

Done.

So I was hoping that this name wouldn't cause confusion with the
CompositorFrameSink but I might be wrong. :P Open to suggestions for a different
name.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:299: DCHECK_EQ(FramePipelineStage::NONE,
current_pipeline_stage_);
On 2016/09/26 21:36:52, danakj wrote:
> can you write these in the same order you'd write a == b, with the variable
> first and the constant second. Don't try match EXPECT_EQ for these.

Done. I was actually looking for comments above the macro since I wasn't sure
what the preferred style is here.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:325: // Pull updates for all layers from the
client.
On 2016/09/26 21:36:53, danakj wrote:
> Do you really want all layers? Is this something to be improved?

I'm glad you asked. So in the regular case we do have some optimizations where
we try not to pull updates for some layers (I stumbled onto crbug.com/591561
when trying to understand this). The layer identification is in
draw_property_utils::FindLayersThatNeedUpdates and would require building
PropertyTrees here. While that's okay, we still wouldn't need to send them to
the client, I wasn't sure if the data saving is significant enough that we
should worry about it. Do you have a suggestion about this?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:336: max_pipeline_stage_for_current_frame_ =
On 2016/09/26 21:36:52, danakj wrote:
> This is a weird way to write
> 
> if (layers_updated)
>   max_pipe...  = ..COMMIT;

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:363: // We can not wait for updates
dispatched from the client about the state of
On 2016/09/26 21:36:53, danakj wrote:
> Why can't you wait?

Because it used to end up throttling updates from the browser. This happened
with resize updates I think, so we had a case where the browser would not send a
resize update to the renderer till it knew that a frame from the previous update
had been swapped, and since commits were client driven it would in-effect end up
delaying the engine asking the client for a commit itself.

May be that would not be necessary now since we will always push commits from
the engine to the client, we can dispatch these callbacks when we get a commit
ack from the client. For now it would be better to keep it consistent with the
existing implementation though.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote_unittest.cc (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:44:
UpdateTrackingCompositorProtoStateSink() : num_updates_received_(0) {}
On 2016/09/26 21:36:54, danakj wrote:
>  = default

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:55: int num_updates_received_;
On 2016/09/26 21:36:53, danakj wrote:
>  = 0

So this always confuses me, what is the preferred style here? I've seen inlined
and in the ctor in different places.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:77:
MOCK_METHOD0(DidUpdateLayerTreeHost, void());
On 2016/09/26 21:36:53, danakj wrote:
> Group together

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:89: static scoped_refptr<MockLayer>
Create(bool update) {
On 2016/09/26 21:36:54, danakj wrote:
> Just make the constructor public pls, delete this

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:101: explicit MockLayer(bool update)
: update_(update), did_update_(false) {}
On 2016/09/26 21:36:53, danakj wrote:
> drop did_update from here

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:105: bool did_update_;
On 2016/09/26 21:36:53, danakj wrote:
>  = false

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:115: // We don't want tree sync
requests to trigger commits.
On 2016/09/26 21:36:54, danakj wrote:
> Why not?

Because we always set the root layer at the beginning of the test, which messes
with the tests that are testing which stage of the pipeline we go to.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:132: :
compositor_proto_state_sink_(nullptr),
On 2016/09/26 21:36:53, danakj wrote:
> init these where they defined

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:159: layer_tree_host_.reset();
On 2016/09/26 21:36:54, danakj wrote:
>  = nullptr?

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:187:
UpdateTrackingCompositorProtoStateSink* compositor_proto_state_sink_;
On 2016/09/26 21:36:53, danakj wrote:
>  = nullptr

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:190: bool
needs_animate_during_main_frame_;
On 2016/09/26 21:36:53, danakj wrote:
>  = false

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:191: bool
needs_commit_during_main_frame_;
On 2016/09/26 21:36:53, danakj wrote:
>  = false

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:298:
EXPECT_BEGIN_MAIN_FRAME(mock_layer_tree_host_client_, 2);
On 2016/09/26 21:36:54, danakj wrote:
> maybe use a temp var to point out what this 2 is and give it a name (same for
> the 1s elsewhere?)

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:321: // A layer update during a main
frame should result in a commit.
On 2016/09/26 21:36:54, danakj wrote:
> The layer add isn't during the main frame here, is it?

Nope. The main frame wouldn't start until the runloop.

https://codereview.chromium.org/2362073002/diff/60001/cc/test/fake_compositor...
File cc/test/fake_compositor_proto_state_sink.h (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/test/fake_compositor...
cc/test/fake_compositor_proto_state_sink.h:27:
std::unique_ptr<CompositorProtoState> compositor_proto_state) override{};
On 2016/09/26 21:36:54, danakj wrote:
> git cl format? no ;

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:16: return
s_layer_tree_host_sequence_number.GetNext() + 1;
On 2016/09/26 21:36:54, danakj wrote:
> It's not clear to me why this is here and not just in LTHRemote, like does
> LTHRemote and LTHInProc need to share the same namespace for these ids?

The API said that it will give you a process global id for the LayerTreeHost, so
I figured I'll respect that and make sure that LayerTreeHost ids never collide.
I don't think we'll ever have a case where you create both (in-process and
remote) in the same process, other than tests, so happy to keep it in LTHRemote
if that's better.

Khushal

Now that I think about it, we should add a NotSupported to the Layer type ...

4 years, 2 months ago (2016-09-27 03:46:45 UTC) #10

David Trainor- moved to gerrit

Sorry for the delays Khushal. I'm finally caught up (except for this one!). Will take ...

4 years, 2 months ago (2016-09-27 04:12:39 UTC) #11

danakj

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_proto_state_sink_client.h File cc/blimp/compositor_proto_state_sink_client.h (right): https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_proto_state_sink_client.h#newcode29 cc/blimp/compositor_proto_state_sink_client.h:29: virtual base::SingleThreadTaskRunner* GetMainTaskRunner() = 0; On 2016/09/27 01:07:48, Khushal ...

4 years, 2 months ago (2016-09-27 20:32:43 UTC) #12

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_pro...
File cc/blimp/compositor_proto_state_sink_client.h (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_pro...
cc/blimp/compositor_proto_state_sink_client.h:29: virtual
base::SingleThreadTaskRunner* GetMainTaskRunner() = 0;
On 2016/09/27 01:07:48, Khushal wrote:
> On 2016/09/26 21:36:52, danakj wrote:
> > Why do you need this?
> 
> My understanding was that the task runner that is given to the compositor for
> the main thread is created using the renderer scheduler. I was initially using
> ThreadTaskRunnerHandler::Get but that would be for the render thread's message
> loop right?

If you passed in the task runner you already have it so why do you need to query
it from cc again? Can we just use the thing we passed to cc?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:83: LOG(ERROR) << "UIResourceManager
requested.";
On 2016/09/27 01:07:48, Khushal wrote:
> On 2016/09/26 21:36:52, danakj wrote:
> > How about notreached and return null then?
> 
> This is used by Painted scrollbars only, which blimp never uses. But I'm not
> sure if there is a layer type added that ends up relying on this, then if it
> should result in a crash.

My response would be "let's find out then" but I in fact know its only used by
painted scrollbars and (subclasses of) UIResourceLayer

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:102: // Compositor-worker not supported.
On 2016/09/27 01:07:49, Khushal wrote:
> On 2016/09/26 21:36:52, danakj wrote:
> > Can we NOTREACHED?
> 
> I think this is all setup code which will still run, even if the page doesn't
> use compositor worker.

Perhaps we should be turning the setup code off if we're not going to be using
it?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:135: // Since we never have a
CompositorFrameSink, this is always a no-op.
On 2016/09/27 01:07:49, Khushal wrote:
> On 2016/09/26 21:36:52, danakj wrote:
> > Can we NOTREACHED?
> 
> Well, the API says can be called safely anytime, so I did not want to fail
here.
> 
> > 
> > ps anything that is NOTREACHED in here maybe should not be part of the LTH
> > interface, and should be only part of the inprocess thing.
> 
> The renderer still has to deal with a common interface. Its similar to how we
> have the Composite method which is effectively a NOTREACHED in
> LayerTreeHostInProcess for threaded mode.
> 
> The other option to keep it only on the InProcess thing would be to static
cast
> in RenderWidgetCompositor and have it be aware of InProcess vs remote use I
> guess. Is that better?

I thought this is only used by browser compositors (much like Composite() and
other single-thread things). I don't see a call from RenderWidgetCompositor.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:173: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/27 01:07:49, Khushal wrote:
> On 2016/09/26 21:36:53, danakj wrote:
> > I don't know about this. This causes the compositor to produce a frame.
> > RenderWidget pokes compositor to do this at certain times. I feel like they
> may
> > be redundant but it has nothing to do with in/out of process.
> 
> Its redundant in the blimp case. The RenderWidget would poke the compositor
> about it when, for instance, the browser drops the delegated frame and it
needs
> to ask the compositor to produce another one. All the delegated frame
management
> and such calls will be relevant on the client only.
> 
> I'm not sure what we should be doing differently here?

Ok actually it looks like browser compositors (ui::Compositor) only calls this.
So NOTREACHED so we find out if something else calls it one day?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:178: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/26 21:36:52, danakj wrote:
> Same.

However RenderWidget calls this when it wants a resize ack for instance:
https://cs.chromium.org/chromium/src/content/renderer/devtools/render_widget_...

Idk the details of that but I'm worried about just ignoring it. It doesn't seem
related to the browser dropping frames.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:183: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/27 01:07:48, Khushal wrote:
> On 2016/09/26 21:36:53, danakj wrote:
> > This is for testing, to ensure that it completes a draw so that we can look
at
> > what is drawn. This is unrelated to in/out of process.
> 
> Still no need to send anything to the client though?

I guess it depends if you're running a pixel test. Do you plan to ever?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:188: // We don't throttle commits right now
so this is not relevant.
On 2016/09/27 01:07:50, Khushal wrote:
> On 2016/09/26 21:36:52, danakj wrote:
> > NOTREACHED?
> 
> NOTIMPLEMENTED is better I think. The engine will wait for an Ack from the
> client before sending another frame (which does throttle commits in a way) so
> it's possible to get into this state.
> 
> What I was commenting on was that there is no throttling at the moment where
we
> don't send updates, even if there are no pending frames, to restrict data use.
> But not relevant to add a comment about that here.

OK I'm not actually sure how this will interact with the engine.. it comes from
https://cs.chromium.org/chromium/src/content/renderer/input/render_widget_inp...
when input took too long to handle?

The idea is that we try to finish a commit faster since blink is not going to
handle input until then. Why do you not want the client to unblock commit and
get a new main frame going in this case? I'm missing what this has to do with
not sending compositor updates.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:210: debug_state_ = debug_state;
On 2016/09/27 01:07:49, Khushal wrote:
> On 2016/09/26 21:36:53, danakj wrote:
> > How does this get to the client? Should it not SetNeedsCommit or something?
> Why
> > does this not do the Unite thing that LTHInProc does?
> 
> If there is a need to see debug info, then that call should need to be made on
> the LayerTreeHostInProcess directly on the client. I did not want to add this
to
> the protocol since this is used for debugging only.

Ok, why store it at all then?

How does inspector work with blimp? Would it be running on the engine? It would
right? Cuz it would be setting these on the engine then.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:221: NOTIMPLEMENTED();
On 2016/09/27 01:07:49, Khushal wrote:
> On 2016/09/26 21:36:53, danakj wrote:
> > Or NOTREACHED?
> 
> I don't think we want to say that calling this method in this mode is an
error.
> It's something that I'll need to look into to understand how this should work
in
> blimp mode so I don't want to make things crash on this in the meanwhile.

My thot is that if it doesn't work to use NOTREACHED so you find out when things
(bots or similar) are suddenly trying to use it because things can be surprising
otherwise. If you expect code to get here then it doesn't make sense of course.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:233: return &surface_sequence_generator_;
On 2016/09/27 01:07:49, Khushal wrote:
> On 2016/09/26 21:36:53, danakj wrote:
> > This is for surfacelayer also like below. So why supported? Are you trying
to
> > avoid branching in blink or something? But if you tried to use a surface it
> > wouldn't work if it's not supported..
> 
> Yeah, it won't work. I still wasn't sure if I should be making things crash in
> that case. Parts of the page wouldn't render correctly but we don't need to
> crash.

We shouldn't crash, the code that calls this should not be used. Like we should
early out of dead code as early as possible not defer it to the very end of the
pipe and do all the work then drop it. So I think it should be an error to get
here and it should crash, but no code should get here if it's not going to work.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:325: // Pull updates for all layers from the
client.
On 2016/09/27 01:07:50, Khushal wrote:
> On 2016/09/26 21:36:53, danakj wrote:
> > Do you really want all layers? Is this something to be improved?
> 
> I'm glad you asked. So in the regular case we do have some optimizations where
> we try not to pull updates for some layers (I stumbled onto crbug.com/591561
> when trying to understand this). The layer identification is in
> draw_property_utils::FindLayersThatNeedUpdates and would require building
> PropertyTrees here. While that's okay, we still wouldn't need to send them to
> the client, I wasn't sure if the data saving is significant enough that we
> should worry about it. Do you have a suggestion about this?

I think not optimizing this will make the initial commit heavier for large
pages, instead of deferring it. But you're using the viewport to defer sending
pictures and layer metadata isn't that big so maybe it's fine. If it's something
you want to look into more just leave a TODO and point it at a bug? If not then
this is fine. IMO it might be worth looking at WRT data savings but it's not a
low hanging fruit like saving data when sending recordings.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:363: // We can not wait for updates
dispatched from the client about the state of
On 2016/09/27 01:07:49, Khushal wrote:
> On 2016/09/26 21:36:53, danakj wrote:
> > Why can't you wait?
> 
> Because it used to end up throttling updates from the browser. This happened
> with resize updates I think, so we had a case where the browser would not send
a
> resize update to the renderer till it knew that a frame from the previous
update
> had been swapped, and since commits were client driven it would in-effect end
up
> delaying the engine asking the client for a commit itself.
> 
> May be that would not be necessary now since we will always push commits from
> the engine to the client, we can dispatch these callbacks when we get a commit
> ack from the client. For now it would be better to keep it consistent with the
> existing implementation though.

OK Can you TODO pointing at a bug with this information between the 2 of them.

Khushal

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_proto_state_sink_client.h File cc/blimp/compositor_proto_state_sink_client.h (right): https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_proto_state_sink_client.h#newcode29 cc/blimp/compositor_proto_state_sink_client.h:29: virtual base::SingleThreadTaskRunner* GetMainTaskRunner() = 0; On 2016/09/27 20:32:43, danakj ...

4 years, 2 months ago (2016-09-27 23:46:56 UTC) #13

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_pro...
File cc/blimp/compositor_proto_state_sink_client.h (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_pro...
cc/blimp/compositor_proto_state_sink_client.h:29: virtual
base::SingleThreadTaskRunner* GetMainTaskRunner() = 0;
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/27 01:07:48, Khushal wrote:
> > On 2016/09/26 21:36:52, danakj wrote:
> > > Why do you need this?
> > 
> > My understanding was that the task runner that is given to the compositor
for
> > the main thread is created using the renderer scheduler. I was initially
using
> > ThreadTaskRunnerHandler::Get but that would be for the render thread's
message
> > loop right?
> 
> If you passed in the task runner you already have it so why do you need to
query
> it from cc again? Can we just use the thing we passed to cc?

Oh you mean like pass it to the CompositorProtoStateSink when its built? That
makes sense, we are going to go out to the ContentRendererClient from
RenderThreadImpl to make it so we can pass the task runner there. I'll add a
ctor for the CompositorProtoStateSink that takes a task runner instead. Does
that sound correct?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:83: LOG(ERROR) << "UIResourceManager
requested.";
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/27 01:07:48, Khushal wrote:
> > On 2016/09/26 21:36:52, danakj wrote:
> > > How about notreached and return null then?
> > 
> > This is used by Painted scrollbars only, which blimp never uses. But I'm not
> > sure if there is a layer type added that ends up relying on this, then if it
> > should result in a crash.
> 
> My response would be "let's find out then" but I in fact know its only used by
> painted scrollbars and (subclasses of) UIResourceLayer

Okay, so we have turned turn off the use of PaintedScrollbarLayers in blink and
UIResourceLayers are never used in the renderer. So NOTREACHED is better to know
if another use case gets added.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:102: // Compositor-worker not supported.
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/27 01:07:49, Khushal wrote:
> > On 2016/09/26 21:36:52, danakj wrote:
> > > Can we NOTREACHED?
> > 
> > I think this is all setup code which will still run, even if the page
doesn't
> > use compositor worker.
> 
> Perhaps we should be turning the setup code off if we're not going to be using
> it?

Sure, we can do it in blink also. For all the features that blimp does not
support, blink is a better place to turn them off. I added a TODO to look into
that and other such features.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:135: // Since we never have a
CompositorFrameSink, this is always a no-op.
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/27 01:07:49, Khushal wrote:
> > On 2016/09/26 21:36:52, danakj wrote:
> > > Can we NOTREACHED?
> > 
> > Well, the API says can be called safely anytime, so I did not want to fail
> here.
> > 
> > > 
> > > ps anything that is NOTREACHED in here maybe should not be part of the LTH
> > > interface, and should be only part of the inprocess thing.
> > 
> > The renderer still has to deal with a common interface. Its similar to how
we
> > have the Composite method which is effectively a NOTREACHED in
> > LayerTreeHostInProcess for threaded mode.
> > 
> > The other option to keep it only on the InProcess thing would be to static
> cast
> > in RenderWidgetCompositor and have it be aware of InProcess vs remote use I
> > guess. Is that better?
> 
> I thought this is only used by browser compositors (much like Composite() and
> other single-thread things). I don't see a call from RenderWidgetCompositor.

Its not there right now but I don't think its wrong to call it from the renderer
compositor, is it? Its much like how we had the output surface lost flag check
that was removed, so the caller wouldn't have to care. If we are asserting that
only single-threaded mode should support it then may be we should remove it from
the threaded compositor also?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:173: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/27 01:07:49, Khushal wrote:
> > On 2016/09/26 21:36:53, danakj wrote:
> > > I don't know about this. This causes the compositor to produce a frame.
> > > RenderWidget pokes compositor to do this at certain times. I feel like
they
> > may
> > > be redundant but it has nothing to do with in/out of process.
> > 
> > Its redundant in the blimp case. The RenderWidget would poke the compositor
> > about it when, for instance, the browser drops the delegated frame and it
> needs
> > to ask the compositor to produce another one. All the delegated frame
> management
> > and such calls will be relevant on the client only.
> > 
> > I'm not sure what we should be doing differently here?
> 
> Ok actually it looks like browser compositors (ui::Compositor) only calls
this.
> So NOTREACHED so we find out if something else calls it one day?

I would be happier to keep consistency with the threaded compositor for these
things, until we can either have the call sites understand that they can not use
something in blimp in particular.

I can remove it from both the threaded and here if you think the renderer should
never need to use this and add a comment on LayerTreeHost about it?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:178: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/26 21:36:52, danakj wrote:
> > Same.
> 
> However RenderWidget calls this when it wants a resize ack for instance:
>
https://cs.chromium.org/chromium/src/content/renderer/devtools/render_widget_...
> 
> Idk the details of that but I'm worried about just ignoring it. It doesn't
seem
> related to the browser dropping frames.

Ah, good point. Thanks for pointing that case out. What do you think about doing
a PostTask immediately here for DidCompleteSwapBuffers?

The thing is, for the blimp case, faithfully sending this to the client and
relaying back the swap ack to the engine just doesn't make sense. Usually this
call would be made when something in the system wants to make sure that a frame
is pushed to the display to throttle things etc. In this case, if there is a
high network delay or network partition, making decisions based on compositing
progress wouldn't make sense. Perhaps sending a frame update to the client
should be seen by the system as the frame being shown to the user.

For now I can immediately ack the draw and add a TODO to see if it makes more
sense to look at the call-sites that use this?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:183: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/27 01:07:48, Khushal wrote:
> > On 2016/09/26 21:36:53, danakj wrote:
> > > This is for testing, to ensure that it completes a draw so that we can
look
> at
> > > what is drawn. This is unrelated to in/out of process.
> > 
> > Still no need to send anything to the client though?
> 
> I guess it depends if you're running a pixel test. Do you plan to ever?

For pixel tests my plan was to have an implementation of LayerTreeHostRemote
that owns a LayerTreeHostInProcess, and override these calls so they can be
directed to that instead. Pixel tests would end up using CopyRequests also,
which this class can not handle, so for those you do need the actual compositor
there.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:188: // We don't throttle commits right now
so this is not relevant.
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/27 01:07:50, Khushal wrote:
> > On 2016/09/26 21:36:52, danakj wrote:
> > > NOTREACHED?
> > 
> > NOTIMPLEMENTED is better I think. The engine will wait for an Ack from the
> > client before sending another frame (which does throttle commits in a way)
so
> > it's possible to get into this state.
> > 
> > What I was commenting on was that there is no throttling at the moment where
> we
> > don't send updates, even if there are no pending frames, to restrict data
use.
> > But not relevant to add a comment about that here.
> 
> OK I'm not actually sure how this will interact with the engine.. it comes
from
>
https://cs.chromium.org/chromium/src/content/renderer/input/render_widget_inp...
> when input took too long to handle?

So this was when I had looked at the code a while ago, correct me if I'm wrong.
What I understood was that this helps in the case where the renderer gets an
input event and needs a commit to ack the event to the browser. But the
compositor is still working on the pending tree from the previous commit so now
all input is blocked. This is used to kick the compositor to have it prioritize
the pending tree over the active tree and activate faster, so we can accept the
next commit.

> 
> The idea is that we try to finish a commit faster since blink is not going to
> handle input until then. Why do you not want the client to unblock commit and
> get a new main frame going in this case? I'm missing what this has to do with
> not sending compositor updates.

For blimp, raster and draw on the client is hardly the bottleneck, it is most
likely the transport. The input is blocked because you have a compositor update
delivery pending and you need an Ack from the client that the update was
received before you can send another one.

At the same time, input will be queued on the client instead. We can't just keep
streaming input events to the engine without receiving an Ack or a frame in
response. So if the compositor needs to be pushed to activate faster, we will do
so on the client.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:210: debug_state_ = debug_state;
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/27 01:07:49, Khushal wrote:
> > On 2016/09/26 21:36:53, danakj wrote:
> > > How does this get to the client? Should it not SetNeedsCommit or
something?
> > Why
> > > does this not do the Unite thing that LTHInProc does?
> > 
> > If there is a need to see debug info, then that call should need to be made
on
> > the LayerTreeHostInProcess directly on the client. I did not want to add
this
> to
> > the protocol since this is used for debugging only.
> 
> Ok, why store it at all then?

Because things take a const&. I looked through the settings and everything that
is there was cc internal (main thread or impl thread things), which should run
on the client only.

> 
> How does inspector work with blimp? Would it be running on the engine? It
would
> right? Cuz it would be setting these on the engine then.

That is a good question, and I'm not sure yet. One way would be to do something
similar to pixel tests, where you have a version of the LayerTreeHostRemote that
has a LayerTreeHostInProcess so you can still run the elements in the rendering
pipeline that are different (state (de)-serialization, image encoding).
But this won't work for android client. If we have to actually serialize this to
be able to support that, we can do that then. I'm a bit disinclined to add
things to the protocol messages till there is a need for it.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:221: NOTIMPLEMENTED();
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/27 01:07:49, Khushal wrote:
> > On 2016/09/26 21:36:53, danakj wrote:
> > > Or NOTREACHED?
> > 
> > I don't think we want to say that calling this method in this mode is an
> error.
> > It's something that I'll need to look into to understand how this should
work
> in
> > blimp mode so I don't want to make things crash on this in the meanwhile.
> 
> My thot is that if it doesn't work to use NOTREACHED so you find out when
things
> (bots or similar) are suddenly trying to use it because things can be
surprising
> otherwise. If you expect code to get here then it doesn't make sense of
course.

Okay, you're right. This one's used by Gpu benchmarking only, so better to
NOTREACHED instead till we support it.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:233: return &surface_sequence_generator_;
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/27 01:07:49, Khushal wrote:
> > On 2016/09/26 21:36:53, danakj wrote:
> > > This is for surfacelayer also like below. So why supported? Are you trying
> to
> > > avoid branching in blink or something? But if you tried to use a surface
it
> > > wouldn't work if it's not supported..
> > 
> > Yeah, it won't work. I still wasn't sure if I should be making things crash
in
> > that case. Parts of the page wouldn't render correctly but we don't need to
> > crash.
> 
> We shouldn't crash, the code that calls this should not be used. Like we
should
> early out of dead code as early as possible not defer it to the very end of
the
> pipe and do all the work then drop it. So I think it should be an error to get
> here and it should crash, but no code should get here if it's not going to
work.

Fair point. Added a TODO to turn this off in blink instead. In the meanwhile,
since this is a known issue that we have to get to, the preference is to render
as much of the page as we can correctly for ones that use a not supported
feature. Later on we'll add handling for these cases at an early stage in blink
instead.

+dtrainor on this one too.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:325: // Pull updates for all layers from the
client.
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/27 01:07:50, Khushal wrote:
> > On 2016/09/26 21:36:53, danakj wrote:
> > > Do you really want all layers? Is this something to be improved?
> > 
> > I'm glad you asked. So in the regular case we do have some optimizations
where
> > we try not to pull updates for some layers (I stumbled onto crbug.com/591561
> > when trying to understand this). The layer identification is in
> > draw_property_utils::FindLayersThatNeedUpdates and would require building
> > PropertyTrees here. While that's okay, we still wouldn't need to send them
to
> > the client, I wasn't sure if the data saving is significant enough that we
> > should worry about it. Do you have a suggestion about this?
> 
> I think not optimizing this will make the initial commit heavier for large
> pages, instead of deferring it. But you're using the viewport to defer sending
> pictures and layer metadata isn't that big so maybe it's fine. If it's
something
> you want to look into more just leave a TODO and point it at a bug? If not
then
> this is fine. IMO it might be worth looking at WRT data savings but it's not a
> low hanging fruit like saving data when sending recordings.

Thanks. Added a TODO.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:363: // We can not wait for updates
dispatched from the client about the state of
On 2016/09/27 20:32:43, danakj wrote:
> On 2016/09/27 01:07:49, Khushal wrote:
> > On 2016/09/26 21:36:53, danakj wrote:
> > > Why can't you wait?
> > 
> > Because it used to end up throttling updates from the browser. This happened
> > with resize updates I think, so we had a case where the browser would not
send
> a
> > resize update to the renderer till it knew that a frame from the previous
> update
> > had been swapped, and since commits were client driven it would in-effect
end
> up
> > delaying the engine asking the client for a commit itself.
> > 
> > May be that would not be necessary now since we will always push commits
from
> > the engine to the client, we can dispatch these callbacks when we get a
commit
> > ack from the client. For now it would be better to keep it consistent with
the
> > existing implementation though.
> 
> OK Can you TODO pointing at a bug with this information between the 2 of them.

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote_unittest.cc (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:60: MockLayerTreeHostClient() {}
On 2016/09/26 21:36:53, danakj wrote:
>  =default

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:61: ~MockLayerTreeHostClient()
override {}
On 2016/09/26 21:36:53, danakj wrote:
>  = default?

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:68: void
set_update_host_callback(base::Closure callback) {
On 2016/09/26 21:36:54, danakj wrote:
> Move this so it's not in the middle of the client implementation

Done.

danakj

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_host_remote.cc File cc/blimp/layer_tree_host_remote.cc (right): https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_host_remote.cc#newcode173 cc/blimp/layer_tree_host_remote.cc:173: // The engine shouldn't need to care about draws. ...

4 years, 2 months ago (2016-09-28 00:32:50 UTC) #14

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:173: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/27 23:46:56, Khushal wrote:
> On 2016/09/27 20:32:43, danakj wrote:
> > On 2016/09/27 01:07:49, Khushal wrote:
> > > On 2016/09/26 21:36:53, danakj wrote:
> > > > I don't know about this. This causes the compositor to produce a frame.
> > > > RenderWidget pokes compositor to do this at certain times. I feel like
> they
> > > may
> > > > be redundant but it has nothing to do with in/out of process.
> > > 
> > > Its redundant in the blimp case. The RenderWidget would poke the
compositor
> > > about it when, for instance, the browser drops the delegated frame and it
> > needs
> > > to ask the compositor to produce another one. All the delegated frame
> > management
> > > and such calls will be relevant on the client only.
> > > 
> > > I'm not sure what we should be doing differently here?
> > 
> > Ok actually it looks like browser compositors (ui::Compositor) only calls
> this.
> > So NOTREACHED so we find out if something else calls it one day?
> 
> I would be happier to keep consistency with the threaded compositor for these
> things, until we can either have the call sites understand that they can not
use
> something in blimp in particular.
> 
> I can remove it from both the threaded and here if you think the renderer
should
> never need to use this and add a comment on LayerTreeHost about it?

Leaving it empty feels like a landmine to me, I'd prefer NOTREACHED if it's not
currently used, so we know when it does get used is all. And we can consider
removing all the NOTREACHED from the API at some point maybe, if it makes sense
to (I'm doing similar things to OutputSurface/CompositorFrameSink now but it
took a long time till that made sense).

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:178: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/27 23:46:56, Khushal wrote:
> On 2016/09/27 20:32:43, danakj wrote:
> > On 2016/09/26 21:36:52, danakj wrote:
> > > Same.
> > 
> > However RenderWidget calls this when it wants a resize ack for instance:
> >
>
https://cs.chromium.org/chromium/src/content/renderer/devtools/render_widget_...
> > 
> > Idk the details of that but I'm worried about just ignoring it. It doesn't
> seem
> > related to the browser dropping frames.
> 
> Ah, good point. Thanks for pointing that case out. What do you think about
doing
> a PostTask immediately here for DidCompleteSwapBuffers?

It sounds sketchy in that it's likely to interfere with swap counting if you're
seeing swap acks from multiple producers.


> The thing is, for the blimp case, faithfully sending this to the client and
> relaying back the swap ack to the engine just doesn't make sense. Usually this
> call would be made when something in the system wants to make sure that a
frame
> is pushed to the display to throttle things etc. In this case, if there is a
> high network delay or network partition, making decisions based on compositing
> progress wouldn't make sense. Perhaps sending a frame update to the client
> should be seen by the system as the frame being shown to the user.
> 
> For now I can immediately ack the draw and add a TODO to see if it makes more
> sense to look at the call-sites that use this?

You probably do not want resize to be lockstep for blimp cuz doing that over the
network is insane, so you probably never want the client side to expect a resize
ack or something. A TODO sounds okay, posting DidCompleteSwapBuffers feels scary
and we should figure it out better.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:183: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/27 23:46:56, Khushal wrote:
> On 2016/09/27 20:32:43, danakj wrote:
> > On 2016/09/27 01:07:48, Khushal wrote:
> > > On 2016/09/26 21:36:53, danakj wrote:
> > > > This is for testing, to ensure that it completes a draw so that we can
> look
> > at
> > > > what is drawn. This is unrelated to in/out of process.
> > > 
> > > Still no need to send anything to the client though?
> > 
> > I guess it depends if you're running a pixel test. Do you plan to ever?
> 
> For pixel tests my plan was to have an implementation of LayerTreeHostRemote
> that owns a LayerTreeHostInProcess, and override these calls so they can be
> directed to that instead. Pixel tests would end up using CopyRequests also,
> which this class can not handle, so for those you do need the actual
compositor
> there.

I'm thinking more like gpu pixel tests:
https://www.chromium.org/developers/how-tos/gpu-wrangling#TOC-Test-Suites not
cc_unittests. They run a full chrome instance and need to capture the output for
a frame they submit, and this is used to be sure the frame they submit leads to
a draw even if it happens to not damage anything for example.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:188: // We don't throttle commits right now
so this is not relevant.
On 2016/09/27 23:46:56, Khushal wrote:
> On 2016/09/27 20:32:43, danakj wrote:
> > On 2016/09/27 01:07:50, Khushal wrote:
> > > On 2016/09/26 21:36:52, danakj wrote:
> > > > NOTREACHED?
> > > 
> > > NOTIMPLEMENTED is better I think. The engine will wait for an Ack from the
> > > client before sending another frame (which does throttle commits in a way)
> so
> > > it's possible to get into this state.
> > > 
> > > What I was commenting on was that there is no throttling at the moment
where
> > we
> > > don't send updates, even if there are no pending frames, to restrict data
> use.
> > > But not relevant to add a comment about that here.
> > 
> > OK I'm not actually sure how this will interact with the engine.. it comes
> from
> >
>
https://cs.chromium.org/chromium/src/content/renderer/input/render_widget_inp...
> > when input took too long to handle?
> 
> So this was when I had looked at the code a while ago, correct me if I'm
wrong.
> What I understood was that this helps in the case where the renderer gets an
> input event and needs a commit to ack the event to the browser. But the
> compositor is still working on the pending tree from the previous commit so
now
> all input is blocked. This is used to kick the compositor to have it
prioritize
> the pending tree over the active tree and activate faster, so we can accept
the
> next commit.
> 
> > 
> > The idea is that we try to finish a commit faster since blink is not going
to
> > handle input until then. Why do you not want the client to unblock commit
and
> > get a new main frame going in this case? I'm missing what this has to do
with
> > not sending compositor updates.
> 
> For blimp, raster and draw on the client is hardly the bottleneck, it is most
> likely the transport. The input is blocked because you have a compositor
update
> delivery pending and you need an Ack from the client that the update was
> received before you can send another one.
> 
> At the same time, input will be queued on the client instead. We can't just
keep
> streaming input events to the engine without receiving an Ack or a frame in
> response. So if the compositor needs to be pushed to activate faster, we will
do
> so on the client.

OK Cool that all sounds good, can you put this in comment form (and if you know
where maybe you can point where in the client we'll do said pushing)?

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:210: debug_state_ = debug_state;
On 2016/09/27 23:46:56, Khushal wrote:
> On 2016/09/27 20:32:43, danakj wrote:
> > On 2016/09/27 01:07:49, Khushal wrote:
> > > On 2016/09/26 21:36:53, danakj wrote:
> > > > How does this get to the client? Should it not SetNeedsCommit or
> something?
> > > Why
> > > > does this not do the Unite thing that LTHInProc does?
> > > 
> > > If there is a need to see debug info, then that call should need to be
made
> on
> > > the LayerTreeHostInProcess directly on the client. I did not want to add
> this
> > to
> > > the protocol since this is used for debugging only.
> > 
> > Ok, why store it at all then?
> 
> Because things take a const&. I looked through the settings and everything
that
> is there was cc internal (main thread or impl thread things), which should run
> on the client only.

I can see why you made a member then, but why replace it with a new value here.
It looks misleading. I'd prefer a comment saying this is a dead end and
explaining why we're dropping calls to this on the engine (or a NOTREACHED if we
think we should implement something here).


> > 
> > How does inspector work with blimp? Would it be running on the engine? It
> would
> > right? Cuz it would be setting these on the engine then.
> 
> That is a good question, and I'm not sure yet. One way would be to do
something
> similar to pixel tests, where you have a version of the LayerTreeHostRemote
that
> has a LayerTreeHostInProcess so you can still run the elements in the
rendering
> pipeline that are different (state (de)-serialization, image encoding).
> But this won't work for android client. If we have to actually serialize this
to
> be able to support that, we can do that then. I'm a bit disinclined to add
> things to the protocol messages till there is a need for it.

Im not actually sure if you can bring up inspector in android or not, maybe it
has to be on another client already, but that client probably talks to blink..
on the engine, to do things like turn on layer borders.

https://codereview.chromium.org/2362073002/diff/100001/cc/blimp/compositor_pr...
File cc/blimp/compositor_proto_state.h (right):

https://codereview.chromium.org/2362073002/diff/100001/cc/blimp/compositor_pr...
cc/blimp/compositor_proto_state.h:16: class CC_EXPORT CompositorProtoState {
Its like a CompositorMainFrame almost, or is it exactly that..
RemoteCompositorMainFrame? And RemoteCompositor could be the interface? wdyt..

Khushal

Thanks Dana. Fixed the main task runner plumbing that you mentioned earlier as well. PTAL. ...

4 years, 2 months ago (2016-09-28 19:47:16 UTC) #15

Thanks Dana. Fixed the main task runner plumbing that you mentioned earlier as
well. PTAL.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_pro...
File cc/blimp/compositor_proto_state_sink_client.h (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/compositor_pro...
cc/blimp/compositor_proto_state_sink_client.h:29: virtual
base::SingleThreadTaskRunner* GetMainTaskRunner() = 0;
On 2016/09/27 23:46:55, Khushal wrote:
> On 2016/09/27 20:32:43, danakj wrote:
> > On 2016/09/27 01:07:48, Khushal wrote:
> > > On 2016/09/26 21:36:52, danakj wrote:
> > > > Why do you need this?
> > > 
> > > My understanding was that the task runner that is given to the compositor
> for
> > > the main thread is created using the renderer scheduler. I was initially
> using
> > > ThreadTaskRunnerHandler::Get but that would be for the render thread's
> message
> > > loop right?
> > 
> > If you passed in the task runner you already have it so why do you need to
> query
> > it from cc again? Can we just use the thing we passed to cc?
> 
> Oh you mean like pass it to the CompositorProtoStateSink when its built? That
> makes sense, we are going to go out to the ContentRendererClient from
> RenderThreadImpl to make it so we can pass the task runner there. I'll add a
> ctor for the CompositorProtoStateSink that takes a task runner instead. Does
> that sound correct?

Done.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:173: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/28 00:32:50, danakj wrote:
> On 2016/09/27 23:46:56, Khushal wrote:
> > On 2016/09/27 20:32:43, danakj wrote:
> > > On 2016/09/27 01:07:49, Khushal wrote:
> > > > On 2016/09/26 21:36:53, danakj wrote:
> > > > > I don't know about this. This causes the compositor to produce a
frame.
> > > > > RenderWidget pokes compositor to do this at certain times. I feel like
> > they
> > > > may
> > > > > be redundant but it has nothing to do with in/out of process.
> > > > 
> > > > Its redundant in the blimp case. The RenderWidget would poke the
> compositor
> > > > about it when, for instance, the browser drops the delegated frame and
it
> > > needs
> > > > to ask the compositor to produce another one. All the delegated frame
> > > management
> > > > and such calls will be relevant on the client only.
> > > > 
> > > > I'm not sure what we should be doing differently here?
> > > 
> > > Ok actually it looks like browser compositors (ui::Compositor) only calls
> > this.
> > > So NOTREACHED so we find out if something else calls it one day?
> > 
> > I would be happier to keep consistency with the threaded compositor for
these
> > things, until we can either have the call sites understand that they can not
> use
> > something in blimp in particular.
> > 
> > I can remove it from both the threaded and here if you think the renderer
> should
> > never need to use this and add a comment on LayerTreeHost about it?
> 
> Leaving it empty feels like a landmine to me, I'd prefer NOTREACHED if it's
not
> currently used, so we know when it does get used is all. And we can consider
> removing all the NOTREACHED from the API at some point maybe, if it makes
sense
> to (I'm doing similar things to OutputSurface/CompositorFrameSink now but it
> took a long time till that made sense).

Done. I'll remove it from the threaded one as well in a follow up patch.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:178: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/28 00:32:50, danakj wrote:
> On 2016/09/27 23:46:56, Khushal wrote:
> > On 2016/09/27 20:32:43, danakj wrote:
> > > On 2016/09/26 21:36:52, danakj wrote:
> > > > Same.
> > > 
> > > However RenderWidget calls this when it wants a resize ack for instance:
> > >
> >
>
https://cs.chromium.org/chromium/src/content/renderer/devtools/render_widget_...
> > > 
> > > Idk the details of that but I'm worried about just ignoring it. It doesn't
> > seem
> > > related to the browser dropping frames.
> > 
> > Ah, good point. Thanks for pointing that case out. What do you think about
> doing
> > a PostTask immediately here for DidCompleteSwapBuffers?
> 
> It sounds sketchy in that it's likely to interfere with swap counting if
you're
> seeing swap acks from multiple producers.
> 
> 
> > The thing is, for the blimp case, faithfully sending this to the client and
> > relaying back the swap ack to the engine just doesn't make sense. Usually
this
> > call would be made when something in the system wants to make sure that a
> frame
> > is pushed to the display to throttle things etc. In this case, if there is a
> > high network delay or network partition, making decisions based on
compositing
> > progress wouldn't make sense. Perhaps sending a frame update to the client
> > should be seen by the system as the frame being shown to the user.
> > 
> > For now I can immediately ack the draw and add a TODO to see if it makes
more
> > sense to look at the call-sites that use this?
> 
> You probably do not want resize to be lockstep for blimp cuz doing that over
the
> network is insane, so you probably never want the client side to expect a
resize
> ack or something. A TODO sounds okay, posting DidCompleteSwapBuffers feels
scary
> and we should figure it out better.

In this case rather than the client, its elements in the renderer expecting an
ack. I added a TODO about this.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:183: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/28 00:32:50, danakj wrote:
> On 2016/09/27 23:46:56, Khushal wrote:
> > On 2016/09/27 20:32:43, danakj wrote:
> > > On 2016/09/27 01:07:48, Khushal wrote:
> > > > On 2016/09/26 21:36:53, danakj wrote:
> > > > > This is for testing, to ensure that it completes a draw so that we can
> > look
> > > at
> > > > > what is drawn. This is unrelated to in/out of process.
> > > > 
> > > > Still no need to send anything to the client though?
> > > 
> > > I guess it depends if you're running a pixel test. Do you plan to ever?
> > 
> > For pixel tests my plan was to have an implementation of LayerTreeHostRemote
> > that owns a LayerTreeHostInProcess, and override these calls so they can be
> > directed to that instead. Pixel tests would end up using CopyRequests also,
> > which this class can not handle, so for those you do need the actual
> compositor
> > there.
> 
> I'm thinking more like gpu pixel tests:
> https://www.chromium.org/developers/how-tos/gpu-wrangling#TOC-Test-Suites not
> cc_unittests. They run a full chrome instance and need to capture the output
for
> a frame they submit, and this is used to be sure the frame they submit leads
to
> a draw even if it happens to not damage anything for example.

Ah, okay. Can you point me to where this is done? Is it here,
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
The use cases I saw for this are we force a repaint if the renderer is re-shown
and the other one was to grab screenshots (I'm assuming you meant this one),
which is a fun problem in itself because you can't really get a screenshot from
there because the browser process on the engine wouldn't have a CompositorFrame,
they will be produced on the client only.

I'm not sure how gpu pixel tests are supposed to run for blimp, because when we
say a full Chrome instance, what does that mean in this mode? If we run Chrome
in blimp mode, there will be no content layer, so the code which is relying on
grabbing screenshots there has to go through something in blimp, and then will
likely be making this call on the LayerTreeHostInProcess there.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:188: // We don't throttle commits right now
so this is not relevant.
On 2016/09/28 00:32:50, danakj wrote:
> On 2016/09/27 23:46:56, Khushal wrote:
> > On 2016/09/27 20:32:43, danakj wrote:
> > > On 2016/09/27 01:07:50, Khushal wrote:
> > > > On 2016/09/26 21:36:52, danakj wrote:
> > > > > NOTREACHED?
> > > > 
> > > > NOTIMPLEMENTED is better I think. The engine will wait for an Ack from
the
> > > > client before sending another frame (which does throttle commits in a
way)
> > so
> > > > it's possible to get into this state.
> > > > 
> > > > What I was commenting on was that there is no throttling at the moment
> where
> > > we
> > > > don't send updates, even if there are no pending frames, to restrict
data
> > use.
> > > > But not relevant to add a comment about that here.
> > > 
> > > OK I'm not actually sure how this will interact with the engine.. it comes
> > from
> > >
> >
>
https://cs.chromium.org/chromium/src/content/renderer/input/render_widget_inp...
> > > when input took too long to handle?
> > 
> > So this was when I had looked at the code a while ago, correct me if I'm
> wrong.
> > What I understood was that this helps in the case where the renderer gets an
> > input event and needs a commit to ack the event to the browser. But the
> > compositor is still working on the pending tree from the previous commit so
> now
> > all input is blocked. This is used to kick the compositor to have it
> prioritize
> > the pending tree over the active tree and activate faster, so we can accept
> the
> > next commit.
> > 
> > > 
> > > The idea is that we try to finish a commit faster since blink is not going
> to
> > > handle input until then. Why do you not want the client to unblock commit
> and
> > > get a new main frame going in this case? I'm missing what this has to do
> with
> > > not sending compositor updates.
> > 
> > For blimp, raster and draw on the client is hardly the bottleneck, it is
most
> > likely the transport. The input is blocked because you have a compositor
> update
> > delivery pending and you need an Ack from the client that the update was
> > received before you can send another one.
> > 
> > At the same time, input will be queued on the client instead. We can't just
> keep
> > streaming input events to the engine without receiving an Ack or a frame in
> > response. So if the compositor needs to be pushed to activate faster, we
will
> do
> > so on the client.
> 
> OK Cool that all sounds good, can you put this in comment form (and if you
know
> where maybe you can point where in the client we'll do said pushing)?

Done. Didn't add where on the client because that's TBD. I'll update here once
that's fixed.

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:210: debug_state_ = debug_state;
On 2016/09/28 00:32:50, danakj wrote:
> On 2016/09/27 23:46:56, Khushal wrote:
> > On 2016/09/27 20:32:43, danakj wrote:
> > > On 2016/09/27 01:07:49, Khushal wrote:
> > > > On 2016/09/26 21:36:53, danakj wrote:
> > > > > How does this get to the client? Should it not SetNeedsCommit or
> > something?
> > > > Why
> > > > > does this not do the Unite thing that LTHInProc does?
> > > > 
> > > > If there is a need to see debug info, then that call should need to be
> made
> > on
> > > > the LayerTreeHostInProcess directly on the client. I did not want to add
> > this
> > > to
> > > > the protocol since this is used for debugging only.
> > > 
> > > Ok, why store it at all then?
> > 
> > Because things take a const&. I looked through the settings and everything
> that
> > is there was cc internal (main thread or impl thread things), which should
run
> > on the client only.
> 
> I can see why you made a member then, but why replace it with a new value
here.
> It looks misleading. I'd prefer a comment saying this is a dead end and
> explaining why we're dropping calls to this on the engine (or a NOTREACHED if
we
> think we should implement something here).

Good point. I made it a NOTREACHED for now. Will come back to adding it here if
it turns out that it's necessary.

> 
> 
> > > 
> > > How does inspector work with blimp? Would it be running on the engine? It
> > would
> > > right? Cuz it would be setting these on the engine then.
> > 
> > That is a good question, and I'm not sure yet. One way would be to do
> something
> > similar to pixel tests, where you have a version of the LayerTreeHostRemote
> that
> > has a LayerTreeHostInProcess so you can still run the elements in the
> rendering
> > pipeline that are different (state (de)-serialization, image encoding).
> > But this won't work for android client. If we have to actually serialize
this
> to
> > be able to support that, we can do that then. I'm a bit disinclined to add
> > things to the protocol messages till there is a need for it.
> 
> Im not actually sure if you can bring up inspector in android or not, maybe it
> has to be on another client already, but that client probably talks to blink..
> on the engine, to do things like turn on layer borders.

https://codereview.chromium.org/2362073002/diff/100001/cc/blimp/compositor_pr...
File cc/blimp/compositor_proto_state.h (right):

https://codereview.chromium.org/2362073002/diff/100001/cc/blimp/compositor_pr...
cc/blimp/compositor_proto_state.h:16: class CC_EXPORT CompositorProtoState {
On 2016/09/28 00:32:50, danakj wrote:
> Its like a CompositorMainFrame almost, or is it exactly that..
> RemoteCompositorMainFrame? And RemoteCompositor could be the interface? wdyt..

If the main frame includes PropertyTrees and other computed things too, then
not.
StateSink is fine I guess, in some sense there is a parallel here that the
CompositorFrameSink is supposed to transport CompositorFrames to their
destination, and CompositorProtoStateSink is transporting the paint output fed
to the compositor.

danakj

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_host_remote.cc File cc/blimp/layer_tree_host_remote.cc (right): https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_host_remote.cc#newcode183 cc/blimp/layer_tree_host_remote.cc:183: // The engine shouldn't need to care about draws. ...

4 years, 2 months ago (2016-09-29 23:59:35 UTC) #16

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:183: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/28 19:47:16, Khushal wrote:
> On 2016/09/28 00:32:50, danakj wrote:
> > On 2016/09/27 23:46:56, Khushal wrote:
> > > On 2016/09/27 20:32:43, danakj wrote:
> > > > On 2016/09/27 01:07:48, Khushal wrote:
> > > > > On 2016/09/26 21:36:53, danakj wrote:
> > > > > > This is for testing, to ensure that it completes a draw so that we
can
> > > look
> > > > at
> > > > > > what is drawn. This is unrelated to in/out of process.
> > > > > 
> > > > > Still no need to send anything to the client though?
> > > > 
> > > > I guess it depends if you're running a pixel test. Do you plan to ever?
> > > 
> > > For pixel tests my plan was to have an implementation of
LayerTreeHostRemote
> > > that owns a LayerTreeHostInProcess, and override these calls so they can
be
> > > directed to that instead. Pixel tests would end up using CopyRequests
also,
> > > which this class can not handle, so for those you do need the actual
> > compositor
> > > there.
> > 
> > I'm thinking more like gpu pixel tests:
> > https://www.chromium.org/developers/how-tos/gpu-wrangling#TOC-Test-Suites
not
> > cc_unittests. They run a full chrome instance and need to capture the output
> for
> > a frame they submit, and this is used to be sure the frame they submit leads
> to
> > a draw even if it happens to not damage anything for example.
> 
> Ah, okay. Can you point me to where this is done? Is it here,
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
> The use cases I saw for this are we force a repaint if the renderer is
re-shown
> and the other one was to grab screenshots (I'm assuming you meant this one),
> which is a fun problem in itself because you can't really get a screenshot
from
> there because the browser process on the engine wouldn't have a
CompositorFrame,
> they will be produced on the client only.

Yes it's the screenshots thing. It's also interesting for not-blimp cuz the
renderer doesn't have a screenshot either, it has to come from out of process.
For you it will include network trips (if you want to be able to run these).

> I'm not sure how gpu pixel tests are supposed to run for blimp, because when
we
> say a full Chrome instance, what does that mean in this mode? If we run Chrome
> in blimp mode, there will be no content layer, so the code which is relying on
> grabbing screenshots there has to go through something in blimp, and then will
> likely be making this call on the LayerTreeHostInProcess there.

It would be an engine and a client with some glue in between mocking a network
I'd imagine. I'm not sure what kind of end-to-end testing you want or need
really. Blink has pretty good end-to-end testing already, what you need to worry
about is any logic that is changed for blimp specifically. Ie code in LTHRemote
and its interactions with the blimp client would be all the changes (or anything
you change elsewhere but then you can test that without blimp which might be
easier).

I'm fine with saying you don't need to support this but then I'd say NOTREACHED
and explain in comment why (not just that we don't do it but what path it
supports and that we don't support that path).

https://codereview.chromium.org/2362073002/diff/100001/cc/blimp/compositor_pr...
File cc/blimp/compositor_proto_state.h (right):

https://codereview.chromium.org/2362073002/diff/100001/cc/blimp/compositor_pr...
cc/blimp/compositor_proto_state.h:16: class CC_EXPORT CompositorProtoState {
On 2016/09/28 19:47:16, Khushal wrote:
> On 2016/09/28 00:32:50, danakj wrote:
> > Its like a CompositorMainFrame almost, or is it exactly that..
> > RemoteCompositorMainFrame? And RemoteCompositor could be the interface?
wdyt..
> 
> If the main frame includes PropertyTrees and other computed things too, then
> not.

The main frame is the entire state of the main thread, all the layers, the
viewport, the dsf, the etc... the commit is applying the main frame to the other
side.

> StateSink is fine I guess, in some sense there is a parallel here that the
> CompositorFrameSink is supposed to transport CompositorFrames to their
> destination, and CompositorProtoStateSink is transporting the paint output fed
> to the compositor.

Yeah, I just wouldn't get too into the name copying, cuz if everything looks the
same its hard to tell them apart. I don't want every interface that takes a
struct to be called a sink. CompositorFrameSink is okay but TBH doesn't describe
what it is very well, I just don't know anything better. I won't block you on
naming this similarly but I kinda think it could be more descriptive about what
it is beyond the data type you pass to a function.

Khushal

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_host_remote.cc File cc/blimp/layer_tree_host_remote.cc (right): https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_host_remote.cc#newcode183 cc/blimp/layer_tree_host_remote.cc:183: // The engine shouldn't need to care about draws. ...

4 years, 2 months ago (2016-09-30 02:11:18 UTC) #17

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote.cc:183: // The engine shouldn't need to care
about draws. CompositorFrames are never
On 2016/09/29 23:59:35, danakj wrote:
> On 2016/09/28 19:47:16, Khushal wrote:
> > On 2016/09/28 00:32:50, danakj wrote:
> > > On 2016/09/27 23:46:56, Khushal wrote:
> > > > On 2016/09/27 20:32:43, danakj wrote:
> > > > > On 2016/09/27 01:07:48, Khushal wrote:
> > > > > > On 2016/09/26 21:36:53, danakj wrote:
> > > > > > > This is for testing, to ensure that it completes a draw so that we
> can
> > > > look
> > > > > at
> > > > > > > what is drawn. This is unrelated to in/out of process.
> > > > > > 
> > > > > > Still no need to send anything to the client though?
> > > > > 
> > > > > I guess it depends if you're running a pixel test. Do you plan to
ever?
> > > > 
> > > > For pixel tests my plan was to have an implementation of
> LayerTreeHostRemote
> > > > that owns a LayerTreeHostInProcess, and override these calls so they can
> be
> > > > directed to that instead. Pixel tests would end up using CopyRequests
> also,
> > > > which this class can not handle, so for those you do need the actual
> > > compositor
> > > > there.
> > > 
> > > I'm thinking more like gpu pixel tests:
> > > https://www.chromium.org/developers/how-tos/gpu-wrangling#TOC-Test-Suites
> not
> > > cc_unittests. They run a full chrome instance and need to capture the
output
> > for
> > > a frame they submit, and this is used to be sure the frame they submit
leads
> > to
> > > a draw even if it happens to not damage anything for example.
> > 
> > Ah, okay. Can you point me to where this is done? Is it here,
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
> > The use cases I saw for this are we force a repaint if the renderer is
> re-shown
> > and the other one was to grab screenshots (I'm assuming you meant this one),
> > which is a fun problem in itself because you can't really get a screenshot
> from
> > there because the browser process on the engine wouldn't have a
> CompositorFrame,
> > they will be produced on the client only.
> 
> Yes it's the screenshots thing. It's also interesting for not-blimp cuz the
> renderer doesn't have a screenshot either, it has to come from out of process.
> For you it will include network trips (if you want to be able to run these).

There is a super-basic complete end-to-end integration test we have right now
which uses a local host and takes a screenshot on the client to verify the
output
(https://cs.chromium.org/chromium/src/blimp/engine/browser_tests/integration_t...).
I am not as familiar with the framework used for the gpu tests, but I would want
to understand why would we need to have the grabbing of screenshots go through
the engine at all. The client has everything to generate the screenshot right?

> 
> > I'm not sure how gpu pixel tests are supposed to run for blimp, because when
> we
> > say a full Chrome instance, what does that mean in this mode? If we run
Chrome
> > in blimp mode, there will be no content layer, so the code which is relying
on
> > grabbing screenshots there has to go through something in blimp, and then
will
> > likely be making this call on the LayerTreeHostInProcess there.
> 
> It would be an engine and a client with some glue in between mocking a network
> I'd imagine. I'm not sure what kind of end-to-end testing you want or need
> really. Blink has pretty good end-to-end testing already, what you need to
worry
> about is any logic that is changed for blimp specifically. Ie code in
LTHRemote
> and its interactions with the blimp client would be all the changes (or
anything
> you change elsewhere but then you can test that without blimp which might be
> easier).

> 
> I'm fine with saying you don't need to support this but then I'd say
NOTREACHED
> and explain in comment why (not just that we don't do it but what path it
> supports and that we don't support that path).

I would prefer to NOTREACHED too, mostly because the other use-case of this API
is the browser asking the renderer to repaint after visibility changes, and we
will have to re-structure the Chrome code on the client to send these requests
to the compositor on the client. The engine wouldn't even know when the tabs
become hidden or be involved in any of this frame management.

I have NOTREACHED for now, but I'm quite sure that there will be cases where
these calls will still happen on the engine and I'll probably be coming back
here when that happens.

https://codereview.chromium.org/2362073002/diff/100001/cc/blimp/compositor_pr...
File cc/blimp/compositor_proto_state.h (right):

https://codereview.chromium.org/2362073002/diff/100001/cc/blimp/compositor_pr...
cc/blimp/compositor_proto_state.h:16: class CC_EXPORT CompositorProtoState {
On 2016/09/29 23:59:35, danakj wrote:
> On 2016/09/28 19:47:16, Khushal wrote:
> > On 2016/09/28 00:32:50, danakj wrote:
> > > Its like a CompositorMainFrame almost, or is it exactly that..
> > > RemoteCompositorMainFrame? And RemoteCompositor could be the interface?
> wdyt..
> > 
> > If the main frame includes PropertyTrees and other computed things too, then
> > not.
> 
> The main frame is the entire state of the main thread, all the layers, the
> viewport, the dsf, the etc... the commit is applying the main frame to the
other
> side.
> 
> > StateSink is fine I guess, in some sense there is a parallel here that the
> > CompositorFrameSink is supposed to transport CompositorFrames to their
> > destination, and CompositorProtoStateSink is transporting the paint output
fed
> > to the compositor.
> 
> Yeah, I just wouldn't get too into the name copying, cuz if everything looks
the
> same its hard to tell them apart. I don't want every interface that takes a
> struct to be called a sink. CompositorFrameSink is okay but TBH doesn't
describe
> what it is very well, I just don't know anything better. I won't block you on
> naming this similarly but I kinda think it could be more descriptive about
what
> it is beyond the data type you pass to a function.

I kept the CompositorProtoState and changed the interface to
RemoteCompositorBridge, mostly because ¯\_(ツ)_/¯. Added a comment on the class
to describe what it does though.

Khushal

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

4 years, 2 months ago (2016-09-30 19:16:46 UTC) #18

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-30 19:17:18 UTC) #19

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-30 19:52:54 UTC) #20

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-30 19:52:55 UTC) #21

Khushal

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

4 years, 2 months ago (2016-09-30 20:49:32 UTC) #22

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-30 20:50:01 UTC) #23

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-30 22:05:39 UTC) #24

commit-bot: I haz the power

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

4 years, 2 months ago (2016-09-30 22:05:41 UTC) #25

danakj

LGTMN https://codereview.chromium.org/2362073002/diff/60001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/2362073002/diff/60001/cc/trees/layer_tree_host.cc#newcode16 cc/trees/layer_tree_host.cc:16: return s_layer_tree_host_sequence_number.GetNext() + 1; On 2016/09/27 01:07:50, Khushal ...

4 years, 2 months ago (2016-09-30 22:37:12 UTC) #26

LGTMN

https://codereview.chromium.org/2362073002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2362073002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:16: return
s_layer_tree_host_sequence_number.GetNext() + 1;
On 2016/09/27 01:07:50, Khushal wrote:
> On 2016/09/26 21:36:54, danakj wrote:
> > It's not clear to me why this is here and not just in LTHRemote, like does
> > LTHRemote and LTHInProc need to share the same namespace for these ids?
> 
> The API said that it will give you a process global id for the LayerTreeHost,
so
> I figured I'll respect that and make sure that LayerTreeHost ids never
collide.
> I don't think we'll ever have a case where you create both (in-process and
> remote) in the same process, other than tests, so happy to keep it in
LTHRemote
> if that's better.

I think I'd prefer it as an implementation detail of each lth.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:38: source_frame_number_(0),
can you move constant initializers like this out to the definitions in the
header?

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:105:
swap_promise_manager_.QueueSwapPromise(std::move(swap_promise));
API feedback it's a bit weird that this class can return the swap promise
manager and also has API methods that are nothing but calls thru to the swap
promise manager, I think if GetSwapPromiseManager is there, QueueSwapPromise
shouldn't be

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:117: // The visibility of the compositor is
controlled on the client.
".. but we fake it for blink because .."

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:222: NOTIMPLEMENTED() << "We shouldn't be
sending fling gestures to the engine";
NOTREACHED() unless we are sending fling gestures, then the wording suggests a
TODO and a bug

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:394: MainTaskRunner()->PostTask(
This is the only user of MainTaskRunner? Just use the provider directly and drop
that method?

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
File cc/blimp/layer_tree_host_remote.h (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.h:31: // any resources (OutputSurface,
TaskGraphRunner, etc.) needed by the
TBH commenting on the absence of these is a bit weird here, like all classes
take the things they need. This LTH impl doesn't use those so they aren't
params.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.h:102: // RemoteCompositorBridgeClient
implementation.
nit: Im not a super fan of private inheriting functions cuz you can just upcast
and call them it feels misleading, it hides the functions with real private
things etc. Just want to say that, up to you.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.h:112: int id_;
const?

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/remote_compos...
File cc/blimp/remote_compositor_bridge.h (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/remote_compos...
cc/blimp/remote_compositor_bridge.h:20: // The RemoteCompositorBridge is the
compositor's communication bridge to the
It's not the compositor's communication bridge. It's the remote LTH
implementation's interface to talk to the blimp client which has a compositor in
it, right?

I think this is mostly about framing but here's what I have in my head where
this code is going architecturally, and I would like comments and dependencies
to match that if it seems right:

- LayerTreeHost is no longer a compositor, it is a public API that is exposed to
blink and content/renderer/.
- LayerTreeHostInProcess is an implementation of LTH that is a compositor.
- LayerTreeHostRemote is an implementation of LTH that talks to a blimp client.
- Owners of LayerTreeHostInProcess continue to be embedders of a compositor,
these are ui::Compositor, android ui, blimp client, unit tests.
- There are a few clear layers here though: blink <-> blimp engine impl
(LTHRemote) <-> blimp client (cc embedder) <-> cc layer compositor
(LTHInProcess)
- Nothing skips past these layers, so the LTHRemote talks to blimp client, not
to a compositor. Moving between these layers should normally go through an
abstract interface so we can replace it in tests and test a single layer on its
own.

Does that make sense? This relates to thinking about how to test things too. We
don't test LTHRemote by attaching it to a LTHInProcess. We test its interactions
with blimp client. We can test blimp client interactions with cc but adding each
layer to the same test increases the amount of work to maintain the test over
time and the likelyhood of it becoming flaky/failing due to unrelated code
change.

https://codereview.chromium.org/2362073002/diff/220001/cc/test/fake_remote_co...
File cc/test/fake_remote_compositor_bridge.h (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/test/fake_remote_co...
cc/test/fake_remote_compositor_bridge.h:17: // An implementation of the
RemoteCompositorBridge for tests that pumps
tests that pump

https://codereview.chromium.org/2362073002/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.h:57: virtual UIResourceManager* GetUIResourceManager()
= 0;
can stay const?

https://codereview.chromium.org/2362073002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.h:205: static int GenerateHostId();
Ya, prefer to keep this inside each impl.

danakj

On 2016/09/30 22:37:12, danakj wrote: > LGTMN I mean, LGTM.

4 years, 2 months ago (2016-09-30 22:37:30 UTC) #27

danakj

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_host_remote.cc File cc/blimp/layer_tree_host_remote.cc (right): https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_host_remote.cc#newcode117 cc/blimp/layer_tree_host_remote.cc:117: // The visibility of the compositor is controlled on ...

4 years, 2 months ago (2016-09-30 22:38:31 UTC) #28

Khushal

Thanks Dana. https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_host_remote.cc File cc/blimp/layer_tree_host_remote.cc (right): https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_host_remote.cc#newcode38 cc/blimp/layer_tree_host_remote.cc:38: source_frame_number_(0), On 2016/09/30 22:37:13, danakj wrote: > ...

4 years, 2 months ago (2016-10-01 00:53:48 UTC) #29

Thanks Dana.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:38: source_frame_number_(0),
On 2016/09/30 22:37:13, danakj wrote:
> can you move constant initializers like this out to the definitions in the
> header?

Done.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:105:
swap_promise_manager_.QueueSwapPromise(std::move(swap_promise));
On 2016/09/30 22:37:13, danakj wrote:
> API feedback it's a bit weird that this class can return the swap promise
> manager and also has API methods that are nothing but calls thru to the swap
> promise manager, I think if GetSwapPromiseManager is there, QueueSwapPromise
> shouldn't be

For this one you would have to talk to piman@,
https://codereview.chromium.org/2323423002/diff/80001/content/renderer/gpu/re...

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:117: // The visibility of the compositor is
controlled on the client.
On 2016/09/30 22:38:31, danakj wrote:
> On 2016/09/30 22:37:13, danakj wrote:
> > ".. but we fake it for blink because .."
> 
> oops i lied we shouldnt say blink here. "for the client"

Not sure what you mean, we don't fake anything here, we just store what the
embedder gave us. Only, we don't send anything to the client because don't need
to. Updated the comment.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:222: NOTIMPLEMENTED() << "We shouldn't be
sending fling gestures to the engine";
On 2016/09/30 22:37:13, danakj wrote:
> NOTREACHED() unless we are sending fling gestures, then the wording suggests a
> TODO and a bug

This will probably be a long running bug to understand what should be done here.
Added a bug.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:394: MainTaskRunner()->PostTask(
On 2016/09/30 22:37:13, danakj wrote:
> This is the only user of MainTaskRunner? Just use the provider directly and
drop
> that method?

Done.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
File cc/blimp/layer_tree_host_remote.h (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.h:31: // any resources (OutputSurface,
TaskGraphRunner, etc.) needed by the
On 2016/09/30 22:37:13, danakj wrote:
> TBH commenting on the absence of these is a bit weird here, like all classes
> take the things they need. This LTH impl doesn't use those so they aren't
> params.

Removed.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.h:102: // RemoteCompositorBridgeClient
implementation.
On 2016/09/30 22:37:13, danakj wrote:
> nit: Im not a super fan of private inheriting functions cuz you can just
upcast
> and call them it feels misleading, it hides the functions with real private
> things etc. Just want to say that, up to you.

I just think that it makes it more explicit that this is meant to be private. If
they have to cast it to be able to call it, they'll know they are probably doing
something wrong.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.h:112: int id_;
On 2016/09/30 22:37:13, danakj wrote:
> const?

Done.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/remote_compos...
File cc/blimp/remote_compositor_bridge.h (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/remote_compos...
cc/blimp/remote_compositor_bridge.h:20: // The RemoteCompositorBridge is the
compositor's communication bridge to the
On 2016/09/30 22:37:13, danakj wrote:
> It's not the compositor's communication bridge. It's the remote LTH
> implementation's interface to talk to the blimp client which has a compositor
in
> it, right?

Yup, the class structure is going to be that blimp will have an impl of this
class to take the protos (then diff/cache or whatever optimizations it wants to
do). On the client side the blimp code will have a compositor which owns a
LTHInProcess, and a CompositorStateDeserializer to deserialize the state into
the LayerTree.

> 
> I think this is mostly about framing but here's what I have in my head where
> this code is going architecturally, and I would like comments and dependencies
> to match that if it seems right:
> 
> - LayerTreeHost is no longer a compositor, it is a public API that is exposed
to
> blink and content/renderer/.
> - LayerTreeHostInProcess is an implementation of LTH that is a compositor.
> - LayerTreeHostRemote is an implementation of LTH that talks to a blimp
client.
> - Owners of LayerTreeHostInProcess continue to be embedders of a compositor,
> these are ui::Compositor, android ui, blimp client, unit tests.
> - There are a few clear layers here though: blink <-> blimp engine impl
> (LTHRemote) <-> blimp client (cc embedder) <-> cc layer compositor
> (LTHInProcess)
> - Nothing skips past these layers, so the LTHRemote talks to blimp client, not
> to a compositor. Moving between these layers should normally go through an
> abstract interface so we can replace it in tests and test a single layer on
its
> own.

This sounds mostly like what I had in mind, each layer has an interface to talk
to the next layer in the pipeline. At the same time, each layer has a
responsibility so that so long as you have an implementation that faithfully
does that, you can replace the layer in between 2 components and test the
behaviour of the system together. So the layer between LTHRemote and
CompositorStateDeserializer on the client's job is to correctly transport the
data provided between these 2 ends. There can be optimizations in the transport
but these 2 ends don't need to aware of it.

One of the primary reasons for having this code in cc is that it is strongly
tied to the cc APIs and data. Sure, we have an impl of LTHRemote, which keeps
blimp specific quirks outside rest of cc, but for instance, if a callback is
added to the LTHClient that blink now depends on, we have to make sure it gets
updated in both places. Similarly, the data we are transporting is cc data
structures and any updates to these have to be made together. Basically cc/blimp
is saying here is a way you can record cc state for deferred use and later if
you give it to back to me, you'll get the same result. The best parallel here
would be an SkPicture. Now if skia changes anything, its their responsibility to
make sure that the recording API works correctly.

The only way I could think of getting some testing to do this with cc is to mock
out the layer between LTHRemote and LTHInProcess (assuming its a dumb transport)
and run all the existing tests, and any future tests added here. If a new
property is added, I hope we have tests to cover their use through the cc
pipeline.

I understand if you think that is going overboard, but I'm not sure how I can
convince myself that regressions won't creep in from this transition layer
between blink and cc not being maintained. If there is another testing strategy
you have in mind, I'm all open to hear that.

> 
> Does that make sense? This relates to thinking about how to test things too.
We
> don't test LTHRemote by attaching it to a LTHInProcess. We test its
interactions
> with blimp client. We can test blimp client interactions with cc but adding
each
> layer to the same test increases the amount of work to maintain the test over
> time and the likelyhood of it becoming flaky/failing due to unrelated code
> change.

https://codereview.chromium.org/2362073002/diff/220001/cc/test/fake_remote_co...
File cc/test/fake_remote_compositor_bridge.h (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/test/fake_remote_co...
cc/test/fake_remote_compositor_bridge.h:17: // An implementation of the
RemoteCompositorBridge for tests that pumps
On 2016/09/30 22:37:13, danakj wrote:
> tests that pump

Done.

https://codereview.chromium.org/2362073002/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.h:57: virtual UIResourceManager* GetUIResourceManager()
= 0;
On 2016/09/30 22:37:13, danakj wrote:
> can stay const?

Done.

https://codereview.chromium.org/2362073002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.h:205: static int GenerateHostId();
On 2016/09/30 22:37:13, danakj wrote:
> Ya, prefer to keep this inside each impl.

Done.

Khushal

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

4 years, 2 months ago (2016-10-01 01:01:00 UTC) #30

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-01 01:01:50 UTC) #31

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-01 01:20:38 UTC) #32

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/builds/3259)

4 years, 2 months ago (2016-10-01 01:20:39 UTC) #33

enne (OOO)

On 2016/09/30 at 22:37:12, danakj wrote: > - LayerTreeHost is no longer a compositor, it ...

4 years, 2 months ago (2016-10-03 17:38:02 UTC) #34

Khushal

On 2016/10/03 17:38:02, enne wrote: > On 2016/09/30 at 22:37:12, danakj wrote: > > > ...

4 years, 2 months ago (2016-10-03 20:11:48 UTC) #35

Khushal

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the ...

4 years, 2 months ago (2016-10-03 20:11:57 UTC) #37

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-03 20:12:22 UTC) #38

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-03 20:33:04 UTC) #39

commit-bot: I haz the power

Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/209883) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, ...

4 years, 2 months ago (2016-10-03 20:33:06 UTC) #40

Khushal

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the ...

4 years, 2 months ago (2016-10-03 20:55:36 UTC) #42

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-03 20:56:17 UTC) #43

danakj

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_host_remote.cc File cc/blimp/layer_tree_host_remote.cc (right): https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_host_remote.cc#newcode105 cc/blimp/layer_tree_host_remote.cc:105: swap_promise_manager_.QueueSwapPromise(std::move(swap_promise)); On 2016/10/01 00:53:48, Khushal wrote: > On 2016/09/30 ...

4 years, 2 months ago (2016-10-03 21:02:48 UTC) #44

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:105:
swap_promise_manager_.QueueSwapPromise(std::move(swap_promise));
On 2016/10/01 00:53:48, Khushal wrote:
> On 2016/09/30 22:37:13, danakj wrote:
> > API feedback it's a bit weird that this class can return the swap promise
> > manager and also has API methods that are nothing but calls thru to the swap
> > promise manager, I think if GetSwapPromiseManager is there, QueueSwapPromise
> > shouldn't be
> 
> For this one you would have to talk to piman@,
>
https://codereview.chromium.org/2323423002/diff/80001/content/renderer/gpu/re...

Thanks for the pointer.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:117: // The visibility of the compositor is
controlled on the client.
On 2016/10/01 00:53:48, Khushal wrote:
> On 2016/09/30 22:38:31, danakj wrote:
> > On 2016/09/30 22:37:13, danakj wrote:
> > > ".. but we fake it for blink because .."
> > 
> > oops i lied we shouldnt say blink here. "for the client"
> 
> Not sure what you mean, we don't fake anything here, we just store what the
> embedder gave us. Only, we don't send anything to the client because don't
need
> to. Updated the comment.

What I mean by fake, this doesn't actually change anything on the LayerTreeHost,
except that you can read the variable back. That suggests to me that this isn't
the right place for this state maybe.

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
File cc/blimp/layer_tree_host_remote.h (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.h:102: // RemoteCompositorBridgeClient
implementation.
On 2016/10/01 00:53:48, Khushal wrote:
> On 2016/09/30 22:37:13, danakj wrote:
> > nit: Im not a super fan of private inheriting functions cuz you can just
> upcast
> > and call them it feels misleading, it hides the functions with real private
> > things etc. Just want to say that, up to you.
> 
> I just think that it makes it more explicit that this is meant to be private.
If
> they have to cast it to be able to call it, they'll know they are probably
doing
> something wrong.

They may cast it accidentally. It can be done implicitly.

commit-bot: I haz the power

Description was changed from ========== cc/blimp: Add a LayerTreeHostRemote implementation. This sets up the framework ...

4 years, 2 months ago (2016-10-03 22:12:04 UTC) #46

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-03 22:12:06 UTC) #47

horo

A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/2384333002/ by horo@chromium.org. ...

4 years, 2 months ago (2016-10-03 23:42:57 UTC) #48

Khushal

Description was changed from ========== cc/blimp: Add a LayerTreeHostRemote implementation. This sets up the framework ...

4 years, 2 months ago (2016-10-04 01:23:18 UTC) #49

Khushal

https://codereview.chromium.org/2362073002/diff/300001/cc/blimp/layer_tree_host_remote_unittest.cc File cc/blimp/layer_tree_host_remote_unittest.cc (right): https://codereview.chromium.org/2362073002/diff/300001/cc/blimp/layer_tree_host_remote_unittest.cc#newcode75 cc/blimp/layer_tree_host_remote_unittest.cc:75: MOCK_METHOD1(BeginMainFrame, void(const BeginFrameArgs& args)); Mocking this method was messing ...

4 years, 2 months ago (2016-10-04 01:24:24 UTC) #50

Khushal

Description was changed from ========== cc/blimp: Add a LayerTreeHostRemote implementation. This sets up the framework ...

4 years, 2 months ago (2016-10-04 01:24:44 UTC) #51

Khushal

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the ...

4 years, 2 months ago (2016-10-04 01:25:00 UTC) #53

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-04 01:25:25 UTC) #54

Khushal

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_host_remote.h File cc/blimp/layer_tree_host_remote.h (right): https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_host_remote.h#newcode102 cc/blimp/layer_tree_host_remote.h:102: // RemoteCompositorBridgeClient implementation. On 2016/10/03 21:02:48, danakj wrote: > ...

4 years, 2 months ago (2016-10-04 01:27:09 UTC) #55

danakj

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_host_remote.h File cc/blimp/layer_tree_host_remote.h (right): https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_host_remote.h#newcode102 cc/blimp/layer_tree_host_remote.h:102: // RemoteCompositorBridgeClient implementation. On 2016/10/04 01:27:09, Khushal wrote: > ...

4 years, 2 months ago (2016-10-04 01:40:57 UTC) #56

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
File cc/blimp/layer_tree_host_remote.h (right):

https://codereview.chromium.org/2362073002/diff/220001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.h:102: // RemoteCompositorBridgeClient
implementation.
On 2016/10/04 01:27:09, Khushal wrote:
> On 2016/10/03 21:02:48, danakj wrote:
> > On 2016/10/01 00:53:48, Khushal wrote:
> > > On 2016/09/30 22:37:13, danakj wrote:
> > > > nit: Im not a super fan of private inheriting functions cuz you can just
> > > upcast
> > > > and call them it feels misleading, it hides the functions with real
> private
> > > > things etc. Just want to say that, up to you.
> > > 
> > > I just think that it makes it more explicit that this is meant to be
> private.
> > If
> > > they have to cast it to be able to call it, they'll know they are probably
> > doing
> > > something wrong.
> > 
> > They may cast it accidentally. It can be done implicitly.
> 
> Can you share an example of this one? I thought that unless something takes
the
> base class pointer you'll have to do an explicit cast. Just want to know
because
> I do use this pattern a lot...

BaseClass* GetSubclass() {
  Subclass* s = ...
  return s;
}
GetSubclass()->Inherited();

or
SubClass* GetSubclass() {
  Subclass* s = ...
  return s;
}
BaseClass *b = ...
...
if (something)
  b = GetSubclass();
...
b->Inherited();

Like nothing is preventing you from putting the subclass into a variable of the
interface, and calling that, and with sufficient distance created by the ...s
above I think this becomes less trivial. You're right that you need to have some
code naming the base class somewhere for it to happen. But I'm not sure it means
someone will know they are doing something wrong so much as they will know that
they need to get the client* before using these methods, but that doesn't mean
they should be using the client interface.

And given enough time, there will be some test poking that method directly
anyway. :)

commit-bot: I haz the power

Description was changed from ========== cc/blimp: Add a LayerTreeHostRemote implementation. This sets up the framework ...

4 years, 2 months ago (2016-10-04 02:49:32 UTC) #57

commit-bot: I haz the power

Description was changed from ========== cc/blimp: Add a LayerTreeHostRemote implementation. This sets up the framework ...

4 years, 2 months ago (2016-10-04 02:51:48 UTC) #59

commit-bot: I haz the power

4 years, 2 months ago (2016-10-04 02:51:49 UTC) #60

Message was sent while issue was closed.

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

Issue 2362073002: cc/blimp: Add a LayerTreeHostRemote implementation. (Closed)

Description

Patch Set 1 #

Patch Set 2 : bind to client. #

Patch Set 3 : tests #

Patch Set 4 : source frame number #

Patch Set 5 : Addressed comments #

Patch Set 6 : Remaining comments from #4 #

Patch Set 7 : Addressed some more comments. #

Patch Set 8 : actually upload .cc file #

Patch Set 9 : Renames. #

Patch Set 10 : missed a comment #

Patch Set 11 : Rebase #

Patch Set 12 : win #

Patch Set 13 : some more comments #

Patch Set 14 : missed an export #

Patch Set 15 : minor comment update #

Patch Set 16 : Rebase #

Patch Set 17 : begin_frame_args #

Messages