cc/blimp: Add synchronization for scroll/scale state.

cc/blimp: Add synchronization for scroll/scale state.

Add the protocol messages and the primitives to synchronize mutations
to scroll and scale state on the engine and client. The protocol has
been kept largely consistent with the message passing between the main
and impl thread, as follows:

1) Both the engine and client can send an update to the other peer at
any time, but must wait for an ack from the other peer before sending
another update.

2) If the engine receives an update from the client, it must send the
resulting frame with the acknowledgement for this update. This is
consistent with the behaviour in the threaded compositor where the
main thread always responds with a BeginMainFrameAborted or commit
for deltas sent from the impl thread.

However, as opposed to the threaded compositor, the communication
between the engine and client is completely asynchronous. Enabling
this requires state tracking on the client to track the deltas
seen by the engine, when pushing the resulting frame update to
the impl thread, so the impl thread can apply those deltas
post-commit instead of expecting the main thread to reflect these
values. The change adds a SyncedPropertyRemote primitive, which acts
as the main thread equivalent of SyncedProperty, to perform this for
all state that can be modified on the engine or client.

BUG=648442
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[Khushal, 2016-10-24 20:48:58]

Description was changed from

==========
cc/blimp: Add synchronization for scroll/scale state.

BUG=
==========

to

==========
cc/blimp: Add synchronization for scroll/scale state.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Khushal, 2016-10-24 21:22:31]

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

[Khushal, 2016-10-24 21:22:32]

This is super rough, I was trying to set everything up to make it work.

I've put most of the state tracking craziness in SyncedPropertyRemote. Its
almost the main thread equivalent of SyncedProperty to keep the engine and
client LayerTree in sync. The quirk is that the state on the main thread, that
is used to build PropertyTrees and create the pending tree has to stay
consistent with what came from the engine. So each time the delta is reported to
the main thread, we force-set it back to what the engine gave, and then
separately send the un-resolved deltas to the impl thread to add to the pending
base in SyncedProperty post commit. This seems to work for a few pages with fix
pos elements that I tried on.

Also right now the commit flow for blimp mirrors the cc commit flow. So in cc,
you send the deltas in the main frame and always get back the commit/abort
commit in the response. In the current case, the ack for the scroll deltas is
also bundled with the main frame.

But going forward my aim is to decouple these. We want to make sure that tap
events are hit-tested correctly on the engine. So my plan is to send scroll
deltas to the engine and run the main frame immediately, to make sure that blink
and cc are in sync screen space wise for the context of the tap event, but not
actually send this frame to the client. Then when the tap is processed by blink,
that's the frame that will be sent to the client. Does that make sense?

And can you take a quick look at this? I'm going to add aelias@ for a final
review too.

[Khushal, 2016-10-25 02:30:16]

Description was changed from

==========
cc/blimp: Add synchronization for scroll/scale state.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc/blimp: Add synchronization for scroll/scale state.

Add the protocol messages and the primitives to synchronize mutations
to scroll and scale state on the engine and client. The protocol has
been kept largely consistent with the message passing between the main
and impl thread, as follows:

1) Both the engine and client can send an update to the other peer at
any time, but must wait for an ack from the other peer before sending
another update.

2) If the engine receives an update from the client, it must sent the
resulting frame with the acknowledgement for this update. This is
consistent with the behaviour in the threaded compositor where the
main thread always responds with a BeginMainFrameAborted or commit
for deltas sent to the impl thread.

However, as opposed to the threaded compositor, the communication
between the engine and client is completely asynchronous. Enabling
this requires state tracking on the client to track the deltas
seen by the engine, when pushing the resulting frame update to
the impl thread, so the impl thread can apply those deltas
post-commit instead of expecting the main thread to reflect these
values. The change adds a SyncedPropertyRemote primitive, which acts
as the main thread equivalent of SyncedProperty, to perform this for
all state that can be modified on the engine or client.

BUG=648442
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Khushal, 2016-10-25 02:45:49]

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

[Khushal, 2016-10-25 02:45:49]

I think this is ready for a more formal review now. Still need to add tests but
wanted to get feedback on the overall approach.

+aelias/danakj for cc/
+dtrainor for blimp/

[Khushal, 2016-10-25 03:13:23]

Description was changed from

==========
cc/blimp: Add synchronization for scroll/scale state.

Add the protocol messages and the primitives to synchronize mutations
to scroll and scale state on the engine and client. The protocol has
been kept largely consistent with the message passing between the main
and impl thread, as follows:

1) Both the engine and client can send an update to the other peer at
any time, but must wait for an ack from the other peer before sending
another update.

2) If the engine receives an update from the client, it must sent the
resulting frame with the acknowledgement for this update. This is
consistent with the behaviour in the threaded compositor where the
main thread always responds with a BeginMainFrameAborted or commit
for deltas sent to the impl thread.

However, as opposed to the threaded compositor, the communication
between the engine and client is completely asynchronous. Enabling
this requires state tracking on the client to track the deltas
seen by the engine, when pushing the resulting frame update to
the impl thread, so the impl thread can apply those deltas
post-commit instead of expecting the main thread to reflect these
values. The change adds a SyncedPropertyRemote primitive, which acts
as the main thread equivalent of SyncedProperty, to perform this for
all state that can be modified on the engine or client.

BUG=648442
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc/blimp: Add synchronization for scroll/scale state.

Add the protocol messages and the primitives to synchronize mutations
to scroll and scale state on the engine and client. The protocol has
been kept largely consistent with the message passing between the main
and impl thread, as follows:

1) Both the engine and client can send an update to the other peer at
any time, but must wait for an ack from the other peer before sending
another update.

2) If the engine receives an update from the client, it must send the
resulting frame with the acknowledgement for this update. This is
consistent with the behaviour in the threaded compositor where the
main thread always responds with a BeginMainFrameAborted or commit
for deltas sent from the impl thread.

However, as opposed to the threaded compositor, the communication
between the engine and client is completely asynchronous. Enabling
this requires state tracking on the client to track the deltas
seen by the engine, when pushing the resulting frame update to
the impl thread, so the impl thread can apply those deltas
post-commit instead of expecting the main thread to reflect these
values. The change adds a SyncedPropertyRemote primitive, which acts
as the main thread equivalent of SyncedProperty, to perform this for
all state that can be modified on the engine or client.

BUG=648442
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-10-25 22:14:43]

Seems good I can't think of any obvious ways to do it differently. Have aelias
review it in detail tho, thanks for working on it!

https://codereview.chromium.org/2445093002/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2445093002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:344: // If the state was committed to the
pending tree, apply any reflected deltas
sync tree

https://codereview.chromium.org/2445093002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:345: // that were not applied on the main
thread.
applied during the main frame

https://codereview.chromium.org/2445093002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:347:
pending_tree()->ApplyReflectedMainFrameState();
Shouldn't it do this on the sync tree always?

[Khushal, 2016-10-25 22:44:33]

https://codereview.chromium.org/2445093002/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2445093002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:347:
pending_tree()->ApplyReflectedMainFrameState();
On 2016/10/25 22:14:43, danakj wrote:
> Shouldn't it do this on the sync tree always?

Hmmm, if I copy to the pending base here, I don't think it will work. Looking at
the example of page scale, we PushFromMainThread during commit, and when
committing to the active tree, PushPendingToActive in the same call. And since
this happens post commit, the value will never make it to the active tree in
that mode.

I could probably check in LayerTreeImpl when over-writing the value. If its the
active tree, add the delta to the active base, otherwise the pending base. How
does that sound?

[danakj, 2016-10-25 23:55:55]

https://codereview.chromium.org/2445093002/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2445093002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:347:
pending_tree()->ApplyReflectedMainFrameState();
On 2016/10/25 22:44:33, Khushal wrote:
> On 2016/10/25 22:14:43, danakj wrote:
> > Shouldn't it do this on the sync tree always?
> 
> Hmmm, if I copy to the pending base here, I don't think it will work. Looking
at
> the example of page scale, we PushFromMainThread during commit, and when
> committing to the active tree, PushPendingToActive in the same call. And since
> this happens post commit, the value will never make it to the active tree in
> that mode.
> 
> I could probably check in LayerTreeImpl when over-writing the value. If its
the
> active tree, add the delta to the active base, otherwise the pending base. How
> does that sound?

That sounds ok I think. If this sounds like complexity you won't use then maybe
this condition should be (!CommitToActive()) to show that intent more clearly
and DCHECK that otherwise its always empty unapplied stuff or something.

[Khushal, 2016-10-26 01:03:29]

https://codereview.chromium.org/2445093002/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2445093002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:344: // If the state was committed to the
pending tree, apply any reflected deltas
On 2016/10/25 22:14:43, danakj wrote:
> sync tree

Done.

https://codereview.chromium.org/2445093002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:345: // that were not applied on the main
thread.
On 2016/10/25 22:14:43, danakj wrote:
> applied during the main frame

Done.

https://codereview.chromium.org/2445093002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:347:
pending_tree()->ApplyReflectedMainFrameState();
On 2016/10/25 23:55:55, danakj wrote:
> On 2016/10/25 22:44:33, Khushal wrote:
> > On 2016/10/25 22:14:43, danakj wrote:
> > > Shouldn't it do this on the sync tree always?
> > 
> > Hmmm, if I copy to the pending base here, I don't think it will work.
Looking
> at
> > the example of page scale, we PushFromMainThread during commit, and when
> > committing to the active tree, PushPendingToActive in the same call. And
since
> > this happens post commit, the value will never make it to the active tree in
> > that mode.
> > 
> > I could probably check in LayerTreeImpl when over-writing the value. If its
> the
> > active tree, add the delta to the active base, otherwise the pending base.
How
> > does that sound?
> 
> That sounds ok I think. If this sounds like complexity you won't use then
maybe
> this condition should be (!CommitToActive()) to show that intent more clearly
> and DCHECK that otherwise its always empty unapplied stuff or something.

Its not that complex to do, but we won't use it. So I avoided it. Added some
DCHECKs to make sure only the pending tree gets this state.

[David Trainor- moved to gerrit, 2016-10-26 01:40:11]

lgtm % other reviewers.

[David Trainor- moved to gerrit, 2016-10-26 01:40:23]

notlgtm sorry wrong CL!

[David Trainor- moved to gerrit, 2016-10-26 01:42:09]

On 2016/10/26 01:40:23, David Trainor wrote:
> notlgtm sorry wrong CL!

not lgtm.  Turn red darn you message!  You know what I mean khushal >:/.

[aelias_OOO_until_Jul13, 2016-10-26 04:04:12]

https://codereview.chromium.org/2445093002/diff/60001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2445093002/diff/60001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:171: // TODO(khushalsagar): Fix
before landing, the code here assumes that every
Why are there extra commit, is this because the acks count as a commit?  Perhaps
they ought to be a separate message type that doesn't cause a CC commit per se? 
(This problem is actually the Blimp analogue of aborted commits...)

https://codereview.chromium.org/2445093002/diff/60001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:365: void
BlimpCompositor::SendClientStateUpdateIfPossible() {
I don't really think "IfPossible" method names give useful information, I'd
suggest just "FlushClientState"

https://codereview.chromium.org/2445093002/diff/60001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/2445093002/diff/60001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.h:247: bool client_state_modified_
= false;
"dirty" is the jargon for this, so I suggest "client_state_dirty_"

https://codereview.chromium.org/2445093002/diff/60001/blimp/engine/app/settin...
File blimp/engine/app/settings_manager.cc (right):

https://codereview.chromium.org/2445093002/diff/60001/blimp/engine/app/settin...
blimp/engine/app/settings_manager.cc:34: prefs->use_solid_color_scrollbars =
true;
Does this belong in this patch?

https://codereview.chromium.org/2445093002/diff/60001/cc/base/synced_property.h
File cc/base/synced_property.h (right):

https://codereview.chromium.org/2445093002/diff/60001/cc/base/synced_property...
cc/base/synced_property.h:60: void AddUnappliedDeltaToPendingBase(typename
T::ValueType unapplied_delta) {
Could this new method be avoided by applying the delta to the argument passed
into PushFromMainThread instead?

https://codereview.chromium.org/2445093002/diff/60001/cc/blimp/synced_propert...
File cc/blimp/synced_property_remote.h (right):

https://codereview.chromium.org/2445093002/diff/60001/cc/blimp/synced_propert...
cc/blimp/synced_property_remote.h:66:
DCHECK(sent_but_unapplied_delta_from_impl_thread_.get() ==
DCHECK_EQ

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

https://codereview.chromium.org/2445093002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:343: if (!CommitToActiveTree())
Can you DCHECK(!CommitToActiveTree()) inside the if
(reflected_main_frame_state_) {} block instead?

[Khushal, 2016-10-26 19:02:35]

https://codereview.chromium.org/2445093002/diff/60001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2445093002/diff/60001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:171: // TODO(khushalsagar): Fix
before landing, the code here assumes that every
On 2016/10/26 04:04:12, aelias wrote:
> Why are there extra commit, is this because the acks count as a commit? 
Perhaps
> they ought to be a separate message type that doesn't cause a CC commit per
se? 
> (This problem is actually the Blimp analogue of aborted commits...)

Yeah, its because what would have aborted commits in the general case, a state
update from the impl thread, now always leads to a commit because we have to
send unresolved deltas back. I don't think it should be that expensive though,
since we shouldn't have a tree sync, push properties will only be for layers
with scroll changes, and we shouldn't need to rebuild property trees either.

And I think the code here is using this outstanding_commits_ logic to know when
a frame received from the engine was drawn, so it can make a copy request on the
Layer after that.

May be the easier way to do this is to use SwapPromises?

https://codereview.chromium.org/2445093002/diff/60001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:365: void
BlimpCompositor::SendClientStateUpdateIfPossible() {
On 2016/10/26 04:04:12, aelias wrote:
> I don't really think "IfPossible" method names give useful information, I'd
> suggest just "FlushClientState"

Done.

https://codereview.chromium.org/2445093002/diff/60001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/2445093002/diff/60001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.h:247: bool client_state_modified_
= false;
On 2016/10/26 04:04:12, aelias wrote:
> "dirty" is the jargon for this, so I suggest "client_state_dirty_"

Done.

https://codereview.chromium.org/2445093002/diff/60001/blimp/engine/app/settin...
File blimp/engine/app/settings_manager.cc (right):

https://codereview.chromium.org/2445093002/diff/60001/blimp/engine/app/settin...
blimp/engine/app/settings_manager.cc:34: prefs->use_solid_color_scrollbars =
true;
On 2016/10/26 04:04:12, aelias wrote:
> Does this belong in this patch?

I ran into a lot of crashes because the engine would build
PaintedScrollbarLayers, which we explicitly don't support. This is intended but
I guess was missed in the change that added scrollbar support.

https://codereview.chromium.org/2445093002/diff/60001/cc/base/synced_property.h
File cc/base/synced_property.h (right):

https://codereview.chromium.org/2445093002/diff/60001/cc/base/synced_property...
cc/base/synced_property.h:60: void AddUnappliedDeltaToPendingBase(typename
T::ValueType unapplied_delta) {
On 2016/10/26 04:04:12, aelias wrote:
> Could this new method be avoided by applying the delta to the argument passed
> into PushFromMainThread instead?

It could, I'll just end up doing it at the calling sites using accessors.

But I actually like the fact that SyncedProperty is a nice self-contained class.
It felt like Pending/Active base were not really meant to be publicly accessible
and I just feel sad using them. 

I can change it if you prefer this logic on the outside instead.

https://codereview.chromium.org/2445093002/diff/60001/cc/blimp/synced_propert...
File cc/blimp/synced_property_remote.h (right):

https://codereview.chromium.org/2445093002/diff/60001/cc/blimp/synced_propert...
cc/blimp/synced_property_remote.h:66:
DCHECK(sent_but_unapplied_delta_from_impl_thread_.get() ==
On 2016/10/26 04:04:12, aelias wrote:
> DCHECK_EQ

The compiler yells at me if I use this with scroll offsets.

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

https://codereview.chromium.org/2445093002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:343: if (!CommitToActiveTree())
On 2016/10/26 04:04:12, aelias wrote:
> Can you DCHECK(!CommitToActiveTree()) inside the if
> (reflected_main_frame_state_) {} block instead?

I just DCHECKed !IsActiveTree() in that if block instead. That should be enough
right? Since in the CommitToActiveTree mode, there will be no pending tree.

[aelias_OOO_until_Jul13, 2016-10-27 02:09:55]

https://codereview.chromium.org/2445093002/diff/60001/blimp/client/core/compo...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2445093002/diff/60001/blimp/client/core/compo...
blimp/client/core/compositor/blimp_compositor.cc:171: // TODO(khushalsagar): Fix
before landing, the code here assumes that every
On 2016/10/26 at 19:02:35, Khushal wrote:
> On 2016/10/26 04:04:12, aelias wrote:
> > Why are there extra commit, is this because the acks count as a commit? 
Perhaps
> > they ought to be a separate message type that doesn't cause a CC commit per
se? 
> > (This problem is actually the Blimp analogue of aborted commits...)
> 
> Yeah, its because what would have aborted commits in the general case, a state
update from the impl thread, now always leads to a commit because we have to
send unresolved deltas back. I don't think it should be that expensive though,
since we shouldn't have a tree sync, push properties will only be for layers
with scroll changes, and we shouldn't need to rebuild property trees either.

Indeed, aborted commits are something of a microoptimization so I don't think
it's mandatory for Blimp to have them.  Your overall commit rate is likely an
order of magnitude lower than a local renderer anyway.

> 
> And I think the code here is using this outstanding_commits_ logic to know
when a frame received from the engine was drawn, so it can make a copy request
on the Layer after that.
> 
> May be the easier way to do this is to use SwapPromises?

Yes, SwapPromises sounds like a good fit for that.

https://codereview.chromium.org/2445093002/diff/60001/cc/base/synced_property.h
File cc/base/synced_property.h (right):

https://codereview.chromium.org/2445093002/diff/60001/cc/base/synced_property...
cc/base/synced_property.h:60: void AddUnappliedDeltaToPendingBase(typename
T::ValueType unapplied_delta) {
On 2016/10/26 at 19:02:35, Khushal wrote:
> On 2016/10/26 04:04:12, aelias wrote:
> > Could this new method be avoided by applying the delta to the argument
passed
> > into PushFromMainThread instead?
> 
> It could, I'll just end up doing it at the calling sites using accessors.
> 
> But I actually like the fact that SyncedProperty is a nice self-contained
class. It felt like Pending/Active base were not really meant to be publicly
accessible and I just feel sad using them. 
> 
> I can change it if you prefer this logic on the outside instead.

Right, I'd like to keep it self-contained and with a relatively simple data
flow, that's why I'd prefer to avoid introducing this method applying an ad hoc
mutation after the fact, if possible.  I'm not sure what you mean by "accessor",
but what I'm proposing is that you apply the mutation at an earlier time, to the
argument that gets passed into PushFromMainThread.

[Khushal, 2016-10-27 04:26:26]

https://codereview.chromium.org/2445093002/diff/60001/cc/base/synced_property.h
File cc/base/synced_property.h (right):

https://codereview.chromium.org/2445093002/diff/60001/cc/base/synced_property...
cc/base/synced_property.h:60: void AddUnappliedDeltaToPendingBase(typename
T::ValueType unapplied_delta) {
On 2016/10/27 02:09:55, aelias wrote:
> On 2016/10/26 at 19:02:35, Khushal wrote:
> > On 2016/10/26 04:04:12, aelias wrote:
> > > Could this new method be avoided by applying the delta to the argument
> passed
> > > into PushFromMainThread instead?
> > 
> > It could, I'll just end up doing it at the calling sites using accessors.
> > 
> > But I actually like the fact that SyncedProperty is a nice self-contained
> class. It felt like Pending/Active base were not really meant to be publicly
> accessible and I just feel sad using them. 
> > 
> > I can change it if you prefer this logic on the outside instead.
> 
> Right, I'd like to keep it self-contained and with a relatively simple data
> flow, that's why I'd prefer to avoid introducing this method applying an ad
hoc
> mutation after the fact, if possible.  I'm not sure what you mean by
"accessor",
> but what I'm proposing is that you apply the mutation at an earlier time, to
the
> argument that gets passed into PushFromMainThread.

Oh, I see what you mean now.

The place where PushFromMainThread happens for pushing this value to the pending
tree is here,
https://cs.chromium.org/chromium/src/cc/trees/property_tree.cc?rcl=1477524050....

The easiest way I can think of doing this there is to send a map of layer id to
delta in the reflected state instead. And query for the delta for a Layer on the
LayerTreeImpl when performing the Push.
To be honest that seems like more plumbing to me. And I think the place I'm
doing it at right now keeps it much more isolated from the code that we
generally run for state sync during the commit.
Which one do you prefer?

[Khushal, 2016-10-27 04:38:32]

https://codereview.chromium.org/2445093002/diff/60001/cc/base/synced_property.h
File cc/base/synced_property.h (right):

https://codereview.chromium.org/2445093002/diff/60001/cc/base/synced_property...
cc/base/synced_property.h:60: void AddUnappliedDeltaToPendingBase(typename
T::ValueType unapplied_delta) {
On 2016/10/27 04:26:26, Khushal wrote:
> On 2016/10/27 02:09:55, aelias wrote:
> > On 2016/10/26 at 19:02:35, Khushal wrote:
> > > On 2016/10/26 04:04:12, aelias wrote:
> > > > Could this new method be avoided by applying the delta to the argument
> > passed
> > > > into PushFromMainThread instead?
> > > 
> > > It could, I'll just end up doing it at the calling sites using accessors.
> > > 
> > > But I actually like the fact that SyncedProperty is a nice self-contained
> > class. It felt like Pending/Active base were not really meant to be publicly
> > accessible and I just feel sad using them. 
> > > 
> > > I can change it if you prefer this logic on the outside instead.
> > 
> > Right, I'd like to keep it self-contained and with a relatively simple data
> > flow, that's why I'd prefer to avoid introducing this method applying an ad
> hoc
> > mutation after the fact, if possible.  I'm not sure what you mean by
> "accessor",
> > but what I'm proposing is that you apply the mutation at an earlier time, to
> the
> > argument that gets passed into PushFromMainThread.
> 
> Oh, I see what you mean now.
> 
> The place where PushFromMainThread happens for pushing this value to the
pending
> tree is here,
>
https://cs.chromium.org/chromium/src/cc/trees/property_tree.cc?rcl=1477524050....
> 
> The easiest way I can think of doing this there is to send a map of layer id
to
> delta in the reflected state instead. And query for the delta for a Layer on
the
> LayerTreeImpl when performing the Push.
> To be honest that seems like more plumbing to me. And I think the place I'm
> doing it at right now keeps it much more isolated from the code that we
> generally run for state sync during the commit.
> Which one do you prefer?

Or, may be this can be done here, right before
sync_tree->UpdatePropertyTreeScrollOffset instead,
https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_in_process.cc?p....

This will be exactly before the scroll offsets are pushed to the pending tree.

[aelias_OOO_until_Jul13, 2016-10-27 07:01:31]

https://codereview.chromium.org/2445093002/diff/60001/cc/base/synced_property.h
File cc/base/synced_property.h (right):

https://codereview.chromium.org/2445093002/diff/60001/cc/base/synced_property...
cc/base/synced_property.h:60: void AddUnappliedDeltaToPendingBase(typename
T::ValueType unapplied_delta) {
How about registering a did_scroll_callback that calls
cc::Layer::SetScrollOffset() with the appropriate counterscroll (similarly in 
CompositorStateDeserializer::ApplyViewportDeltas for page scale if needed)? 
That could avoid CC changes entirely I think?

Alternatively, if that doesn't work because I'm missing some nuance, I'd
consider resurrecting https://codereview.chromium.org/1385123003
scrollCompensationAdjustment.  That CC feature was small and super targeted to
the fixed-position element problem, and probably easier to reason about than yet
another layer of synced_property reflection, which is becoming a flow too
complex to keep in mind all at once.

[aelias_OOO_until_Jul13, 2016-10-27 08:30:58]

I'm also looking into how to rebaseline the fixed-pos elements to perma-(0,0)
which as I've mentioned is the proper fix.   I think it's actually really easy
if we ignore the case of non-composited fixed-pos elements (which are extremely
rare on high-dpi, would look completely broken on Blimp anyway, and will go away
eventually with slimming paint v2).  I think it would just involve the
following:

1) Always set offsetForFixedPosition to 0 in LayoutView:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...

2) In TransformTree::UpdateLocalTransform, make sure the fixed-position elements
are repositioned appropriately given the new coordinate space:
https://cs.chromium.org/chromium/src/cc/trees/property_tree.cc?dr=C&q=propert...
(basically where the scroll_snap code I linked above moved to).

Maybe that behavior could be put behind a setting for now to avoid affecting
lo-dpi platforms which have the non-composited ones.  And I might reuse the
setting to turn on fractional scroll offsets on high-dpi (the original reason
why I explored this idea).  Could you give that a try and see if you get correct
output?  Tien-Ren can probably help with the details.

[Khushal, 2016-10-28 01:37:34]

On 2016/10/27 08:30:58, aelias wrote:
> I'm also looking into how to rebaseline the fixed-pos elements to perma-(0,0)
> which as I've mentioned is the proper fix.   I think it's actually really easy
> if we ignore the case of non-composited fixed-pos elements (which are
extremely
> rare on high-dpi, would look completely broken on Blimp anyway, and will go
away
> eventually with slimming paint v2).  I think it would just involve the
> following:
> 
> 1) Always set offsetForFixedPosition to 0 in LayoutView:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
> 
> 2) In TransformTree::UpdateLocalTransform, make sure the fixed-position
elements
> are repositioned appropriately given the new coordinate space:
>
https://cs.chromium.org/chromium/src/cc/trees/property_tree.cc?dr=C&q=propert...
> (basically where the scroll_snap code I linked above moved to).
> 
> Maybe that behavior could be put behind a setting for now to avoid affecting
> lo-dpi platforms which have the non-composited ones.  And I might reuse the
> setting to turn on fractional scroll offsets on high-dpi (the original reason
> why I explored this idea).  Could you give that a try and see if you get
correct
> output?  Tien-Ren can probably help with the details.

I changed the point at which the deltas are applied to be at push time, so we
don't need to make any changes to SyncedProperty, that you suggested earlier.
And also added a TODO to investigate removing this with SPV2. If blink is
generating PropertyTrees, then we should be able to simply reflect the scroll
offset I think. +ajuma for taking a look at the PropertyTrees part also.

[Khushal, 2016-10-28 01:37:51]

khushalsagar@chromium.org changed reviewers:
+ ajuma@chromium.org

[Khushal, 2016-10-28 01:38:34]

Also, sorry the patch got rather large. This one also includes the tests.

[aelias_OOO_until_Jul13, 2016-10-28 03:40:20]

OK, as discussed offline, I'm satisfied this general approach is the best we can
do before we get both SPv2 and unified hit testing.

https://codereview.chromium.org/2445093002/diff/120001/blimp/client/core/comp...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.cc:171: // TODO(khushalsagar): Fix
before landing, the code here assumes that every
Could you also upload this fix for code review since it seems like it'll be
nontrivial?  Might make sense to split into separate patch for landing before
this one moving the readback to use SwapPromise, since I think it's isolatable.

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
File cc/blimp/compositor_state_deserializer.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
cc/blimp/compositor_state_deserializer.cc:145: gfx::Vector2d
scroll_delta_vector(scroll_offset_delta.x(),
gfx::Vector2dF

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
cc/blimp/compositor_state_deserializer.cc:190: gfx::Vector2d
scroll_delta_vector(scroll_offset_delta.x(),
gfx::Vector2dF

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
File cc/blimp/compositor_state_deserializer_unittest.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
cc/blimp/compositor_state_deserializer_unittest.cc:394:
scroll_update.scroll_delta = gfx::Vector2d(delta.x(), delta.y());
gfx::Vector2dF

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

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:424: gfx::Vector2d inner_viewport_delta;
gfx::Vector2dF

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:430: gfx::Vector2d scroll_delta =
ProtoToVector2d(scroll_update.scroll_delta());
gfx::Vector2dF

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/layer_tree_ho...
File cc/blimp/layer_tree_host_remote_unittest.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote_unittest.cc:42: gfx::Vector2d
SerializeScrollUpdate(
gfx::Vector2dF

https://codereview.chromium.org/2445093002/diff/120001/cc/trees/layer_tree.cc
File cc/trees/layer_tree.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/trees/layer_tree.cc...
cc/trees/layer_tree.cc:405: inputs_.page_scale_factor *
unapplied_page_scale_delta,
Hmm, this resembles the approach I suggested for scrolling which you argued is
incorrect because it ends up applying to the transform, isn't that also the case
here?

https://codereview.chromium.org/2445093002/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_in_process.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_in_process.cc:482: gfx::Vector2d scroll_delta =
scroll_update.scroll_delta;
gfx::Vector2dF

https://codereview.chromium.org/2445093002/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_scroll.cc:2174:
gfx::Vector2d(outer_viewport_offset_deltas_[delta_to_reflect].x(),
gfx::Vector2dF

[Khushal, 2016-10-28 04:11:17]

https://codereview.chromium.org/2445093002/diff/120001/blimp/client/core/comp...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.cc:171: // TODO(khushalsagar): Fix
before landing, the code here assumes that every
On 2016/10/28 03:40:19, aelias wrote:
> Could you also upload this fix for code review since it seems like it'll be
> nontrivial?  Might make sense to split into separate patch for landing before
> this one moving the readback to use SwapPromise, since I think it's
isolatable.

Actually the problem is interesting here. If we want to use SwapPromises for
this, it has to be using a commit to push them to impl, which is easy for the
new code path where we use a threaded compositor on the client. You can have a
local commit at any time.

But the old code keeps the split point in the middle of Proxy, the main thread
on the client is as good as dead, and you can't do local commits. They land
here: https://cs.chromium.org/chromium/src/cc/trees/remote_channel_impl.cc?l=190
One of the reasons being that if a main frame request had been sent to the
server, the next commit has to include resolution of deltas. :P

So I think one way to address is to structure the BlimpCompositor API correctly
so the call to BlimpCompositor specifies that it is a copy request for the
current main frame. And then keep this outstanding_commits_ logic for the old
path, and use SwapPromises for the updated code path.

Doing that in a separate patch sounds like a good idea.

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
File cc/blimp/compositor_state_deserializer_unittest.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
cc/blimp/compositor_state_deserializer_unittest.cc:394:
scroll_update.scroll_delta = gfx::Vector2d(delta.x(), delta.y());
On 2016/10/28 03:40:20, aelias wrote:
> gfx::Vector2dF

cc still uses Vector2d for sending updates from impl? And that's the struct I
ended up using for sending the reflected deltas back as well...

[Khushal, 2016-10-28 07:43:56]

https://codereview.chromium.org/2445093002/diff/120001/blimp/client/core/comp...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.cc:171: // TODO(khushalsagar): Fix
before landing, the code here assumes that every
On 2016/10/28 04:11:17, Khushal wrote:
> On 2016/10/28 03:40:19, aelias wrote:
> > Could you also upload this fix for code review since it seems like it'll be
> > nontrivial?  Might make sense to split into separate patch for landing
before
> > this one moving the readback to use SwapPromise, since I think it's
> isolatable.
> 
> Actually the problem is interesting here. If we want to use SwapPromises for
> this, it has to be using a commit to push them to impl, which is easy for the
> new code path where we use a threaded compositor on the client. You can have a
> local commit at any time.
> 
> But the old code keeps the split point in the middle of Proxy, the main thread
> on the client is as good as dead, and you can't do local commits. They land
> here:
https://cs.chromium.org/chromium/src/cc/trees/remote_channel_impl.cc?l=190
> One of the reasons being that if a main frame request had been sent to the
> server, the next commit has to include resolution of deltas. :P
> 
> So I think one way to address is to structure the BlimpCompositor API
correctly
> so the call to BlimpCompositor specifies that it is a copy request for the
> current main frame. And then keep this outstanding_commits_ logic for the old
> path, and use SwapPromises for the updated code path.
> 
> Doing that in a separate patch sounds like a good idea.

I have uploaded a patch for fixing this separately.
https://codereview.chromium.org/2459823003/

[ajuma, 2016-10-28 16:58:24]

On 2016/10/28 01:37:34, Khushal wrote:
> On 2016/10/27 08:30:58, aelias wrote:
> > I'm also looking into how to rebaseline the fixed-pos elements to
perma-(0,0)
> > which as I've mentioned is the proper fix.   I think it's actually really
easy
> > if we ignore the case of non-composited fixed-pos elements (which are
> extremely
> > rare on high-dpi, would look completely broken on Blimp anyway, and will go
> away
> > eventually with slimming paint v2).  I think it would just involve the
> > following:
> > 
> > 1) Always set offsetForFixedPosition to 0 in LayoutView:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
> > 
> > 2) In TransformTree::UpdateLocalTransform, make sure the fixed-position
> elements
> > are repositioned appropriately given the new coordinate space:
> >
>
https://cs.chromium.org/chromium/src/cc/trees/property_tree.cc?dr=C&q=propert...
> > (basically where the scroll_snap code I linked above moved to).
> > 
> > Maybe that behavior could be put behind a setting for now to avoid affecting
> > lo-dpi platforms which have the non-composited ones.  And I might reuse the
> > setting to turn on fractional scroll offsets on high-dpi (the original
reason
> > why I explored this idea).  Could you give that a try and see if you get
> correct
> > output?  Tien-Ren can probably help with the details.
> 
> I changed the point at which the deltas are applied to be at push time, so we
> don't need to make any changes to SyncedProperty, that you suggested earlier.
> And also added a TODO to investigate removing this with SPV2. If blink is
> generating PropertyTrees, then we should be able to simply reflect the scroll
> offset I think. +ajuma for taking a look at the PropertyTrees part also.

The PropertyTrees stuff in LayerTreeHostInProcess::FinishCommitOnImplThread
lgtm. Are there other property tree related changes I should look at?

[Khushal, 2016-10-28 21:52:41]

On 2016/10/28 16:58:24, ajuma wrote:
> On 2016/10/28 01:37:34, Khushal wrote:
> > On 2016/10/27 08:30:58, aelias wrote:
> > > I'm also looking into how to rebaseline the fixed-pos elements to
> perma-(0,0)
> > > which as I've mentioned is the proper fix.   I think it's actually really
> easy
> > > if we ignore the case of non-composited fixed-pos elements (which are
> > extremely
> > > rare on high-dpi, would look completely broken on Blimp anyway, and will
go
> > away
> > > eventually with slimming paint v2).  I think it would just involve the
> > > following:
> > > 
> > > 1) Always set offsetForFixedPosition to 0 in LayoutView:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
> > > 
> > > 2) In TransformTree::UpdateLocalTransform, make sure the fixed-position
> > elements
> > > are repositioned appropriately given the new coordinate space:
> > >
> >
>
https://cs.chromium.org/chromium/src/cc/trees/property_tree.cc?dr=C&q=propert...
> > > (basically where the scroll_snap code I linked above moved to).
> > > 
> > > Maybe that behavior could be put behind a setting for now to avoid
affecting
> > > lo-dpi platforms which have the non-composited ones.  And I might reuse
the
> > > setting to turn on fractional scroll offsets on high-dpi (the original
> reason
> > > why I explored this idea).  Could you give that a try and see if you get
> > correct
> > > output?  Tien-Ren can probably help with the details.
> > 
> > I changed the point at which the deltas are applied to be at push time, so
we
> > don't need to make any changes to SyncedProperty, that you suggested
earlier.
> > And also added a TODO to investigate removing this with SPV2. If blink is
> > generating PropertyTrees, then we should be able to simply reflect the
scroll
> > offset I think. +ajuma for taking a look at the PropertyTrees part also.
> 
> The PropertyTrees stuff in LayerTreeHostInProcess::FinishCommitOnImplThread
> lgtm. Are there other property tree related changes I should look at?

Nope, that was it. Thanks! :)

[aelias_OOO_until_Jul13, 2016-10-29 02:35:53]

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
File cc/blimp/compositor_state_deserializer_unittest.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
cc/blimp/compositor_state_deserializer_unittest.cc:394:
scroll_update.scroll_delta = gfx::Vector2d(delta.x(), delta.y());
On 2016/10/28 at 04:11:17, Khushal wrote:
> On 2016/10/28 03:40:20, aelias wrote:
> > gfx::Vector2dF
> 
> cc still uses Vector2d for sending updates from impl? And that's the struct I
ended up using for sending the reflected deltas back as well...

That's a loose end we shouldn't propagate further, please use Vector2dF
everywhere and put in an explicit floor() call somewhere if the truncation is
actually needed.

[kuzminruslan, 2016-10-30 13:18:40]

kuzminruslan@icloud.com changed reviewers:
+ kuzminruslan@icloud.com

[kuzminruslan, 2016-10-30 13:18:41]

https://codereview.chromium.org/2445093002/diff/160001/blimp/client/core/comp...
File blimp/client/core/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/2445093002/diff/160001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.h:113: 
perhaps forgotten private

https://codereview.chromium.org/2445093002/diff/160001/cc/blimp/compositor_st...
File cc/blimp/compositor_state_deserializer.h (right):

https://codereview.chromium.org/2445093002/diff/160001/cc/blimp/compositor_st...
cc/blimp/compositor_state_deserializer.h:47: using ScrollCallback =
base::Callback<void(int engine_layer_id)>;
Perhaps not needed

https://codereview.chromium.org/2445093002/diff/160001/cc/test/layer_tree_hos...
File cc/test/layer_tree_host_remote_for_testing.h (right):

https://codereview.chromium.org/2445093002/diff/160001/cc/test/layer_tree_hos...
cc/test/layer_tree_host_remote_for_testing.h:11: #include
"ui/gfx/geometry/scroll_offset.h"
Perhaps not needed header

[Khushal, 2016-10-31 21:02:01]

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
File cc/blimp/compositor_state_deserializer.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
cc/blimp/compositor_state_deserializer.cc:145: gfx::Vector2d
scroll_delta_vector(scroll_offset_delta.x(),
On 2016/10/28 03:40:20, aelias wrote:
> gfx::Vector2dF

Done.

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
cc/blimp/compositor_state_deserializer.cc:190: gfx::Vector2d
scroll_delta_vector(scroll_offset_delta.x(),
On 2016/10/28 03:40:19, aelias wrote:
> gfx::Vector2dF

Done.

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
File cc/blimp/compositor_state_deserializer_unittest.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/compositor_st...
cc/blimp/compositor_state_deserializer_unittest.cc:394:
scroll_update.scroll_delta = gfx::Vector2d(delta.x(), delta.y());
On 2016/10/29 02:35:53, aelias wrote:
> On 2016/10/28 at 04:11:17, Khushal wrote:
> > On 2016/10/28 03:40:20, aelias wrote:
> > > gfx::Vector2dF
> > 
> > cc still uses Vector2d for sending updates from impl? And that's the struct
I
> ended up using for sending the reflected deltas back as well...
> 
> That's a loose end we shouldn't propagate further, please use Vector2dF
> everywhere and put in an explicit floor() call somewhere if the truncation is
> actually needed.

Done.

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

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:424: gfx::Vector2d inner_viewport_delta;
On 2016/10/28 03:40:20, aelias wrote:
> gfx::Vector2dF

Done.

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.cc:430: gfx::Vector2d scroll_delta =
ProtoToVector2d(scroll_update.scroll_delta());
On 2016/10/28 03:40:20, aelias wrote:
> gfx::Vector2dF

Done.

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/layer_tree_ho...
File cc/blimp/layer_tree_host_remote_unittest.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote_unittest.cc:42: gfx::Vector2d
SerializeScrollUpdate(
On 2016/10/28 03:40:20, aelias wrote:
> gfx::Vector2dF

Done.

https://codereview.chromium.org/2445093002/diff/120001/cc/trees/layer_tree.cc
File cc/trees/layer_tree.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/trees/layer_tree.cc...
cc/trees/layer_tree.cc:405: inputs_.page_scale_factor *
unapplied_page_scale_delta,
On 2016/10/28 03:40:20, aelias wrote:
> Hmm, this resembles the approach I suggested for scrolling which you argued is
> incorrect because it ends up applying to the transform, isn't that also the
case
> here?

I did a little bit more digging, and finally landed onto exactly what's
different. Its basically how the layer tree hierarchy is mapped to property
trees and the way this is handled differently on the main thread,
https://cs.chromium.org/chromium/src/cc/trees/property_tree.h?l=208

From what I understand, in PropertyTrees, the |source_node_id| is not the same
as the transform parent id for fixed position layers. The source to parent
offset is set when updating the property tree transforms on the main thread side
to account for the layer's position, but this is disabled on the compositor
thread since no compositor thread effects would change this.

This change is working with this by updating the offsets with the deltas on the
compositor thread side property trees, which is necessary because for the scroll
offsets blink has seen, it will re-position the layers to account for them, but
the additional delta accumulated on the client main thread now has to be applied
the same way deltas are applied on the compositor thread. I guess with Spv2 if
blink is generating property trees, they should set the same hierarchy and
update them the same way for fixed position elements, the way cc does?
+ajuma since he has a lot more context on this too.

https://codereview.chromium.org/2445093002/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_in_process.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_in_process.cc:482: gfx::Vector2d scroll_delta =
scroll_update.scroll_delta;
On 2016/10/28 03:40:20, aelias wrote:
> gfx::Vector2dF

Done.

https://codereview.chromium.org/2445093002/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/2445093002/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_scroll.cc:2174:
gfx::Vector2d(outer_viewport_offset_deltas_[delta_to_reflect].x(),
On 2016/10/28 03:40:20, aelias wrote:
> gfx::Vector2dF

Done.

[Khushal, 2016-10-31 23:00:27]

https://codereview.chromium.org/2445093002/diff/160001/blimp/client/core/comp...
File blimp/client/core/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/2445093002/diff/160001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.h:113: 
On 2016/10/30 13:18:40, kuzminruslan wrote:
> perhaps forgotten private

I'm actually making it public because the tests need to call them.

https://codereview.chromium.org/2445093002/diff/160001/cc/blimp/compositor_st...
File cc/blimp/compositor_state_deserializer.h (right):

https://codereview.chromium.org/2445093002/diff/160001/cc/blimp/compositor_st...
cc/blimp/compositor_state_deserializer.h:47: using ScrollCallback =
base::Callback<void(int engine_layer_id)>;
On 2016/10/30 13:18:41, kuzminruslan wrote:
> Perhaps not needed

Thanks! Done.

https://codereview.chromium.org/2445093002/diff/160001/cc/test/layer_tree_hos...
File cc/test/layer_tree_host_remote_for_testing.h (right):

https://codereview.chromium.org/2445093002/diff/160001/cc/test/layer_tree_hos...
cc/test/layer_tree_host_remote_for_testing.h:11: #include
"ui/gfx/geometry/scroll_offset.h"
On 2016/10/30 13:18:41, kuzminruslan wrote:
> Perhaps not needed header

Done.

[Khushal, 2016-10-31 23:29:54]

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

[commit-bot: I haz the power, 2016-10-31 23:30:37]

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

[commit-bot: I haz the power, 2016-10-31 23:34:57]

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

[commit-bot: I haz the power, 2016-10-31 23:34:59]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Khushal, 2016-11-02 04:05:28]

friendly ping. :)

[aelias_OOO_until_Jul13, 2016-11-02 07:06:08]

Sorry for the delay in review, I've been oversubscribed this week.  lgtm for cc/
directory, but I haven't examined cc/blimp/ and blimp/ again -- over to
dtrainor@ for reviewing that part.

[Khushal, 2016-11-02 07:38:30]

On 2016/11/02 07:06:08, aelias wrote:
> Sorry for the delay in review, I've been oversubscribed this week.  lgtm for
cc/
> directory, but I haven't examined cc/blimp/ and blimp/ again -- over to
> dtrainor@ for reviewing that part.

Thanks. :)

[David Trainor- moved to gerrit, 2016-11-03 05:34:59]

lgtm.  thanks khushal!

https://codereview.chromium.org/2445093002/diff/240001/blimp/client/core/comp...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2445093002/diff/240001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.cc:492:
message.set_frame_ack(false);
Should we just not set this if we're not ack'ing the frame?

https://codereview.chromium.org/2445093002/diff/240001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_remote_compositor_bridge.cc (right):

https://codereview.chromium.org/2445093002/diff/240001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_remote_compositor_bridge.cc:25:
remote_proto_channel_->SetProtoReceiver(nullptr);
Nice catch.

[Khushal, 2016-11-03 06:17:28]

Thanks David!

https://codereview.chromium.org/2445093002/diff/240001/blimp/client/core/comp...
File blimp/client/core/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/2445093002/diff/240001/blimp/client/core/comp...
blimp/client/core/compositor/blimp_compositor.cc:492:
message.set_frame_ack(false);
On 2016/11/03 05:34:59, David Trainor wrote:
> Should we just not set this if we're not ack'ing the frame?

I guess we could just do a has_frame_ack instead. But these protos are far from
what the final version is going to look like anyway. So fine for now.

https://codereview.chromium.org/2445093002/diff/240001/blimp/engine/renderer/...
File blimp/engine/renderer/blimp_remote_compositor_bridge.cc (right):

https://codereview.chromium.org/2445093002/diff/240001/blimp/engine/renderer/...
blimp/engine/renderer/blimp_remote_compositor_bridge.cc:25:
remote_proto_channel_->SetProtoReceiver(nullptr);
On 2016/11/03 05:34:59, David Trainor wrote:
> Nice catch.

I'm the one who had missed it earlier. :P There is a test also now.

[Khushal, 2016-11-03 19:56:43]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-11-03 19:56:44]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
aelias@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2445093002/#ps260001
(title: "fix document manager test")

[commit-bot: I haz the power, 2016-11-03 19:57:50]

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

[commit-bot: I haz the power, 2016-11-03 20:03:49]

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

[commit-bot: I haz the power, 2016-11-03 20:03:51]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Khushal, 2016-11-03 22:06:59]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-11-03 22:07:00]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
dtrainor@chromium.org, ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2445093002/#ps280001
(title: "gn check")

[commit-bot: I haz the power, 2016-11-03 22:08:15]

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

[commit-bot: I haz the power, 2016-11-03 22:53:32]

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

[commit-bot: I haz the power, 2016-11-03 22:53:34]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Khushal, 2016-11-04 01:46:25]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-11-04 01:46:26]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
dtrainor@chromium.org, ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2445093002/#ps300001
(title: "forgot include in test")

[commit-bot: I haz the power, 2016-11-04 01:47:36]

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

[commit-bot: I haz the power, 2016-11-04 02:16:31]

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

[commit-bot: I haz the power, 2016-11-04 02:16:33]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Khushal, 2016-11-04 02:33:46]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-11-04 02:33:46]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
dtrainor@chromium.org, ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2445093002/#ps320001
(title: "test compile")

[commit-bot: I haz the power, 2016-11-04 02:34:41]

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

[commit-bot: I haz the power, 2016-11-04 05:53:45]

Description was changed from

==========
cc/blimp: Add synchronization for scroll/scale state.

Add the protocol messages and the primitives to synchronize mutations
to scroll and scale state on the engine and client. The protocol has
been kept largely consistent with the message passing between the main
and impl thread, as follows:

1) Both the engine and client can send an update to the other peer at
any time, but must wait for an ack from the other peer before sending
another update.

2) If the engine receives an update from the client, it must send the
resulting frame with the acknowledgement for this update. This is
consistent with the behaviour in the threaded compositor where the
main thread always responds with a BeginMainFrameAborted or commit
for deltas sent from the impl thread.

However, as opposed to the threaded compositor, the communication
between the engine and client is completely asynchronous. Enabling
this requires state tracking on the client to track the deltas
seen by the engine, when pushing the resulting frame update to
the impl thread, so the impl thread can apply those deltas
post-commit instead of expecting the main thread to reflect these
values. The change adds a SyncedPropertyRemote primitive, which acts
as the main thread equivalent of SyncedProperty, to perform this for
all state that can be modified on the engine or client.

BUG=648442
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc/blimp: Add synchronization for scroll/scale state.

Add the protocol messages and the primitives to synchronize mutations
to scroll and scale state on the engine and client. The protocol has
been kept largely consistent with the message passing between the main
and impl thread, as follows:

1) Both the engine and client can send an update to the other peer at
any time, but must wait for an ack from the other peer before sending
another update.

2) If the engine receives an update from the client, it must send the
resulting frame with the acknowledgement for this update. This is
consistent with the behaviour in the threaded compositor where the
main thread always responds with a BeginMainFrameAborted or commit
for deltas sent from the impl thread.

However, as opposed to the threaded compositor, the communication
between the engine and client is completely asynchronous. Enabling
this requires state tracking on the client to track the deltas
seen by the engine, when pushing the resulting frame update to
the impl thread, so the impl thread can apply those deltas
post-commit instead of expecting the main thread to reflect these
values. The change adds a SyncedPropertyRemote primitive, which acts
as the main thread equivalent of SyncedProperty, to perform this for
all state that can be modified on the engine or client.

BUG=648442
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-11-04 05:53:50]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-11-04 05:56:39]

Description was changed from

==========
cc/blimp: Add synchronization for scroll/scale state.

Add the protocol messages and the primitives to synchronize mutations
to scroll and scale state on the engine and client. The protocol has
been kept largely consistent with the message passing between the main
and impl thread, as follows:

1) Both the engine and client can send an update to the other peer at
any time, but must wait for an ack from the other peer before sending
another update.

2) If the engine receives an update from the client, it must send the
resulting frame with the acknowledgement for this update. This is
consistent with the behaviour in the threaded compositor where the
main thread always responds with a BeginMainFrameAborted or commit
for deltas sent from the impl thread.

However, as opposed to the threaded compositor, the communication
between the engine and client is completely asynchronous. Enabling
this requires state tracking on the client to track the deltas
seen by the engine, when pushing the resulting frame update to
the impl thread, so the impl thread can apply those deltas
post-commit instead of expecting the main thread to reflect these
values. The change adds a SyncedPropertyRemote primitive, which acts
as the main thread equivalent of SyncedProperty, to perform this for
all state that can be modified on the engine or client.

BUG=648442
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc/blimp: Add synchronization for scroll/scale state.

Add the protocol messages and the primitives to synchronize mutations
to scroll and scale state on the engine and client. The protocol has
been kept largely consistent with the message passing between the main
and impl thread, as follows:

1) Both the engine and client can send an update to the other peer at
any time, but must wait for an ack from the other peer before sending
another update.

2) If the engine receives an update from the client, it must send the
resulting frame with the acknowledgement for this update. This is
consistent with the behaviour in the threaded compositor where the
main thread always responds with a BeginMainFrameAborted or commit
for deltas sent from the impl thread.

However, as opposed to the threaded compositor, the communication
between the engine and client is completely asynchronous. Enabling
this requires state tracking on the client to track the deltas
seen by the engine, when pushing the resulting frame update to
the impl thread, so the impl thread can apply those deltas
post-commit instead of expecting the main thread to reflect these
values. The change adds a SyncedPropertyRemote primitive, which acts
as the main thread equivalent of SyncedProperty, to perform this for
all state that can be modified on the engine or client.

BUG=648442
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/c48291b306dcaf99e5bf33451a30f9ac4fc7ab0f
Cr-Commit-Position: refs/heads/master@{#429801}
==========

[commit-bot: I haz the power, 2016-11-04 05:56:40]

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