Hide fullscreen rotation jank

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index b9898a869d9c3e42e0e206cce3d8230a93d8aee9..abfc895af972fb5d73236415ff73899b636922c4 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -562,6 +562,21 @@ void RenderWidgetHostViewAndroid::WasResized() {
   host_->WasResized();
 }
 
+void RenderWidgetHostViewAndroid::DidToggleFullscreenModeForTab(
+    bool entered_fullscreen,
+    bool will_cause_resize) {
+  if (!fullscreen_transitioning_) {
+    fullscreen_transitioning_ = true;

[Khushal, 2017/06/07 05:36:01]

The reason why I suggested the enum was so its easy to distinguish which
transition we are handling, and that they are happening in the expected order.
Right now we set this bool for the transition in and out of fullscreen and for
size changes during fullscreen. And its hard to follow what initiated a
transition change.

The logic for the size change jank during fullscreen still needs to know what
the current fullscreen state is. So the triggers/conditions that define jank
also end up being in multiple places.

I think you can have one function you call from every place where the state
could have changed (fullscreen toggling, resize, new frame). Update the
fullscreen state there accordingly and evict the frame if you have to.

[liberato (no reviews please), 2017/06/13 22:06:42]

+1. it took me several reads to notice that |fullscreen_transitioning_| has two
meanings.

[steimel, 2017/06/13 23:15:44]

Done.

+    // TODO(steimel): Do we always want to await a resize? Danger of getting
+    // stuck with a black screen if we await one that's not going to occur.
+    // However, even when |will_cause_resize| == false, I still see a resize
+    // occurring, which means that even if there are cases in which we don't
+    // want to await a resize, we won't be able to tell from that parameter.
+    awaiting_resize_ = true;
+  }
+  HideFullscreenTransitionJank();
+}
+
 void RenderWidgetHostViewAndroid::SetSize(const gfx::Size& size) {
   // Ignore the given size as only the Java code has the power to
   // resize the view on Android.
@@ -1181,6 +1196,35 @@ void RenderWidgetHostViewAndroid::DidCreateNewRendererCompositorFrameSink(
   surface_returned_resources_.clear();
 }
 
+bool RenderWidgetHostViewAndroid::HasFullscreenTransitionJank(
+    const cc::CompositorFrame& frame) const {
+  // We only check for fullscreen jank during a fullscreen transition.
+  if (fullscreen_transitioning_) {

[liberato (no reviews please), 2017/06/13 22:06:42]

if (!fullscreen_transitioning_)
  return false;

one might also want to "if (!fullscreen && !transitioning)", so that
"transitioning" can just mean "entering / exiting fullscreen".  any size
mismatch in fullscreen mode is cause to hide the display.  then one could avoid
setting or checking |transitioning| in OnPhysicalBackingSizeChanged.

[steimel, 2017/06/13 23:15:44]

This has been completely refactored now :)

+    gfx::Size frame_size = frame.render_pass_list.back()->output_rect.size();
+    bool mismatched_layer_sizes = view_.GetPhysicalBackingSize() != frame_size;
+
+    bool mismatched_fullscreen_states =
+        content_view_core_ &&
+        (content_view_core_->GetWebContents()->IsFullscreen() !=
+         frame.metadata.is_fullscreen);
+
+    return mismatched_fullscreen_states || mismatched_layer_sizes ||
+           awaiting_resize_;

[Khushal, 2017/06/07 05:36:01]

When do we hit this case? Where layer sizes and fullscreen states match but we
still need to wait for a resize?

[steimel, 2017/06/13 23:15:44]

Discussed offline

+  }
+
+  return false;
+}
+
+void RenderWidgetHostViewAndroid::HideFullscreenTransitionJank() {
+  EvictDelegatedFrame();
+  UpdateBackgroundColor(SK_ColorBLACK);
+}
+
+void RenderWidgetHostViewAndroid::EndFullscreenTransition() {
+  fullscreen_transitioning_ = false;
+  awaiting_resize_ = false;
+}
+
 void RenderWidgetHostViewAndroid::SubmitCompositorFrame(
     const cc::LocalSurfaceId& local_surface_id,
     cc::CompositorFrame frame) {
@@ -1189,6 +1233,8 @@ void RenderWidgetHostViewAndroid::SubmitCompositorFrame(
     return;
   }
 
+  bool has_jank = HasFullscreenTransitionJank(frame);
+
   last_scroll_offset_ = frame.metadata.root_scroll_offset;
   DCHECK(!frame.render_pass_list.empty());
 
@@ -1225,6 +1271,12 @@ void RenderWidgetHostViewAndroid::SubmitCompositorFrame(
   // As the metadata update may trigger view invalidation, always call it after
   // any potential compositor scheduling.
   OnFrameMetadataUpdated(std::move(metadata), is_transparent);
+
+  if (has_jank) {
+    HideFullscreenTransitionJank();
+  } else if (fullscreen_transitioning_) {
+    EndFullscreenTransition();
+  }
 }
 
 void RenderWidgetHostViewAndroid::DestroyDelegatedContent() {
@@ -1240,6 +1292,7 @@ void RenderWidgetHostViewAndroid::DestroyDelegatedContent() {
 
   frame_evictor_->DiscardedFrame();
   delegated_frame_host_->DestroyDelegatedContent();
+  current_surface_size_.SetSize(0, 0);
 }
 
 void RenderWidgetHostViewAndroid::OnDidNotProduceFrame(
@@ -1938,9 +1991,13 @@ void RenderWidgetHostViewAndroid::SetContentViewCore(
     }
     if (content_view_core) {
       content_view_core->AddObserver(this);
+      content::WebContentsObserver::Observe(
+          content_view_core->GetWebContents());
       ui::ViewAndroid* parent_view = content_view_core->GetViewAndroid();
       parent_view->AddChild(&view_);
       parent_view->GetLayer()->AddChild(view_.GetLayer());
+    } else {
+      content::WebContentsObserver::Observe(NULL);
     }
     content_view_core_ = content_view_core;
   }
@@ -2021,6 +2078,16 @@ void RenderWidgetHostViewAndroid::OnGestureEvent(
 }
 
 void RenderWidgetHostViewAndroid::OnPhysicalBackingSizeChanged() {
+  // If we're entering a fullscreen transition, show black until the transition
+  // is completed.
+  if (content_view_core_) {
+    bool is_fullscreen = content_view_core_->GetWebContents()->IsFullscreen();

[Khushal, 2017/06/07 05:36:01]

Shouldn't be able to include WebContents here but its transitively coming from
ContentViewCoreImpl. Woops.

I'd suggest use host_->delegate()->IsFullscreenForTab.

[steimel, 2017/06/13 23:15:44]

Refactored to store what we get when WebContents tells us fullscreen state has
changed.

+    if (is_fullscreen || fullscreen_transitioning_) {
+      fullscreen_transitioning_ = true;
+      awaiting_resize_ = false;
+      HideFullscreenTransitionJank();
+    }
+  }
   WasResized();
 }