Fix and reenable TopControlsSlideControllerTest.TestFocusEditableElements

This test verifies that when an editable element is focused within the
page, top-chrome is forced-shown and is kept so until the element is
blurred.

The implementation of this behavior used the deprecated NotificationService
for which we have now a replacement call in WebContentsObserver.
This CL removes the use of the deprecated code.

The flakiness in the test was due to a timing issue. In the cases when the
test fails, I found that the generated fling events, cause gesture scrolling
to be set to start and stop immediately in succession without generating
any calls from the renderer to the browser to set the shown ratios.

This was fixed by giving those events a chance to propagate, by waiting for
at least 2 render frame submissions before generating the next one.

With this fix, I ran the test with --gtest_repeat=40 and all instances passed.

BUG=1049178
TEST=browser_tests --gtest_filter=TopControlsSlideControllerTest.*Editable*
	--test-launcher-bot-mode --gtest_repeat=40

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

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

@@ -20,10 +20,6 @@
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_service.h"
-#include "content/public/browser/notification_source.h"
-#include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/web_contents.h"
@@ -217,6 +213,14 @@ class TopControlsSlideTabObserver
     UpdateBrowserControlsStateShown(/*animate=*/true);
   }
 
+  void OnFocusChangedInPage(content::FocusedNodeDetails* details) override {
+    // Even if a non-editable node gets focused, if top-chrome is fully shown,
+    // we should also update the browser controls state constraints so that
+    // top-chrome is able to be hidden again.
+    if (details->is_editable_node || shown_ratio_ == 1.f)
+      UpdateBrowserControlsStateShown(/*animate=*/true);
+  }
+
   // PermissionRequestManager::Observer:
   void OnBubbleAdded() override {
     UpdateBrowserControlsStateShown(/*animate=*/true);
@@ -283,9 +287,6 @@ TopControlsSlideControllerChromeOS::TopControlsSlideControllerChromeOS(
   observed_omni_box_ = browser_view->GetLocationBarView()->omnibox_view();
   observed_omni_box_->AddObserver(this);
 
-  registrar_.Add(this, content::NOTIFICATION_FOCUS_CHANGED_IN_PAGE,
-                 content::NotificationService::AllSources());
-
   if (ash::TabletMode::Get())
     ash::TabletMode::Get()->AddObserver(this);
 
@@ -510,33 +511,6 @@ void TopControlsSlideControllerChromeOS::SetTabNeedsAttentionAt(
   UpdateBrowserControlsStateShown(/*web_contents=*/nullptr, /*animate=*/true);
 }
 
-void TopControlsSlideControllerChromeOS::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  // TODO(afakhry): It would be nice to add a WebContentsObserver method that
-  // broadcasts this event.
-  if (type != content::NOTIFICATION_FOCUS_CHANGED_IN_PAGE)
-    return;
-
-  // Make sure this notification is meant for us.
-  content::WebContents* active_contents = browser_view_->GetActiveWebContents();
-  content::RenderViewHost* render_view_host =
-      content::Source<content::RenderViewHost>(source).ptr();
-  if (!active_contents || content::WebContents::FromRenderViewHost(
-                              render_view_host) != active_contents) {
-    return;
-  }
-
-  content::FocusedNodeDetails* node_details =
-      content::Details<content::FocusedNodeDetails>(details).ptr();
-  // If a non-editable node gets focused and top-chrome is fully shown, we
-  // should also update the browser controls state constraints so that
-  // top-chrome is able to be hidden again.
-  if (node_details->is_editable_node || shown_ratio_ == 1.f)
-    UpdateBrowserControlsStateShown(active_contents, /*animate=*/true);
-}
-
 void TopControlsSlideControllerChromeOS::OnDisplayMetricsChanged(
     const display::Display& display,
     uint32_t changed_metrics) {

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

@@ -14,8 +14,6 @@
 #include "chrome/browser/chromeos/accessibility/accessibility_manager.h"
 #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
 #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"
 #include "ui/views/view_observer.h"
 
@@ -41,7 +39,6 @@ class TopControlsSlideTabObserver;
 class TopControlsSlideControllerChromeOS : public TopControlsSlideController,
                                            public ash::TabletModeObserver,
                                            public TabStripModelObserver,
-                                           public content::NotificationObserver,
                                            public display::DisplayObserver,
                                            public views::ViewObserver {
  public:
@@ -70,11 +67,6 @@ class TopControlsSlideControllerChromeOS : public TopControlsSlideController,
       const TabStripSelectionChange& selection) override;
   void SetTabNeedsAttentionAt(int index, bool attention) override;
 
-  // content::NotificationObserver:
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
-
   // display::DisplayObserver:
   void OnDisplayMetricsChanged(const display::Display& display,
                                uint32_t changed_metrics) override;
@@ -191,8 +183,6 @@ class TopControlsSlideControllerChromeOS : public TopControlsSlideController,
                  std::unique_ptr<TopControlsSlideTabObserver>>
       observed_tabs_;
 
-  content::NotificationRegistrar registrar_;
-
   std::unique_ptr<chromeos::AccessibilityStatusSubscription>
       accessibility_status_subscription_;
 

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

@@ -61,25 +61,6 @@ enum class ScrollDirection {
   kDown,
 };
 
-// Using the given |generator| and the start and end points, it generates a
-// gesture scroll sequence with appropriate velocity so that fling gesture
-// scrolls are generated.
-void GenerateGestureFlingScrollSequence(ui::test::EventGenerator* generator,
-                                        const gfx::Point& start_point,
-                                        const gfx::Point& end_point) {
-  DCHECK(generator);
-  generator->GestureScrollSequenceWithCallback(
-      start_point, end_point,
-      generator->CalculateScrollDurationForFlingVelocity(
-          start_point, end_point, 100 /* velocity */, 2 /* steps */),
-      2 /* steps */,
-      base::BindRepeating([](ui::EventType, const gfx::Vector2dF&) {
-        // Give the event a chance to propagate to renderer before sending the
-        // next one.
-        base::RunLoop().RunUntilIdle();
-      }));
-}
-
 // Checks that the translation part of the two given transforms are equal.
 void CompareTranslations(const gfx::Transform& t1, const gfx::Transform& t2) {
   const gfx::Vector2dF t1_translation = t1.To2dTranslation();
@@ -505,6 +486,33 @@ class TopControlsSlideControllerTest : public InProcessBrowserTest {
     CompareTranslations(expected_transform, root_view_layer->transform());
   }
 
+  // Using the given |generator| and the start and end points, it generates a
+  // gesture scroll sequence with appropriate velocity so that fling gesture
+  // scrolls are generated.
+  void GenerateGestureFlingScrollSequence(ui::test::EventGenerator* generator,
+                                          const gfx::Point& start_point,
+                                          const gfx::Point& end_point) {
+    DCHECK(generator);
+    content::WebContents* contents = browser_view()->GetActiveWebContents();
+    generator->GestureScrollSequenceWithCallback(
+        start_point, end_point,
+        generator->CalculateScrollDurationForFlingVelocity(
+            start_point, end_point, 100 /* velocity */, 2 /* steps */),
+        2 /* steps */,
+        base::BindRepeating(
+            [](content::WebContents* contents, ui::EventType,
+               const gfx::Vector2dF&) {
+              // Give the event a chance to propagate to renderer before sending
+              // the next one by waiting for at least 2 render frame
+              // submissions.
+              content::RenderFrameSubmissionObserver submission_observer(
+                  contents);
+              while (submission_observer.render_frame_count() < 2)
+                submission_observer.WaitForAnyFrameSubmission();
+            },
+            contents));
+  }
+
   // Generates a gesture fling scroll sequence to scroll the current page in the
   // given |direction|, and waits for and verifies that top-chrome reaches the
   // given |target_state|.
@@ -781,9 +789,8 @@ IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest, TestClosingATab) {
                                TopChromeShownState::kFullyHidden);
 }
 
-// Disabled for flakes. See http://crbug.com/1049178
 IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest,
-                       DISABLED_TestFocusEditableElements) {
+                       TestFocusEditableElements) {
   ToggleTabletMode();
   ASSERT_TRUE(GetTabletModeEnabled());
   EXPECT_TRUE(top_controls_slide_controller()->IsEnabled());
@@ -794,6 +801,7 @@ IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest,
   OpenUrlAtIndex(embedded_test_server()->GetURL("/top_controls_scroll.html"),
                  0);
 
+  SCOPED_TRACE("Initial scroll to hide the top controls.");
   ScrollAndExpectTopChromeToBe(ScrollDirection::kDown,
                                TopChromeShownState::kFullyHidden);
 
@@ -822,6 +830,7 @@ IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest,
 
   // Focus on the editable element in the page and expect that top-chrome will
   // be shown.
+  SCOPED_TRACE("Focus an editable element in the page.");
   TopControlsShownRatioWaiter waiter(top_controls_slide_controller());
   content::WebContents* contents = browser_view()->GetActiveWebContents();
   bool bool_result = false;
@@ -835,18 +844,22 @@ IN_PROC_BROWSER_TEST_F(TopControlsSlideControllerTest,
   // Now try scrolling in a way that would normally hide top-chrome, and expect
   // that top-chrome will be forced shown as long as the editable element is
   // focused.
+  SCOPED_TRACE("Attempting a scroll which will not hide the top controls.");
   ScrollAndExpectTopChromeToBe(ScrollDirection::kDown,
                                TopChromeShownState::kFullyShown);
 
   // Now blur the focused editable element. Expect that top-chrome can now be
   // hidden with gesture scrolls.
   bool_result = false;
+  SCOPED_TRACE("Bluring the focus away from the editable element.");
   ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
       contents, get_js_function_body(false /* should_focus */), &bool_result));
   EXPECT_TRUE(bool_result);
   // Evaluate an empty sentence to make sure that the event processing is done
   // in the content.
   ignore_result(content::EvalJs(contents, ";"));
+
+  SCOPED_TRACE("Scroll to hide should now work.");
   ScrollAndExpectTopChromeToBe(ScrollDirection::kDown,
                                TopChromeShownState::kFullyHidden);
 }