Harmonize LayerTreeHost/LayerTreeHostImpl synchronization steps

cc/trees/layer_tree_host_impl.h

 // Copyright 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CC_TREES_LAYER_TREE_HOST_IMPL_H_
 #define CC_TREES_LAYER_TREE_HOST_IMPL_H_
 
 #include <stddef.h>
 
 #include <bitset>
@@ skipping 114 matching lines @@
   // Called when output surface asks for a draw.
   virtual void OnDrawForCompositorFrameSink(
       bool resourceless_software_draw) = 0;
 
   virtual void NeedsImplSideInvalidation() = 0;
 
  protected:
   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
     : public InputHandler,
       public TileManagerClient,
       public CompositorFrameSinkClient,
       public BrowserControlsOffsetManagerClient,
       public ScrollbarAnimationControllerClient,
       public VideoFrameControllerClient,
       public LayerTreeMutatorClient,
@@ skipping 451 matching lines @@
   LayerTreeMutator* mutator() { return mutator_.get(); }
 
   LayerImpl* ViewportMainScrollLayer();
 
   void QueueImageDecode(sk_sp<const SkImage> image,
                         const base::Callback<void(bool)>& embedder_callback);
   std::vector<base::Closure> TakeCompletedImageDecodeCallbacks();
 
   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,
       LayerTreeHostImplClient* client,
       TaskRunnerProvider* task_runner_provider,
       RenderingStatsInstrumentation* rendering_stats_instrumentation,
       TaskGraphRunner* task_graph_runner,
       std::unique_ptr<MutatorHost> mutator_host,
       int id,
       scoped_refptr<base::SequencedTaskRunner> image_worker_task_runner);
@@ skipping 248 matching lines @@
   // begin main frame. These callbacks must only be called on the main thread.
   std::vector<base::Closure> completed_image_decode_callbacks_;
 
   // These are used to transfer usage of touch and wheel scrolls to the main
   // thread.
   bool has_scrolled_by_wheel_;
   bool has_scrolled_by_touch_;
 
   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);
 };
 
 }  // namespace cc
 
 #endif  // CC_TREES_LAYER_TREE_HOST_IMPL_H_