New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 662619 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Eliminate the use of SyncedProperty on the main thread.

Project Member Reported by khushals...@chromium.org, Nov 5 2016

Issue description

The ScrollTree maintains a map of layer id to SyncedProperty for the scroll offset, which is mirrored on the main and impl thread. This has made following the use of SyncedProperty extremely confusing, since PushFromMainThread is now used when pushing a scroll update from the main to impl thread, and when simply updating the scroll offset for a layer on the main thread.

The use of SyncedProperty on the main thread here is pretty redundant too, since all it is being used for is storing a single value. I think it would be cleaner to re-structure the code to not use SyncedProperty instances on the main thread PropertyTrees.

I'll try to come to this if I have time anytime soon. Leaving it open for now in case someone wants to get to it sooner.
 

Comment 1 by ajuma@chromium.org, Nov 7 2016

Cc: sunxd@chromium.org weiliangc@chromium.org
Cc: -khushals...@chromium.org
Owner: khushals...@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5889b84078b874e7682a4634b1fce6eedcac67fb

commit 5889b84078b874e7682a4634b1fce6eedcac67fb
Author: khushalsagar <khushalsagar@chromium.org>
Date: Thu Jan 12 19:37:33 2017

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}

[modify] https://crrev.com/5889b84078b874e7682a4634b1fce6eedcac67fb/cc/layers/layer.cc
[modify] https://crrev.com/5889b84078b874e7682a4634b1fce6eedcac67fb/cc/test/fake_layer_tree_host.cc
[modify] https://crrev.com/5889b84078b874e7682a4634b1fce6eedcac67fb/cc/trees/layer_tree_host_in_process.cc
[modify] https://crrev.com/5889b84078b874e7682a4634b1fce6eedcac67fb/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/5889b84078b874e7682a4634b1fce6eedcac67fb/cc/trees/layer_tree_impl.h
[modify] https://crrev.com/5889b84078b874e7682a4634b1fce6eedcac67fb/cc/trees/property_tree.cc
[modify] https://crrev.com/5889b84078b874e7682a4634b1fce6eedcac67fb/cc/trees/property_tree.h
[modify] https://crrev.com/5889b84078b874e7682a4634b1fce6eedcac67fb/cc/trees/tree_synchronizer_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment