ui: Rework CompositorResizeLock interaction with DelegatedFrameHost

content/browser/renderer_host/delegated_frame_host.cc

Index: content/browser/renderer_host/delegated_frame_host.cc
diff --git a/content/browser/renderer_host/delegated_frame_host.cc b/content/browser/renderer_host/delegated_frame_host.cc
index ed054ca41863bf769c613d639ba74b5995c0a703..438c9e172c43fe30f60adf9dbcaed1b67e10fa11 100644
--- a/content/browser/renderer_host/delegated_frame_host.cc
+++ b/content/browser/renderer_host/delegated_frame_host.cc
@@ -51,7 +51,6 @@ DelegatedFrameHost::DelegatedFrameHost(const cc::FrameSinkId& frame_sink_id,
       skipped_frames_(false),
       background_color_(SK_ColorRED),
       current_scale_factor_(1.f),
-      can_lock_compositor_(YES_CAN_LOCK),
       delegated_frame_evictor_(new DelegatedFrameEvictor(this)) {
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
   factory->GetContextFactory()->AddObserver(this);
@@ -83,45 +82,27 @@ void DelegatedFrameHost::WasHidden() {
 }
 
 void DelegatedFrameHost::MaybeCreateResizeLock() {
-  if (!ShouldCreateResizeLock())
-    return;
-  DCHECK(compositor_);
-
-  bool defer_compositor_lock =
-      can_lock_compositor_ == NO_PENDING_RENDERER_FRAME ||
-      can_lock_compositor_ == NO_PENDING_COMMIT;
-
-  if (can_lock_compositor_ == YES_CAN_LOCK)
-    can_lock_compositor_ = YES_DID_LOCK;
+  DCHECK(!resize_lock_);
 
-  resize_lock_ = client_->DelegatedFrameHostCreateResizeLock();
-  if (!defer_compositor_lock) {
-    bool locked = resize_lock_->Lock();
-    DCHECK(locked);
-  }
-}
+  if (!compositor_)
+    return;
 
-bool DelegatedFrameHost::ShouldCreateResizeLock() {
-  static const bool is_disabled =
-      base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kDisableResizeLock);
-  if (is_disabled)
-    return false;
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kDisableResizeLock))
+    return;
 
   if (!client_->DelegatedFrameCanCreateResizeLock())
-    return false;
-
-  if (resize_lock_)
-    return false;
+    return;
 
   gfx::Size desired_size = client_->DelegatedFrameHostDesiredSizeInDIP();
-  if (desired_size == current_frame_size_in_dip_ || desired_size.IsEmpty())
-    return false;
-
-  if (!compositor_)
-    return false;
+  if (desired_size.IsEmpty())
+    return;
+  if (desired_size == current_frame_size_in_dip_)
+    return;
 
-  return true;
+  resize_lock_ = client_->DelegatedFrameHostCreateResizeLock();
+  bool locked = resize_lock_->Lock();
+  DCHECK(locked);
 }
 
 void DelegatedFrameHost::CopyFromCompositingSurface(
@@ -253,13 +234,8 @@ void DelegatedFrameHost::BeginFrameDidNotSwap(const cc::BeginFrameAck& ack) {
 }
 
 bool DelegatedFrameHost::ShouldSkipFrame(gfx::Size size_in_dip) const {
-  // Should skip a frame only when another frame from the renderer is guaranteed
-  // to replace it. Otherwise may cause hangs when the renderer is waiting for
-  // the completion of latency infos (such as when taking a Snapshot.)
-  if (can_lock_compositor_ == NO_PENDING_RENDERER_FRAME ||
-      can_lock_compositor_ == NO_PENDING_COMMIT || !resize_lock_.get())
+  if (!resize_lock_)
     return false;
-
   return size_in_dip != resize_lock_->expected_size();
 }
 
@@ -268,7 +244,8 @@ void DelegatedFrameHost::WasResized() {
           current_frame_size_in_dip_ &&
       !client_->DelegatedFrameHostIsVisible())
     EvictDelegatedFrame();
-  MaybeCreateResizeLock();
+  if (!resize_lock_)
+    MaybeCreateResizeLock();
   UpdateGutters();
 }
 
@@ -488,9 +465,6 @@ void DelegatedFrameHost::SubmitCompositorFrame(
         damage_rect_in_dip);
   }
 
-  if (compositor_)
-    can_lock_compositor_ = NO_PENDING_COMMIT;
-
   if (has_frame_) {
     delegated_frame_evictor_->SwappedFrame(
         client_->DelegatedFrameHostIsVisible());
@@ -710,18 +684,19 @@ void DelegatedFrameHost::CopyFromCompositingSurfaceHasResultForVideo(
 // DelegatedFrameHost, ui::CompositorObserver implementation:
 
 void DelegatedFrameHost::OnCompositingDidCommit(ui::Compositor* compositor) {
-  if (can_lock_compositor_ == NO_PENDING_COMMIT) {
-    can_lock_compositor_ = YES_CAN_LOCK;
-    if (resize_lock_ && resize_lock_->Lock())
-      can_lock_compositor_ = YES_DID_LOCK;
-  }
+  // If |create_resize_lock_after_commit_| then we should have popped the old
+  // lock already.
+  DCHECK(!resize_lock_ || !create_resize_lock_after_commit_);
+
   if (resize_lock_ &&
       resize_lock_->expected_size() == current_frame_size_in_dip_) {
     resize_lock_.reset();
-    client_->DelegatedFrameHostResizeLockWasReleased();
-    // We may have had a resize while we had the lock (e.g. if the lock expired,
-    // or if the UI still gave us some resizes), so make sure we grab a new lock
-    // if necessary.
+    // We had a lock but the UI may have resized in the meantime.
+    create_resize_lock_after_commit_ = true;
+  }
+
+  if (create_resize_lock_after_commit_) {
+    create_resize_lock_after_commit_ = false;
     MaybeCreateResizeLock();
   }
 }
@@ -733,10 +708,14 @@ void DelegatedFrameHost::OnCompositingStarted(ui::Compositor* compositor,
 
 void DelegatedFrameHost::OnCompositingLockStateChanged(
     ui::Compositor* compositor) {
-  // A compositor lock that is part of a resize lock timed out. We
-  // should display a renderer frame.
-  if (!compositor->IsLocked() && can_lock_compositor_ == YES_DID_LOCK) {
-    can_lock_compositor_ = NO_PENDING_RENDERER_FRAME;
+  if (resize_lock_ && resize_lock_->timed_out()) {
+    // A compositor lock that is part of a resize lock timed out. We allow
+    // the UI to produce a frame before locking it again, so we don't lock here.
+    // We release the |resize_lock_| though to allow any other resizes that are
+    // desired at the same time since we're allowing the UI to make a frame
+    // which will gutter anyways.

[piman, 2017/03/31 00:31:55]

Ok, here I think I know why we wanted the NO_PENDING_RENDERER_FRAME state (with
create_resize_lock_after_commit_ = true being like the NO_PENDING_COMMIT state).
If we take a lock again at the next commit, if it happens before we got a frame
from the renderer, we won't grab the update (because ShouldSkipFrame will be
true) - so basically during the resize of a slow renderer, we may be stuck with
the first frame that we had, and not change until we stop resizing.

Now, I think that when we're in the ShouldSkipFrame case, we should still take
the renderer frame. I don't think there's a reason not to anymore, with
delegated rendering (it was important in the old days). That would fix it. wdyt?

[danakj, 2017/03/31 19:34:42]

Ah okay, so maybe allow_one_renderer_frame_ = true. But it's not tied to the UI
compositor importantly, as it used to be.

+    resize_lock_.reset();
+    create_resize_lock_after_commit_ = true;
   }
 }
 
@@ -794,10 +773,7 @@ void DelegatedFrameHost::SetCompositor(ui::Compositor* compositor) {
 void DelegatedFrameHost::ResetCompositor() {
   if (!compositor_)
     return;
-  if (resize_lock_) {
-    resize_lock_.reset();
-    client_->DelegatedFrameHostResizeLockWasReleased();
-  }
+  resize_lock_.reset();
   if (compositor_->HasObserver(this))
     compositor_->RemoveObserver(this);
   if (vsync_manager_) {