📰 Do not refresh Tiles while visible

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 672f2d35e673e132780153281a41e11706c7062e..97358343430531bad0ffadafc8a6a99cfc4c22f2 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
@@ -141,6 +141,18 @@
      * @see #getTile(String)
      */
     private Tile[] mTiles = new Tile[0];
+
+    /** Most recently received tile data that has not been displayed yet. */
+    @Nullable
+    private Tile[] mPendingTileData;
+
+    /**
+     * URL of the most recently removed tile. Used to identify when a tile removal is confirmed by
+     * the tile backend.
+     */
+    @Nullable
+    private String mPendingRemovalUrl;
+
     private boolean mHasReceivedData;
 
     /**
@@ -183,13 +195,9 @@ public TileGroup(Context context, SuggestionsUiDelegate uiDelegate,
     @Override
     public void onMostVisitedURLsAvailable(final String[] titles, final String[] urls,
             final String[] whitelistIconPaths, final int[] sources) {
-        boolean isInitialLoad = !mHasReceivedData;
-        mHasReceivedData = true;
-
-        Tile[] newTiles = new Tile[titles.length];
+        boolean removalCompleted = mPendingRemovalUrl != null;
         Set<String> addedUrls = new HashSet<>();
-        boolean countChanged = isInitialLoad || mTiles.length != titles.length;
-        boolean dataChanged = countChanged;
+        mPendingTileData = new Tile[titles.length];
         for (int i = 0; i < titles.length; i++) {
             assert urls[i] != null; // We assume everywhere that the url is not null.
 
@@ -200,22 +208,15 @@ public void onMostVisitedURLsAvailable(final String[] titles, final String[] url
                 continue;
             }
 
-            newTiles[i] = new Tile(titles[i], urls[i], whitelistIconPaths[i], i, sources[i]);
-            if (newTiles[i].importData(getTile(urls[i]))) dataChanged = true;
+            mPendingTileData[i] =
+                    new Tile(titles[i], urls[i], whitelistIconPaths[i], i, sources[i]);
             addedUrls.add(urls[i]);
-        }
-
-        if (!dataChanged) return;
 
-        mTiles = newTiles;
-
-        if (mOfflineModelObserver != null) {
-            mOfflineModelObserver.updateOfflinableSuggestionsAvailability();
+            if (urls[i].equals(mPendingRemovalUrl)) removalCompleted = false;
         }
 
-        if (countChanged) mObserver.onTileCountChanged();
-        if (isInitialLoad) mObserver.onLoadTaskCompleted();
-        mObserver.onTileDataChanged();
+        if (removalCompleted) mPendingRemovalUrl = null;
+        if (!mHasReceivedData || !mUiDelegate.isVisible() || removalCompleted) loadTiles();
     }
 
     @Override
@@ -278,6 +279,11 @@ public boolean hasReceivedData() {
         return mHasReceivedData;
     }
 
+    /** To be called when the view displaying the tile group becomes visible. */
+    public void onSwitchToForeground() {
+        if (mPendingTileData != null) loadTiles();
+    }
+
     /**
      * 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.
@@ -321,6 +327,33 @@ private boolean loadWhitelistIcon(Tile tile, LargeIconCallback iconCallback) {
         return true;
     }
 
+    /** Loads tile data from {@link #mPendingTileData} and clears it afterwards. */
+    private void loadTiles() {
+        assert mPendingTileData != null;
+
+        boolean isInitialLoad = !mHasReceivedData;
+        mHasReceivedData = true;
+
+        boolean countChanged = isInitialLoad || mTiles.length != mPendingTileData.length;
+        boolean dataChanged = countChanged;
+        for (Tile newTile : mPendingTileData) {
+            if (newTile.importData(getTile(newTile.getUrl()))) dataChanged = true;

[Bernhard Bauer, 2017/03/09 18:38:12]

Nit: it technically fits onto one line, but this is getting to the limit of
readability. I would consider breaking and using braces.

+        }
+
+        mTiles = mPendingTileData;
+        mPendingTileData = null;
+
+        if (!dataChanged) return;
+
+        if (mOfflineModelObserver != null) {
+            mOfflineModelObserver.updateOfflinableSuggestionsAvailability();
+        }
+
+        if (countChanged) mObserver.onTileCountChanged();
+        if (isInitialLoad) mObserver.onLoadTaskCompleted();
+        mObserver.onTileDataChanged();
+    }
+
     /** @return A tile matching the provided URL, or {@code null} if none is found. */
     @Nullable
     private Tile getTile(String url) {
@@ -400,6 +433,9 @@ public void removeItem() {
             Tile tile = getTile(mUrl);
             if (tile == null) return;
 
+            // Note: This does not track all the removals, but will track the most recent one. If
+            // that removal is committed, it's good enough for change detection.
+            mPendingRemovalUrl = mUrl;
             mTileGroupDelegate.removeMostVisitedItem(tile);
         }