Remove lifecycle updates for preferred content size changes

On platforms that need preferred content size updates (MacOS and
Android), an additional blink lifecycle update is issued if layout
changes because ContentsPreferredMinimumSize is called after layout. In
most cases this is unnecessary because layout is already up-to-date.

On platforms that use PreferredSizeChangedMode (e.g, MacOS), the
preferred size is updated after layout (RVI::DidUpdateMainFrameLayout)
and when the PreferredSizeChangedMode setting is enabled. When called
from DidUpdateMainFrameLayout, no lifecycle update is needed. When the
setting is first enabled, this patch now explicitly updates the
lifecycle before updating the preferred size.

On Android (AwRenderViewExt), the preferred size update does not appear
to need to force a lifecycle update because the common path in
AwRenderViewExt::UpdateContentsSize uses DocumentSize which does not
force a layout update.

This patch removes the forced lifecycle update from
ContentsPreferredMinimumSize and adds a DCHECK that is is only called
with up-to-date layout. An integration test has been added that ensures
no extra lifecycle updates occur. This would have caught
https://crbug.com/866981.

This patch changes the expectations for media/video-zoom-controls.html
which was different on MacOS compared to Windows and Linux. The media
controls code is racy with layout and will put up a bad frame (see:
MediaControlsImpl::NotifyElementSizeChanged). Before this patch, the
preferred content size layout timer would occur before the media
controls layout timer and cause different controls to show up on MacOS.
With this patch, the expectation on MacOS now matches other platforms.

Bug: 868983

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I80098edd844bb88e3c64a54cc42b54063ec1c583
Reviewed-on: https://chromium-review.googlesource.com/1153972
Reviewed-by: Philip Rogers <pdr@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581383}

android_webview/renderer/aw_render_view_ext.cc

@@ -22,27 +22,18 @@ void AwRenderViewExt::RenderViewCreated(content::RenderView* render_view) {
 }
 
 void AwRenderViewExt::DidCommitCompositorFrame() {
-  PostCheckContentsSize();
+  UpdateContentsSize();
 }
 
 void AwRenderViewExt::DidUpdateMainFrameLayout() {
-  PostCheckContentsSize();
+  UpdateContentsSize();
 }
 
 void AwRenderViewExt::OnDestruct() {
   delete this;
 }
 
-void AwRenderViewExt::PostCheckContentsSize() {
-  if (check_contents_size_timer_.IsRunning())
-    return;
-
-  check_contents_size_timer_.Start(FROM_HERE,
-      base::TimeDelta::FromMilliseconds(0), this,
-      &AwRenderViewExt::CheckContentsSize);
-}
-
-void AwRenderViewExt::CheckContentsSize() {
+void AwRenderViewExt::UpdateContentsSize() {
   blink::WebView* webview = render_view()->GetWebView();
   content::RenderFrame* main_render_frame = render_view()->GetMainRenderFrame();
 

android_webview/renderer/aw_render_view_ext.h

@@ -27,11 +27,9 @@ class AwRenderViewExt : public content::RenderViewObserver {
   void DidUpdateMainFrameLayout() override;
   void OnDestruct() override;
 
-  void CheckContentsSize();
-  void PostCheckContentsSize();
+  void UpdateContentsSize();
 
   gfx::Size last_sent_contents_size_;
-  base::OneShotTimer check_contents_size_timer_;
 
   DISALLOW_COPY_AND_ASSIGN(AwRenderViewExt);
 };

content/renderer/render_view_browsertest.cc

@@ -414,7 +414,7 @@ class RenderViewImplTest : public RenderViewTest {
   }
 
   const gfx::Size& GetPreferredSize() {
-    view()->CheckPreferredSize();
+    view()->UpdatePreferredSize();
     return view()->preferred_size_;
   }
 

content/renderer/render_view_impl.cc

@@ -569,7 +569,6 @@ void RenderViewImpl::Initialize(
   page_zoom_level_ = 0;
 
   nav_state_sync_timer_.SetTaskRunner(task_runner);
-  check_preferred_size_timer_.SetTaskRunner(std::move(task_runner));
 }
 
 RenderViewImpl::~RenderViewImpl() {
@@ -1667,17 +1666,7 @@ void RenderViewImpl::DidUpdateMainFrameLayout() {
   for (auto& observer : observers_)
     observer.DidUpdateMainFrameLayout();
 
-  // We don't always want to set up a timer, only if we've been put in that
-  // mode by getting a |ViewMsg_EnablePreferredSizeChangedMode|
-  // message.
-  if (!send_preferred_size_changes_ || !webview())
-    return;
-
-  if (check_preferred_size_timer_.IsRunning())
-    return;
-  check_preferred_size_timer_.Start(FROM_HERE,
-                                    TimeDelta::FromMilliseconds(0), this,
-                                    &RenderViewImpl::CheckPreferredSize);
+  UpdatePreferredSize();
 }
 
 void RenderViewImpl::NavigateBackForwardSoon(int offset) {
@@ -1744,7 +1733,7 @@ gfx::RectF RenderViewImpl::ElementBoundsInWindow(
   return gfx::RectF(bounding_box_in_window);
 }
 
-void RenderViewImpl::CheckPreferredSize() {
+void RenderViewImpl::UpdatePreferredSize() {
   // We don't always want to send the change messages over IPC, only if we've
   // been put in that mode by getting a |ViewMsg_EnablePreferredSizeChangedMode|
   // message.
@@ -1861,12 +1850,17 @@ void RenderViewImpl::OnEnablePreferredSizeChangedMode() {
     return;
   send_preferred_size_changes_ = true;
 
-  // Start off with an initial preferred size notification (in case
-  // |DidUpdateMainFrameLayout| was already called).
-  // TODO(pdr): |DidUpdateMainFrameLayout| should only be called with up-to-date
-  // layout but that may not be the case (see: NetworkServiceRestartBrowserTest.
-  // WindowOpenXHR). A lifecycle update should be done before this call.
-  DidUpdateMainFrameLayout();
+  if (!webview())
+    return;
+
+  // We need to ensure |UpdatePreferredSize| gets called. If a layout is needed,
+  // force an update here which will call |DidUpdateMainFrameLayout|.
+  webview()->UpdateLifecycle(WebWidget::LifecycleUpdate::kLayout);
+
+  // If a layout was not needed, |DidUpdateMainFrameLayout| will not be called.
+  // We explicitly update the preferred size here to ensure the preferred size
+  // notification is sent.
+  UpdatePreferredSize();
 }
 
 void RenderViewImpl::OnDisableScrollbarsForSmallWindows(

content/renderer/render_view_impl.h

@@ -480,8 +480,9 @@ class CONTENT_EXPORT RenderViewImpl : private RenderWidget,
   // and put it in the same position in the .cc file.
 
   // Misc private functions ----------------------------------------------------
-  // Check whether the preferred size has changed.
-  void CheckPreferredSize();
+  // Check whether the preferred size has changed. This should only be called
+  // with up-to-date layout.
+  void UpdatePreferredSize();
 
 #if defined(OS_ANDROID)
   // Make the video capture devices (e.g. webcam) stop/resume delivering video
@@ -640,10 +641,6 @@ class CONTENT_EXPORT RenderViewImpl : private RenderWidget,
   // when layout() recomputes but doesn't actually change sizes.
   gfx::Size preferred_size_;
 
-  // Used to delay determining the preferred size (to avoid intermediate
-  // states for the sizes).
-  base::OneShotTimer check_preferred_size_timer_;
-
   // Used to indicate the zoom level to be used during subframe loads, since
   // they should match page zoom level.
   double page_zoom_level_ = 0;

third_party/WebKit/LayoutTests/VirtualTestSuites

@@ -29,6 +29,11 @@
     "base": "fast/animationworklet",
     "args": ["--enable-threaded-compositing"]
   },
+  {
+    "prefix": "threaded",
+    "base": "lifecycle",
+    "args": ["--enable-threaded-compositing"]
+  },
   {
     "prefix": "threaded",
     "base": "http/tests/worklet",

third_party/WebKit/LayoutTests/lifecycle/background-change-lifecycle-count.html

@@ -0,0 +1,52 @@
+<!doctype html>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+
+<div id="box" style="width: 100px; height: 100px; background: blue;"></div>
+
+<script>
+  var t = async_test('Integration test for background style change updates');
+
+  t.step(function() {
+    assert_true(!!window.internals, 'Test requires window.internals');
+  });
+
+  // Loading is non-deterministic so record the initial count after onload
+  // plus a paint.
+  onload = function() {
+    requestAnimationFrame(function() {
+      // Windows flakily sends mouse events on the first frame so wait an
+      // additional frame before starting the test.
+      requestAnimationFrame(function() {
+        var initialLifecycleCount = internals.LifecycleUpdateCount();
+
+        box.style.background = 'green';
+
+        requestAnimationFrame(function() {
+          var postChangeCount = internals.LifecycleUpdateCount();
+          t.step(function() {
+            // A background style change should have only resulted in a single
+            // lifecycle update. If this ever fails, something is causing an
+            // unnecessary lifecycle update.
+            assert_equals(postChangeCount, initialLifecycleCount + 1,
+                'a style change should cause one lifecycle update');
+          });
+
+          // A timeout is used so we can capture unnecessary updates. 250ms was
+          // chosen because it is the lowest value that would reliably fail for
+          // https://crbug.com/866981 and https://crbug.com/868983 on bots; lower
+          // values would just become flaky.
+          setTimeout(function() {
+            t.step(function() {
+              // The timeout should cause a single additional update. If this ever
+              // fails, something is causing an unnecessary lifecycle update.
+              assert_equals(internals.LifecycleUpdateCount(), postChangeCount + 1,
+                  'a timeout should cause one lifecycle update');
+            });
+            t.done();
+          }, 250);
+        });
+      });
+    });
+  }
+</script>

third_party/WebKit/LayoutTests/platform/mac/media/video-zoom-controls-expected.txt

@@ -15,10 +15,22 @@ layer at (57,85) size 240x180
   LayoutBlockFlow (positioned) {DIV} at (0,0) size 240x180
 layer at (57,85) size 240x180
   LayoutFlexibleBox {DIV} at (0,0) size 240x180
-layer at (57,85) size 240x228 backgroundClip at (57,85) size 240x180 clip at (59,87) size 237x178
-  LayoutButton (relative positioned) {INPUT} at (0,0) size 240x228 [border: (1.50px solid #D8D8D8) (1.50px solid #D1D1D1) (1.50px solid #BABABA) (1.50px solid #D1D1D1)]
-    LayoutFlexibleBox (anonymous) at (12,77.25) size 216x72
+layer at (57,85) size 240x156 clip at (59,87) size 237x153
+  LayoutButton (relative positioned) {INPUT} at (0,0) size 240x156 [border: (1.50px solid #D8D8D8) (1.50px solid #D1D1D1) (1.50px solid #BABABA) (1.50px solid #D1D1D1)]
+    LayoutFlexibleBox (anonymous) at (12,41.25) size 216x72
       LayoutBlockFlow {DIV} at (72,0) size 72x72 [bgcolor=#FFFFFFE6]
+layer at (57,169) size 240x72 scrollWidth 267
+  LayoutFlexibleBox (relative positioned) {DIV} at (0,84) size 240x72
+    LayoutBlockFlow {DIV} at (24,0) size 46.72x72 [color=#FFFFFF]
+      LayoutText {#text} at (0,22) size 47x28
+        text run at (0,22) width 47: "0:00"
+    LayoutBlockFlow {DIV} at (76.72,-72) size 46.72x144 [color=#FFFFFF]
+      LayoutText {#text} at (0,22) size 47x100
+        text run at (0,22) width 7: "/"
+        text run at (0,94) width 47: "0:06"
+    LayoutBlockFlow {DIV} at (123.44,72) size 0x0
+    LayoutButton {INPUT} at (123.44,0) size 72x72
+    LayoutButton {INPUT} at (195.44,0) size 72x72
 layer at (57,241) size 240x24
   LayoutSlider {INPUT} at (0,156) size 240x24 [color=#909090]
     LayoutFlexibleBox {DIV} at (24,0) size 192x6
@@ -41,10 +53,22 @@ layer at (57,310) size 240x180
   LayoutBlockFlow (positioned) {DIV} at (0,0) size 240x180
 layer at (57,310) size 240x180
   LayoutFlexibleBox {DIV} at (0,0) size 240x180
-layer at (57,310) size 240x228 backgroundClip at (43,291) size 268x218 clip at (59,312) size 237x197
-  LayoutButton (relative positioned) {INPUT} at (0,0) size 240x228 [border: (1.50px solid #D8D8D8) (1.50px solid #D1D1D1) (1.50px solid #BABABA) (1.50px solid #D1D1D1)]
-    LayoutFlexibleBox (anonymous) at (12,77.25) size 216x72
+layer at (57,310) size 240x156 clip at (59,312) size 237x153
+  LayoutButton (relative positioned) {INPUT} at (0,0) size 240x156 [border: (1.50px solid #D8D8D8) (1.50px solid #D1D1D1) (1.50px solid #BABABA) (1.50px solid #D1D1D1)]
+    LayoutFlexibleBox (anonymous) at (12,41.25) size 216x72
       LayoutBlockFlow {DIV} at (72,0) size 72x72 [bgcolor=#FFFFFFE6]
+layer at (57,394) size 240x72 backgroundClip at (47,373) size 249x113 clip at (57,394) size 239x72 scrollWidth 267
+  LayoutFlexibleBox (relative positioned) {DIV} at (0,84) size 240x72
+    LayoutBlockFlow {DIV} at (24,0) size 46.72x72 [color=#FFFFFF]
+      LayoutText {#text} at (0,22) size 47x28
+        text run at (0,22) width 47: "0:00"
+    LayoutBlockFlow {DIV} at (76.72,-72) size 46.72x144 [color=#FFFFFF]
+      LayoutText {#text} at (0,22) size 47x100
+        text run at (0,22) width 7: "/"
+        text run at (0,94) width 47: "0:06"
+    LayoutBlockFlow {DIV} at (123.44,72) size 0x0
+    LayoutButton {INPUT} at (123.44,0) size 72x72
+    LayoutButton {INPUT} at (195.44,0) size 72x72
 layer at (57,466) size 240x24
   LayoutSlider {INPUT} at (0,156) size 240x24 [color=#909090]
     LayoutFlexibleBox {DIV} at (24,0) size 192x6

third_party/WebKit/LayoutTests/platform/mac/virtual/video-surface-layer/media/video-zoom-controls-expected.txt

@@ -0,0 +1,86 @@
+layer at (0,0) size 800x600
+  LayoutView at (0,0) size 800x600
+layer at (0,0) size 800x600
+  LayoutBlockFlow {HTML} at (0,0) size 800x600
+    LayoutBlockFlow {BODY} at (12,12) size 776x543
+      LayoutBlockFlow {P} at (0,0) size 776x28
+        LayoutText {#text} at (0,0) size 278x28
+          text run at (0,0) width 278: "Zoomed video with controls."
+layer at (57,85) size 240x180
+  LayoutVideo {VIDEO} at (45,73) size 240x180
+layer at (57,85) size 240x180
+  LayoutFlexibleBox (relative positioned) {DIV} at (0,0) size 240x180
+    LayoutFlexibleBox {DIV} at (0,0) size 240x180
+layer at (57,85) size 240x180
+  LayoutBlockFlow (positioned) {DIV} at (0,0) size 240x180
+layer at (57,85) size 240x180
+  LayoutFlexibleBox {DIV} at (0,0) size 240x180
+layer at (57,85) size 240x156 clip at (59,87) size 237x153
+  LayoutButton (relative positioned) {INPUT} at (0,0) size 240x156 [border: (1.50px solid #D8D8D8) (1.50px solid #D1D1D1) (1.50px solid #BABABA) (1.50px solid #D1D1D1)]
+    LayoutFlexibleBox (anonymous) at (12,41.25) size 216x72
+      LayoutBlockFlow {DIV} at (72,0) size 72x72 [bgcolor=#FFFFFFE6]
+layer at (57,169) size 240x72 scrollWidth 339
+  LayoutFlexibleBox (relative positioned) {DIV} at (0,84) size 240x72
+    LayoutBlockFlow {DIV} at (24,0) size 46.72x72 [color=#FFFFFF]
+      LayoutText {#text} at (0,22) size 47x28
+        text run at (0,22) width 47: "0:00"
+    LayoutBlockFlow {DIV} at (76.72,-72) size 46.72x144 [color=#FFFFFF]
+      LayoutText {#text} at (0,22) size 47x100
+        text run at (0,22) width 7: "/"
+        text run at (0,94) width 47: "0:06"
+    LayoutBlockFlow {DIV} at (123.44,72) size 0x0
+    LayoutButton {INPUT} at (123.44,0) size 72x72
+    LayoutButton {INPUT} at (195.44,0) size 72x72
+    LayoutButton {INPUT} at (267.44,0) size 72x72
+layer at (57,241) size 240x24
+  LayoutSlider {INPUT} at (0,156) size 240x24 [color=#909090]
+    LayoutFlexibleBox {DIV} at (24,0) size 192x6
+layer at (81,241) size 192x6
+  LayoutBlockFlow (relative positioned) {DIV} at (0,0) size 192x6 [bgcolor=#FFFFFF4D]
+layer at (81,235) size 18x18
+  LayoutBlockFlow (relative positioned) {DIV} at (0,-6) size 18x18 [bgcolor=#FFFFFF]
+layer at (81,241) size 192x6 scrollWidth 288
+  LayoutBlockFlow (positioned) {DIV} at (0,0) size 192x6
+layer at (81,241) size 0x6
+  LayoutBlockFlow (positioned) {DIV} at (0,0) size 0x6 [bgcolor=#FFFFFF]
+layer at (81,241) size 288x6 backgroundClip at (81,241) size 192x6 clip at (81,241) size 192x6
+  LayoutBlockFlow (positioned) {DIV} at (0,0) size 288x6 [bgcolor=#FFFFFF8A]
+layer at (57,310) size 240x180
+  LayoutVideo {VIDEO} at (45,298) size 240x180
+layer at (57,310) size 240x180
+  LayoutFlexibleBox (relative positioned) {DIV} at (0,0) size 240x180
+    LayoutFlexibleBox {DIV} at (0,0) size 240x180
+layer at (57,310) size 240x180
+  LayoutBlockFlow (positioned) {DIV} at (0,0) size 240x180
+layer at (57,310) size 240x180
+  LayoutFlexibleBox {DIV} at (0,0) size 240x180
+layer at (57,310) size 240x156 clip at (59,312) size 237x153
+  LayoutButton (relative positioned) {INPUT} at (0,0) size 240x156 [border: (1.50px solid #D8D8D8) (1.50px solid #D1D1D1) (1.50px solid #BABABA) (1.50px solid #D1D1D1)]
+    LayoutFlexibleBox (anonymous) at (12,41.25) size 216x72
+      LayoutBlockFlow {DIV} at (72,0) size 72x72 [bgcolor=#FFFFFFE6]
+layer at (57,394) size 240x72 backgroundClip at (47,373) size 249x113 clip at (57,394) size 239x72 scrollWidth 339
+  LayoutFlexibleBox (relative positioned) {DIV} at (0,84) size 240x72
+    LayoutBlockFlow {DIV} at (24,0) size 46.72x72 [color=#FFFFFF]
+      LayoutText {#text} at (0,22) size 47x28
+        text run at (0,22) width 47: "0:00"
+    LayoutBlockFlow {DIV} at (76.72,-72) size 46.72x144 [color=#FFFFFF]
+      LayoutText {#text} at (0,22) size 47x100
+        text run at (0,22) width 7: "/"
+        text run at (0,94) width 47: "0:06"
+    LayoutBlockFlow {DIV} at (123.44,72) size 0x0
+    LayoutButton {INPUT} at (123.44,0) size 72x72
+    LayoutButton {INPUT} at (195.44,0) size 72x72
+    LayoutButton {INPUT} at (267.44,0) size 72x72
+layer at (57,466) size 240x24
+  LayoutSlider {INPUT} at (0,156) size 240x24 [color=#909090]
+    LayoutFlexibleBox {DIV} at (24,0) size 192x6
+layer at (81,466) size 192x6
+  LayoutBlockFlow (relative positioned) {DIV} at (0,0) size 192x6 [bgcolor=#FFFFFF4D]
+layer at (81,460) size 18x18
+  LayoutBlockFlow (relative positioned) {DIV} at (0,-6) size 18x18 [bgcolor=#FFFFFF]
+layer at (81,466) size 192x6 backgroundClip at (70,448) size 190x40 clip at (81,466) size 179x6 scrollWidth 288
+  LayoutBlockFlow (positioned) {DIV} at (0,0) size 192x6
+layer at (81,466) size 0x6
+  LayoutBlockFlow (positioned) {DIV} at (0,0) size 0x6 [bgcolor=#FFFFFF]
+layer at (81,466) size 288x6 backgroundClip at (70,448) size 190x40 clip at (70,448) size 190x40
+  LayoutBlockFlow (positioned) {DIV} at (0,0) size 288x6 [bgcolor=#FFFFFF8A]

third_party/WebKit/LayoutTests/virtual/threaded/lifecycle/README.txt

@@ -0,0 +1 @@
+# This suite runs only the tests in this directory with --enable-threaded-compositing

third_party/blink/public/web/web_view.h

@@ -72,13 +72,15 @@ class WebView : protected WebWidget {
 
   // WebWidget overrides.
   using WebWidget::Close;
+  using WebWidget::LifecycleUpdate;
   using WebWidget::Size;
   using WebWidget::Resize;
   using WebWidget::ResizeVisualViewport;
   using WebWidget::DidEnterFullscreen;
   using WebWidget::DidExitFullscreen;
   using WebWidget::BeginFrame;
   using WebWidget::UpdateAllLifecyclePhases;
+  using WebWidget::UpdateLifecycle;
   using WebWidget::PaintContent;
   using WebWidget::LayoutAndPaintAsync;
   using WebWidget::CompositeAndReadbackAsync;
@@ -266,7 +268,8 @@ class WebView : protected WebWidget {
   // Returns the "preferred" contents size, defined as the preferred minimum
   // width of the main document's contents and the minimum height required to
   // display the main document without scrollbars.  The returned size has the
-  // page zoom factor applied.
+  // page zoom factor applied. The lifecycle must be updated to at least layout
+  // before calling this (see: |UpdateLifecycle|).
   virtual WebSize ContentsPreferredMinimumSize() = 0;
 
   // Sets the display mode of the web app.

third_party/blink/public/web/web_widget.h

@@ -92,10 +92,10 @@ class WebWidget {
   // and it may result in calls to WebWidgetClient::didInvalidateRect.
   virtual void UpdateAllLifecyclePhases() { UpdateLifecycle(); }
 
-  // Selectively runs all lifecycle phases or all phases excluding paint. The
-  // latter can be used to trigger side effects of updating layout and
-  // animations if painting is not required.
-  enum class LifecycleUpdate { kPrePaint, kAll };
+  // By default, all phases are updated by |UpdateLifecycle| (e.g., style,
+  // layout, prepaint, paint, etc. See: document_lifecycle.h). |LifecycleUpdate|
+  // can be used to only update to a specific lifecycle phase.
+  enum class LifecycleUpdate { kLayout, kPrePaint, kAll };
   virtual void UpdateLifecycle(
       LifecycleUpdate requested_update = LifecycleUpdate::kAll) {}