cc: Don't create SyncedPropety instances on the main thread.

cc: Don't create SyncedPropety instances on the main thread.

SyncedProperty is used as a synchronization primitive to track state on
the impl thread, necessary to resolve conflicts for concurrent main-impl
mutations. As such it should only exist on the impl thread.

Currently, the ScrollTree creates these on the main thread while they
only store the current main thread value. This is unnecessary complexity
that can be avoided. This change makes ScrollTree store just the offset
on the main thread, and a SyncedProperty for the offset on the impl thread.

BUG=662619
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2621403003
Cr-Commit-Position: refs/heads/master@{#443325}
Committed: https://chromium.googlesource.com/chromium/src/+/5889b84078b874e7682a4634b1fce6eedcac67fb

[Khushal, 2017-01-11 19:44:47]

Description was changed from

==========
cc: Don't create SyncedPropety instances on the main thread.

SyncedProperty is used as a synchronization primitive to track state on
the impl thread, necessary to resolve conflicts for concurrent main-impl
mutations. As such it should only exist on the impl thread.

Currently, the ScrollTree creates these on the main thread while they
only store the current main thread value. This is unnecessary complexity
that can be avoided. This change makes ScrollTree store just the offset
on the main thread, and a SyncedProperty for the offset on the impl thread.

BUG=662619
==========

to

==========
cc: Don't create SyncedPropety instances on the main thread.

SyncedProperty is used as a synchronization primitive to track state on
the impl thread, necessary to resolve conflicts for concurrent main-impl
mutations. As such it should only exist on the impl thread.

Currently, the ScrollTree creates these on the main thread while they
only store the current main thread value. This is unnecessary complexity
that can be avoided. This change makes ScrollTree store just the offset
on the main thread, and a SyncedProperty for the offset on the impl thread.

BUG=662619
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Khushal, 2017-01-11 19:47:38]

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

[Khushal, 2017-01-11 19:47:39]

I think some comments still need to be updated but otherwise does this look
better?

[Khushal, 2017-01-11 21:54:56]

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

[commit-bot: I haz the power, 2017-01-11 21:55:39]

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

[ajuma, 2017-01-11 22:35:51]

Nice work, this looks great!

https://codereview.chromium.org/2621403003/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2621403003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1366: // destroyed on the main thread.
More precisely, destroyed or became non-scrollable

https://codereview.chromium.org/2621403003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1435: return true;
Should this first check if the value changed?

https://codereview.chromium.org/2621403003/diff/1/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2621403003/diff/1/cc/trees/property_tree.h#ne...
cc/trees/property_tree.h:384: // value from the LayerTree while on the impl
thread this is the current value
nit: "for the LayerTree" might be clearer that we're not actually calling into
the LayerTree to get the value

https://codereview.chromium.org/2621403003/diff/1/cc/trees/property_tree.h#ne...
cc/trees/property_tree.h:429: const SyncedScrollOffset* synced_scroll_offset(int
layer_id) const;
I know you're just moving this code in this CL, but it should really be
something like GetSyncedScrollOffset since it's not a trivial getter. But since
this isn't new, feel free to leave it for a follow-up CL.

[Khushal, 2017-01-11 23:04:53]

Thanks Ali. Just one question about why the updates are done as a step after
synchronizing PropertyTrees.

https://codereview.chromium.org/2621403003/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2621403003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1366: // destroyed on the main thread.
On 2017/01/11 22:35:51, ajuma wrote:
> More precisely, destroyed or became non-scrollable

Done.

https://codereview.chromium.org/2621403003/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:1435: return true;
On 2017/01/11 22:35:51, ajuma wrote:
> Should this first check if the value changed?

We don't use the result on the main thread side, on impl its needed for an
update to draw properties. But it would be better to return based on the initial
value. Done.

https://codereview.chromium.org/2621403003/diff/1/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2621403003/diff/1/cc/trees/property_tree.h#ne...
cc/trees/property_tree.h:384: // value from the LayerTree while on the impl
thread this is the current value
On 2017/01/11 22:35:51, ajuma wrote:
> nit: "for the LayerTree" might be clearer that we're not actually calling into
> the LayerTree to get the value

Makes sense. Done.

https://codereview.chromium.org/2621403003/diff/1/cc/trees/property_tree.h#ne...
cc/trees/property_tree.h:429: const SyncedScrollOffset* synced_scroll_offset(int
layer_id) const;
On 2017/01/11 22:35:51, ajuma wrote:
> I know you're just moving this code in this CL, but it should really be
> something like GetSyncedScrollOffset since it's not a trivial getter. But
since
> this isn't new, feel free to leave it for a follow-up CL.

Good point. Done.

https://codereview.chromium.org/2621403003/diff/20001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2621403003/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1118: // Maps for ScrollOffsets/SyncedScrollOffsets
are intentionally ommitted here
Does this comment look accurate?

[ajuma, 2017-01-12 01:06:13]

Thanks, lgtm

https://codereview.chromium.org/2621403003/diff/20001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2621403003/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1118: // Maps for ScrollOffsets/SyncedScrollOffsets
are intentionally ommitted here
On 2017/01/11 23:04:53, Khushal wrote:
> Does this comment look accurate?

Yes, might be worth adding that the animation change is clobber_active_value
getting set, and that once this gets added to the main thread scroll tree we
could push the scroll updates here.

[Khushal, 2017-01-12 01:20:50]

Thanks Ali!

https://codereview.chromium.org/2621403003/diff/20001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2621403003/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1118: // Maps for ScrollOffsets/SyncedScrollOffsets
are intentionally ommitted here
On 2017/01/12 01:06:12, ajuma wrote:
> On 2017/01/11 23:04:53, Khushal wrote:
> > Does this comment look accurate?
> 
> Yes, might be worth adding that the animation change is clobber_active_value
> getting set, and that once this gets added to the main thread scroll tree we
> could push the scroll updates here.

Done.

[Khushal, 2017-01-12 01:20:56]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-01-12 01:20:57]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2621403003/#ps40001
(title: "Comment update")

[commit-bot: I haz the power, 2017-01-12 01:21:44]

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

[commit-bot: I haz the power, 2017-01-12 03:07:22]

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

[commit-bot: I haz the power, 2017-01-12 03:07:23]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Khushal, 2017-01-12 17:55:32]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-01-12 17:55:32]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2621403003/#ps60001
(title: "win test")

[commit-bot: I haz the power, 2017-01-12 17:56:31]

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

[commit-bot: I haz the power, 2017-01-12 19:37:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484243732539770,
"parent_rev": "2a852a6b15eaa9828fe51ce4b5deb4f837302cc9", "commit_rev":
"5889b84078b874e7682a4634b1fce6eedcac67fb"}

[commit-bot: I haz the power, 2017-01-12 19:38:26]

Description was changed from

==========
cc: Don't create SyncedPropety instances on the main thread.

SyncedProperty is used as a synchronization primitive to track state on
the impl thread, necessary to resolve conflicts for concurrent main-impl
mutations. As such it should only exist on the impl thread.

Currently, the ScrollTree creates these on the main thread while they
only store the current main thread value. This is unnecessary complexity
that can be avoided. This change makes ScrollTree store just the offset
on the main thread, and a SyncedProperty for the offset on the impl thread.

BUG=662619
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Don't create SyncedPropety instances on the main thread.

SyncedProperty is used as a synchronization primitive to track state on
the impl thread, necessary to resolve conflicts for concurrent main-impl
mutations. As such it should only exist on the impl thread.

Currently, the ScrollTree creates these on the main thread while they
only store the current main thread value. This is unnecessary complexity
that can be avoided. This change makes ScrollTree store just the offset
on the main thread, and a SyncedProperty for the offset on the impl thread.

BUG=662619
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2621403003
Cr-Commit-Position: refs/heads/master@{#443325}
Committed:
https://chromium.googlesource.com/chromium/src/+/5889b84078b874e7682a4634b1fc...
==========

[commit-bot: I haz the power, 2017-01-12 19:38:27]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/5889b84078b874e7682a4634b1fc...