TopControlsSlide: Allow only a single BrowserView::Layout() once sliding begins

If BrowserView::Layout() gets called multiple times while sliding
the top controls is in progress, it could break sliding altogether,
or at least will make its initiation janky.

This CL allows only a single layout to go through once sliding begins.

BUG=1034047, 950981
TEST=Manually, and expanded an existing test.

Change-Id: I7b906fc2956aa455b573083d7cdaa40812141bfb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1968320Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726637}

chrome/browser/ui/views/frame/browser_view.cc

@@ -2415,6 +2415,17 @@ void BrowserView::Layout() {
   if (!initialized_ || in_process_fullscreen_)
     return;
 
+  // Allow only a single layout operation once top controls sliding begins.
+  if (top_controls_slide_controller_ &&
+      top_controls_slide_controller_->IsEnabled() &&
+      top_controls_slide_controller_->IsTopControlsSlidingInProgress()) {
+    if (did_first_layout_while_top_controls_are_sliding_)
+      return;
+    did_first_layout_while_top_controls_are_sliding_ = true;
+  } else {
+    did_first_layout_while_top_controls_are_sliding_ = false;
+  }
+
   views::View::Layout();
 
   // TODO(jamescook): Why was this in the middle of layout code?

chrome/browser/ui/views/frame/browser_view.h

@@ -837,6 +837,14 @@ class BrowserView : public BrowserWindow,
   // This is non-null on Chrome OS only.
   std::unique_ptr<TopControlsSlideController> top_controls_slide_controller_;
 
+  // Used to allow a single layout operation once the top controls slide
+  // behavior starts. This needed since sliding the top controls and the page
+  // contents is done using layer transform. A layout operation while sliding is
+  // in progress might break the view, and will make sliding feel janky.
+  // A single layout is needed right before sliding begins. (See
+  // TopControlsSlideControllerChromeOS::OnBeginSliding()).
+  bool did_first_layout_while_top_controls_are_sliding_ = false;
+
   std::unique_ptr<ImmersiveModeController> immersive_mode_controller_;
 
   ScopedObserver<ui::MaterialDesignController,

chrome/browser/ui/views/frame/top_controls_slide_controller.h

@@ -53,6 +53,10 @@ class TopControlsSlideController {
   // Returns true while gesture scrolls are in progress.
   virtual bool IsTopControlsGestureScrollInProgress() const = 0;
 
+  // Returns true while the top controls are sliding up or down, and hasn't
+  // reached a final steady state.
+  virtual bool IsTopControlsSlidingInProgress() const = 0;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(TopControlsSlideController);
 };

chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos.cc

@@ -4,7 +4,10 @@
 
 #include "chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos.h"
 
+#include <vector>
+
 #include "ash/public/cpp/tablet_mode.h"
+#include "base/auto_reset.h"
 #include "base/bind.h"
 #include "chrome/browser/permissions/permission_request_manager.h"
 #include "chrome/browser/search/search.h"
@@ -27,7 +30,9 @@
 #include "content/public/common/browser_controls_state.h"
 #include "extensions/common/constants.h"
 #include "ui/aura/window.h"
+#include "ui/aura/window_tree_host.h"
 #include "ui/compositor/scoped_layer_animation_settings.h"
+#include "ui/display/screen.h"
 #include "ui/views/controls/native/native_view_host.h"
 
 namespace {
@@ -299,6 +304,7 @@ TopControlsSlideControllerChromeOS::TopControlsSlideControllerChromeOS(
     ash::TabletMode::Get()->AddObserver(this);
 
   browser_view_->browser()->tab_strip_model()->AddObserver(this);
+  display::Screen::GetScreen()->AddObserver(this);
 
   chromeos::AccessibilityManager* accessibility_manager =
       chromeos::AccessibilityManager::Get();
@@ -315,6 +321,7 @@ TopControlsSlideControllerChromeOS::TopControlsSlideControllerChromeOS(
 TopControlsSlideControllerChromeOS::~TopControlsSlideControllerChromeOS() {
   OnEnabledStateChanged(false);
 
+  display::Screen::GetScreen()->RemoveObserver(this);
   browser_view_->browser()->tab_strip_model()->RemoveObserver(this);
 
   if (ash::TabletMode::Get())
@@ -334,6 +341,9 @@ void TopControlsSlideControllerChromeOS::SetShownRatio(
     float ratio) {
   DCHECK(contents);
 
+  if (pause_updates_)
+    return;
+
   // Make sure the value tracked per tab is always updated even when sliding is
   // disabled, so that we're always synchronized with the renderer.
   DCHECK(observed_tabs_.count(contents));
@@ -362,7 +372,7 @@ void TopControlsSlideControllerChromeOS::SetShownRatio(
     return;
   }
 
-  // Skip |shown_ratio_| update if the changes is not from the active
+  // Skip |shown_ratio_| update if the changes are not from the active
   // WebContents.
   if (contents != browser_view_->GetActiveWebContents())
     return;
@@ -406,8 +416,19 @@ bool TopControlsSlideControllerChromeOS::DoBrowserControlsShrinkRendererSize(
 
 void TopControlsSlideControllerChromeOS::SetTopControlsGestureScrollInProgress(
     bool in_progress) {
+  if (is_gesture_scrolling_in_progress_ == in_progress)
+    return;
+
   is_gesture_scrolling_in_progress_ = in_progress;
 
+  if (update_state_after_gesture_scrolling_ends_) {
+    DCHECK(!is_gesture_scrolling_in_progress_);
+    DCHECK(pause_updates_);
+    OnEnabledStateChanged(CanEnable(base::nullopt));
+    update_state_after_gesture_scrolling_ends_ = false;
+    pause_updates_ = false;
+  }
+
   if (!IsEnabled())
     return;
 
@@ -446,6 +467,11 @@ bool TopControlsSlideControllerChromeOS::IsTopControlsGestureScrollInProgress()
   return is_gesture_scrolling_in_progress_;
 }
 
+bool TopControlsSlideControllerChromeOS::IsTopControlsSlidingInProgress()
+    const {
+  return is_sliding_in_progress_;
+}
+
 void TopControlsSlideControllerChromeOS::OnTabletModeStarted() {
   OnEnabledStateChanged(CanEnable(base::nullopt));
 }
@@ -523,6 +549,54 @@ void TopControlsSlideControllerChromeOS::Observe(
     UpdateBrowserControlsStateShown(active_contents, true /* animate */);
 }
 
+void TopControlsSlideControllerChromeOS::OnDisplayMetricsChanged(
+    const display::Display& display,
+    uint32_t changed_metrics) {
+  if (!IsEnabled())
+    return;
+
+  if (!is_sliding_in_progress_ || !is_gesture_scrolling_in_progress_)
+    return;
+
+  // If any of the below display metrics changes while both sliding and gesture
+  // scrolling are in progress, we force-set the top controls to be fully shown,
+  // and temporarily disables the state of the top controls sliding feature
+  // until the user lifts their finger to end gesture scrolling, at which point
+  // we set it back to its correct value.
+  // This is necessary, since this way the browser view will layout properly,
+  // avoiding having a broken page or a broken browser view if one of the below
+  // changes happen while the top controls are not in a steady state.
+  constexpr int kCheckedMetrics =
+      display::DisplayObserver::DISPLAY_METRIC_BOUNDS |
+      display::DisplayObserver::DISPLAY_METRIC_WORK_AREA |
+      display::DisplayObserver::DISPLAY_METRIC_DEVICE_SCALE_FACTOR |
+      display::DisplayObserver::DISPLAY_METRIC_ROTATION |
+      display::DisplayObserver::DISPLAY_METRIC_PRIMARY |
+      display::DisplayObserver::DISPLAY_METRIC_MIRROR_STATE;
+
+  if ((changed_metrics & kCheckedMetrics) == 0)
+    return;
+
+  if (browser_view_->GetNativeWindow()->GetHost()->GetDisplayId() !=
+      display.id()) {
+    return;
+  }
+
+  content::WebContents* active_contents = browser_view_->GetActiveWebContents();
+  if (!active_contents)
+    return;
+
+  update_state_after_gesture_scrolling_ends_ = true;
+  {
+    // Setting |is_gesture_scrolling_in_progress_| to false temporarily will end
+    // the sliding when we set the shown ratio to a terminal value of 1.f.
+    base::AutoReset<bool> resetter{&is_gesture_scrolling_in_progress_, false};
+    SetShownRatio(active_contents, 1.f);
+  }
+  pause_updates_ = true;
+  OnEnabledStateChanged(false);
+}
+
 bool TopControlsSlideControllerChromeOS::CanEnable(
     base::Optional<bool> fullscreen_state) const {
   return IsTabletModeEnabled() &&

chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos.h

@@ -16,6 +16,7 @@
 #include "chrome/browser/ui/views/frame/top_controls_slide_controller.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "ui/display/display_observer.h"
 
 class BrowserView;
 class TopControlsSlideTabObserver;
@@ -36,11 +37,11 @@ class TopControlsSlideTabObserver;
 // - Exiting tablet mode.
 // - Entering immersive fullscreen mode.
 // - Page security level changes.
-class TopControlsSlideControllerChromeOS
-    : public TopControlsSlideController,
-      public ash::TabletModeObserver,
-      public TabStripModelObserver,
-      public content::NotificationObserver {
+class TopControlsSlideControllerChromeOS : public TopControlsSlideController,
+                                           public ash::TabletModeObserver,
+                                           public TabStripModelObserver,
+                                           public content::NotificationObserver,
+                                           public display::DisplayObserver {
  public:
   explicit TopControlsSlideControllerChromeOS(BrowserView* browser_view);
   ~TopControlsSlideControllerChromeOS() override;
@@ -54,6 +55,7 @@ class TopControlsSlideControllerChromeOS
       const content::WebContents* contents) const override;
   void SetTopControlsGestureScrollInProgress(bool in_progress) override;
   bool IsTopControlsGestureScrollInProgress() const override;
+  bool IsTopControlsSlidingInProgress() const override;
 
   // ash::TabletModeObserver:
   void OnTabletModeStarted() override;
@@ -71,6 +73,10 @@ class TopControlsSlideControllerChromeOS
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
+  // display::DisplayObserver:
+  void OnDisplayMetricsChanged(const display::Display& display,
+                               uint32_t changed_metrics) override;
+
  private:
   // Returns true if this feature can be turned on. If |fullscreen_state| is
   // supplied, it will be used in calculating the result, otherwise the current
@@ -145,6 +151,18 @@ class TopControlsSlideControllerChromeOS
   // re-laid out.
   bool is_sliding_in_progress_ = false;
 
+  // Set to true when the state of this feature is temporarily disabled when
+  // some display metrics change while sliding and scrolling are both in
+  // progress, so that once the user lifts their finger to end the gesture
+  // scrolling, the enabled state is updated to the correct value.
+  bool update_state_after_gesture_scrolling_ends_ = false;
+
+  // True when it's desired to ignore shown ratios sent by the renderer for this
+  // tab. This happens when a display property changes while sliding and gesture
+  // scrolling are in progress, in which case we force-set the top controls to
+  // fully shown, and ignore any renderer-sent values.
+  bool pause_updates_ = false;
+
   // We need to observe the tab's web contents to listen to events that affect
   // the browser top controls shown state for each tab.
   base::flat_map<content::WebContents*,

chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos_browsertest.cc

@@ -39,9 +39,12 @@
 #include "ui/aura/window_tree_host.h"
 #include "ui/display/display.h"
 #include "ui/display/display_switches.h"
+#include "ui/display/screen.h"
+#include "ui/events/event_constants.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/views/controls/native/native_view_host.h"
+#include "ui/views/view.h"
 #include "ui/views/view_observer.h"
 
 namespace {
@@ -83,6 +86,34 @@ void CompareTranslations(const gfx::Transform& t1, const gfx::Transform& t2) {
   EXPECT_FLOAT_EQ(t1_translation.y(), t2_translation.y());
 }
 
+// A test view that will be added as a child to the BrowserView to verify how
+// many times it's laid out while sliding is in progress.
+class LayoutTestView : public views::View {
+ public:
+  explicit LayoutTestView(BrowserView* parent) {
+    DCHECK(parent);
+    parent->AddChildView(this);
+    parent->Layout();
+    layout_count_ = 0;
+  }
+  ~LayoutTestView() override = default;
+  LayoutTestView(const LayoutTestView&) = delete;
+  LayoutTestView& operator=(const LayoutTestView&) = delete;
+
+  int layout_count() const { return layout_count_; }
+
+  void Reset() {
+    InvalidateLayout();
+    layout_count_ = 0;
+  }
+
+  // views::View:
+  void Layout() override { ++layout_count_; }
+
+ private:
+  int layout_count_ = 0;
+};
+
 // Waits for the first non-empty paint for a given WebContents. To be able to
 // test gesture scroll events reliably, we must wait until the tab is fully
 // painted. Otherwise, the events will be ignored.
@@ -173,6 +204,10 @@ class TestController : public TopControlsSlideController {
     return real_controller_->IsTopControlsGestureScrollInProgress();
   }
 
+  bool IsTopControlsSlidingInProgress() const override {
+    return real_controller_->IsTopControlsSlidingInProgress();
+  }
+
  private:
   std::unique_ptr<TopControlsSlideController> real_controller_;
 
@@ -241,6 +276,52 @@ class TopControlsShownRatioWaiter : public TestControllerObserver {
   DISALLOW_COPY_AND_ASSIGN(TopControlsShownRatioWaiter);
 };
 
+// Waits for a given |is_gesture_scrolling_in_progress_| value.
+class GestureScrollInProgressChangeWaiter : public TestControllerObserver {
+ public:
+  explicit GestureScrollInProgressChangeWaiter(TestController* controller)
+      : controller_(controller) {
+    controller_->AddObserver(this);
+  }
+
+  ~GestureScrollInProgressChangeWaiter() override {
+    controller_->RemoveObserver(this);
+  }
+
+  GestureScrollInProgressChangeWaiter(
+      const GestureScrollInProgressChangeWaiter&) = delete;
+  GestureScrollInProgressChangeWaiter& operator=(
+      const GestureScrollInProgressChangeWaiter&) = delete;
+
+  // TestControllerObserver:
+  void OnShownRatioChanged(float shown_ratio) override {}
+
+  void OnGestureScrollInProgressChanged(bool in_progress) override {
+    if (in_progress == waited_for_in_progress_state_ && run_loop_)
+      std::move(run_loop_)->Quit();
+  }
+
+  void WaitForInProgressState(bool in_progress_state) {
+    if (controller_->IsTopControlsGestureScrollInProgress() ==
+        in_progress_state)
+      return;
+
+    waited_for_in_progress_state_ = in_progress_state;
+    // Use kNestableTasksAllowed to make it possible to wait inside a posted
+    // task.
+    run_loop_ = std::make_unique<base::RunLoop>(
+        base::RunLoop::Type::kNestableTasksAllowed);
+    run_loop_->Run();
+  }
+
+ private:
+  TestController* controller_;
+
+  std::unique_ptr<base::RunLoop> run_loop_;
+
+  bool waited_for_in_progress_state_ = false;
+};
+
 }  // namespace
 
 class TopControlsSlideControllerTest : public InProcessBrowserTest {
@@ -373,15 +454,23 @@ class TopControlsSlideControllerTest : public InProcessBrowserTest {
   // scrolled by touch gestures in such a way that will result in the top
   // controls shown ratio becoming a fractional value (i.e. sliding top-chrome
   // is in progress).
+  // The |layout_test_view| will be used to verify that the BrowserView doesn't
+  // get laid out more than once while sliding is in progress.
   // The |expected_shrink_renderer_size| will be checked against the
   // `DoBrowserControlsShrinkRendererSize` bit while sliding.
-  void CheckIntermediateScrollStep(bool expected_shrink_renderer_size) {
+  void CheckIntermediateScrollStep(LayoutTestView* layout_test_view,
+                                   bool expected_shrink_renderer_size) {
     const float shown_ratio = top_controls_slide_controller()->GetShownRatio();
 
     // This should only be used to verify the state of the browser while sliding
     // is in progress.
     ASSERT_TRUE(shown_ratio != 1.f && shown_ratio != 0.f);
 
+    // While scrolling is in progress, only a single layout is expected.
+    EXPECT_TRUE(
+        top_controls_slide_controller()->IsTopControlsSlidingInProgress());
+    EXPECT_EQ(layout_test_view->layout_count(), 1);
+
     const int top_controls_height = browser_view()->GetTopControlsHeight();
     EXPECT_NE(top_controls_height, 0);
 
@@ -1065,6 +1154,7 @@ IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest,
       event_generator.CalculateScrollDurationForFlingVelocity(
           start_point, end_point, 2 /* velocity */, scroll_steps);
 
+  LayoutTestView layout_test_view{browser_view()};
   {
     // We will start scrolling while top-chrome is fully shown, in which case
     // the `DoBrowserControlsShrinkRendererSize` bit is true ...
@@ -1079,7 +1169,9 @@ IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest,
         top_controls_slide_controller(),
         base::BindOnce(
             &TopControlsSlideControllerTest::CheckIntermediateScrollStep,
-            base::Unretained(this), expected_shrink_renderer_size));
+            base::Unretained(this), &layout_test_view,
+            expected_shrink_renderer_size));
+    layout_test_view.Reset();
     event_generator.GestureScrollSequence(start_point, end_point,
                                           scroll_step_delay, scroll_steps);
     fractional_ratio_waiter.Wait();
@@ -1105,8 +1197,9 @@ IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest,
         top_controls_slide_controller(),
         base::BindOnce(
             &TopControlsSlideControllerTest::CheckIntermediateScrollStep,
-            base::Unretained(this), expected_shrink_renderer_size));
-
+            base::Unretained(this), &layout_test_view,
+            expected_shrink_renderer_size));
+    layout_test_view.Reset();
     event_generator.GestureScrollSequence(end_point, start_point,
                                           scroll_step_delay, scroll_steps);
     fractional_ratio_waiter.Wait();
@@ -1121,6 +1214,66 @@ IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest,
   }
 }
 
+IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest,
+                       DisplayMetricsChangeWhileInProgress) {
+  ToggleTabletMode();
+  ASSERT_TRUE(GetTabletModeEnabled());
+  EXPECT_TRUE(top_controls_slide_controller()->IsEnabled());
+  EXPECT_FLOAT_EQ(top_controls_slide_controller()->GetShownRatio(), 1.f);
+
+  OpenUrlAtIndex(embedded_test_server()->GetURL("/top_controls_scroll.html"),
+                 0);
+
+  // Triggers a display metrics change event while both gesture scrolling and
+  // sliding are in progress.
+  auto rotate_display_while_in_progress =
+      [](TestController* slide_controller,
+         ui::test::EventGenerator* generator) {
+        ASSERT_TRUE(slide_controller->IsTopControlsGestureScrollInProgress());
+        ASSERT_TRUE(slide_controller->IsTopControlsSlidingInProgress());
+
+        // Trigger the keyboard shrotcut for changing the device scale factor.
+        // This should result in a display metric change.
+        constexpr int kFlags = ui::EF_CONTROL_DOWN | ui::EF_SHIFT_DOWN;
+        generator->PressKey(ui::VKEY_OEM_PLUS, kFlags);
+        generator->ReleaseKey(ui::VKEY_OEM_PLUS, kFlags);
+
+        // Test that as result of the above, sliding has been temporarily
+        // disabled, and that the top controls are fully shown.
+        EXPECT_FALSE(slide_controller->IsEnabled());
+        EXPECT_FLOAT_EQ(slide_controller->GetShownRatio(), 1.f);
+        // Even though gesture scrolling hasn't ended.
+        EXPECT_TRUE(slide_controller->IsTopControlsGestureScrollInProgress());
+      };
+
+  aura::Window* browser_window = browser()->window()->GetNativeWindow();
+  ui::test::EventGenerator event_generator(browser_window->GetRootWindow(),
+                                           browser_window);
+  const gfx::Point start_point = event_generator.current_screen_location();
+  const gfx::Point end_point = start_point + gfx::Vector2d(0, -100);
+
+  IntermediateShownRatioWaiter fractional_ratio_waiter(
+      top_controls_slide_controller(),
+      base::BindOnce(rotate_display_while_in_progress,
+                     top_controls_slide_controller(), &event_generator));
+
+  event_generator.set_current_screen_location(start_point);
+  event_generator.PressTouch();
+  event_generator.MoveTouch(end_point);
+  fractional_ratio_waiter.Wait();
+
+  // Release touch and wait for gesture scrolling to end.
+  GestureScrollInProgressChangeWaiter waiter{top_controls_slide_controller()};
+  event_generator.ReleaseTouch();
+  waiter.WaitForInProgressState(false);
+
+  // Expect that sliding has been re-enabled, and the top controls are still
+  // fully shown.
+  EXPECT_TRUE(top_controls_slide_controller()->IsEnabled());
+  EXPECT_FLOAT_EQ(top_controls_slide_controller()->GetShownRatio(), 1.f);
+  CheckBrowserLayout(browser_view(), TopChromeShownState::kFullyShown);
+}
+
 IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest, TestPermissionBubble) {
   ToggleTabletMode();
   ASSERT_TRUE(GetTabletModeEnabled());