Harmonize LayerTreeHost/LayerTreeHostImpl synchronization steps

cc/trees/layer_tree_host_impl.h

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 already the values they get.

+  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 you're trying to be
conservative and not have a perf impact from your change, but I think there's
some overhead to #ifdefs in terms of development friction or landing things that
break in one mode but not another (in both directions).

Is there a way to minimize the amount of #ifdefs here or remove them entirely?
What about just not #ifdefing anything, and maybe having SyncStateComplete exist
but return true always in release?

I'm ok with having sync state exist and advance in release as well.

[pdr., 2017/05/15 18:30:24]

I totes agree with this. Done.

(In blink we've been more hard-line about not letting debug code in release, but
I think that's wrong for cases like this one).

+  // 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't return bool, and instead
DCHECKed that state is the state + 1 or is NOT_SYNCING when LAST_SYNC_STATE.

+  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 two others?

+      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 CompositorThreadLifecycle class
modeled on the same design pattern as DocumentLifecycle, if the cc
owners agree.

[pdr., 2017/05/15 18:30:24]

I've rewritten this code as a LayerTreeLifecycle class on LayerTreeImpl.

Enne and Chris, PTAL.

+    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 LayerTreeHostImpl and not
LayerTreeImpl.  Sure, I realize that there's only one compositor thread and one
pending tree and so you can't be committing and activating at the same time. 
But, if you put it on LayerTreeImpl, then you can DCHECK in the accessors
themselves instead of making callers remember to do it.

Also, initialize in headers instead of the ctor implementation if you can.

[chrishtr, 2017/05/12 23:13:29]

Perhaps there should be two of these, one for each thread. Each thread has its
own lifecycle.

+#endif
+
   ImplThreadPhase impl_thread_phase_;
 
   DISALLOW_COPY_AND_ASSIGN(LayerTreeHostImpl);