📰 Ensure NTP Tiles keep tracking recent data

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

Index: chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
index af1c4a2e389543dd0d1b70f5bfcdcb57f7dc348f..15a6e8f366877a484122f32557ffb04dabd88ad8 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
@@ -13,7 +13,6 @@
 import android.support.annotation.Nullable;
 import android.support.v4.graphics.drawable.RoundedBitmapDrawable;
 import android.support.v4.graphics.drawable.RoundedBitmapDrawableFactory;
-import android.text.TextUtils;
 import android.view.ContextMenu;
 import android.view.ContextMenu.ContextMenuInfo;
 import android.view.LayoutInflater;
@@ -24,7 +23,6 @@
 
 import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.Callback;
-import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.favicon.LargeIconBridge.LargeIconCallback;
@@ -78,7 +76,8 @@
      */
     public interface Observer {
         /**
-         * Called when any of the tile data has changed, such as an icon, url, or title.
+         * Called when the tile group is initialised and when any of the tile data has changed,
+         * such as an icon, url, or title.
          */
         void onTileDataChanged();
 
@@ -115,20 +114,37 @@
     private final ContextMenuManager mContextMenuManager;
     private final Delegate mTileGroupDelegate;
     private final Observer mObserver;
+    private final int mTitleLinesCount;
+    private final int mMinIconSize;
+    private final int mDesiredIconSize;
     private final RoundedIconGenerator mIconGenerator;
 
-    private Tile[] mTiles;
-    private int mMinIconSize;
-    private int mDesiredIconSize;
-    boolean mHasReceivedData;
+    /**
+     * Source of truth for the tile data. Since the objects can change when the data is updated,
+     * other objects should not hold references to them but keep track of the URL instead, and use
+     * it to retrieve a {@link Tile}.
+     * @see #getTile(String)
+     */
+    private Tile[] mTiles = new Tile[0];
+    private boolean mHasReceivedData;
 
-    public TileGroup(SuggestionsUiDelegate uiDelegate, ContextMenuManager contextMenuManager,
-            Delegate tileGroupDelegate, Observer observer) {
-        mContext = ContextUtils.getApplicationContext();
+    /**
+     * @param context Used for initialisation and resolving resources.
+     * @param uiDelegate Delegate used to interact with the rest of the system.
+     * @param contextMenuManager Used to handle context menu invocations on the tiles.
+     * @param tileGroupDelegate Used for interactions with the Most Visited backend.
+     * @param observer Will be notified of changes to the tile data.
+     * @param titleLines The number of text lines to use for each tile title.
+     */
+    public TileGroup(Context context, SuggestionsUiDelegate uiDelegate,
+            ContextMenuManager contextMenuManager, Delegate tileGroupDelegate, Observer observer,
+            int titleLines) {
+        mContext = context;
         mUiDelegate = uiDelegate;
         mContextMenuManager = contextMenuManager;
         mTileGroupDelegate = tileGroupDelegate;
         mObserver = observer;
+        mTitleLinesCount = titleLines;
 
         Resources resources = mContext.getResources();
         mDesiredIconSize = resources.getDimensionPixelSize(R.dimen.tile_view_icon_size);
@@ -167,15 +183,12 @@ public void onResult(Set<String> offlineUrls) {
 
     @Override
     public void onIconMadeAvailable(String siteUrl) {
-        // Get a large icon for the matching tile.
-        for (Tile tile : mTiles) {
-            if (tile.getUrl().equals(siteUrl)) {
-                LargeIconCallback iconCallback =
-                        new LargeIconCallbackImpl(tile, /* trackLoadTask = */ false);
-                mUiDelegate.getLargeIconForUrl(siteUrl, mMinIconSize, iconCallback);
-                break;
-            }
-        }
+        Tile tile = getTile(siteUrl);
+        if (tile == null) return; // The tile might have been removed.
+
+        LargeIconCallback iconCallback =
+                new LargeIconCallbackImpl(siteUrl, /* trackLoadTask = */ false);
+        mUiDelegate.getLargeIconForUrl(siteUrl, mMinIconSize, iconCallback);
     }
 
     /**
@@ -184,6 +197,7 @@ public void onIconMadeAvailable(String siteUrl) {
      * @param maxResults The maximum number of sites to retrieve.
      */
     public void startObserving(int maxResults) {
+        mObserver.onLoadTaskAdded();
         mTileGroupDelegate.setMostVisitedSitesObserver(this, maxResults);
     }
 
@@ -192,83 +206,28 @@ public void startObserving(int maxResults) {
      * possible because view inflation and icon loading are slow.
      * @param tileGridLayout The layout to render the tile views into.
      * @param trackLoadTasks Whether to track load tasks.
-     * @param titleLines The number of text lines to use for each tile title.
      */
-    public void renderTileViews(
-            TileGridLayout tileGridLayout, boolean trackLoadTasks, int titleLines) {
+    public void renderTileViews(TileGridLayout tileGridLayout, boolean trackLoadTasks) {
         // Map the old tile views by url so they can be reused later.
         Map<String, TileView> oldTileViews = new HashMap<>();
         int childCount = tileGridLayout.getChildCount();
         for (int i = 0; i < childCount; i++) {
             TileView tileView = (TileView) tileGridLayout.getChildAt(i);
-            oldTileViews.put(tileView.getTile().getUrl(), tileView);
+            oldTileViews.put(tileView.getUrl(), tileView);
         }
 
         // Remove all views from the layout because even if they are reused later they'll have to be
         // added back in the correct order.
         tileGridLayout.removeAllViews();
 
-        for (final Tile tile : mTiles) {
-            // First see if an old view can be reused.
-            if (oldTileViews.containsKey(tile.getUrl())) {
-                TileView oldTileView = oldTileViews.get(tile.getUrl());
-                if (TextUtils.equals(tile.getTitle(), oldTileView.getTile().getTitle())
-                        && tile.isOfflineAvailable() == oldTileView.getTile().isOfflineAvailable()
-                        && TextUtils.equals(tile.getWhitelistIconPath(),
-                                   oldTileView.getTile().getWhitelistIconPath())) {
-                    // Prevent further reuse. https://crbug.com/690926
-                    assert oldTileView.getParent() == null;
-                    oldTileViews.remove(tile.getUrl());
-
-                    tileGridLayout.addView(oldTileView);
-
-                    // Re-render the icon because it may not have been painted when re-added.
-                    oldTileView.renderIcon();
-                    continue;
-                }
+        for (Tile tile : mTiles) {
+            TileView tileView = oldTileViews.get(tile.getUrl());
+            if (tileView == null) {
+                tileView = buildTileView(tile, tileGridLayout, trackLoadTasks, mTitleLinesCount);
+            } else {
+                tileView.updateIfDataChanged(tile);
             }
 
-            // No view was reused, create a new one.
-            TileView tileView = buildTileView(tile, tileGridLayout, trackLoadTasks, titleLines);
-
-            tileView.setOnClickListener(new OnClickListener() {
-                @Override
-                public void onClick(View view) {
-                    mTileGroupDelegate.openMostVisitedItem(WindowOpenDisposition.CURRENT_TAB, tile);
-                }
-            });
-
-            tileView.setOnCreateContextMenuListener(new OnCreateContextMenuListener() {
-                @Override
-                public void onCreateContextMenu(
-                        ContextMenu menu, View view, ContextMenuInfo menuInfo) {
-                    mContextMenuManager.createContextMenu(
-                            menu, view, new ContextMenuManager.Delegate() {
-                                @Override
-                                public void openItem(int windowDisposition) {
-                                    mTileGroupDelegate.openMostVisitedItem(windowDisposition, tile);
-                                }
-
-                                @Override
-                                public void removeItem() {
-                                    mTileGroupDelegate.removeMostVisitedItem(tile);
-                                }
-
-                                @Override
-                                public String getUrl() {
-                                    return tile.getUrl();
-                                }
-
-                                @Override
-                                public boolean isItemSupported(@ContextMenuItemId int menuItemId) {
-                                    return true;
-                                }
-
-                                @Override
-                                public void onContextMenuCreated() {}
-                            });
-                }
-            });
             tileGridLayout.addView(tileView);
         }
     }
@@ -283,19 +242,34 @@ public boolean hasReceivedData() {
 
     private void buildTiles(String[] titles, String[] urls, String[] whitelistIconPaths,
             @Nullable Set<String> offlineUrls, int[] sources) {
-        int oldTileCount = mTiles == null ? 0 : mTiles.length;
-        mTiles = new Tile[titles.length];
-
         boolean isInitialLoad = !mHasReceivedData;
         mHasReceivedData = true;
 
+        Tile[] newTiles = new Tile[titles.length];
+        Set<String> addedUrls = new HashSet<>();
+        boolean countChanged = isInitialLoad || mTiles.length != titles.length;
+        boolean dataChanged = countChanged;
         for (int i = 0; i < titles.length; i++) {
+            assert urls[i] != null; // We assume everywhere that the url is not null.
+
+            // TODO(dgn): Add UMA to track the cause of https://crbug.com/690926. Checking this
+            // should not even be necessary as the backend is supposed to send non dupes URLs.
+            if (addedUrls.contains(urls[i])) {
+                assert false : "Incoming NTP Tiles are not unique. Dupe on " + urls[i];
+                continue;
+            }
+
             boolean offlineAvailable = offlineUrls != null && offlineUrls.contains(urls[i]);
-            mTiles[i] = new Tile(
+            newTiles[i] = new Tile(
                     titles[i], urls[i], whitelistIconPaths[i], offlineAvailable, i, sources[i]);
+            if (newTiles[i].importData(getTile(urls[i]))) dataChanged = true;
+            addedUrls.add(urls[i]);
         }
 
-        if (oldTileCount != mTiles.length) mObserver.onTileCountChanged();
+        if (!dataChanged) return;
+
+        mTiles = newTiles;
+        if (countChanged) mObserver.onTileCountChanged();
         if (isInitialLoad) mObserver.onLoadTaskCompleted();
         mObserver.onTileDataChanged();
     }
@@ -314,12 +288,18 @@ private TileView buildTileView(
                                     .inflate(R.layout.tile_view, parentView, false);
         tileView.initialize(tile, titleLines);
 
-        LargeIconCallback iconCallback = new LargeIconCallbackImpl(tile, trackLoadTask);
+        // Note: It is important that the callbacks below don't keep a reference to the tile or
+        // modify them as there is no guarantee that the same tile would be used to update the view.
+        LargeIconCallback iconCallback = new LargeIconCallbackImpl(tile.getUrl(), trackLoadTask);
         if (trackLoadTask) mObserver.onLoadTaskAdded();
         if (!loadWhitelistIcon(tile, iconCallback)) {
             mUiDelegate.getLargeIconForUrl(tile.getUrl(), mMinIconSize, iconCallback);
         }
 
+        TileInteractionDelegate delegate = new TileInteractionDelegate(tile.getUrl());
+        tileView.setOnClickListener(delegate);
+        tileView.setOnCreateContextMenuListener(delegate);
+
         return tileView;
     }
 
@@ -336,24 +316,38 @@ private boolean loadWhitelistIcon(Tile tile, LargeIconCallback iconCallback) {
         return true;
     }
 
+    /** @return A tile matching the provided URL, or {@code null} if none is found. */
+    @Nullable
+    private Tile getTile(String url) {
+        for (Tile tile : mTiles) {
+            if (tile.getUrl().equals(url)) return tile;
+        }
+        return null;
+    }
+
     private class LargeIconCallbackImpl implements LargeIconCallback {
-        private final Tile mTile;
+        private final String mUrl;
         private final boolean mTrackLoadTask;
 
-        private LargeIconCallbackImpl(Tile tile, boolean trackLoadTask) {
-            mTile = tile;
+        private LargeIconCallbackImpl(String url, boolean trackLoadTask) {
+            mUrl = url;
             mTrackLoadTask = trackLoadTask;
         }
 
         @Override
         public void onLargeIconAvailable(
                 @Nullable Bitmap icon, int fallbackColor, boolean isFallbackColorDefault) {
+            if (mTrackLoadTask) mObserver.onLoadTaskCompleted();
+
+            Tile tile = getTile(mUrl);
+            if (tile == null) return; // The tile might have been removed.
+
             if (icon == null) {
                 mIconGenerator.setBackgroundColor(fallbackColor);
-                icon = mIconGenerator.generateIconForUrl(mTile.getUrl());
-                mTile.setIcon(new BitmapDrawable(mContext.getResources(), icon));
-                mTile.setType(isFallbackColorDefault ? MostVisitedTileType.ICON_DEFAULT
-                                                     : MostVisitedTileType.ICON_COLOR);
+                icon = mIconGenerator.generateIconForUrl(mUrl);
+                tile.setIcon(new BitmapDrawable(mContext.getResources(), icon));
+                tile.setType(isFallbackColorDefault ? MostVisitedTileType.ICON_DEFAULT
+                                                    : MostVisitedTileType.ICON_COLOR);
             } else {
                 RoundedBitmapDrawable roundedIcon =
                         RoundedBitmapDrawableFactory.create(mContext.getResources(), icon);
@@ -363,11 +357,64 @@ public void onLargeIconAvailable(
                 roundedIcon.setCornerRadius(cornerRadius);
                 roundedIcon.setAntiAlias(true);
                 roundedIcon.setFilterBitmap(true);
-                mTile.setIcon(roundedIcon);
-                mTile.setType(MostVisitedTileType.ICON_REAL);
+
+                tile.setIcon(roundedIcon);
+                tile.setType(MostVisitedTileType.ICON_REAL);
             }
-            mObserver.onTileIconChanged(mTile);
-            if (mTrackLoadTask) mObserver.onLoadTaskCompleted();
+
+            mObserver.onTileIconChanged(tile);
+        }
+    }
+
+    private class TileInteractionDelegate
+            implements ContextMenuManager.Delegate, OnClickListener, OnCreateContextMenuListener {
+        private final String mUrl;
+
+        public TileInteractionDelegate(String url) {
+            mUrl = url;
+        }
+
+        @Override
+        public void onClick(View view) {
+            Tile tile = getTile(mUrl);
+            if (tile == null) return;
+
+            mTileGroupDelegate.openMostVisitedItem(WindowOpenDisposition.CURRENT_TAB, tile);
+        }
+
+        @Override
+        public void openItem(int windowDisposition) {
+            Tile tile = getTile(mUrl);
+            if (tile == null) return;
+
+            mTileGroupDelegate.openMostVisitedItem(windowDisposition, tile);
+        }
+
+        @Override
+        public void removeItem() {
+            Tile tile = getTile(mUrl);
+            if (tile == null) return;
+
+            mTileGroupDelegate.removeMostVisitedItem(tile);
+        }
+
+        @Override
+        public String getUrl() {
+            return mUrl;
+        }
+
+        @Override
+        public boolean isItemSupported(@ContextMenuItemId int menuItemId) {
+            return true;
+        }
+
+        @Override
+        public void onContextMenuCreated() {}
+
+        @Override
+        public void onCreateContextMenu(
+                ContextMenu contextMenu, View view, ContextMenuInfo contextMenuInfo) {
+            mContextMenuManager.createContextMenu(contextMenu, view, this);
         }
     }
 }