[NTP] Refactor LocalNTPCustomLinksTest to wait for the tiles to reload instead of opening a new tab

When checking for updated tiles, wait for the tiles to reload instead of
opening another NTP.

Also fix the incorrect removeEventListener in local_ntp_test_utils.

Bug: 903265
Change-Id: If48fac6eced637721c6b199492db2edee06370c4
Reviewed-on: https://chromium-review.googlesource.com/c/1334807
Commit-Queue: Kristi Park <kristipark@chromium.org>
Reviewed-by: Ramya Nagarajan <ramyan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609871}

chrome/browser/ui/search/local_ntp_browsertest.cc

@@ -626,9 +626,8 @@ IN_PROC_BROWSER_TEST_F(LocalNTPCustomLinksTest, ShowsAddCustomLinkButton) {
   EXPECT_TRUE(has_add_button);
 }
 
-// Flaky on Windows, https://crbug.com/903265
 IN_PROC_BROWSER_TEST_F(LocalNTPCustomLinksTest,
-                       DISABLED_DontShowAddCustomLinkButtonWhenMaxLinks) {
+                       DontShowAddCustomLinkButtonWhenMaxLinks) {
   content::WebContents* active_tab =
       local_ntp_test_utils::OpenNewTab(browser(), GURL("about:blank"));
 
@@ -640,7 +639,6 @@ IN_PROC_BROWSER_TEST_F(LocalNTPCustomLinksTest,
 
   // Get the iframe and add to maximum number of tiles.
   content::RenderFrameHost* iframe = GetMostVisitedIframe(active_tab);
-
   for (int i = kDefaultMostVisitedItemCount; i < kDefaultCustomLinkMaxCount;
        ++i) {
     std::string rid = std::to_string(i + 100);
@@ -648,29 +646,25 @@ IN_PROC_BROWSER_TEST_F(LocalNTPCustomLinksTest,
     std::string title = "url for " + rid;
     // Add most visited tiles via the EmbeddedSearch API. rid = -1 means add new
     // most visited tile.
-    EXPECT_TRUE(content::ExecuteScript(
+    local_ntp_test_utils::ExecuteScriptOnNTPAndWaitUntilLoaded(
         iframe,
         "window.chrome.embeddedSearch.newTabPage.updateCustomLink(-1, '" + url +
-            "', '" + title + "')"));
+            "', '" + title + "')");
   }
   // Confirm that there are max number of custom link tiles.
   observer.WaitForNumberOfItems(kDefaultCustomLinkMaxCount);
 
-  // Open a new tab and check if the add button still exists
-  active_tab = local_ntp_test_utils::OpenNewTab(browser(), GURL("about:blank"));
-  local_ntp_test_utils::NavigateToNTPAndWaitUntilLoaded(browser());
-  iframe = GetMostVisitedIframe(active_tab);
-
-  // Check there is no add button in the iframe.
+  // Check there is no add button in the iframe. Make sure not to select from
+  // old tiles that are in the process of being deleted.
   bool no_add_button = false;
   ASSERT_TRUE(instant_test_utils::GetBoolFromJS(
-      iframe, "document.querySelectorAll('.md-add-icon').length === 0",
+      iframe,
+      "document.querySelectorAll('#mv-tiles .md-add-icon').length === 0",
       &no_add_button));
   EXPECT_TRUE(no_add_button);
 }
 
-// Flaky on Windows, https://crbug.com/903265
-IN_PROC_BROWSER_TEST_F(LocalNTPCustomLinksTest, DISABLED_Reorder) {
+IN_PROC_BROWSER_TEST_F(LocalNTPCustomLinksTest, Reorder) {
   content::WebContents* active_tab =
       local_ntp_test_utils::OpenNewTab(browser(), GURL("about:blank"));
 
@@ -687,42 +681,36 @@ IN_PROC_BROWSER_TEST_F(LocalNTPCustomLinksTest, DISABLED_Reorder) {
     std::string rid = std::to_string(i + 100);
     std::string url = "https://" + rid + ".com";
     std::string title = "url for " + rid;
-    ASSERT_TRUE(content::ExecuteScript(
+    local_ntp_test_utils::ExecuteScriptOnNTPAndWaitUntilLoaded(
         iframe,
         "window.chrome.embeddedSearch.newTabPage.updateCustomLink(-1, '" + url +
-            "', '" + title + "')"));
+            "', '" + title + "')");
   }
   // Confirm that there are max number of custom link tiles.
   observer.WaitForNumberOfItems(kDefaultCustomLinkMaxCount);
-  // Open a new tab to get the updated links.
-  active_tab = local_ntp_test_utils::OpenNewTab(browser(), GURL("about:blank"));
-  local_ntp_test_utils::NavigateToNTPAndWaitUntilLoaded(browser());
-  iframe = GetMostVisitedIframe(active_tab);
 
-  // Get the title of the tile at index 1.
+  // Get the title of the tile at index 1. Make sure not to select from old
+  // tiles that are in the process of being deleted.
   std::string title;
   ASSERT_TRUE(instant_test_utils::GetStringFromJS(
-      iframe, "document.querySelectorAll('.md-tile .md-title')[1].innerText",
+      iframe, "document.querySelectorAll('#mv-tiles .md-title')[1].innerText",
       &title));
 
   // Move the tile to the front.
   std::string tid;
   ASSERT_TRUE(instant_test_utils::GetStringFromJS(
       iframe,
-      "document.querySelectorAll('.md-tile')[1].getAttribute('data-tid')",
+      "document.querySelectorAll('#mv-tiles "
+      ".md-tile')[1].getAttribute('data-tid')",
       &tid));
-  EXPECT_TRUE(content::ExecuteScript(
+  local_ntp_test_utils::ExecuteScriptOnNTPAndWaitUntilLoaded(
       iframe, "window.chrome.embeddedSearch.newTabPage.reorderCustomLink(" +
-                  tid + ", 0)"));
-  // Open a new tab to get the updated links.
-  active_tab = local_ntp_test_utils::OpenNewTab(browser(), GURL("about:blank"));
-  local_ntp_test_utils::NavigateToNTPAndWaitUntilLoaded(browser());
-  iframe = GetMostVisitedIframe(active_tab);
+                  tid + ", 0)");
 
   // Check that the first tile is the tile that was moved.
   std::string new_title;
   ASSERT_TRUE(instant_test_utils::GetStringFromJS(
-      iframe, "document.querySelectorAll('.md-tile .md-title')[0].innerText",
+      iframe, "document.querySelectorAll('#mv-tiles .md-title')[0].innerText",
       &new_title));
   EXPECT_EQ(new_title, title);
 
@@ -730,21 +718,18 @@ IN_PROC_BROWSER_TEST_F(LocalNTPCustomLinksTest, DISABLED_Reorder) {
   std::string end_index = std::to_string(kDefaultCustomLinkMaxCount - 1);
   ASSERT_TRUE(instant_test_utils::GetStringFromJS(
       iframe,
-      "document.querySelectorAll('.md-tile')[0].getAttribute('data-tid')",
+      "document.querySelectorAll('#mv-tiles "
+      ".md-tile')[0].getAttribute('data-tid')",
       &tid));
-  EXPECT_TRUE(content::ExecuteScript(
+  local_ntp_test_utils::ExecuteScriptOnNTPAndWaitUntilLoaded(
       iframe, "window.chrome.embeddedSearch.newTabPage.reorderCustomLink(" +
-                  tid + ", " + end_index + ")"));
-  // Open a new tab to get the updated links.
-  active_tab = local_ntp_test_utils::OpenNewTab(browser(), GURL("about:blank"));
-  local_ntp_test_utils::NavigateToNTPAndWaitUntilLoaded(browser());
-  iframe = GetMostVisitedIframe(active_tab);
+                  tid + ", " + end_index + ")");
 
   // Check that the last tile is the tile that was moved.
   new_title = std::string();
   ASSERT_TRUE(instant_test_utils::GetStringFromJS(
       iframe,
-      "document.querySelectorAll('.md-tile .md-title')[" + end_index +
+      "document.querySelectorAll('#mv-tiles .md-title')[" + end_index +
           "].innerText",
       &new_title));
   EXPECT_EQ(new_title, title);

chrome/browser/ui/search/local_ntp_test_utils.cc

@@ -30,30 +30,11 @@
 
 namespace local_ntp_test_utils {
 
-content::WebContents* OpenNewTab(Browser* browser, const GURL& url) {
-  ui_test_utils::NavigateToURLWithDisposition(
-      browser, url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
-      ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB |
-          ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
-  return browser->tab_strip_model()->GetActiveWebContents();
-}
-
-void NavigateToNTPAndWaitUntilLoaded(Browser* browser, int delay) {
-  content::WebContents* active_tab =
-      browser->tab_strip_model()->GetActiveWebContents();
-
-  ASSERT_FALSE(search::IsInstantNTP(active_tab));
-
-  // Attach a message queue *before* navigating to the NTP, to make sure we
-  // don't miss the 'loaded' message due to some race condition.
-  content::DOMMessageQueue msg_queue(active_tab);
-
-  // Navigate to the NTP.
-  ui_test_utils::NavigateToURL(browser, GURL(chrome::kChromeUINewTabURL));
-  ASSERT_TRUE(search::IsInstantNTP(active_tab));
-  ASSERT_EQ(GURL(chrome::kChromeSearchLocalNtpUrl),
-            active_tab->GetController().GetVisibleEntry()->GetURL());
+namespace {
 
+void WaitUntilTilesLoaded(content::WebContents* active_tab,
+                          content::DOMMessageQueue* msg_queue,
+                          int delay) {
   // At this point, the MV iframe may or may not have been fully loaded. Once
   // it loads, it sends a 'loaded' postMessage to the page. Check if the page
   // has already received that, and if not start listening for it. It's
@@ -64,23 +45,26 @@ void NavigateToNTPAndWaitUntilLoaded(Browser* browser, int delay) {
       active_tab, base::StringPrintf(R"js(
       (function() {
         if (tilesAreLoaded) {
+          tilesAreLoaded = false;
           return true;
         }
-        let loaded = window.addEventListener('message', function(event) {
+        const loaded = (event) => {
           if (event.data.cmd == 'loaded') {
             window.removeEventListener('message', loaded);
             setTimeout(() => {
-              domAutomationController.send('NavigateToNTPAndWaitUntilLoaded');
+              tilesAreLoaded = false;
+              domAutomationController.send('WaitUntilTilesLoaded');
             }, %d);
           }
-        });
+        };
+        window.addEventListener('message', loaded);
         return false;
       })()
       )js", delay), &mv_tiles_loaded));
 
   std::string message;
   // Get rid of the message that the GetBoolFromJS call produces.
-  ASSERT_TRUE(msg_queue.PopMessage(&message));
+  ASSERT_TRUE(msg_queue->PopMessage(&message));
 
   if (mv_tiles_loaded) {
     // The tiles are already loaded, i.e. we missed the 'loaded' message. All
@@ -88,11 +72,60 @@ void NavigateToNTPAndWaitUntilLoaded(Browser* browser, int delay) {
     return;
   }
 
-  // Not loaded yet. Wait for the "NavigateToNTPAndWaitUntilLoaded" message.
-  ASSERT_TRUE(msg_queue.WaitForMessage(&message));
-  ASSERT_EQ("\"NavigateToNTPAndWaitUntilLoaded\"", message);
+  // Not loaded yet. Wait for the "WaitUntilTilesLoaded" message.
+  ASSERT_TRUE(msg_queue->WaitForMessage(&message));
+  ASSERT_EQ("\"WaitUntilTilesLoaded\"", message);
   // There shouldn't be any other messages.
-  ASSERT_FALSE(msg_queue.PopMessage(&message));
+  ASSERT_FALSE(msg_queue->PopMessage(&message));
+}
+
+}  // namespace
+
+content::WebContents* OpenNewTab(Browser* browser, const GURL& url) {
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser, url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB |
+          ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  return browser->tab_strip_model()->GetActiveWebContents();
+}
+
+void NavigateToNTPAndWaitUntilLoaded(Browser* browser, int delay) {
+  content::WebContents* active_tab =
+      browser->tab_strip_model()->GetActiveWebContents();
+
+  ASSERT_FALSE(search::IsInstantNTP(active_tab));
+
+  // Attach a message queue *before* navigating to the NTP, to make sure we
+  // don't miss the 'loaded' message due to some race condition.
+  content::DOMMessageQueue msg_queue(active_tab);
+
+  // Navigate to the NTP.
+  ui_test_utils::NavigateToURL(browser, GURL(chrome::kChromeUINewTabURL));
+  ASSERT_TRUE(search::IsInstantNTP(active_tab));
+  ASSERT_EQ(GURL(chrome::kChromeSearchLocalNtpUrl),
+            active_tab->GetController().GetVisibleEntry()->GetURL());
+
+  WaitUntilTilesLoaded(active_tab, &msg_queue, delay);
+}
+
+void ExecuteScriptOnNTPAndWaitUntilLoaded(content::RenderFrameHost* host,
+                                          const std::string& script) {
+  content::WebContents* contents =
+      content::WebContents::FromRenderFrameHost(host);
+
+  ASSERT_TRUE(search::IsInstantNTP(contents));
+
+  // Attach a message queue *before* executing the script, to make sure we don't
+  // miss the 'loaded' message due to some race condition.
+  content::DOMMessageQueue msg_queue(contents);
+
+  // Execute the script, and get rid of the message that the ExecuteScript call
+  // produces.
+  EXPECT_TRUE(content::ExecuteScript(host, script));
+  std::string message;
+  ASSERT_TRUE(msg_queue.PopMessage(&message));
+
+  WaitUntilTilesLoaded(contents, &msg_queue, /*delay=*/0);
 }
 
 bool SwitchBrowserLanguageToFrench() {

chrome/browser/ui/search/local_ntp_test_utils.h

@@ -12,6 +12,7 @@ class GURL;
 class Profile;
 
 namespace content {
+class RenderFrameHost;
 class WebContents;
 }  // namespace content
 
@@ -26,6 +27,13 @@ content::WebContents* OpenNewTab(Browser* browser, const GURL& url);
 // additional amount of time after the page reports as loaded.
 void NavigateToNTPAndWaitUntilLoaded(Browser* browser, int delay = 0);
 
+// Executes a script on the NTP, verifies it executes successfully, and waits
+// until the NTP tiles are reloaded. Note that simply waiting for the script
+// execution to complete is not enough, since the MV iframe receives the tiles
+// asynchronously.
+void ExecuteScriptOnNTPAndWaitUntilLoaded(content::RenderFrameHost* host,
+                                          const std::string& script);
+
 // Switches the browser language to French, and returns true iff successful.
 bool SwitchBrowserLanguageToFrench();