Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3123)

Unified Diff: cc/trees/layer_tree_host_impl.h

Issue 2873313004: Harmonize LayerTreeHost/LayerTreeHostImpl synchronization steps (Closed)
Patch Set: More harmonious Created 3 years, 7 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: cc/trees/layer_tree_host_impl.h
diff --git a/cc/trees/layer_tree_host_impl.h b/cc/trees/layer_tree_host_impl.h
index 6b53b46c2d93cc0762504f59fb46855a80443b63..fe323738cf9538eb397aece693c48ebb3bb4d895 100644
--- a/cc/trees/layer_tree_host_impl.h
+++ b/cc/trees/layer_tree_host_impl.h
@@ -132,6 +132,21 @@ class LayerTreeHostImplClient {
virtual ~LayerTreeHostImplClient() {}
};
+#if DCHECK_IS_ON()
+// The following states are the main steps performed when syncing properties
+// using either LayerTreeHost (|FinishCommitOnImplThread|) or LayerTreeHostImpl
+// (|ActivateSyncTree|). These states follow the order of the enum values. See:
+// |AdvanceSyncState(...)|.
+enum SyncState {
+ NOT_SYNCING = 0,
enne (OOO) 2017/05/12 17:25:06 Don't need value initialization here. This is alr
+ BEGINNING_SYNC = 1,
+ SYNCED_PROPERTY_TREES = 2,
+ SYNCED_LAYER_PROPERTIES = 3,
+ SYNCED_LAYER_TREE_HOST_PROPERTIES = 4,
+ LAST_SYNC_STATE = SYNCED_LAYER_TREE_HOST_PROPERTIES,
+};
+#endif
+
// LayerTreeHostImpl owns the LayerImpl trees as well as associated rendering
// state.
class CC_EXPORT LayerTreeHostImpl
@@ -603,6 +618,26 @@ class CC_EXPORT LayerTreeHostImpl
void ClearImageCacheOnNavigation();
+#if DCHECK_IS_ON()
enne (OOO) 2017/05/12 17:25:06 There's an awful lot of #ifdefs here. I get that
pdr. 2017/05/15 18:30:24 I totes agree with this. Done. (In blink we've be
+ // When syncing from LayerTreeHost (|FinishCommitOnImplThread|) or
+ // LayerTreeHostImpl (|ActivateSyncTree|), the steps should should follow the
+ // order defined by |SyncState|. This updates the current state and returns
+ // true if the state transition is allowed, and returns false otherwise.
enne (OOO) 2017/05/12 17:25:06 I think this would be cleaner if the function didn
+ bool AdvanceSyncState(SyncState state) {
+ bool resetting = state == NOT_SYNCING && sync_state_ == LAST_SYNC_STATE;
+ if (state > sync_state_ || resetting) {
chrishtr 2017/05/12 23:13:29 Is it ever ok to skip one of the states in between
+ sync_state_ = state;
+ return true;
+ }
+ return false;
+ }
+ // Returns true if properties are not currently in the process of syncing, or
+ // if the current |SyncState| has advanced past the passed in value.
+ bool SyncStateComplete(SyncState state) {
chrishtr 2017/05/12 23:13:29 Also, it woudl be great if this could go in a Comp
pdr. 2017/05/15 18:30:24 I've rewritten this code as a LayerTreeLifecycle c
+ return !sync_state_ || sync_state_ >= state;
+ }
+#endif
+
protected:
LayerTreeHostImpl(
const LayerTreeSettings& settings,
@@ -871,6 +906,12 @@ class CC_EXPORT LayerTreeHostImpl
bool touchpad_and_wheel_scroll_latching_enabled_;
+#if DCHECK_IS_ON()
+ // Used to enforce dependencies while syncing properties. See: |SyncState| and
+ // |AdvanceSyncState(...)|.
+ SyncState sync_state_;
enne (OOO) 2017/05/12 17:25:06 Hmmmmmm. I'm super surprised to see this on Layer
chrishtr 2017/05/12 23:13:29 Perhaps there should be two of these, one for each
+#endif
+
ImplThreadPhase impl_thread_phase_;
DISALLOW_COPY_AND_ASSIGN(LayerTreeHostImpl);

Powered by Google App Engine
This is Rietveld 408576698