[NTP][RQ] Adds support for repeatable queries in NTP MV tiles (Part 3)

- Shows the search loupe icon not only for repeatable query tiles, but
  all query tiles including query custom links when repeatable queries
  are enabled. This is for UI consistency when a repeatable query tile
  becomes a shortcut and the info about the source of the tile is lost.

- Does not show the toast buttons when a query tile is removed unless
  it is a custom link. Removal is not reversible for non custom link
  query tiles. Also disables Ctrl+Z when toast buttons are not showing.

Bug: 1138578
Change-Id: Ib26ad07ce8d12da70c7916f2aaf79f65fabc3dce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2491991Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820946}

chrome/browser/resources/new_tab_page/most_visited.js

@@ -574,8 +574,7 @@ class MostVisitedElement extends PolymerElement {
     const modifier = isMac ? e.metaKey && !e.ctrlKey : e.ctrlKey && !e.metaKey;
     if (modifier && e.key === 'z') {
       e.preventDefault();
-      this.pageHandler_.undoMostVisitedTileAction();
-      this.$.toast.hide();
+      this.onUndoClick_();
     }
   }
 
@@ -627,6 +626,9 @@ class MostVisitedElement extends PolymerElement {
 
   /** @private */
   onRestoreDefaultsClick_() {
+    if (!this.$.toast.open || !this.showToastButtons_) {
+      return;
+    }
     this.$.toast.hide();
     this.pageHandler_.restoreMostVisitedDefaults();
   }
@@ -734,6 +736,9 @@ class MostVisitedElement extends PolymerElement {
 
   /** @private */
   onUndoClick_() {
+    if (!this.$.toast.open || !this.showToastButtons_) {
+      return;
+    }
     this.$.toast.hide();
     this.pageHandler_.undoMostVisitedTileAction();
   }
@@ -803,9 +808,13 @@ class MostVisitedElement extends PolymerElement {
    * @private
    */
   async tileRemove_(index) {
-    const {title, url} = this.tiles_[index];
+    const {url, isQueryTile} = this.tiles_[index];
     this.pageHandler_.deleteMostVisitedTile(url);
-    this.toast_('linkRemovedMsg', /* showButtons= */ true);
+    // Do not show the toast buttons when a query tile is removed unless it is a
+    // custom link. Removal is not reversible for non custom link query tiles.
+    this.toast_(
+        'linkRemovedMsg',
+        /* showButtons= */ this.customLinksEnabled_ || !isQueryTile);
     this.tileFocus_(index);
   }
 

chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc

@@ -34,6 +34,7 @@
 #include "chrome/browser/search/background/ntp_background_service.h"
 #include "chrome/browser/search/background/ntp_background_service_factory.h"
 #include "chrome/browser/search/instant_service.h"
+#include "chrome/browser/search/ntp_features.h"
 #include "chrome/browser/search/one_google_bar/one_google_bar_service_factory.h"
 #include "chrome/browser/search/promos/promo_service_factory.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
@@ -1176,6 +1177,8 @@ void NewTabPageHandler::NtpThemeChanged(const NtpTheme& ntp_theme) {
 
 void NewTabPageHandler::MostVisitedInfoChanged(
     const InstantMostVisitedInfo& info) {
+  auto* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(profile_);
   std::vector<new_tab_page::mojom::MostVisitedTilePtr> list;
   auto result = new_tab_page::mojom::MostVisitedInfo::New();
   for (auto& tile : info.items) {
@@ -1193,7 +1196,10 @@ void NewTabPageHandler::MostVisitedInfoChanged(
     value->title_source = static_cast<int32_t>(tile.title_source);
     value->data_generation_time = tile.data_generation_time;
     value->is_query_tile =
-        tile.source == ntp_tiles::TileSource::REPEATABLE_QUERIES_SERVICE;
+        base::FeatureList::IsEnabled(ntp_features::kNtpRepeatableQueries) &&
+        template_url_service &&
+        template_url_service->IsSearchResultsPageFromDefaultSearchProvider(
+            tile.url);
     list.push_back(std::move(value));
   }
   result->custom_links_enabled = !info.use_most_visited;

chrome/test/data/webui/new_tab_page/most_visited_test.js

@@ -45,14 +45,14 @@ suite('NewTabPageMostVisitedTest', () => {
   }
 
   /**
-   * @param {number} n
+   * @param {number|!Array} n
    * @param {boolean=} customLinksEnabled
    * @param {boolean=} visible
    * @return {!Promise}
    * @private
    */
   async function addTiles(n, customLinksEnabled = true, visible = true) {
-    const tiles = Array(n).fill(0).map((x, i) => {
+    const tiles = Array.isArray(n) ? n : Array(n).fill(0).map((x, i) => {
       const char = String.fromCharCode(i + /* 'a' */ 97);
       return {
         title: char,
@@ -100,6 +100,12 @@ suite('NewTabPageMostVisitedTest', () => {
     updateScreenWidth(true, true);
   }
 
+  suiteSetup(() => {
+    loadTimeData.overrideValues({
+      linkRemovedMsg: '',
+    });
+  });
+
   setup(() => {
     PolymerTest.clearBody();
 
@@ -577,14 +583,71 @@ suite('NewTabPageMostVisitedTest', () => {
     assertFalse(actionMenu.open);
     assertEquals('https://b/', (await deleteCalled).url);
     assertTrue(mostVisited.$.toast.open);
+    // Toast buttons are visible.
+    assertTrue(!!$$(mostVisited, '#undo'));
+    assertTrue(!!$$(mostVisited, '#restore'));
+  });
+
+  test('remove query with action menu', async () => {
+    const {actionMenu, actionMenuRemove: removeButton} = mostVisited.$;
+    await addTiles([{
+      title: 'title',
+      titleDirection: mojoBase.mojom.TextDirection.LEFT_TO_RIGHT,
+      url: {url: 'https://search-url/'},
+      source: 0,
+      titleSource: 0,
+      isQueryTile: true,
+      dataGenerationTime: {internalValue: BigInt(0)},
+    }]);
+    const actionMenuButton = queryTiles()[0].querySelector('#actionMenuButton');
+
+    assertFalse(actionMenu.open);
+    actionMenuButton.click();
+    assertTrue(actionMenu.open);
+    const deleteCalled = testProxy.handler.whenCalled('deleteMostVisitedTile');
+    assertFalse(mostVisited.$.toast.open);
+    removeButton.click();
+    assertEquals('https://search-url/', (await deleteCalled).url);
+    assertTrue(mostVisited.$.toast.open);
+    // Toast buttons are visible.
+    assertTrue(!!$$(mostVisited, '#undo'));
+    assertTrue(!!$$(mostVisited, '#restore'));
   });
 
   test('remove with icon button (customLinksEnabled=false)', async () => {
     await addTiles(1, /* customLinksEnabled */ false);
     const removeButton = queryTiles()[0].querySelector('#removeButton');
     const deleteCalled = testProxy.handler.whenCalled('deleteMostVisitedTile');
+    assertFalse(mostVisited.$.toast.open);
     removeButton.click();
     assertEquals('https://a/', (await deleteCalled).url);
+    assertTrue(mostVisited.$.toast.open);
+    // Toast buttons are visible.
+    assertTrue(!!$$(mostVisited, '#undo'));
+    assertTrue(!!$$(mostVisited, '#restore'));
+  });
+
+  test('remove query with icon button (customLinksEnabled=false)', async () => {
+    await addTiles(
+        [{
+          title: 'title',
+          titleDirection: mojoBase.mojom.TextDirection.LEFT_TO_RIGHT,
+          url: {url: 'https://search-url/'},
+          source: 0,
+          titleSource: 0,
+          isQueryTile: true,
+          dataGenerationTime: {internalValue: BigInt(0)},
+        }],
+        /* customLinksEnabled */ false);
+    const removeButton = queryTiles()[0].querySelector('#removeButton');
+    const deleteCalled = testProxy.handler.whenCalled('deleteMostVisitedTile');
+    assertFalse(mostVisited.$.toast.open);
+    removeButton.click();
+    assertEquals('https://search-url/', (await deleteCalled).url);
+    assertTrue(mostVisited.$.toast.open);
+    // Toast buttons are not visible.
+    assertFalse(!!$$(mostVisited, '#undo'));
+    assertFalse(!!$$(mostVisited, '#restore'));
   });
 
   test('tile url is set to href of <a>', async () => {
@@ -603,12 +666,14 @@ suite('NewTabPageMostVisitedTest', () => {
     assertTrue(mostVisited.$.toast.open);
   });
 
-  test('ctrl+z undo and toast hidden', async () => {
+  test('ctrl+z triggers undo and hides toast', async () => {
     const {toast} = mostVisited.$;
-    toast.show();
+    assertFalse(toast.open);
+    mostVisited.toast_('linkRemovedMsg', /* showButtons= */ true);
+    await flushTasks();
+    assertTrue(toast.open);
     const undoCalled =
         testProxy.handler.whenCalled('undoMostVisitedTileAction');
-    assertTrue(toast.open);
     mostVisited.dispatchEvent(new KeyboardEvent('keydown', {
       bubbles: true,
       ctrlKey: !isMac,
@@ -619,13 +684,29 @@ suite('NewTabPageMostVisitedTest', () => {
     assertFalse(toast.open);
   });
 
+  test('ctrl+z does nothing if toast buttons are not showing', async () => {
+    const {toast} = mostVisited.$;
+    assertFalse(toast.open);
+    mostVisited.toast_('linkRemovedMsg', /* showButtons= */ false);
+    await flushTasks();
+    assertTrue(toast.open);
+    mostVisited.dispatchEvent(new KeyboardEvent('keydown', {
+      bubbles: true,
+      ctrlKey: !isMac,
+      key: 'z',
+      metaKey: isMac,
+    }));
+    assertEquals(
+        0, testProxy.handler.getCallCount('undoMostVisitedTileAction'));
+    assertTrue(toast.open);
+  });
+
   test('toast restore defaults button', async () => {
     const wait = testProxy.handler.whenCalled('restoreMostVisitedDefaults');
     const {toast} = mostVisited.$;
-    toast.querySelector('dom-if').if = true;
-    await flushTasks();
     assertFalse(toast.open);
-    toast.show('');
+    mostVisited.toast_('linkRemovedMsg', /* showButtons= */ true);
+    await flushTasks();
     assertTrue(toast.open);
     toast.querySelector('#restore').click();
     await wait;
@@ -635,10 +716,9 @@ suite('NewTabPageMostVisitedTest', () => {
   test('toast undo button', async () => {
     const wait = testProxy.handler.whenCalled('undoMostVisitedTileAction');
     const {toast} = mostVisited.$;
-    toast.querySelector('dom-if').if = true;
-    await flushTasks();
     assertFalse(toast.open);
-    toast.show('');
+    mostVisited.toast_('linkRemovedMsg', /* showButtons= */ true);
+    await flushTasks();
     assertTrue(toast.open);
     toast.querySelector('#undo').click();
     await wait;
@@ -727,44 +807,30 @@ suite('NewTabPageMostVisitedTest', () => {
   });
 
   test('RIGHT_TO_LEFT tile title text direction', async () => {
-    const tilesRendered = eventToPromise('dom-change', mostVisited.$.tiles);
-    testProxy.callbackRouterRemote.setMostVisitedInfo({
-      customLinksEnabled: true,
-      tiles: [{
-        title: 'title',
-        titleDirection: mojoBase.mojom.TextDirection.RIGHT_TO_LEFT,
-        url: {url: 'https://url/'},
-        source: 0,
-        titleSource: 0,
-        isQueryTile: false,
-        dataGenerationTime: {internalValue: 0},
-      }],
-      visible: true,
-    });
-    await testProxy.callbackRouterRemote.$.flushForTesting();
-    await tilesRendered;
+    await addTiles([{
+      title: 'title',
+      titleDirection: mojoBase.mojom.TextDirection.RIGHT_TO_LEFT,
+      url: {url: 'https://url/'},
+      source: 0,
+      titleSource: 0,
+      isQueryTile: false,
+      dataGenerationTime: {internalValue: BigInt(0)},
+    }]);
     const [tile] = queryTiles();
     const titleElement = tile.querySelector('.tile-title');
     assertEquals('rtl', window.getComputedStyle(titleElement).direction);
   });
 
   test('LEFT_TO_RIGHT tile title text direction', async () => {
-    const tilesRendered = eventToPromise('dom-change', mostVisited.$.tiles);
-    testProxy.callbackRouterRemote.setMostVisitedInfo({
-      customLinksEnabled: true,
-      tiles: [{
-        title: 'title',
-        titleDirection: mojoBase.mojom.TextDirection.LEFT_TO_RIGHT,
-        url: {url: 'https://url/'},
-        source: 0,
-        titleSource: 0,
-        isQueryTile: false,
-        dataGenerationTime: {internalValue: 0},
-      }],
-      visible: true,
-    });
-    await testProxy.callbackRouterRemote.$.flushForTesting();
-    await tilesRendered;
+    await addTiles([{
+      title: 'title',
+      titleDirection: mojoBase.mojom.TextDirection.LEFT_TO_RIGHT,
+      url: {url: 'https://url/'},
+      source: 0,
+      titleSource: 0,
+      isQueryTile: false,
+      dataGenerationTime: {internalValue: BigInt(0)},
+    }]);
     const [tile] = queryTiles();
     const titleElement = tile.querySelector('.tile-title');
     assertEquals('ltr', window.getComputedStyle(titleElement).direction);