WebView SurfaceSyncThrottling Fullscreen Fix

WebView Fullscreen transitions are being signalled incorrectly in two
steps of ViewAndroid::OnPhysicalBackingSizeChanged.
SurfaceSyncThrottling ends up detecting this as a step in a rotation a
we incorrectly wait for a subsequent resize that never comes. This leads
to displaying a blank space under the fullscreen content.

This patch relands a portion of the non-SurfaceSyncThrottling path that
was removed in:
  https://chromium-review.googlesource.com/c/chromium/src/+/3445360
Just the functional logic, and feature flag. The additional metricing
is excluded. The revert itself was not landed directly as it was not
clean. This is being relanded to allow us to control the rollout on
WebView in M100.

This patch also fixes the error, by clearing fullscreen rotation flag
upon the upon the activation of RenderFrameMetadata for rotations.
Allowing the WebView double signalling to not trigger the throttling.

(cherry picked from commit 33dc7bb)

Bug: 1302964
Change-Id: Id323e6bbab8c45fc96c7b1143850c9ce32dd6421
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3510288
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#978969}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3522705
Auto-Submit: Jonathan Ross <jonross@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4896@{#535}
Cr-Branched-From: 1f63ff4-refs/heads/main@{#972766}

cc/trees/layer_tree_host.cc

@@ -1311,7 +1311,7 @@ void LayerTreeHost::SetViewportRectAndScale(
   // If a new viz::LocalSurfaceId has been provided, and the viewport has
   // changed, we need not begin new frames until it has activated.
   if (previous_local_surface_id != local_surface_id_from_parent &&
-      device_viewport_rect_changed) {
+      device_viewport_rect_changed && features::IsSurfaceSyncThrottling()) {
     SetTargetLocalSurfaceId(local_surface_id_from_parent);
   }
 

components/viz/common/features.cc

@@ -155,6 +155,12 @@ const base::Feature kUseRealVideoColorSpaceForDisplay{
     "UseRealVideoColorSpaceForDisplay", base::FEATURE_DISABLED_BY_DEFAULT};
 #endif
 
+// Used by CC to throttle frame production of older surfaces. Used by the
+// Browser to batch SurfaceSync calls sent to the Renderer for properties can
+// change in close proximity to each other.
+const base::Feature kSurfaceSyncThrottling{"SurfaceSyncThrottling",
+                                           base::FEATURE_ENABLED_BY_DEFAULT};
+
 const base::Feature kDrawPredictedInkPoint{"DrawPredictedInkPoint",
                                            base::FEATURE_DISABLED_BY_DEFAULT};
 const char kDraw1Point12Ms[] = "1-pt-12ms";
@@ -326,6 +332,10 @@ bool UseRealVideoColorSpaceForDisplay() {
 }
 #endif
 
+bool IsSurfaceSyncThrottling() {
+  return base::FeatureList::IsEnabled(kSurfaceSyncThrottling);
+}
+
 // Used by Viz to determine if viz::DisplayScheduler should dynamically adjust
 // its frame deadline. Returns the percentile of historic draw times to base the
 // deadline on. Or absl::nullopt if the feature is disabled.

components/viz/common/features.h

@@ -49,6 +49,7 @@ VIZ_COMMON_EXPORT extern const base::Feature kUsePlatformDelegatedInk;
 #if BUILDFLAG(IS_ANDROID)
 VIZ_COMMON_EXPORT extern const base::Feature kUseSurfaceLayerForVideoDefault;
 #endif
+VIZ_COMMON_EXPORT extern const base::Feature kSurfaceSyncThrottling;
 VIZ_COMMON_EXPORT extern const base::Feature kDynamicSchedulerForDraw;
 VIZ_COMMON_EXPORT extern const base::Feature kDynamicSchedulerForClients;
 #if BUILDFLAG(IS_MAC)
@@ -92,6 +93,7 @@ VIZ_COMMON_EXPORT bool ShouldUsePlatformDelegatedInk();
 VIZ_COMMON_EXPORT bool UseSurfaceLayerForVideo();
 VIZ_COMMON_EXPORT bool UseRealVideoColorSpaceForDisplay();
 #endif
+VIZ_COMMON_EXPORT bool IsSurfaceSyncThrottling();
 VIZ_COMMON_EXPORT absl::optional<double> IsDynamicSchedulerEnabledForDraw();
 VIZ_COMMON_EXPORT absl::optional<double> IsDynamicSchedulerEnabledForClients();
 VIZ_COMMON_EXPORT int MaxOverlaysConsidered();

content/browser/renderer_host/render_widget_host_view_android.cc

@@ -243,7 +243,8 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
       page_scale_(1.f),
       min_page_scale_(1.f),
       max_page_scale_(1.f),
-      mouse_wheel_phase_handler_(this) {
+      mouse_wheel_phase_handler_(this),
+      is_surface_sync_throttling_(features::IsSurfaceSyncThrottling()) {
   // Set the layer which will hold the content layer for this view. The content
   // layer is managed by the DelegatedFrameHost.
   view_.SetLayer(cc::Layer::Create());
@@ -1207,7 +1208,7 @@ bool RenderWidgetHostViewAndroid::CanSynchronizeVisualProperties() {
   //
   // We should instead wait for the full set of new visual properties to be
   // available, and deliver them to the Renderer in one single update.
-  if (in_rotation_)
+  if (in_rotation_ && is_surface_sync_throttling_)
     return false;
   return true;
 }
@@ -1593,7 +1594,7 @@ void RenderWidgetHostViewAndroid::ShowInternal() {
       navigation_while_hidden_ = false;
       delegated_frame_host_->DidNavigate();
     }
-  } else if (rotation_override) {
+  } else if (rotation_override && is_surface_sync_throttling_) {
     // If a rotation occurred while this was not visible, we need to allocate a
     // new viz::LocalSurfaceId and send the current visual properties to the
     // Renderer. Otherwise there will be no content at all to display.
@@ -2784,6 +2785,10 @@ void RenderWidgetHostViewAndroid::BeginRotationBatching() {
 
 void RenderWidgetHostViewAndroid::EndRotationBatching() {
   in_rotation_ = false;
+  // Always clear when ending batching. As WebView can trigger multiple
+  // OnPhysicalBackingSizeChanged which would re-trigger rotation if we were
+  // still tracking `fullscreen_rotation_`. crbug.com/1302964
+  fullscreen_rotation_ = false;
   DCHECK(!rotation_metrics_.empty());
   const auto delta = rotation_metrics_.back().first - base::TimeTicks();
   TRACE_EVENT_NESTABLE_ASYNC_END1(

content/browser/renderer_host/render_widget_host_view_android.h

@@ -620,6 +620,8 @@ class CONTENT_EXPORT RenderWidgetHostViewAndroid
 
   base::android::ScopedJavaGlobalRef<jobject> obj_;
 
+  bool is_surface_sync_throttling_ = false;
+
   base::WeakPtrFactory<RenderWidgetHostViewAndroid> weak_ptr_factory_{this};
 };
 

content/browser/renderer_host/render_widget_host_view_android_unittest.cc

@@ -386,12 +386,12 @@ TEST_F(RenderWidgetHostViewAndroidTest, RenderFrameSubmittedBeforeNavigation) {
   EXPECT_TRUE(post_nav_local_surface_id.IsNewerThan(initial_local_surface_id));
 }
 
-// Tests Rotation improvements, where multiple updates to VisualProperties are
-// batched.
+// Tests Rotation improvements that are behind the
+// features::kSurfaceSyncThrottling flag.
 class RenderWidgetHostViewAndroidRotationTest
     : public RenderWidgetHostViewAndroidTest {
  public:
-  RenderWidgetHostViewAndroidRotationTest() = default;
+  RenderWidgetHostViewAndroidRotationTest();
   ~RenderWidgetHostViewAndroidRotationTest() override {}
 
   void OnDidUpdateVisualPropertiesComplete(
@@ -402,8 +402,16 @@ class RenderWidgetHostViewAndroidRotationTest
   RenderWidgetHostViewAndroid* CreateRenderWidgetHostViewAndroid(
       RenderWidgetHostImpl* widget_host,
       gfx::NativeView parent_native_view) override;
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
+RenderWidgetHostViewAndroidRotationTest::
+    RenderWidgetHostViewAndroidRotationTest() {
+  scoped_feature_list_.InitAndEnableFeature(features::kSurfaceSyncThrottling);
+}
+
 void RenderWidgetHostViewAndroidRotationTest::
     OnDidUpdateVisualPropertiesComplete(
         const cc::RenderFrameMetadata& metadata) {
@@ -624,19 +632,34 @@ TEST_F(RenderWidgetHostViewAndroidRotationTest, FullscreenRotation) {
   }
 
   // When exiting rotation the order of visual property updates can differ.
+  // Though we need to always allow Surface Sync to continue, as WebView will
+  // send two OnPhysicalBackingSizeChanged in a row to exit fullscreen. While
+  // non-WebView can alternate some combination of 1-2
+  // OnPhysicalBackingSizeChanged and OnSynchronizedDisplayPropertiesChanged.
   delegate()->set_is_fullscreen(false);
   rwhva->OnPhysicalBackingSizeChanged(/* deadline_override= */ absl::nullopt);
-  EXPECT_FALSE(rwhva->CanSynchronizeVisualProperties());
-  EXPECT_EQ(post_rotation_local_surface_id, rwhva->GetLocalSurfaceId());
-
-  rwhva->OnSynchronizedDisplayPropertiesChanged(/* rotation= */ true);
   EXPECT_TRUE(rwhva->CanSynchronizeVisualProperties());
   const viz::LocalSurfaceId post_fullscreen_local_surface_id =
       rwhva->GetLocalSurfaceId();
   EXPECT_NE(post_rotation_local_surface_id, post_fullscreen_local_surface_id);
   EXPECT_TRUE(post_fullscreen_local_surface_id.is_valid());
   EXPECT_TRUE(post_fullscreen_local_surface_id.IsNewerThan(
       post_rotation_local_surface_id));
+
+  // When rotation begins again it should block Surface Sync again.
+  rwhva->OnSynchronizedDisplayPropertiesChanged(/* rotation= */ true);
+  EXPECT_FALSE(rwhva->CanSynchronizeVisualProperties());
+  EXPECT_EQ(post_fullscreen_local_surface_id, rwhva->GetLocalSurfaceId());
+
+  // When rotation has completed we should begin Surface Sync again.
+  rwhva->OnPhysicalBackingSizeChanged(/* deadline_override= */ absl::nullopt);
+  const viz::LocalSurfaceId post_fullscreen_rotation_local_surface_id =
+      rwhva->GetLocalSurfaceId();
+  EXPECT_NE(post_fullscreen_local_surface_id,
+            post_fullscreen_rotation_local_surface_id);
+  EXPECT_TRUE(post_fullscreen_rotation_local_surface_id.is_valid());
+  EXPECT_TRUE(post_fullscreen_rotation_local_surface_id.IsNewerThan(
+      post_fullscreen_local_surface_id));
   EXPECT_TRUE(rwhva->CanSynchronizeVisualProperties());
 
   {