Invalidate LocalSurfaceId on Hidden Navigation

Currently RenderWidgetHostViewAura allocates a new LocalSurfaceId upon
Navigation. However if this navigation is for a hidden view, then we are
allocating very early. This has skewed the statistics for embedding times.

This updates RenderWidgetHostViewAura::DidNavigate to Invalidate the
LocalSurfaceId if hidden. A new one will be allocated once the view is shown.

Bug: 655231
Change-Id: I86cc4efade8b927149f57323281a87b0dcd1aac3
Reviewed-on: https://chromium-review.googlesource.com/c/1347446
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611319}

content/browser/renderer_host/browser_compositor_view_mac.mm

@@ -356,14 +356,23 @@ BrowserCompositorMac::CollectSurfaceIdsForEviction() {
 }
 
 void BrowserCompositorMac::DidNavigate() {
-  // The first navigation does not need a new LocalSurfaceID. The renderer can
-  // use the ID that was already provided.
-  if (!is_first_navigation_)
-    dfh_local_surface_id_allocator_.GenerateId();
-  delegated_frame_host_->EmbedSurface(
-      dfh_local_surface_id_allocator_.GetCurrentLocalSurfaceId(), dfh_size_dip_,
-      cc::DeadlinePolicy::UseExistingDeadline());
-  client_->OnBrowserCompositorSurfaceIdChanged();
+  if (render_widget_host_is_hidden_) {
+    // Navigating while hidden should not allocate a new LocalSurfaceID. Once
+    // sizes are ready, or we begin to Show, we can then allocate the new
+    // LocalSurfaceId.
+    dfh_local_surface_id_allocator_.Invalidate();
+  } else {
+    // The first navigation does not need a new LocalSurfaceID. The renderer can
+    // use the ID that was already provided.
+    if (!is_first_navigation_) {
+      dfh_local_surface_id_allocator_.GenerateId();
+    }
+    delegated_frame_host_->EmbedSurface(
+        dfh_local_surface_id_allocator_.GetCurrentLocalSurfaceId(),
+        dfh_size_dip_, cc::DeadlinePolicy::UseExistingDeadline());
+    client_->OnBrowserCompositorSurfaceIdChanged();
+  }
+
   delegated_frame_host_->DidNavigate();
   is_first_navigation_ = false;
 }

content/browser/renderer_host/render_widget_host_view_android.cc

@@ -2374,14 +2374,20 @@ void RenderWidgetHostViewAndroid::DidNavigate() {
     RenderWidgetHostViewBase::DidNavigate();
     return;
   }
-
-  if (is_first_navigation_) {
-    SynchronizeVisualProperties(
-        cc::DeadlinePolicy::UseExistingDeadline(),
-        local_surface_id_allocator_.GetCurrentLocalSurfaceIdAllocation());
+  if (!is_showing_) {
+    // Navigating while hidden should not allocate a new LocalSurfaceID. Once
+    // sizes are ready, or we begin to Show, we can then allocate the new
+    // LocalSurfaceId.
+    local_surface_id_allocator_.Invalidate();
   } else {
-    SynchronizeVisualProperties(cc::DeadlinePolicy::UseExistingDeadline(),
-                                base::nullopt);
+    if (is_first_navigation_) {
+      SynchronizeVisualProperties(
+          cc::DeadlinePolicy::UseExistingDeadline(),
+          local_surface_id_allocator_.GetCurrentLocalSurfaceIdAllocation());
+    } else {
+      SynchronizeVisualProperties(cc::DeadlinePolicy::UseExistingDeadline(),
+                                  base::nullopt);
+    }
   }
   delegated_frame_host_->DidNavigate();
   is_first_navigation_ = false;

content/browser/renderer_host/render_widget_host_view_aura.cc

@@ -2249,6 +2249,9 @@ void RenderWidgetHostViewAura::InternalSetBounds(const gfx::Rect& rect) {
   if (!in_bounds_changed_)
     window_->SetBounds(rect);
 
+  // Even if not showing yet, we need to synchronize on size. As the renderer
+  // needs to begin layout. Waiting until we show to start layout leads to
+  // significant delays in embedding the first shown surface (500+ ms.)
   SynchronizeVisualProperties(cc::DeadlinePolicy::UseDefaultDeadline(),
                               window_->GetLocalSurfaceIdAllocation());
 
@@ -2557,14 +2560,21 @@ RenderWidgetHostViewAura::DidUpdateVisualProperties(
 }
 
 void RenderWidgetHostViewAura::DidNavigate() {
-  // The first navigation does not need a new LocalSurfaceID. The renderer can
-  // use the ID that was already provided.
-  if (is_first_navigation_) {
-    SynchronizeVisualProperties(cc::DeadlinePolicy::UseExistingDeadline(),
-                                window_->GetLocalSurfaceIdAllocation());
+  if (!IsShowing()) {
+    // Navigating while hidden should not allocate a new LocalSurfaceID. Once
+    // sizes are ready, or we begin to Show, we can then allocate the new
+    // LocalSurfaceId.
+    window_->InvalidateLocalSurfaceId();
   } else {
-    SynchronizeVisualProperties(cc::DeadlinePolicy::UseExistingDeadline(),
-                                base::nullopt);
+    if (is_first_navigation_) {
+      // The first navigation does not need a new LocalSurfaceID. The renderer
+      // can use the ID that was already provided.
+      SynchronizeVisualProperties(cc::DeadlinePolicy::UseExistingDeadline(),
+                                  window_->GetLocalSurfaceIdAllocation());
+    } else {
+      SynchronizeVisualProperties(cc::DeadlinePolicy::UseExistingDeadline(),
+                                  base::nullopt);
+    }
   }
   if (delegated_frame_host_)
     delegated_frame_host_->DidNavigate();

content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

@@ -5781,6 +5781,9 @@ TEST_F(RenderWidgetHostViewAuraSurfaceSynchronizationTest,
       view_->GetLocalSurfaceIdAllocation().local_surface_id();
   EXPECT_TRUE(id0.is_valid());
 
+  // No LocalSurfaceId will be allocated if the view is hidden during
+  // naviagtion.
+  view_->Show();
   // No new LocalSurfaceId should be allocated for the first navigation and the
   // timer should not fire.
   widget_host_->DidNavigate(1);