Hide fullscreen rotation jank

content/browser/renderer_host/render_widget_host_view_android.cc

 // Copyright (c) 2012 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.
 
 #include "content/browser/renderer_host/render_widget_host_view_android.h"
 
 #include <android/bitmap.h>
 
 #include <utility>
 
@@ skipping 544 matching lines @@
 
 RenderWidgetHost*
 RenderWidgetHostViewAndroid::GetRenderWidgetHost() const {
   return host_;
 }
 
 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.
   default_bounds_ = gfx::Rect(default_bounds_.origin(), size);
 }
 
 void RenderWidgetHostViewAndroid::SetBounds(const gfx::Rect& rect) {
   default_bounds_ = rect;
 }
 
@@ skipping 599 matching lines @@
                                     bad_message::RWH_BAD_FRAME_SINK_REQUEST);
     return;
   }
   delegated_frame_host_->CompositorFrameSinkChanged();
   renderer_compositor_frame_sink_ = renderer_compositor_frame_sink;
   // Accumulated resources belong to the old RendererCompositorFrameSink and
   // should not be returned.
   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) {
   if (!delegated_frame_host_) {
     DCHECK(!using_browser_compositor_);
     return;
   }
 
+  bool has_jank = HasFullscreenTransitionJank(frame);
+
   last_scroll_offset_ = frame.metadata.root_scroll_offset;
   DCHECK(!frame.render_pass_list.empty());
 
   cc::RenderPass* root_pass = frame.render_pass_list.back().get();
   current_surface_size_ = root_pass->output_rect.size();
   bool is_transparent = root_pass->has_transparent_background;
 
   cc::CompositorFrameMetadata metadata = frame.metadata.Clone();
 
   bool has_content = !current_surface_size_.IsEmpty();
@@ skipping 16 matching lines @@
     frame_evictor_->SwappedFrame(!host_->is_hidden());
     AcknowledgeBeginFrame(ack);
   }
 
   if (host_->is_hidden())
     RunAckCallbacks();
 
   // 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() {
   DCHECK(!delegated_frame_host_ ||
          delegated_frame_host_->HasDelegatedContent() ==
              frame_evictor_->HasFrame());
 
   if (!delegated_frame_host_)
     return;
 
   if (!delegated_frame_host_->HasDelegatedContent())
     return;
 
   frame_evictor_->DiscardedFrame();
   delegated_frame_host_->DestroyDelegatedContent();
+  current_surface_size_.SetSize(0, 0);
 }
 
 void RenderWidgetHostViewAndroid::OnDidNotProduceFrame(
     const cc::BeginFrameAck& ack) {
   if (!delegated_frame_host_) {
     // We are not using the browser compositor and there's no DisplayScheduler
     // that needs to be notified about the BeginFrameAck, so we can drop it.
     DCHECK(!using_browser_compositor_);
     return;
   }
@@ skipping 678 matching lines @@
         || content_view_core->GetViewportSizeDip().height() != 0;
     if (content_view_core_ || is_size_initialized)
       resize = true;
     if (content_view_core_) {
       content_view_core_->RemoveObserver(this);
       view_.RemoveFromParent();
       view_.GetLayer()->RemoveFromParent();
     }
     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;
   }
 
   BrowserAccessibilityManager* manager = NULL;
   if (host_)
     manager = host_->GetRootBrowserAccessibilityManager();
   if (manager) {
     base::android::ScopedJavaLocalRef<jobject> obj;
     if (content_view_core_)
@@ skipping 60 matching lines @@
   // unintended shift+click interpretation of all accessibility clicks.
   // See crbug.com/443247.
   if (web_gesture.GetType() == blink::WebInputEvent::kGestureTap &&
       web_gesture.GetModifiers() == blink::WebInputEvent::kShiftKey) {
     web_gesture.SetModifiers(blink::WebInputEvent::kNoModifiers);
   }
   SendGestureEvent(web_gesture);
 }
 
 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();
 }
 
 void RenderWidgetHostViewAndroid::OnContentViewCoreDestroyed() {
   SetContentViewCore(NULL);
   overscroll_controller_.reset();
 }
 
 void RenderWidgetHostViewAndroid::OnRootWindowVisibilityChanged(bool visible) {
   TRACE_EVENT1("browser",
@@ skipping 204 matching lines @@
 
   ui::WindowAndroidCompositor* compositor = window_android->GetCompositor();
   if (!compositor)
     return;
 
   overscroll_controller_ = base::MakeUnique<OverscrollControllerAndroid>(
       overscroll_refresh_handler, compositor, view_.GetDipScale());
 }
 
 }  // namespace content