Fix CompositorResizeLock to do something.

ui/compositor/compositor.h

Index: ui/compositor/compositor.h
diff --git a/ui/compositor/compositor.h b/ui/compositor/compositor.h
index f94748b7ff87b257fe7d514caabd7042cde494c1..143d991505becefff30df08c690803fbac8d1a9a 100644
--- a/ui/compositor/compositor.h
+++ b/ui/compositor/compositor.h
@@ -24,6 +24,7 @@
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/compositor/compositor_animation_observer.h"
 #include "ui/compositor/compositor_export.h"
+#include "ui/compositor/compositor_lock.h"
 #include "ui/compositor/compositor_observer.h"
 #include "ui/compositor/layer_animator_collection.h"
 #include "ui/gfx/color_space.h"
@@ -68,7 +69,7 @@ class Layer;
 class Reflector;
 class ScopedAnimationDurationScaleMode;
 
-const int kCompositorLockTimeoutMs = 67;
+constexpr int kCompositorLockTimeoutMs = 67;
 
 class COMPOSITOR_EXPORT ContextFactoryObserver {
  public:
@@ -167,32 +168,6 @@ class COMPOSITOR_EXPORT ContextFactory {
   virtual void RemoveObserver(ContextFactoryObserver* observer) = 0;
 };
 
-// This class represents a lock on the compositor, that can be used to prevent
-// commits to the compositor tree while we're waiting for an asynchronous
-// event. The typical use case is when waiting for a renderer to produce a frame
-// at the right size. The caller keeps a reference on this object, and drops the
-// reference once it desires to release the lock.
-// By default, the lock will be cancelled after a short timeout to ensure
-// responsiveness of the UI, so the compositor tree should be kept in a
-// "reasonable" state while the lock is held. If the compositor sets
-// locks to not time out, then the lock will remain in effect until destroyed.
-// Don't instantiate this class directly, use Compositor::GetCompositorLock.
-class COMPOSITOR_EXPORT CompositorLock
-    : public base::RefCounted<CompositorLock>,
-      public base::SupportsWeakPtr<CompositorLock> {
- private:
-  friend class base::RefCounted<CompositorLock>;
-  friend class Compositor;
-
-  explicit CompositorLock(Compositor* compositor);
-  ~CompositorLock();
-
-  void CancelLock();
-
-  Compositor* compositor_;
-  DISALLOW_COPY_AND_ASSIGN(CompositorLock);
-};
-
 // Compositor object to take care of GPU painting.
 // A Browser compositor object is responsible for generating the final
 // displayable form of pixels comprising a single widget's contents. It draws an
@@ -200,7 +175,8 @@ class COMPOSITOR_EXPORT CompositorLock
 // view hierarchy.
 class COMPOSITOR_EXPORT Compositor
     : NON_EXPORTED_BASE(public cc::LayerTreeHostClient),
-      NON_EXPORTED_BASE(public cc::LayerTreeHostSingleThreadClient) {
+      NON_EXPORTED_BASE(public cc::LayerTreeHostSingleThreadClient),
+      NON_EXPORTED_BASE(public CompositorLockDelegate) {
  public:
   Compositor(const cc::FrameSinkId& frame_sink_id,
              ui::ContextFactory* context_factory,
@@ -325,16 +301,13 @@ class COMPOSITOR_EXPORT Compositor
   void RemoveAnimationObserver(CompositorAnimationObserver* observer);
   bool HasAnimationObserver(const CompositorAnimationObserver* observer) const;
 
-  // Change the timeout behavior for all future locks that are created. Locks
-  // should time out if there is an expectation that the compositor will be
-  // responsive.
-  void SetLocksWillTimeOut(bool locks_will_time_out) {
-    locks_will_time_out_ = locks_will_time_out;
-  }
-
   // Creates a compositor lock. Returns NULL if it is not possible to lock at
-  // this time (i.e. we're waiting to complete a previous unlock).
-  scoped_refptr<CompositorLock> GetCompositorLock();
+  // this time (i.e. we're waiting to complete a previous unlock). If the
+  // timeout is null, then no timeout is used.
+  scoped_refptr<CompositorLock> GetCompositorLock(
+      CompositorLockClient* client,

[piman, 2017/03/24 04:05:31]

2 things.
1- I assume we want the client to outlive the CompositorLock. However, because
the CompositorLock is refcounted, and arbitrary unrelated things may want to
grab one, which will extend the lifetime of the CompositorLock, unbeknownst to
|client|.
2- relatedly, the second caller that tries to grab it will get a CompositorLock
that will not call into |client|

IOW I think the client needs to be put in an ObserverList (so that we can have
more than one and so that they can remove themselves).

[danakj, 2017/03/24 14:49:26]

Thank you, good point.

[danakj, 2017/03/24 16:54:56]

I've made CompositorLock a unique_ptr instead, but the timeout is shared and
implemented in ui::Compositor. Added a test to verify this.

+      base::TimeDelta timeout =
+          base::TimeDelta::FromMilliseconds(kCompositorLockTimeoutMs));
 
   // Internal functions, called back by command-buffer contexts on swap buffer
   // events.
@@ -390,13 +363,9 @@ class COMPOSITOR_EXPORT Compositor
 
  private:
   friend class base::RefCounted<Compositor>;
-  friend class CompositorLock;
-
-  // Called by CompositorLock.
-  void UnlockCompositor();
 
-  // Called to release any pending CompositorLock
-  void CancelCompositorLock();
+  // CompositorLockDelegate implementation.
+  void DoUnlockCompositor() override;
 
   gfx::Size size_;
 
@@ -433,7 +402,6 @@ class COMPOSITOR_EXPORT Compositor
   // layers on.
   float device_scale_factor_;
 
-  bool locks_will_time_out_;
   CompositorLock* compositor_lock_;
 
   LayerAnimatorCollection layer_animator_collection_;