Revert "Revert "Pin the Home Page Tile to the first row in the UI.""

Reverting the revert that supposedly broke waterfall tests here:
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/44127/.
However, successive builds:
* https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/44128/
* https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/44129/
passed *without* containing the revert commit, which means our original commit most likely was
not the root cause of the broken tests in the first place.

Original CL: https://chromium-review.googlesource.com/c/567925/

Original change's description:

> Pin the Home Page Tile to the first row in the UI.
>
> Calculates the number of columns tiles are layed out in
> at initialization and reorders the home page tile so that
> it is pinned to the first row.
> Follow-up issue: https://crbug.com/746274
>
> BUG=732913
>
> Change-Id: Ieeddf88e8262827c7a22787230c386fe3b43cc17
> Reviewed-on: https://chromium-review.googlesource.com/567925
> Commit-Queue: Ondrej Škopek <oskopek@google.com>
> Reviewed-by: Chris Pickel <sfiera@chromium.org>
> Reviewed-by: Nicolas Dossou-Gbété <dgn@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#488203}


BUG=732913

Change-Id: I359fdb6ca03a31626b41626e01731f2970bcb454
Reviewed-on: https://chromium-review.googlesource.com/579907Reviewed-by: Chris Pickel <sfiera@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Commit-Queue: Ondrej Škopek <oskopek@google.com>
Cr-Commit-Position: refs/heads/master@{#488634}

chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java

@@ -289,7 +289,7 @@ public class NewTabPageView extends FrameLayout implements TileGroup.Observer {
         setSearchProviderHasLogo(searchProviderHasLogo);
         mSearchProviderLogoView.showSearchProviderInitialView();
 
-        mTileGroup.startObserving(getMaxTileRows(searchProviderHasLogo) * getMaxTileColumns());
+        mTileGroup.startObserving(getMaxTileRows(searchProviderHasLogo), getMaxTileColumns());
 
         mRecyclerView.init(mUiConfig, mContextMenuManager);
 

chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGrid.java

@@ -44,7 +44,7 @@ public class TileGrid extends OptionalLeaf implements TileGroup.Observer {
         mTileGroup = new TileGroup(ContextUtils.getApplicationContext(), uiDelegate,
                 contextMenuManager, tileGroupDelegate,
                 /* observer = */ this, offlinePageBridge, getTileTitleLines());
-        mTileGroup.startObserving(getMaxTileRows() * MAX_TILE_COLUMNS);
+        mTileGroup.startObserving(getMaxTileRows(), MAX_TILE_COLUMNS);
     }
 
     @Override

chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGridLayout.java

@@ -8,10 +8,13 @@ import android.content.Context;
 import android.content.res.Resources;
 import android.text.TextUtils;
 import android.util.AttributeSet;
+import android.util.DisplayMetrics;
+import android.util.TypedValue;
 import android.view.View;
 import android.widget.FrameLayout;
 
 import org.chromium.base.ApiCompatibilityUtils;
+import org.chromium.base.ContextUtils;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.util.MathUtils;
@@ -20,6 +23,12 @@ import org.chromium.chrome.browser.util.MathUtils;
  * A layout that arranges tiles in a grid.
  */
 public class TileGridLayout extends FrameLayout {
+    public static final int PADDING_START_PX = 0;
+    public static final int PADDING_END_PX = 0;
+
+    public static final int MARGIN_START_DP = 12;
+    public static final int MARGIN_END_DP = 12;
+
     private final int mVerticalSpacing;
     private final int mMinHorizontalSpacing;
     private final int mMaxHorizontalSpacing;
@@ -29,6 +38,36 @@ public class TileGridLayout extends FrameLayout {
     private int mMaxColumns;
     private int mExtraVerticalSpacing;
 
+    /**
+     * Calculate the number of tile columns that will actually be rendered. Uses constants, does not
+     * rely on anything already being rendered. Used for calculating the position of the home page
+     * tile.
+     *
+     * @see TileGroup#renderTileViews(ViewGroup, boolean)
+     * @return The number of rendered columns (at least 1). Still needs to be capped from above
+     * by the maximum number of columns.
+     */
+    public static int calculateNumColumns() {
+        Resources res = ContextUtils.getApplicationContext().getResources();
+        int minHorizontalSpacingPx =
+                res.getDimensionPixelOffset(R.dimen.tile_grid_layout_min_horizontal_spacing);
+        int maxWidthPx = res.getDimensionPixelOffset(R.dimen.tile_grid_layout_max_width);
+
+        DisplayMetrics metrics = res.getDisplayMetrics();
+        int totalWidthPx =
+                Math.min(maxWidthPx, Math.min(metrics.widthPixels, metrics.heightPixels));
+
+        // Determine the number of columns that will fit.
+        int marginsPx = Math.round(TypedValue.applyDimension(
+                TypedValue.COMPLEX_UNIT_DIP, MARGIN_START_DP + MARGIN_END_DP, metrics));
+        int gridWidthPx = totalWidthPx - PADDING_START_PX - PADDING_END_PX - marginsPx;
+        int childWidthPx = res.getDimensionPixelOffset(R.dimen.tile_view_width);
+
+        return Math.max(
+                (gridWidthPx + minHorizontalSpacingPx) / (childWidthPx + minHorizontalSpacingPx),
+                1);
+    }
+
     /**
      * Constructor for inflating from XML.
      *
@@ -108,8 +147,7 @@ public class TileGridLayout extends FrameLayout {
         }
 
         // Determine the number of columns that will fit.
-        int gridWidth = totalWidth - ApiCompatibilityUtils.getPaddingStart(this)
-                - ApiCompatibilityUtils.getPaddingEnd(this);
+        int gridWidth = totalWidth - PADDING_START_PX - PADDING_END_PX;
         int childHeight = getChildAt(0).getMeasuredHeight();
         int childWidth = getChildAt(0).getMeasuredWidth();
         int numColumns = MathUtils.clamp(

chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java

@@ -200,6 +200,14 @@ public class TileGroup implements MostVisitedSites.Observer {
 
     private boolean mHasReceivedData;
 
+    /**
+     * The number of columns tiles get rendered in. Precalculated upon calling
+     * {@link #startObserving(int, int)} and constant from then on. Used for pinning the home page
+     * tile to the first tile row.
+     * @see #renderTileViews(ViewGroup, boolean)
+     */
+    private int mNumColumns;
+
     /**
      * @param context Used for initialisation and resolving resources.
      * @param uiDelegate Delegate used to interact with the rest of the system.
@@ -260,7 +268,22 @@ public class TileGroup implements MostVisitedSites.Observer {
             // send non dupes URLs. Remove once https://crbug.com/703628 is fixed.
             if (addedUrls.contains(urls[i])) continue;
 
-            mPendingTiles.add(new Tile(titles[i], urls[i], whitelistIconPaths[i], i, sources[i]));
+            // The home page tile is pinned to the first row of tiles. It will appear on
+            // the position corresponding to its ranking among all tiles (obtained from the
+            // ntp_tiles C++ component). If its position is larger than the number of tiles
+            // in the first row, it will appear on the last position of the first row.
+            // Do note, that the number of tiles in a row (column number) is determined upon
+            // initialization and not changed afterwards.
+            if (sources[i] == TileSource.HOMEPAGE) {
+                int homeTilePosition = Math.min(mPendingTiles.size(), mNumColumns - 1);
+                mPendingTiles.add(homeTilePosition,
+                        new Tile(titles[i], urls[i], whitelistIconPaths[i], homeTilePosition,
+                                sources[i]));
+            } else {
+                mPendingTiles.add(new Tile(titles[i], urls[i], whitelistIconPaths[i],
+                        mPendingTiles.size(), sources[i]));
+            }
+
             addedUrls.add(urls[i]);
 
             if (urls[i].equals(mPendingRemovalUrl)) removalCompleted = false;
@@ -293,11 +316,13 @@ public class TileGroup implements MostVisitedSites.Observer {
     /**
      * Instructs this instance to start listening for data. The {@link TileGroup.Observer} may be
      * called immediately if new data is received synchronously.
-     * @param maxResults The maximum number of sites to retrieve.
+     * @param maxRows The maximum number of rows to fetch.
+     * @param maxColumns The maximum number of columns to fetch.
      */
-    public void startObserving(int maxResults) {
+    public void startObserving(int maxRows, int maxColumns) {
         addTask(TileTask.FETCH_DATA);
-        mTileGroupDelegate.setMostVisitedSitesObserver(this, maxResults);
+        mNumColumns = Math.min(maxColumns, TileGridLayout.calculateNumColumns());
+        mTileGroupDelegate.setMostVisitedSitesObserver(this, maxRows * maxColumns);
     }
 
     /**

chrome/android/java_sources.gni

@@ -1635,6 +1635,7 @@ chrome_test_java_sources = [
   "javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java",
   "javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetUiCaptureTest.java",
   "javatests/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java",
+  "javatests/src/org/chromium/chrome/browser/suggestions/TileGridLayoutTest.java",
   "javatests/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderTest.java",
   "javatests/src/org/chromium/chrome/browser/sync/FakeProfileSyncService.java",
   "javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java",

chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/TileGridLayoutTest.java

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.suggestions;
+
+import android.app.Activity;
+import android.content.pm.ActivityInfo;
+import android.support.test.InstrumentationRegistry;
+import android.support.test.filters.MediumTest;
+import android.support.test.filters.SmallTest;
+import android.util.DisplayMetrics;
+import android.util.TypedValue;
+import android.view.View;
+import android.view.View.MeasureSpec;
+import android.view.ViewGroup;
+import android.widget.LinearLayout;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.base.ApiCompatibilityUtils;
+import org.chromium.base.ThreadUtils;
+import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.base.test.util.Feature;
+import org.chromium.base.test.util.RetryOnFailure;
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.ChromeSwitches;
+import org.chromium.chrome.browser.UrlConstants;
+import org.chromium.chrome.browser.ntp.NewTabPage;
+import org.chromium.chrome.browser.ntp.cards.NewTabPageRecyclerView;
+import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.test.ChromeActivityTestRule;
+import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
+import org.chromium.chrome.test.util.NewTabPageTestUtils;
+import org.chromium.chrome.test.util.browser.RecyclerViewTestUtils;
+import org.chromium.chrome.test.util.browser.suggestions.FakeSuggestionsSource;
+import org.chromium.chrome.test.util.browser.suggestions.SuggestionsDependenciesRule;
+import org.chromium.content.browser.test.util.Criteria;
+import org.chromium.content.browser.test.util.CriteriaHelper;
+import org.chromium.net.test.EmbeddedTestServerRule;
+
+import java.util.Arrays;
+
+/**
+ * Instrumentation tests for the {@link TileGridLayout} on the New Tab Page.
+ */
+@RunWith(ChromeJUnit4ClassRunner.class)
+@CommandLineFlags.Add({
+        ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
+        ChromeActivityTestRule.DISABLE_NETWORK_PREDICTION_FLAG,
+})
+@RetryOnFailure
+public class TileGridLayoutTest {
+    @Rule
+    public ChromeTabbedActivityTestRule mActivityTestRule = new ChromeTabbedActivityTestRule();
+
+    @Rule
+    public SuggestionsDependenciesRule mSuggestionsDeps = new SuggestionsDependenciesRule();
+
+    @Rule
+    public EmbeddedTestServerRule mTestServerRule = new EmbeddedTestServerRule();
+
+    private static final String HOME_PAGE_URL = "http://ho.me/";
+
+    private static final String[] FAKE_MOST_VISITED_URLS = new String[] {
+            "/chrome/test/data/android/navigate/one.html",
+            "/chrome/test/data/android/navigate/two.html",
+            "/chrome/test/data/android/navigate/three.html",
+            "/chrome/test/data/android/navigate/four.html",
+            "/chrome/test/data/android/navigate/five.html",
+            "/chrome/test/data/android/navigate/six.html",
+            "/chrome/test/data/android/navigate/seven.html",
+            "/chrome/test/data/android/navigate/eight.html",
+            "/chrome/test/data/android/navigate/nine.html",
+    };
+
+    private NewTabPage mNtp;
+    private String[] mSiteSuggestionUrls;
+    private FakeMostVisitedSites mMostVisitedSites;
+
+    @Before
+    public void setUp() throws Exception {
+        mSiteSuggestionUrls = mTestServerRule.getServer().getURLs(FAKE_MOST_VISITED_URLS);
+        String[] tmpSiteSuggestionUrls = new String[mSiteSuggestionUrls.length + 1];
+        System.arraycopy(
+                mSiteSuggestionUrls, 0, tmpSiteSuggestionUrls, 0, mSiteSuggestionUrls.length);
+        tmpSiteSuggestionUrls[tmpSiteSuggestionUrls.length - 1] = HOME_PAGE_URL;
+        mSiteSuggestionUrls = tmpSiteSuggestionUrls;
+
+        mMostVisitedSites = new FakeMostVisitedSites();
+        mSuggestionsDeps.getFactory().mostVisitedSites = mMostVisitedSites;
+
+        String[] whitelistIconPaths = new String[mSiteSuggestionUrls.length];
+        Arrays.fill(whitelistIconPaths, "");
+
+        int[] sources = new int[mSiteSuggestionUrls.length];
+        Arrays.fill(sources, TileSource.TOP_SITES);
+        sources[mSiteSuggestionUrls.length - 1] = TileSource.HOMEPAGE;
+
+        mMostVisitedSites.setTileSuggestions(
+                mSiteSuggestionUrls, mSiteSuggestionUrls.clone(), whitelistIconPaths, sources);
+
+        mSuggestionsDeps.getFactory().suggestionsSource = new FakeSuggestionsSource();
+
+        mActivityTestRule.startMainActivityWithURL(UrlConstants.NTP_URL);
+        Tab mTab = mActivityTestRule.getActivity().getActivityTab();
+        NewTabPageTestUtils.waitForNtpLoaded(mTab);
+
+        Assert.assertTrue(mTab.getNativePage() instanceof NewTabPage);
+        mNtp = (NewTabPage) mTab.getNativePage();
+
+        RecyclerViewTestUtils.waitForStableRecyclerView(getRecyclerView());
+    }
+
+    /**
+     * Verifies that the assumed constants in {@link TileGridLayout#calculateNumColumns()}
+     * are correct.
+     */
+    @Test
+    @SmallTest
+    @Feature({"NewTabPage"})
+    public void testNumColumnsPaddingAndMarginSizes() {
+        TileGridLayout tileGridLayout = getTileGridLayout();
+        Assert.assertEquals(TileGridLayout.PADDING_START_PX,
+                ApiCompatibilityUtils.getPaddingStart(tileGridLayout));
+        Assert.assertEquals(
+                TileGridLayout.PADDING_END_PX, ApiCompatibilityUtils.getPaddingEnd(tileGridLayout));
+
+        DisplayMetrics metrics = tileGridLayout.getResources().getDisplayMetrics();
+        LinearLayout.LayoutParams layoutParams =
+                (LinearLayout.LayoutParams) tileGridLayout.getLayoutParams();
+        Assert.assertEquals(convertToPx(TileGridLayout.MARGIN_START_DP, metrics),
+                ApiCompatibilityUtils.getMarginStart(layoutParams));
+        Assert.assertEquals(convertToPx(TileGridLayout.MARGIN_END_DP, metrics),
+                ApiCompatibilityUtils.getMarginEnd(layoutParams));
+    }
+
+    @Test
+    @MediumTest
+    @Feature({"NewTabPage"})
+    public void testNumColumnsCalculatedEqualToMeasurement() {
+        final int maxColumns = 6;
+        final TileGridLayout layout = getTileGridLayout();
+        layout.setMaxRows(1);
+        layout.setMaxColumns(maxColumns);
+
+        reMeasureLayout(layout);
+        int calculatedNumColumns = Math.min(maxColumns, TileGridLayout.calculateNumColumns());
+
+        final Activity activity = mActivityTestRule.getActivity();
+        activity.setRequestedOrientation(ActivityInfo.SCREEN_ORIENTATION_PORTRAIT);
+        InstrumentationRegistry.getInstrumentation().waitForIdleSync();
+        int portraitVisibleTiles = countVisibleTiles(layout, calculatedNumColumns);
+        activity.setRequestedOrientation(ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE);
+        InstrumentationRegistry.getInstrumentation().waitForIdleSync();
+        int landscapeVisibleTiles = countVisibleTiles(layout, calculatedNumColumns);
+        Assert.assertEquals("The calculated number of columns should be equal to the minimum number"
+                        + " of columns across screen orientations.",
+                Math.min(portraitVisibleTiles, landscapeVisibleTiles), calculatedNumColumns);
+    }
+
+    private int convertToPx(int dp, DisplayMetrics metrics) {
+        return Math.round(TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, dp, metrics));
+    }
+
+    private void reMeasureLayout(final ViewGroup group) {
+        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
+            @Override
+            public void run() {
+                group.measure(MeasureSpec.UNSPECIFIED, MeasureSpec.UNSPECIFIED);
+            }
+        });
+
+        // Wait until the view is updated. TODO(oskopek): Is there a better criterion to check for?
+        CriteriaHelper.pollUiThread(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                boolean hasHiddenTile = false;
+                for (int i = 0; i < group.getChildCount(); i++) {
+                    if (!hasHiddenTile) {
+                        hasHiddenTile = group.getChildAt(i).getVisibility() == View.GONE;
+                    }
+                }
+
+                return hasHiddenTile;
+            }
+        });
+    }
+
+    private int countVisibleTiles(ViewGroup group, int calculatedNumColumns) {
+        reMeasureLayout(group);
+        int visibleTileViews = 0;
+        boolean hasHomePage = false;
+        for (int i = 0; i < group.getChildCount(); i++) {
+            TileView tileView = (TileView) group.getChildAt(i);
+            if (tileView.getVisibility() == View.VISIBLE) {
+                visibleTileViews++;
+                if (HOME_PAGE_URL.equals(tileView.getUrl())) {
+                    hasHomePage = true;
+                }
+            }
+        }
+
+        Assert.assertTrue("The calculated number of columns should be less than or equal to"
+                        + "the measured one.",
+                calculatedNumColumns <= visibleTileViews);
+        Assert.assertTrue("Visible tiles should contain the home page tile.", hasHomePage);
+
+        return visibleTileViews;
+    }
+
+    private NewTabPageRecyclerView getRecyclerView() {
+        return mNtp.getNewTabPageView().getRecyclerView();
+    }
+
+    private TileGridLayout getTileGridLayout() {
+        TileGridLayout tileGridLayout =
+                (TileGridLayout) mNtp.getNewTabPageView().findViewById(R.id.tile_grid_layout);
+        Assert.assertNotNull("Unable to retrieve the TileGridLayout.", tileGridLayout);
+        return tileGridLayout;
+    }
+}

chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java

@@ -60,7 +60,8 @@ import java.util.List;
 @Features({@Features.Register(ChromeFeatureList.NTP_OFFLINE_PAGES_FEATURE_NAME),
         @Features.Register(ChromeFeatureList.SUGGESTIONS_HOME_MODERN_LAYOUT)})
 public class TileGroupTest {
-    private static final int MAX_TILES_TO_FETCH = 4;
+    private static final int MAX_COLUMNS_TO_FETCH = 4;
+    private static final int MAX_ROWS_TO_FETCH = 1;
     private static final int TILE_TITLE_LINES = 1;
     private static final String[] URLS = {"https://www.google.com", "https://tellmedadjokes.com"};
 
@@ -93,7 +94,7 @@ public class TileGroupTest {
                 new TileGroup(RuntimeEnvironment.application, mock(SuggestionsUiDelegate.class),
                         mock(ContextMenuManager.class), mTileGroupDelegate, mTileGroupObserver,
                         mock(OfflinePageBridge.class), TILE_TITLE_LINES);
-        tileGroup.startObserving(MAX_TILES_TO_FETCH);
+        tileGroup.startObserving(MAX_ROWS_TO_FETCH, MAX_COLUMNS_TO_FETCH);
 
         notifyTileUrlsAvailable(URLS);
 
@@ -115,7 +116,7 @@ public class TileGroupTest {
                 new TileGroup(RuntimeEnvironment.application, mock(SuggestionsUiDelegate.class),
                         mock(ContextMenuManager.class), mTileGroupDelegate, mTileGroupObserver,
                         mock(OfflinePageBridge.class), TILE_TITLE_LINES);
-        tileGroup.startObserving(MAX_TILES_TO_FETCH);
+        tileGroup.startObserving(MAX_ROWS_TO_FETCH, MAX_COLUMNS_TO_FETCH);
 
         notifyTileUrlsAvailable(/* nothing! */);
 
@@ -205,7 +206,7 @@ public class TileGroupTest {
         TileGroup tileGroup = new TileGroup(RuntimeEnvironment.application, uiDelegate,
                 mock(ContextMenuManager.class), mTileGroupDelegate, mTileGroupObserver,
                 mock(OfflinePageBridge.class), TILE_TITLE_LINES);
-        tileGroup.startObserving(MAX_TILES_TO_FETCH);
+        tileGroup.startObserving(MAX_ROWS_TO_FETCH, MAX_COLUMNS_TO_FETCH);
 
         notifyTileUrlsAvailable(URLS);
 
@@ -220,7 +221,7 @@ public class TileGroupTest {
         TileGroup tileGroup = new TileGroup(RuntimeEnvironment.application, uiDelegate,
                 mock(ContextMenuManager.class), mTileGroupDelegate, mTileGroupObserver,
                 mock(OfflinePageBridge.class), TILE_TITLE_LINES);
-        tileGroup.startObserving(MAX_TILES_TO_FETCH);
+        tileGroup.startObserving(MAX_ROWS_TO_FETCH, MAX_COLUMNS_TO_FETCH);
 
         notifyTileUrlsAvailable(URLS);
         reset(mTileGroupObserver);
@@ -260,7 +261,7 @@ public class TileGroupTest {
         TileGroup tileGroup = new TileGroup(RuntimeEnvironment.application, uiDelegate,
                 mock(ContextMenuManager.class), mTileGroupDelegate, mTileGroupObserver,
                 mock(OfflinePageBridge.class), TILE_TITLE_LINES);
-        tileGroup.startObserving(MAX_TILES_TO_FETCH);
+        tileGroup.startObserving(MAX_ROWS_TO_FETCH, MAX_COLUMNS_TO_FETCH);
         ViewGroup layout = new FrameLayout(RuntimeEnvironment.application, null);
 
         // Initialise the internal list of tiles
@@ -281,7 +282,7 @@ public class TileGroupTest {
         TileGroup tileGroup = new TileGroup(RuntimeEnvironment.application, uiDelegate,
                 mock(ContextMenuManager.class), mTileGroupDelegate, mTileGroupObserver,
                 mock(OfflinePageBridge.class), TILE_TITLE_LINES);
-        tileGroup.startObserving(MAX_TILES_TO_FETCH);
+        tileGroup.startObserving(MAX_ROWS_TO_FETCH, MAX_COLUMNS_TO_FETCH);
         ViewGroup layout = new FrameLayout(RuntimeEnvironment.application, null);
 
         // Initialise the internal list of tiles
@@ -301,7 +302,7 @@ public class TileGroupTest {
         TileGroup tileGroup = new TileGroup(RuntimeEnvironment.application, uiDelegate,
                 mock(ContextMenuManager.class), mTileGroupDelegate, mTileGroupObserver,
                 mock(OfflinePageBridge.class), TILE_TITLE_LINES);
-        tileGroup.startObserving(MAX_TILES_TO_FETCH);
+        tileGroup.startObserving(MAX_ROWS_TO_FETCH, MAX_COLUMNS_TO_FETCH);
         notifyTileUrlsAvailable(URLS);
 
         // Initialise the layout with views whose URLs don't match the ones of the new tiles.
@@ -327,7 +328,7 @@ public class TileGroupTest {
                 new TileGroup(RuntimeEnvironment.application, mock(SuggestionsUiDelegate.class),
                         mock(ContextMenuManager.class), mTileGroupDelegate, mTileGroupObserver,
                         mock(OfflinePageBridge.class), TILE_TITLE_LINES);
-        tileGroup.startObserving(MAX_TILES_TO_FETCH);
+        tileGroup.startObserving(MAX_ROWS_TO_FETCH, MAX_COLUMNS_TO_FETCH);
         notifyTileUrlsAvailable(URLS);
 
         // Initialise the layout with views whose URLs match the ones of the new tiles.
@@ -438,7 +439,7 @@ public class TileGroupTest {
 
     /**
      * Notifies the tile group of new tiles created from the provided URLs. Requires
-     * {@link TileGroup#startObserving(int)} to have been called on the tile group under test.
+     * {@link TileGroup#startObserving(int, int)} to have been called on the tile group under test.
      * @see TileGroup#onMostVisitedURLsAvailable(String[], String[], String[], int[])
      */
     private void notifyTileUrlsAvailable(String... urls) {
@@ -471,7 +472,7 @@ public class TileGroupTest {
         TileGroup tileGroup = new TileGroup(RuntimeEnvironment.application, uiDelegate,
                 mock(ContextMenuManager.class), mTileGroupDelegate, mTileGroupObserver,
                 mock(OfflinePageBridge.class), TILE_TITLE_LINES);
-        tileGroup.startObserving(MAX_TILES_TO_FETCH);
+        tileGroup.startObserving(MAX_ROWS_TO_FETCH, MAX_COLUMNS_TO_FETCH);
         notifyTileUrlsAvailable(urls);
 
         ViewGroup layout = new FrameLayout(RuntimeEnvironment.application, null);

components/ntp_tiles/most_visited_sites.cc

@@ -33,9 +33,6 @@ namespace {
 const base::Feature kDisplaySuggestionsServiceTiles{
     "DisplaySuggestionsServiceTiles", base::FEATURE_ENABLED_BY_DEFAULT};
 
-// The maximum index of the home page tile.
-const size_t kMaxHomeTileIndex = 3;
-
 // Determine whether we need any tiles from PopularSites to fill up a grid of
 // |num_tiles| tiles.
 bool NeedPopularSites(const PrefService* prefs, int num_tiles) {
@@ -404,38 +401,34 @@ NTPTilesVector MostVisitedSites::InsertHomeTile(
 
   const GURL& home_page_url = home_page_client_->GetHomePageUrl();
   NTPTilesVector new_tiles;
-  // Add the home tile to the first four tiles.
-  NTPTile home_tile;
-  home_tile.url = home_page_url;
-  home_tile.title = title;
-  home_tile.source = TileSource::HOMEPAGE;
-
   bool home_tile_added = false;
-  size_t index = 0;
 
-  while (index < tiles.size() && new_tiles.size() < num_sites_) {
-    bool hosts_are_equal = tiles[index].url.host() == home_page_url.host();
+  for (auto& tile : tiles) {
+    if (new_tiles.size() >= num_sites_) {
+      break;
+    }
 
-    // Add the home tile to the first four tiles
-    // or at the position of a tile that has the same host
-    // and is ranked higher.
     // TODO(fhorschig): Introduce a more sophisticated deduplication.
-    if (!home_tile_added && (index >= kMaxHomeTileIndex || hosts_are_equal)) {
-      new_tiles.push_back(std::move(home_tile));
+    if (tile.url.host() == home_page_url.host()) {
+      tile.source = TileSource::HOMEPAGE;
       home_tile_added = true;
-      continue;  // Do not advance the current tile index.
     }
-
-    // Add non-home page tiles.
-    if (!hosts_are_equal) {
-      new_tiles.push_back(std::move(tiles[index]));
-    }
-    ++index;
+    new_tiles.push_back(std::move(tile));
   }
 
   // Add the home page tile if there are less than 4 tiles
   // and none of them is the home page (and there is space left).
-  if (!home_tile_added && new_tiles.size() < num_sites_) {
+  if (!home_tile_added) {
+    // Make room for the home page tile.
+    if (new_tiles.size() >= num_sites_) {
+      new_tiles.pop_back();
+    }
+
+    NTPTile home_tile;
+    home_tile.url = home_page_url;
+    home_tile.title = title;
+    home_tile.source = TileSource::HOMEPAGE;
+
     new_tiles.push_back(std::move(home_tile));
   }
   return new_tiles;

components/ntp_tiles/most_visited_sites_unittest.cc

@@ -549,7 +549,7 @@ TEST_P(MostVisitedSitesTest, ShouldNotIncludeHomePageIfNoTileRequested) {
   base::RunLoop().RunUntilIdle();
 }
 
-TEST_P(MostVisitedSitesTest, ShouldReturnMostPopularPageIfOneTileRequested) {
+TEST_P(MostVisitedSitesTest, ShouldReturnHomePageIfOneTileRequested) {
   FakeHomePageClient* home_page_client = RegisterNewHomePageClient();
   home_page_client->SetHomePageEnabled(true);
   DisableRemoteSuggestions();
@@ -561,14 +561,14 @@ TEST_P(MostVisitedSitesTest, ShouldReturnMostPopularPageIfOneTileRequested) {
       .Times(AnyNumber())
       .WillRepeatedly(Return(false));
   EXPECT_CALL(mock_observer_,
-              OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
-                  "Site 1", "http://site1/", TileSource::TOP_SITES))));
+              OnMostVisitedURLsAvailable(ElementsAre(
+                  MatchesTile("", kHomePageUrl, TileSource::HOMEPAGE))));
   most_visited_sites_->SetMostVisitedURLsObserver(&mock_observer_,
                                                   /*num_sites=*/1);
   base::RunLoop().RunUntilIdle();
 }
 
-TEST_P(MostVisitedSitesTest, ShouldContainHomePageInFirstFourTiles) {
+TEST_P(MostVisitedSitesTest, ShouldReplaceLastTileWithHomePageWhenFull) {
   FakeHomePageClient* home_page_client = RegisterNewHomePageClient();
   home_page_client->SetHomePageEnabled(true);
   DisableRemoteSuggestions();
@@ -588,12 +588,38 @@ TEST_P(MostVisitedSitesTest, ShouldContainHomePageInFirstFourTiles) {
   EXPECT_CALL(mock_observer_, OnMostVisitedURLsAvailable(_))
       .WillOnce(SaveArg<0>(&tiles));
   most_visited_sites_->SetMostVisitedURLsObserver(&mock_observer_,
-                                                  /*num_sites=*/8);
+                                                  /*num_sites=*/4);
   base::RunLoop().RunUntilIdle();
-  // Assert that the home page tile is in the first four tiles.
+  // Assert that the home page replaces the final tile.
   EXPECT_THAT(tiles[3], MatchesTile("", kHomePageUrl, TileSource::HOMEPAGE));
 }
 
+TEST_P(MostVisitedSitesTest, ShouldAppendHomePageWhenNotFull) {
+  FakeHomePageClient* home_page_client = RegisterNewHomePageClient();
+  home_page_client->SetHomePageEnabled(true);
+  DisableRemoteSuggestions();
+  EXPECT_CALL(*mock_top_sites_, GetMostVisitedURLs(_, false))
+      .WillRepeatedly(InvokeCallbackArgument<0>((MostVisitedURLList{
+          MakeMostVisitedURL("Site 1", "http://site1/"),
+          MakeMostVisitedURL("Site 2", "http://site2/"),
+          MakeMostVisitedURL("Site 3", "http://site3/"),
+          MakeMostVisitedURL("Site 4", "http://site4/"),
+          MakeMostVisitedURL("Site 5", "http://site5/"),
+      })));
+  EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
+  EXPECT_CALL(*mock_top_sites_, IsBlacklisted(Eq(GURL(kHomePageUrl))))
+      .Times(AnyNumber())
+      .WillRepeatedly(Return(false));
+  std::vector<NTPTile> tiles;
+  EXPECT_CALL(mock_observer_, OnMostVisitedURLsAvailable(_))
+      .WillOnce(SaveArg<0>(&tiles));
+  most_visited_sites_->SetMostVisitedURLsObserver(&mock_observer_,
+                                                  /*num_sites=*/8);
+  base::RunLoop().RunUntilIdle();
+  // Assert that the home page is appended as the final tile.
+  EXPECT_THAT(tiles[5], MatchesTile("", kHomePageUrl, TileSource::HOMEPAGE));
+}
+
 TEST_P(MostVisitedSitesTest, ShouldDeduplicateHomePageWithTopSites) {
   FakeHomePageClient* home_page_client = RegisterNewHomePageClient();
   home_page_client->SetHomePageEnabled(true);