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..814b548918099ef3bf974f36216e155f2a908d4d 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -559,6 +559,25 @@ RenderWidgetHostViewAndroid::GetRenderWidgetHost() const {
 }
 
 void RenderWidgetHostViewAndroid::WasResized() {
+  // If we're entering a fullscreen transition, show black until the transition
+  // is completed.

[mlamouri (slow - plz ping), 2017/06/05 12:57:44]

We should probably have a content or cc Feature in order to turn this on/off and
have this behind an experiment.

[steimel, 2017/06/06 03:07:54]

Good idea. I don't know much about hiding features behind flags but maybe we can
discuss offline

+  if (content_view_core_ &&
+      view_.GetPhysicalBackingSize() != prev_physical_backing_size_) {
+    bool is_fullscreen = content_view_core_->GetWebContents()->IsFullscreen();
+    if (is_fullscreen || was_fullscreen_) {
+      fullscreen_transitioning_ = true;
+
+      // TODO update this comment. Immediately queue a black frame to hide jank while we wait to receive a
+      // "good" frame via SubmitCompositorFrame.
+      TRACE_EVENT_BEGIN0("frame_eviction", "RenderWidgetHostViewAndroid::WasResized Eviction");
+      EvictDelegatedFrame();
+      TRACE_EVENT_END0("frame_eviction", "RenderWidgetHostViewAndroid::WasResized Eviction");
+      UpdateBackgroundColor(SK_ColorBLACK);
+    }
+    // TODO change logic to not require these variables.
+    was_fullscreen_ = is_fullscreen;
+    prev_physical_backing_size_ = view_.GetPhysicalBackingSize();
+  }
   host_->WasResized();
 }
 
@@ -1181,6 +1200,35 @@ void RenderWidgetHostViewAndroid::DidCreateNewRendererCompositorFrameSink(
   surface_returned_resources_.clear();
 }
 
+bool RenderWidgetHostViewAndroid::CheckForFullscreenTransitionJank(
+    const cc::CompositorFrameMetadata& metadata) {
+  // TODO simplify this logic.
+  if (content_view_core_ &&
+      content_view_core_->GetWebContents()->IsFullscreen() != was_fullscreen_) {
+    was_fullscreen_ = !was_fullscreen_;

[mlamouri (slow - plz ping), 2017/06/05 12:57:44]

Here and below, it is fairly confusing that a method called `CheckFor...` ends
up.

[steimel, 2017/06/06 03:07:54]

I'm assuming you mean "ends up modifying state". I didn't really know how to
name this to correctly imply what it was doing without accidentally implying
that it doesn't change state (outside of the fact that it's not const).
Fortunately, in my cleanup of the logic this function doesn't modify state
anymore, so it's const and hopefully clearer :).

+    fullscreen_transitioning_ = true;
+    return true;
+  }
+
+  // We only check for fullscreen jank during a fullscreen transition.
+  if (fullscreen_transitioning_) {
+    bool mismatched_fullscreen_states =
+        content_view_core_ &&
+        (content_view_core_->GetWebContents()->IsFullscreen() !=
+         metadata.is_fullscreen);
+    bool mismatched_layer_sizes =
+        view_.GetPhysicalBackingSize() != metadata.device_viewport_size;
+    if (mismatched_fullscreen_states || mismatched_layer_sizes) {
+      return true;
+    } else {
+      // Done with current fullscreen transition.
+      fullscreen_transitioning_ = false;
+    }
+  }
+
+  return false;
+}
+
 void RenderWidgetHostViewAndroid::SubmitCompositorFrame(
     const cc::LocalSurfaceId& local_surface_id,
     cc::CompositorFrame frame) {
@@ -1189,6 +1237,8 @@ void RenderWidgetHostViewAndroid::SubmitCompositorFrame(
     return;
   }
 
+  bool has_jank = CheckForFullscreenTransitionJank(frame.metadata);
+
   last_scroll_offset_ = frame.metadata.root_scroll_offset;
   DCHECK(!frame.render_pass_list.empty());
 
@@ -1225,6 +1275,13 @@ 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) {
+    TRACE_EVENT_BEGIN0("frame_eviction", "RenderWidgetHostViewAndroid::SubmitCompositorFrame Eviction");
+    EvictDelegatedFrame();
+    TRACE_EVENT_END0("frame_eviction", "RenderWidgetHostViewAndroid::SubmitCompositorFrame Eviction");
+    UpdateBackgroundColor(SK_ColorBLACK);
+  }

[mlamouri (slow - plz ping), 2017/06/05 12:57:44]

For my culture, why do we have to repeat the logic here?

Also, would it make sense to have this in a different method as the logic is
fairly the same as above?

[steimel, 2017/06/06 03:07:54]

I have fixed this in my cleanup. It was separate here to make it easier to trace
separately from the other frame eviction events while I was debugging an issue.

 }
 
 void RenderWidgetHostViewAndroid::DestroyDelegatedContent() {