WebUI NTP: ensure add shortcut is not displayed on third row

Change-Id: I0991a973c5bf9af88eec4308d5b83607d2904192
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2080787Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Commit-Queue: Esmael Elmoslimany <aee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749964}

chrome/browser/resources/new_tab_page/browser_proxy.js

@@ -56,6 +56,14 @@ export class BrowserProxy {
   createUntrustedIframeSrc(path) {
     return `chrome-untrusted://new-tab-page/${path}`;
   }
+
+  /**
+   * @param {string} query
+   * @return {!MediaQueryList}
+   */
+  matchMedia(query) {
+    return window.matchMedia(query);
+  }
 }
 
 addSingletonGetter(BrowserProxy);

chrome/browser/resources/new_tab_page/most_visited.html

@@ -143,7 +143,7 @@
     }
   }
 </style>
-<div id="container" style="--column-count: [[columnCount_]]">
+<div id="container" style="--column-count: [[columnCount_]];">
   <dom-repeat id="tiles" items="[[tiles_]]">
     <template>
       <a class="tile" draggable="true" href$="[[item.url.url]]"

chrome/browser/resources/new_tab_page/most_visited.js

@@ -80,7 +80,7 @@ class MostVisitedElement extends PolymerElement {
       columnCount_: {
         type: Boolean,
         computed: `computeColumnCount_(tiles_, screenWidth_, maxTiles_,
-            visible_, showAdd_)`,
+            visible_)`,
       },
 
       /** @private */
@@ -132,7 +132,7 @@ class MostVisitedElement extends PolymerElement {
       showAdd_: {
         type: Boolean,
         value: false,
-        computed: 'computeShowAdd_(tiles_, maxTiles_, customLinksEnabled_)',
+        computed: 'computeShowAdd_(tiles_, columnCount_, customLinksEnabled_)',
       },
 
       /** @private */
@@ -216,11 +216,12 @@ class MostVisitedElement extends PolymerElement {
 
     /** @private {!Function} */
     this.boundOnWidthChange_ = this.updateScreenWidth_.bind(this);
+    const {matchMedia} = BrowserProxy.getInstance();
     /** @private {!MediaQueryList} */
-    this.mediaListenerWideWidth_ = window.matchMedia('(min-width: 672px)');
+    this.mediaListenerWideWidth_ = matchMedia('(min-width: 672px)');
     this.mediaListenerWideWidth_.addListener(this.boundOnWidthChange_);
     /** @private {!MediaQueryList} */
-    this.mediaListenerMediumWidth_ = window.matchMedia('(min-width: 560px)');
+    this.mediaListenerMediumWidth_ = matchMedia('(min-width: 560px)');
     this.mediaListenerMediumWidth_.addListener(this.boundOnWidthChange_);
     this.updateScreenWidth_();
     /** @private {!function(Event)} */
@@ -254,9 +255,10 @@ class MostVisitedElement extends PolymerElement {
       maxColumns = 4;
     }
 
-    const tileCount = Math.min(
-        this.maxTiles_,
-        (this.tiles_ ? this.tiles_.length : 0) + (this.showAdd_ ? 1 : 0));
+    const shortcutCount = this.tiles_ ? this.tiles_.length : 0;
+    const canShowAdd = this.maxTiles_ > shortcutCount;
+    const tileCount =
+        Math.min(this.maxTiles_, shortcutCount + (canShowAdd ? 1 : 0));
     const columnCount = tileCount <= maxColumns ?
         tileCount :
         Math.min(maxColumns, Math.ceil(tileCount / 2));
@@ -277,7 +279,7 @@ class MostVisitedElement extends PolymerElement {
    */
   computeShowAdd_() {
     return this.customLinksEnabled_ && this.tiles_ &&
-        this.tiles_.length < this.maxTiles_;
+        this.tiles_.length < this.columnCount_ * 2;
   }
 
   /**

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

@@ -28,6 +28,10 @@ suite('NewTabPageAppTest', () => {
     testProxy.handler.setResultFor('getChromeThemes', Promise.resolve({
       chromeThemes: [],
     }));
+    testProxy.setResultMapperFor('matchMedia', () => ({
+                                                 addListener() {},
+                                                 removeListener() {},
+                                               }));
     BrowserProxy.instance_ = testProxy;
 
     app = document.createElement('ntp-app');

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

@@ -65,6 +65,10 @@ suite('NewTabPageMostVisitedFocusTest', () => {
     PolymerTest.clearBody();
 
     testProxy = createTestProxy();
+    testProxy.setResultMapperFor('matchMedia', () => ({
+                                                 addListener() {},
+                                                 removeListener() {},
+                                               }));
     BrowserProxy.instance_ = testProxy;
 
     mostVisited = document.createElement('ntp-most-visited');

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

@@ -21,6 +21,16 @@ suite('NewTabPageMostVisitedTest', () => {
    */
   let testProxy;
 
+
+  /** @type {!MediaListenerList} */
+  let mediaListenerWideWidth;
+
+  /** @type {!MediaListenerList} */
+  let mediaListenerMediumWidth;
+
+  /** @type {!Function} */
+  let mediaListener;
+
   /**
    * @param {string}
    * @return {!Array<!HTMLElement>}
@@ -64,7 +74,33 @@ suite('NewTabPageMostVisitedTest', () => {
     await tilesRendered;
   }
 
-  setup(() => {
+  /** @private */
+  function assertAddShortcutHidden() {
+    assertTrue(mostVisited.$.addShortcut.hidden);
+  }
+
+  /** @private */
+  function assertAddShortcutShown() {
+    assertFalse(mostVisited.$.addShortcut.hidden);
+  }
+
+  /**
+   * @param {boolean} isWide
+   * @param {boolean} isMedium
+   */
+  function updateScreenWidth(isWide, isMedium) {
+    assertTrue(!!mediaListenerWideWidth);
+    assertTrue(!!mediaListenerMediumWidth);
+    mediaListenerWideWidth.matches = isWide;
+    mediaListenerMediumWidth.matches = isMedium;
+    mediaListener();
+  }
+
+  function wide() {
+    updateScreenWidth(true, true);
+  }
+
+  setup(async () => {
     PolymerTest.clearBody();
 
     testProxy = createTestProxy();
@@ -74,17 +110,37 @@ suite('NewTabPageMostVisitedTest', () => {
     testProxy.handler.setResultFor('updateMostVisitedTile', Promise.resolve({
       success: true,
     }));
+    testProxy.setResultMapperFor('matchMedia', query => {
+      const mediaListenerList = {
+        matches: false,  // Used to determine the screen width.
+        media: query,
+        addListener(listener) {
+          mediaListener = listener;
+        },
+        removeListener() {},
+      };
+      if (query === '(min-width: 672px)') {
+        mediaListenerWideWidth = mediaListenerList;
+      } else if (query === '(min-width: 560px)') {
+        mediaListenerMediumWidth = mediaListenerList;
+      } else {
+        assertTrue(false);
+      }
+      return mediaListenerList;
+    });
     BrowserProxy.instance_ = testProxy;
-
     mostVisited = document.createElement('ntp-most-visited');
     document.body.appendChild(mostVisited);
+
+    await testProxy.whenCalled('matchMedia', 2);
+    wide();
   });
 
   test('empty shows add shortcut only', async () => {
-    assertTrue(mostVisited.$.addShortcut.hidden);
+    assertAddShortcutHidden();
     await addTiles(0);
     assertEquals(0, queryTiles().length);
-    assertFalse(mostVisited.$.addShortcut.hidden);
+    assertAddShortcutShown();
   });
 
   test('clicking on add shortcut opens dialog', () => {
@@ -110,7 +166,7 @@ suite('NewTabPageMostVisitedTest', () => {
   test('four tiles fit on one line with addShortcut', async () => {
     await addTiles(4);
     assertEquals(4, queryTiles().length);
-    assertFalse(mostVisited.$.addShortcut.hidden);
+    assertAddShortcutShown();
     const tops = queryAll('a').map(({offsetTop}) => offsetTop);
     assertEquals(5, tops.length);
     tops.forEach(top => {
@@ -121,7 +177,7 @@ suite('NewTabPageMostVisitedTest', () => {
   test('five tiles are displayed on two rows with addShortcut', async () => {
     await addTiles(5);
     assertEquals(5, queryTiles().length);
-    assertFalse(mostVisited.$.addShortcut.hidden);
+    assertAddShortcutShown();
     const tops = queryAll('a').map(({offsetTop}) => offsetTop);
     assertEquals(6, tops.length);
     const firstRowTop = tops[0];
@@ -138,7 +194,7 @@ suite('NewTabPageMostVisitedTest', () => {
   test('nine tiles are displayed on two rows with addShortcut', async () => {
     await addTiles(9);
     assertEquals(9, queryTiles().length);
-    assertFalse(mostVisited.$.addShortcut.hidden);
+    assertAddShortcutShown();
     const tops = queryAll('a').map(({offsetTop}) => offsetTop);
     assertEquals(10, tops.length);
     const firstRowTop = tops[0];
@@ -155,7 +211,7 @@ suite('NewTabPageMostVisitedTest', () => {
   test('ten tiles are displayed on two rows without addShortcut', async () => {
     await addTiles(10);
     assertEquals(10, queryTiles().length);
-    assertTrue(mostVisited.$.addShortcut.hidden);
+    assertAddShortcutHidden();
     const tops = queryAll('a:not([hidden])').map(a => a.offsetTop);
     assertEquals(10, tops.length);
     const firstRowTop = tops[0];
@@ -172,18 +228,18 @@ suite('NewTabPageMostVisitedTest', () => {
   test('ten tiles is the max tiles displayed', async () => {
     await addTiles(11);
     assertEquals(10, queryTiles().length);
-    assertTrue(mostVisited.$.addShortcut.hidden);
+    assertAddShortcutHidden();
   });
 
   test('eight tiles is the max (customLinksEnabled=false)', async () => {
     await addTiles(11, /* customLinksEnabled */ true);
     assertEquals(10, queryTiles().length);
     assertEquals(0, queryAll('.tile[hidden]').length);
-    assertTrue(mostVisited.$.addShortcut.hidden);
+    assertAddShortcutHidden();
     await addTiles(11, /* customLinksEnabled */ false);
     assertEquals(10, queryTiles().length);
     assertEquals(2, queryAll('.tile[hidden]').length);
-    assertTrue(mostVisited.$.addShortcut.hidden);
+    assertAddShortcutHidden();
     await addTiles(11, /* customLinksEnabled */ true);
     assertEquals(10, queryTiles().length);
     assertEquals(0, queryAll('.tile[hidden]').length);
@@ -191,11 +247,11 @@ suite('NewTabPageMostVisitedTest', () => {
 
   test('7 tiles and no add shortcut (customLinksEnabled=false)', async () => {
     await addTiles(7, /* customLinksEnabled */ true);
-    assertFalse(mostVisited.$.addShortcut.hidden);
+    assertAddShortcutShown();
     await addTiles(7, /* customLinksEnabled */ false);
-    assertTrue(mostVisited.$.addShortcut.hidden);
+    assertAddShortcutHidden();
     await addTiles(7, /* customLinksEnabled */ true);
-    assertFalse(mostVisited.$.addShortcut.hidden);
+    assertAddShortcutShown();
   });
 
   test('no tiles shown when (visible=false)', async () => {
@@ -217,6 +273,44 @@ suite('NewTabPageMostVisitedTest', () => {
     assertTrue(dialog.open);
   });
 
+  suite('test various widths', () => {
+    function medium() {
+      updateScreenWidth(false, true);
+    }
+
+    function narrow() {
+      updateScreenWidth(false, false);
+    }
+
+    test('hide add shortcut if on third row (narrow)', async () => {
+      await addTiles(6);
+      medium();
+      assertAddShortcutShown();
+      narrow();
+      assertAddShortcutHidden();
+      medium();
+      assertAddShortcutShown();
+    });
+
+    test('hide add shortcut if on third row (medium)', async () => {
+      await addTiles(8);
+      wide();
+      assertAddShortcutShown();
+      medium();
+      assertAddShortcutHidden();
+      wide();
+      assertAddShortcutShown();
+    });
+
+    test('hide add shortcut if on third row (medium)', async () => {
+      await addTiles(9);
+      wide();
+      assertAddShortcutShown();
+      await addTiles(10);
+      assertAddShortcutHidden();
+    });
+  });
+
   suite('add dialog', () => {
     /** @private {CrDialogElement} */
     let dialog;

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

@@ -51,38 +51,17 @@ export function assertFocus(element) {
   assertEquals(element, getDeepActiveElement());
 }
 
-/**
- * Creates a |TestBrowserProxy|, which has mock functions for all functions of
- * class |clazz|.
- * @param {Class} clazz
- * @return {TestBrowserProxy}
- */
-export function mock(clazz) {
-  const props = Object.getOwnPropertyNames(clazz.prototype);
-  const mockBrowserProxy = new TestBrowserProxy(props);
-  for (const prop of props) {
-    if (prop == 'constructor') {
-      continue;
-    }
-    mockBrowserProxy[prop] = function() {
-      const args = arguments.length == 1 ? arguments[0] : Array.from(arguments);
-      mockBrowserProxy.methodCalled(prop, args);
-      return mockBrowserProxy.getResultFor(prop, undefined);
-    };
-  }
-  return mockBrowserProxy;
-}
-
 /**
  * Creates a mocked test proxy.
  * @return {TestBrowserProxy}
  */
 export function createTestProxy() {
-  const testProxy = mock(BrowserProxy);
+  const testProxy = TestBrowserProxy.fromClass(BrowserProxy);
   testProxy.callbackRouter = new newTabPage.mojom.PageCallbackRouter();
   testProxy.callbackRouterRemote =
       testProxy.callbackRouter.$.bindNewPipeAndPassRemote();
-  testProxy.handler = mock(newTabPage.mojom.PageHandlerRemote);
+  testProxy.handler =
+      TestBrowserProxy.fromClass(newTabPage.mojom.PageHandlerRemote);
   testProxy.setResultFor('createUntrustedIframeSrc', '');
   return testProxy;
 }

chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js

@@ -28,8 +28,6 @@ class TestSiteSettingsPrefsBrowserProxy extends TestBrowserProxy {
       'fetchBlockAutoplayStatus',
       'fetchZoomLevels',
       'getAllSites',
-      'getCookieSettingDescription',
-      'getRecentSitePermissions',
       'getChooserExceptionList',
       'getDefaultValueForContentType',
       'getFormattedBytes',
@@ -74,6 +72,11 @@ class TestSiteSettingsPrefsBrowserProxy extends TestBrowserProxy {
 
     /** @private {boolean} */
     this.isPatternValidForType_ = true;
+
+    this.mockMethods([
+      'getCookieSettingDescription',
+      'getRecentSitePermissions',
+    ]);
   }
 
   /**
@@ -238,18 +241,6 @@ class TestSiteSettingsPrefsBrowserProxy extends TestBrowserProxy {
     return Promise.resolve(result);
   }
 
-  /** @override */
-  getCookieSettingDescription() {
-    this.methodCalled('getCookieSettingDescription');
-    return this.getResultFor('getCookieSettingDescription');
-  }
-
-  /** @override */
-  getRecentSitePermissions(contentTypes, numSources) {
-    this.methodCalled('getRecentSitePermissions', contentTypes);
-    return this.getResultFor('getRecentSitePermissions');
-  }
-
   /** @override */
   getFormattedBytes(numBytes) {
     this.methodCalled('getFormattedBytes', numBytes);

chrome/test/data/webui/test_browser_proxy.js

@@ -8,7 +8,7 @@
 /**
  * @typedef {{resolver: !PromiseResolver,
  *            callCount: number,
- *            result?: *}}
+ *            resultMapper: Function}}
  */
 let MethodData;
 
@@ -42,10 +42,10 @@ let MethodData;
  */
 /* #export */ class TestBrowserProxy {
   /**
-   * @param {!Array<string>} methodNames Names of all methods whose calls
+   * @param {!Array<string>=} methodNames Names of all methods whose calls
    *     need to be tracked.
    */
-  constructor(methodNames) {
+  constructor(methodNames = []) {
     /** @private {!Map<string, !MethodData>} */
     this.resolverMap_ = new Map();
     methodNames.forEach(methodName => {
@@ -53,29 +53,107 @@ let MethodData;
     });
   }
 
+  /**
+   * Creates a |TestBrowserProxy|, which has mock functions for all functions of
+   * class |clazz|.
+   * @param {Class} clazz
+   * @return {TestBrowserProxy}
+   */
+  static fromClass(clazz) {
+    const methodNames = Object.getOwnPropertyNames(clazz.prototype)
+                            .filter(methodName => methodName !== 'constructor');
+    const proxy = new TestBrowserProxy();
+    proxy.mockMethods(methodNames);
+    return proxy;
+  }
+
+  /**
+   * Creates a mock implementation for each method name. These mocks allow tests
+   * to either set a result when the mock is called using
+   * |setResultFor(methodName)|, or set a result mapper function that will be
+   * invoked when a method is called using |setResultMapperFor(methodName)|.
+   * @param {!Array<string>} methodNames
+   * @protected
+   */
+  mockMethods(methodNames) {
+    methodNames.forEach(methodName => {
+      if (!this.resolverMap_.has(methodName)) {
+        this.createMethodData_(methodName);
+      }
+      this[methodName] = function() {
+        const args = Array.from(arguments);
+        const argObject = {};
+        if (args.length > 1) {
+          argObject.args = args;
+        } else {
+          argObject.arg = args[0];
+        }
+        return this.methodCalledWithResult_(methodName, argObject);
+      }.bind(this);
+    });
+  }
+
   /**
    * Called by subclasses when a tracked method is called from the code that
    * is being tested.
    * @param {string} methodName
-   * @param {*=} opt_arg Optional argument to be forwarded to the testing
+   * @param {*=} args Optional argument to be forwarded to the testing
    *     code, useful for checking whether the proxy method was called with
    *     the expected arguments.
    * @protected
    */
-  methodCalled(methodName, opt_arg) {
+  methodCalled(methodName, args) {
     const methodData = this.resolverMap_.get(methodName);
     methodData.callCount += 1;
-    this.resolverMap_.set(methodName, methodData);
-    methodData.resolver.resolve(opt_arg);
+    const {callCount, resolvers} = methodData;
+    if (resolvers.length >= callCount) {
+      resolvers[callCount - 1].resolve(args);
+    } else {
+      assert(resolvers.length === callCount - 1);
+      const resolver = new PromiseResolver();
+      resolver.resolve(args);
+      resolvers.push(resolver);
+    }
   }
 
   /**
+   * Called by subclasses when a tracked method is called from the code that
+   * is being tested.
    * @param {string} methodName
+   * @param {!{arg: *, args: Array}} argObject Optional argument to be forwarded
+   *     to the testing code, useful for checking whether the proxy method was
+   *     called with the expected arguments. Only |arg| or |args| should be set.
+   * @return {*}
+   * @private
+   */
+  methodCalledWithResult_(methodName, {arg, args}) {
+    assert(arg === undefined || args === undefined);
+    this.methodCalled(methodName, args === undefined ? arg : args);
+    const {resultMapper} = this.resolverMap_.get(methodName);
+    if (resultMapper) {
+      assert(typeof resultMapper === 'function');
+      if (args !== undefined) {
+        return resultMapper(...args);
+      }
+      return resultMapper(arg);
+    }
+  }
+
+  /**
+   * @param {string} methodName
+   * @param {number=} callCount
    * @return {!Promise} A promise that is resolved when the given method
-   *     is called.
+   *     is called |callCount| times.
    */
-  whenCalled(methodName) {
-    return this.getMethodData_(methodName).resolver.promise;
+  whenCalled(methodName, callCount = 1) {
+    const {resolvers} = this.getMethodData_(methodName);
+    for (let i = 0; i < callCount - resolvers.length; i++) {
+      resolvers.push(new PromiseResolver());
+    }
+    if (callCount === 1) {
+      return resolvers[0].promise;
+    }
+    return Promise.all(resolvers.slice(callCount).map(r => r.promise));
   }
 
   /**
@@ -106,24 +184,24 @@ let MethodData;
   }
 
   /**
-   * Sets the return value of a method.
+   * Sets a function |resultMapper| that is called with the original arguments
+   * passed to method named |methodName|. This allows a test to return a unique
+   * object each method invovation or have the returned value be different based
+   * on the arguments.
    * @param {string} methodName
-   * @paran {*} value
+   * @param {!Function} resultMapper
    */
-  setResultFor(methodName, value) {
-    this.getMethodData_(methodName).result = value;
+  setResultMapperFor(methodName, resultMapper) {
+    this.getMethodData_(methodName).resultMapper = resultMapper;
   }
 
   /**
-   * Returns the return value of a method or the default value if no return
-   * value is registered.
+   * Sets the return value of a method.
    * @param {string} methodName
-   * @param {*} defaultValue
-   * @return {*}
+   * @param {*} value
    */
-  getResultFor(methodName, defaultValue) {
-    const methodData = this.getMethodData_(methodName);
-    return 'result' in methodData ? methodData.result : defaultValue;
+  setResultFor(methodName, value) {
+    this.getMethodData_(methodName).resultMapper = () => value;
   }
 
   /**
@@ -146,7 +224,6 @@ let MethodData;
    * @private
    */
   createMethodData_(methodName) {
-    this.resolverMap_.set(
-        methodName, {resolver: new PromiseResolver(), callCount: 0});
+    this.resolverMap_.set(methodName, {resolvers: [], callCount: 0});
   }
 }