cc/blimp: Add a LayerTreeHostRemote implementation.

cc/blimp/layer_tree_host_remote.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "cc/blimp/layer_tree_host_remote.h"
+
+#include "base/memory/ptr_util.h"
+#include "cc/animation/animation_host.h"
+#include "cc/blimp/compositor_proto_state.h"
+#include "cc/blimp/compositor_proto_state_sink.h"
+#include "cc/output/begin_frame_args.h"
+#include "cc/output/compositor_frame_sink.h"
+#include "cc/resources/ui_resource_manager.h"
+#include "cc/trees/layer_tree.h"
+#include "cc/trees/layer_tree_host_client.h"
+#include "cc/trees/layer_tree_host_common.h"
+#include "cc/trees/task_runner_provider.h"
+
+namespace cc {
+namespace {
+// We use a 16ms default frame interval because the rate at which the engine
+// produces main frames doesn't matter.
+base::TimeDelta kDefaultFrameInterval = base::TimeDelta::FromMilliseconds(16);
+}  // namespace
+
+LayerTreeHostRemote::InitParams::InitParams() = default;
+
+LayerTreeHostRemote::InitParams::~InitParams() = default;
+
+LayerTreeHostRemote::LayerTreeHostRemote(InitParams* params)
+    : LayerTreeHostRemote(
+          params,
+          base::MakeUnique<LayerTree>(std::move(params->animation_host),
+                                      this)) {}
+
+LayerTreeHostRemote::LayerTreeHostRemote(InitParams* params,
+                                         std::unique_ptr<LayerTree> layer_tree)
+    : id_(LayerTreeHost::GenerateHostId()),
+      source_frame_number_(0),
+      visible_(false),
+      defer_commits_(false),
+      main_frame_requested_from_state_sink_(false),
+      current_pipeline_stage_(FramePipelineStage::NONE),
+      max_pipeline_stage_for_current_frame_(FramePipelineStage::NONE),
+      requested_pipeline_stage_for_next_frame_(FramePipelineStage::NONE),
+      client_(params->client),
+      task_runner_provider_(
+          TaskRunnerProvider::Create(std::move(params->main_task_runner),
+                                     nullptr)),
+      compositor_proto_state_sink_(
+          std::move(params->compositor_proto_state_sink)),
+      settings_(*params->settings),
+      layer_tree_(std::move(layer_tree)),
+      ui_resource_manager_(base::MakeUnique<UIResourceManager>()),

[danakj, 2016/09/26 21:36:53]

why is it a pointer?

[Khushal, 2016/09/27 01:07:49]

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

+      weak_factory_(this) {
+  DCHECK(task_runner_provider_->IsMainThread());
+  DCHECK(compositor_proto_state_sink_);
+  DCHECK(client_);
+  compositor_proto_state_sink_->BindToClient(this);
+}
+
+LayerTreeHostRemote::~LayerTreeHostRemote() = default;
+
+int LayerTreeHostRemote::GetId() const {
+  return id_;
+}
+
+int LayerTreeHostRemote::SourceFrameNumber() const {
+  return source_frame_number_;
+}
+
+LayerTree* LayerTreeHostRemote::GetLayerTree() {
+  return layer_tree_.get();
+}
+
+const LayerTree* LayerTreeHostRemote::GetLayerTree() const {
+  return layer_tree_.get();
+}
+
+UIResourceManager* LayerTreeHostRemote::GetUIResourceManager() const {
+  // We shouldn't really need a UIResourceManager. The layers which need this
+  // are never used.
+  LOG(ERROR) << "UIResourceManager requested.";

[danakj, 2016/09/26 21:36:52]

How about notreached and return null then?

[Khushal, 2016/09/27 01:07:48]

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.

[danakj, 2016/09/27 20:32:43]

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

[Khushal, 2016/09/27 23:46:56]

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.

+  return ui_resource_manager_.get();
+}
+
+TaskRunnerProvider* LayerTreeHostRemote::GetTaskRunnerProvider() const {
+  return task_runner_provider_.get();
+}
+
+const LayerTreeSettings& LayerTreeHostRemote::GetSettings() const {
+  return settings_;
+}
+
+void LayerTreeHostRemote::SetSurfaceClientId(uint32_t client_id) {
+  // We don't need to care about SurfaceLayers. The Surfaces system is

[danakj, 2016/09/26 21:36:52]

Can we NOTREACHED?

[Khushal, 2016/09/27 01:07:49]

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

+  // relevant on the client only.
+}
+
+void LayerTreeHostRemote::SetLayerTreeMutator(
+    std::unique_ptr<LayerTreeMutator> mutator) {
+  // Compositor-worker not supported.

[danakj, 2016/09/26 21:36:52]

Can we NOTREACHED?

[Khushal, 2016/09/27 01:07:49]

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

[danakj, 2016/09/27 20:32:43]

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

[Khushal, 2016/09/27 23:46:56]

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.

+}
+
+void LayerTreeHostRemote::QueueSwapPromise(
+    std::unique_ptr<SwapPromise> swap_promise) {
+  swap_promise_manager_.QueueSwapPromise(std::move(swap_promise));
+}
+
+SwapPromiseManager* LayerTreeHostRemote::GetSwapPromiseManager() {
+  return &swap_promise_manager_;
+}
+
+void LayerTreeHostRemote::SetHasGpuRasterizationTrigger(bool has_trigger) {
+  // TODO(khushalsagar) : Take care of Gpu raster.

[danakj, 2016/09/26 21:36:52]

Can you point at a bug?

[Khushal, 2016/09/27 01:07:49]

Done.

+}
+
+void LayerTreeHostRemote::SetVisible(bool visible) {
+  // The visibility is controlled on the client. The value here should always be
+  // true and changes only during page transitions.

[danakj, 2016/09/26 21:36:52]

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?

[Khushal, 2016/09/27 01:07:49]

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.

+  visible_ = visible;
+}
+
+bool LayerTreeHostRemote::IsVisible() const {
+  return visible_;
+}
+
+void LayerTreeHostRemote::SetCompositorFrameSink(
+    std::unique_ptr<CompositorFrameSink> compositor_frame_sink) {
+  NOTREACHED() << "We never ask the client for a CompositorFrameSink";

[danakj, 2016/09/26 21:36:53]

"We" is a bit ambiguous here. "The LayerTreeHostClient is never asked for a.." ?

[Khushal, 2016/09/27 01:07:49]

Done.

+}
+
+std::unique_ptr<CompositorFrameSink>
+LayerTreeHostRemote::ReleaseCompositorFrameSink() {
+  // Since we never have a CompositorFrameSink, this is always a no-op.

[danakj, 2016/09/26 21:36:52]

Can we NOTREACHED?

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.

[Khushal, 2016/09/27 01:07:49]

Well, the API says can be called safely anytime, so I did not want to fail here.
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?

[danakj, 2016/09/27 20:32:43]

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.

[Khushal, 2016/09/27 23:46:56]

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?

+  return nullptr;
+}
+
+void LayerTreeHostRemote::SetNeedsAnimate() {
+  MainFrameRequested(FramePipelineStage::ANIMATE);
+}
+
+void LayerTreeHostRemote::SetNeedsUpdateLayers() {
+  MainFrameRequested(FramePipelineStage::UPDATE_LAYERS);
+}
+
+void LayerTreeHostRemote::SetNeedsCommit() {
+  MainFrameRequested(FramePipelineStage::COMMIT);
+}
+
+bool LayerTreeHostRemote::BeginMainFrameRequested() const {
+  return requested_pipeline_stage_for_next_frame_ != FramePipelineStage::NONE;
+}
+
+bool LayerTreeHostRemote::CommitRequested() const {
+  return requested_pipeline_stage_for_next_frame_ == FramePipelineStage::COMMIT;
+}
+
+void LayerTreeHostRemote::SetDeferCommits(bool defer_commits) {
+  defer_commits_ = defer_commits;
+  ScheduleMainFrameIfNecessary();
+}
+
+void LayerTreeHostRemote::LayoutAndUpdateLayers() {
+  NOTREACHED() << "Only supported in single-threaded mode";

[danakj, 2016/09/26 21:36:52]

".. and this class does not support single-thread since it is out of process" ?

[Khushal, 2016/09/27 01:07:49]

Done.

+}
+
+void LayerTreeHostRemote::Composite(base::TimeTicks frame_begin_time) {
+  NOTREACHED() << "Only supported in single-threaded mode";

[danakj, 2016/09/26 21:36:52]

ditto

[Khushal, 2016/09/27 01:07:49]

Done.

+}
+
+void LayerTreeHostRemote::SetNeedsRedraw() {
+  // The engine shouldn't need to care about draws. CompositorFrames are never

[danakj, 2016/09/26 21:36:53]

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.

[Khushal, 2016/09/27 01:07:49]

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?

[danakj, 2016/09/27 20:32:43]

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?

[Khushal, 2016/09/27 23:46:56]

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?

[danakj, 2016/09/28 00:32:50]

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

[Khushal, 2016/09/28 19:47:16]

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

+  // used here.
+}
+
+void LayerTreeHostRemote::SetNeedsRedrawRect(const gfx::Rect& damage_rect) {
+  // The engine shouldn't need to care about draws. CompositorFrames are never

[danakj, 2016/09/26 21:36:52]

Same.

[danakj, 2016/09/27 20:32:43]

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.

[Khushal, 2016/09/27 23:46:56]

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?

[danakj, 2016/09/28 00:32:50]

It sounds sketchy in that it's likely to interfere with swap counting if you're
seeing swap acks from multiple producers.
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.

[Khushal, 2016/09/28 19:47:16]

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

+  // used here.
+}
+
+void LayerTreeHostRemote::SetNextCommitForcesRedraw() {
+  // The engine shouldn't need to care about draws. CompositorFrames are never

[danakj, 2016/09/26 21:36:53]

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.

[Khushal, 2016/09/27 01:07:48]

Still no need to send anything to the client though?

[danakj, 2016/09/27 20:32:43]

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

[Khushal, 2016/09/27 23:46:56]

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.

[danakj, 2016/09/28 00:32:50]

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.

[Khushal, 2016/09/28 19:47:16]

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.

[danakj, 2016/09/29 23:59:35]

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

[Khushal, 2016/09/30 02:11:18]

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

+  // used here.
+}
+
+void LayerTreeHostRemote::NotifyInputThrottledUntilCommit() {
+  // We don't throttle commits right now so this is not relevant.

[danakj, 2016/09/26 21:36:52]

NOTREACHED?

[Khushal, 2016/09/27 01:07:50]

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.

[danakj, 2016/09/27 20:32:43]

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.

[Khushal, 2016/09/27 23:46:56]

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

[danakj, 2016/09/28 00:32:50]

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

[Khushal, 2016/09/28 19:47:16]

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

+}
+
+void LayerTreeHostRemote::UpdateTopControlsState(TopControlsState constraints,
+                                                 TopControlsState current,
+                                                 bool animate) {
+  NOTREACHED() << "Using TopControls animations is not supported";
+}
+
+const base::WeakPtr<InputHandler>& LayerTreeHostRemote::GetInputHandler()
+    const {
+  // Input on the compositor thread is handled on the client, so this is always
+  // null.
+  return input_handler_weak_ptr_;

[danakj, 2016/09/26 21:36:53]

This is kinda awk, maybe follow up to make this return a non-reference so you
can just return nullptr?

[Khushal, 2016/09/27 01:07:49]

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

+}
+
+void LayerTreeHostRemote::DidStopFlinging() {
+  NOTIMPLEMENTED() << "We shouldn't be sending fling gestures to the engine";
+}
+
+void LayerTreeHostRemote::SetDebugState(
+    const LayerTreeDebugState& debug_state) {
+  debug_state_ = debug_state;

[danakj, 2016/09/26 21:36:53]

How does this get to the client? Should it not SetNeedsCommit or something? Why
does this not do the Unite thing that LTHInProc does?

[Khushal, 2016/09/27 01:07:49]

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.

[danakj, 2016/09/27 20:32:43]

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.

[Khushal, 2016/09/27 23:46:56]

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

[danakj, 2016/09/28 00:32:50]

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

[Khushal, 2016/09/28 19:47:16]

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

+}
+
+const LayerTreeDebugState& LayerTreeHostRemote::GetDebugState() const {
+  return debug_state_;
+}
+
+int LayerTreeHostRemote::ScheduleMicroBenchmark(
+    const std::string& benchmark_name,
+    std::unique_ptr<base::Value> value,
+    const MicroBenchmark::DoneCallback& callback) {
+  NOTIMPLEMENTED();

[danakj, 2016/09/26 21:36:53]

Or NOTREACHED?

[Khushal, 2016/09/27 01:07:49]

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.

[danakj, 2016/09/27 20:32:43]

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.

[Khushal, 2016/09/27 23:46:56]

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

+  return 0;
+}
+
+bool LayerTreeHostRemote::SendMessageToMicroBenchmark(
+    int id,
+    std::unique_ptr<base::Value> value) {
+  NOTIMPLEMENTED();

[danakj, 2016/09/26 21:36:53]

Or NOTREACHED?

[Khushal, 2016/09/27 01:07:49]

Same.

+  return false;
+}
+
+SurfaceSequenceGenerator* LayerTreeHostRemote::GetSurfaceSequenceGenerator() {
+  return &surface_sequence_generator_;

[danakj, 2016/09/26 21:36:53]

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

[Khushal, 2016/09/27 01:07:49]

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.

[danakj, 2016/09/27 20:32:43]

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.

[Khushal, 2016/09/27 23:46:56]

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.

+}
+
+void LayerTreeHostRemote::SetNextCommitWaitsForActivation() {
+  // This is used only by layers that need resource synchronization, i.e.,
+  // texture and surface layers, both of which are not supported.
+  NOTIMPLEMENTED() << "Unsupported Layer type used";
+}
+
+void LayerTreeHostRemote::ResetGpuRasterizationTracking() {}

[danakj, 2016/09/26 21:36:52]

TODO? Is there a bug to link to?

[Khushal, 2016/09/27 01:07:49]

Done.

+
+void LayerTreeHostRemote::MainFrameRequested(
+    FramePipelineStage requested_pipeline_stage) {
+  DCHECK_NE(FramePipelineStage::NONE, requested_pipeline_stage);
+
+  swap_promise_manager_.NotifySwapPromiseMonitorsOfSetNeedsCommit();
+
+  // If we are inside a main frame update right now and the requested pipeline
+  // stage is higher than the pipeline stage that we are at, then we'll get to
+  // in this main frame update itself. Update the
+  // |max_pipeline_stage_for_current_frame_| to ensure we go through the
+  // requested pipeline stage.
+  if (current_pipeline_stage_ != FramePipelineStage::NONE &&
+      requested_pipeline_stage > current_pipeline_stage_) {
+    max_pipeline_stage_for_current_frame_ = std::max(
+        max_pipeline_stage_for_current_frame_, requested_pipeline_stage);
+    return;
+  }
+
+  // Update the pipeline stage for the next frame and schedule an update if it
+  // has not been scheduled already.
+  requested_pipeline_stage_for_next_frame_ = std::max(
+      requested_pipeline_stage_for_next_frame_, requested_pipeline_stage);
+
+  ScheduleMainFrameIfNecessary();
+}
+
+void LayerTreeHostRemote::ScheduleMainFrameIfNecessary() {
+  // If the client hasn't asked for a main frame, don't schedule one.
+  if (requested_pipeline_stage_for_next_frame_ == FramePipelineStage::NONE)
+    return;
+
+  // If the client does not want us to run main frame updates right now, don't
+  // schedule one.
+  if (defer_commits_)
+    return;
+
+  // If a main frame request is already pending with the CompositorFrameSink,

[danakj, 2016/09/26 21:36:53]

I don't think you mean CompositorFrameSink

[Khushal, 2016/09/27 01:07:49]

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.

+  // we don't need to scheduler another one.
+  if (main_frame_requested_from_state_sink_)
+    return;
+
+  compositor_proto_state_sink_->ScheduleMainFrame();
+  main_frame_requested_from_state_sink_ = true;
+}
+
+void LayerTreeHostRemote::BeginMainFrame() {
+  DCHECK(main_frame_requested_from_state_sink_);
+
+  main_frame_requested_from_state_sink_ = false;
+
+  // The client might have suspended main frames in the meantime. Early out now,
+  // we'll come back here when they enable main frames again.
+  if (defer_commits_)
+    return;
+
+  DCHECK_EQ(FramePipelineStage::NONE, current_pipeline_stage_);

[danakj, 2016/09/26 21:36:52]

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.

[Khushal, 2016/09/27 01:07:48]

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

+  DCHECK_EQ(FramePipelineStage::NONE, max_pipeline_stage_for_current_frame_);
+  DCHECK_NE(FramePipelineStage::NONE, requested_pipeline_stage_for_next_frame_);
+
+  // Start the main frame. It should go till the requested pipeline stage.
+  max_pipeline_stage_for_current_frame_ =
+      requested_pipeline_stage_for_next_frame_;
+  requested_pipeline_stage_for_next_frame_ = FramePipelineStage::NONE;
+
+  client_->WillBeginMainFrame();
+
+  current_pipeline_stage_ = FramePipelineStage::ANIMATE;
+  base::TimeTicks now = base::TimeTicks::Now();
+  client_->BeginMainFrame(BeginFrameArgs::Create(
+      BEGINFRAME_FROM_HERE, now, now + kDefaultFrameInterval,
+      kDefaultFrameInterval, BeginFrameArgs::NORMAL));
+  // We don't run any animations on the layer because threaded animations are
+  // disabled.
+  // TODO(khushalsagar): Revisit this when adding support for animations.
+  DCHECK(!layer_tree_->animation_host()->needs_push_properties());
+  client_->UpdateLayerTreeHost();
+
+  current_pipeline_stage_ = FramePipelineStage::UPDATE_LAYERS;
+  LayerList layer_list;
+  if (max_pipeline_stage_for_current_frame_ >=
+      FramePipelineStage::UPDATE_LAYERS) {
+    // Pull updates for all layers from the client.

[danakj, 2016/09/26 21:36:53]

Do you really want all layers? Is this something to be improved?

[Khushal, 2016/09/27 01:07:50]

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?

[danakj, 2016/09/27 20:32:43]

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.

[Khushal, 2016/09/27 23:46:56]

Thanks. Added a TODO.

+    LayerTreeHostCommon::CallFunctionForEveryLayer(
+        layer_tree_.get(),
+        [&layer_list](Layer* layer) { layer_list.push_back(layer); });
+
+    bool content_is_suitable_for_gpu = false;
+    bool layers_updated =
+        layer_tree_->UpdateLayers(layer_list, &content_is_suitable_for_gpu);
+
+    // If pulling layer updates resulted in any content updates, we need to go
+    // till the commit stage.
+    max_pipeline_stage_for_current_frame_ =

[danakj, 2016/09/26 21:36:52]

This is a weird way to write

if (layers_updated)
  max_pipe...  = ..COMMIT;

[Khushal, 2016/09/27 01:07:49]

Done.

+        layers_updated ? FramePipelineStage::COMMIT
+                       : max_pipeline_stage_for_current_frame_;
+  }
+
+  current_pipeline_stage_ = FramePipelineStage::COMMIT;
+  client_->WillCommit();
+
+  if (max_pipeline_stage_for_current_frame_ < current_pipeline_stage_) {
+    // There is nothing to commit so break the swap promises.
+    swap_promise_manager_.BreakSwapPromises(
+        SwapPromise::DidNotSwapReason::COMMIT_NO_UPDATE);
+
+    // For the client, the commit was successful.
+    MainFrameComplete();
+    return;
+  }
+
+  // TODO(khushalsagar): Serialize current state/reset dirty state tracking and
+  // return the result to the state sink instead.
+  std::unique_ptr<CompositorProtoState> compositor_state =
+      base::MakeUnique<CompositorProtoState>();
+  compositor_proto_state_sink_->ProcessCompositorStateUpdate(
+      std::move(compositor_state));
+
+  MainFrameComplete();
+
+  // We can not wait for updates dispatched from the client about the state of

[danakj, 2016/09/26 21:36:53]

Why can't you wait?

[Khushal, 2016/09/27 01:07:49]

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.

[danakj, 2016/09/27 20:32:43]

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

[Khushal, 2016/09/27 23:46:56]

Done.

+  // drawing or swaps for frames sent. Since these calls can be used by the
+  // LayerTreeHostClient to throttle further frame updates, so dispatch them
+  // right after the update is processed by the state sink.
+  // TODO(khushalsagar): We can not really know what these callbacks end up
+  // being used for. Consider migrating clients to understand/cope with the fact
+  // that there is no actual compositing happening here.
+  GetMainTaskRunner()->PostTask(
+      FROM_HERE, base::Bind(&LayerTreeHostRemote::DispatchDrawAndSwapCallbacks,
+                            weak_factory_.GetWeakPtr()));
+}
+
+base::SingleThreadTaskRunner* LayerTreeHostRemote::GetMainTaskRunner() {
+  return task_runner_provider_->MainThreadTaskRunner();
+}
+
+void LayerTreeHostRemote::MainFrameComplete() {
+  DCHECK_EQ(current_pipeline_stage_, FramePipelineStage::COMMIT);
+
+  current_pipeline_stage_ = FramePipelineStage::NONE;
+  max_pipeline_stage_for_current_frame_ = FramePipelineStage::NONE;
+  source_frame_number_++;
+
+  client_->DidCommit();
+  client_->DidBeginMainFrame();
+}
+
+void LayerTreeHostRemote::DispatchDrawAndSwapCallbacks() {
+  client_->DidCommitAndDrawFrame();
+  client_->DidCompleteSwapBuffers();
+}
+
+}  // namespace cc