📰 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..a1f5f6f9e022e73a5483973906c38b807c0f850a 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();
 
@@ -116,19 +115,28 @@
     private final Delegate mTileGroupDelegate;
     private final Observer mObserver;
     private final RoundedIconGenerator mIconGenerator;
+    private final int mTitleLinesCount;

[Michael van Ouwerkerk, 2017/02/22 12:11:59]

nit: move this before mIconGenerator as it is assigned earlier.

[dgn, 2017/02/22 17:22:19]

Done.

+    private final int mMinIconSize;
+    private final int mDesiredIconSize;
 
-    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();
+    public TileGroup(Context context, SuggestionsUiDelegate uiDelegate,

[Michael van Ouwerkerk, 2017/02/22 12:11:59]

Maybe it's time for documenting all these params.

[dgn, 2017/02/22 17:22:19]

Done.

+            ContextMenuManager contextMenuManager, Delegate tileGroupDelegate, Observer observer,
+            int tileTitleLines) {

[Michael van Ouwerkerk, 2017/02/22 12:11:59]

nit: maybe rename tileTitleLines to titleLines, as we're in TileGroup and the
'tile' part is pretty clear.

[dgn, 2017/02/22 17:22:19]

Done.

+        mContext = context;
         mUiDelegate = uiDelegate;
         mContextMenuManager = contextMenuManager;
         mTileGroupDelegate = tileGroupDelegate;
         mObserver = observer;
+        mTitleLinesCount = tileTitleLines;
 
         Resources resources = mContext.getResources();
         mDesiredIconSize = resources.getDimensionPixelSize(R.dimen.tile_view_icon_size);
@@ -167,15 +175,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.

[Michael van Ouwerkerk, 2017/02/22 12:11:59]

Should this ever happen? That would be surprising, as we blacklist the url in
the same place that calls this method. If this should never happen, we can
assert that instead.

[dgn, 2017/02/22 17:22:19]

It could also be that other tiles have been fetched that have higher priority
and kicked this one off the group. So it's not rendered in the UI even though
it's not blacklisted.

Also the old code doesn't assert in case it doesn't find the Tile.

+
+        LargeIconCallback iconCallback =
+                new LargeIconCallbackImpl(siteUrl, /* trackLoadTask = */ false);
+        mUiDelegate.getLargeIconForUrl(siteUrl, mMinIconSize, iconCallback);
     }
 
     /**
@@ -184,6 +189,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 +198,31 @@ 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) {
+            // Prevent further reuse. https://crbug.com/690926
+            // TODO(dgn): That implies we had duplicated tiles. WAI?

[Michael van Ouwerkerk, 2017/02/22 12:12:00]

Yea not sure how that ever happened, but it was the only way I could that crash
happening. I do not believe we should ever have duplicate tiles, so we could
chase that further. Maybe with UMA tracking.

[dgn, 2017/02/22 17:22:19]

Moved the check to where we build mTiles, since the map used above means we
should not have duplicates at the level of the TileViews themselves. We'll see
if we still get crashes.

+            TileView tileView = oldTileViews.remove(tile.getUrl());
+            if (tileView == null) {
+                tileView = buildTileView(tile, tileGridLayout, trackLoadTasks, mTitleLinesCount);
+            } else {
+                // TODO(dgn): verify it picks up when forcing the refresh of the drawable is needed.
+                tileView.maybeUpdateView(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,24 +237,29 @@ 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;
 
-        for (int i = 0; i < titles.length; i++) {
+        Tile[] newTiles = new Tile[titles.length];
+        boolean countChanged = isInitialLoad || mTiles.length != newTiles.length;
+        boolean dataChanged = countChanged;

[Michael van Ouwerkerk, 2017/02/22 12:11:59]

This seems wrong, couldn't the data change without affecting the number of
tiles?

[dgn, 2017/02/22 17:22:19]

Yes, but for initialisation purposes, if the count changes, then the data has
changed. Then if the data for the tiles themselves is different, we would also
set the flag. Otherwise, if it's only that a tile was removed, it wouldn't be
marked as a data change, since the remaining tiles are the same as before. It
also ensures that we report the initial load as a data change.

+        for (int i = 0; i < newTiles.length; i++) {
             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;
         }
 
-        if (oldTileCount != mTiles.length) mObserver.onTileCountChanged();
+        if (!dataChanged) return;
+
+        mTiles = newTiles;
+        if (countChanged) mObserver.onTileCountChanged();
         if (isInitialLoad) mObserver.onLoadTaskCompleted();
         mObserver.onTileDataChanged();
     }
 
     /**
+     * TODO update doc, it does not load the view anymore.

[Michael van Ouwerkerk, 2017/02/22 12:11:59]

Maybe do this TODO :-)

[dgn, 2017/02/22 17:22:19]

It's still up to date. Removed the comment.

      * Inflates a new tile view, initializes it, and loads an icon for it.
      * @param tile The tile that holds the data to populate the new tile view.
      * @param parentView The parent of the new tile view.
@@ -314,12 +273,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 hold to the tile or modify them

[Michael van Ouwerkerk, 2017/02/22 12:11:59]

'don't keep a reference to the tile'

[dgn, 2017/02/22 17:22:19]

Done.

+        // as there is no guarantee that the same tile would be used to update the views.
+        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 +301,35 @@ private boolean loadWhitelistIcon(Tile tile, LargeIconCallback iconCallback) {
         return true;
     }
 
+    @Nullable
+    private Tile getTile(String url) {

[Michael van Ouwerkerk, 2017/02/22 12:11:59]

nit: please document under what circumstances this might return null.

[dgn, 2017/02/22 17:22:19]

Done.

+        for (Tile tile : mTiles) {
+            if (tile.getUrl().equals(url)) return tile;

[Michael van Ouwerkerk, 2017/02/22 12:11:59]

use TextUtils.equals?

[dgn, 2017/02/22 17:22:19]

the tile's url should never be null, so TextUtils.equals is not needed here. I
could assert that the url is not null when building tiles if you think we should
really make sure of that.

+        }
+        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) {
+            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 +339,65 @@ 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);
+
+            mObserver.onTileIconChanged(tile);
             if (mTrackLoadTask) mObserver.onLoadTaskCompleted();
         }
     }
+
+    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);
+        }
+    }
 }