Add support for pulling icons for OfflineItems

chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotificationBridgeUi.java

Index: chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotificationBridgeUi.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotificationBridgeUi.java b/chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotificationBridgeUi.java
index d60f9bfd916807bcfea72ba1ae893365edcebf63..7f6323ecac675ff806852db522d9b31649dcbb43 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotificationBridgeUi.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotificationBridgeUi.java
@@ -6,29 +6,51 @@ package org.chromium.chrome.browser.download.items;
 
 import org.chromium.chrome.browser.download.DownloadInfo;
 import org.chromium.chrome.browser.download.DownloadItem;
-import org.chromium.chrome.browser.download.DownloadManagerService;
 import org.chromium.chrome.browser.download.DownloadNotifier;
 import org.chromium.chrome.browser.download.DownloadServiceDelegate;
 import org.chromium.components.offline_items_collection.ContentId;
 import org.chromium.components.offline_items_collection.OfflineContentProvider;
 import org.chromium.components.offline_items_collection.OfflineItem;
 import org.chromium.components.offline_items_collection.OfflineItemState;
+import org.chromium.components.offline_items_collection.OfflineItemVisuals;
 
 import java.util.ArrayList;
+import java.util.HashMap;
 
 /**
  * A glue class that bridges the Profile-attached OfflineContentProvider with the
  * download notification code (SystemDownloadNotifier and DownloadServiceDelegate).
  */
 public class OfflineContentAggregatorNotificationBridgeUi
-        implements DownloadServiceDelegate, OfflineContentProvider.Observer {
+        implements DownloadServiceDelegate, OfflineContentProvider.Observer,
+                   OfflineContentProvider.VisualsCallback {
+    // TODO(dtrainor): Should this just be part of the OfflineContentProvider callback guarantee?
+    private static final OfflineItemVisuals sEmptyOfflineItemVisuals = new OfflineItemVisuals();
+
     private final OfflineContentProvider mProvider;
 
+    private final DownloadNotifier mUi;
+
+    /** Holds a list of {@link OfflineItem} updates that are waiting for visuals. */
+    private final HashMap<ContentId, OfflineItem> mOutstandingRequests = new HashMap<>();
+
+    /**
+     * Holds a list of {@link OfflineItemVisuals} for all {@link OfflineItem}s that are currently in
+     * progress.  Once an {@link OfflineItem} is no longer in progress it will be removed from this
+     * cache.
+     * TODO(dtrainor): Flush this list aggressively if we get onLowMemory/onTrimMemory.
+     * TODO(dtrainor): Add periodic clean up in case something goes wrong with the underlying
+     * downloads.
+     */
+    private final HashMap<ContentId, OfflineItemVisuals> mVisualsCache = new HashMap<>();
+
     /**
      * Creates a new OfflineContentAggregatorNotificationBridgeUi based on {@code provider}.
      */
-    public OfflineContentAggregatorNotificationBridgeUi(OfflineContentProvider provider) {
+    public OfflineContentAggregatorNotificationBridgeUi(
+            OfflineContentProvider provider, DownloadNotifier notifier) {
         mProvider = provider;
+        mUi = notifier;
 
         mProvider.addObserver(this);
     }
@@ -49,21 +71,35 @@ public class OfflineContentAggregatorNotificationBridgeUi
     public void onItemsAdded(ArrayList<OfflineItem> items) {
         for (int i = 0; i < items.size(); i++) {
             OfflineItem item = items.get(i);
-
-            // Only update the UI for new OfflineItems that are in progress or pending.
-            if (item.state == OfflineItemState.IN_PROGRESS
-                    || item.state == OfflineItemState.PENDING) {
-                visuallyUpdateOfflineItem(item);
-            }
+            if (shouldPushNewItemToUi(item)) getVisualsAndUpdateItem(item);
         }
     }
 
     @Override
-    public void onItemRemoved(ContentId id) {}
+    public void onItemRemoved(ContentId id) {
+        mOutstandingRequests.remove(id);
+        mVisualsCache.remove(id);
+        mUi.notifyDownloadCanceled(id);
+    }
 
     @Override
     public void onItemUpdated(OfflineItem item) {
-        visuallyUpdateOfflineItem(item);
+        // Assume that any item sending updates should have them reflected in the UI.
+        getVisualsAndUpdateItem(item);
+    }
+
+    // OfflineContentProvider.VisualsCallback implementation.
+    @Override
+    public void onVisualsAvailable(ContentId id, OfflineItemVisuals visuals) {
+        OfflineItem item = mOutstandingRequests.remove(id);
+        if (item == null) return;
+
+        if (visuals == null) visuals = sEmptyOfflineItemVisuals;
+
+        // Only cache the visuals if the update we are about to push is interesting and we think we
+        // will need them in the future.
+        if (shouldCacheVisuals(item)) mVisualsCache.put(id, visuals);
+        pushItemToUi(item, visuals);
     }
 
     // DownloadServiceDelegate implementation.
@@ -85,34 +121,98 @@ public class OfflineContentAggregatorNotificationBridgeUi
     @Override
     public void destroyServiceDelegate() {}
 
-    /**
-     * Calls into the proper {@link DownloadNotifier} by converting an {@link OfflineItem} to a
-     * {@link DownloadInfo}.
-     * @param item The {@link OfflineItem} that needs a UI refresh.
-     */
-    private void visuallyUpdateOfflineItem(OfflineItem item) {
-        DownloadInfo info = DownloadInfo.fromOfflineItem(item);
-        DownloadNotifier notifier =
-                DownloadManagerService.getDownloadManagerService().getDownloadNotifier();
+    private void getVisualsAndUpdateItem(OfflineItem item) {
+        OfflineItemVisuals visuals = mVisualsCache.get(item.id);
+
+        if (!needsVisualsForUi(item)) {

[nyquist, 2017/04/13 05:08:24]

If you flip this else around you don't need the ! which might make it easier to
read. And if you do that, maybe you could do:
if (needsVisualsForUi(item)) {
  if (!mVisualsCache.containsKey(item.id)) {
    ...
instead of retrieving it at first? And move the getting-of-visual-part to just
before you put the item to the UI?

I don't feel strongly about this style though, particularly since you get that
pesky ! for the containsKey() part.

[David Trainor- moved to gerrit, 2017/04/13 07:20:23]

Done.

+            // We don't need the visuals to show this item at this point.  Cancel any requests.
+            mOutstandingRequests.remove(item.id);
+            mVisualsCache.remove(item.id);
+        } else {
+            if (visuals == null) {
+                // We don't have any visuals for this item yet.  Stash the current OfflineItem and,
+                // if we haven't already, queue up a request for the visuals.
+                // TODO(dtrainor): Check if this delay is too much.  If so, just send the update
+                // through.
+                boolean requestVisuals = !mOutstandingRequests.containsKey(item.id);
+                mOutstandingRequests.put(item.id, item);
+                if (requestVisuals) mProvider.getVisualsForItem(item.id, this);
+                return;
+            }
+        }
+
+        pushItemToUi(item, visuals);
+        // We will no longer be needing the visuals for this item after this notification.
+        if (!shouldCacheVisuals(item)) mVisualsCache.remove(item.id);
+    }
+
+    private void pushItemToUi(OfflineItem item, OfflineItemVisuals visuals) {
+        DownloadInfo info = DownloadInfo.fromOfflineItem(item, visuals);
         switch (item.state) {
             case OfflineItemState.IN_PROGRESS:
-                notifier.notifyDownloadProgress(info, item.creationTimeMs, item.allowMetered);
+                mUi.notifyDownloadProgress(info, item.creationTimeMs, item.allowMetered);
                 break;
             case OfflineItemState.COMPLETE:
-                notifier.notifyDownloadSuccessful(info, -1L, false, false);
+                mUi.notifyDownloadSuccessful(info, -1L, false, false);
                 break;
             case OfflineItemState.CANCELLED:
-                notifier.notifyDownloadCanceled(item.id);
+                mUi.notifyDownloadCanceled(item.id);
                 break;
             case OfflineItemState.INTERRUPTED:
                 // TODO(dtrainor): Push the correct value for auto resume.
-                notifier.notifyDownloadInterrupted(info, true);
+                mUi.notifyDownloadInterrupted(info, true);
                 break;
             case OfflineItemState.PAUSED:
-                notifier.notifyDownloadPaused(info);
+                mUi.notifyDownloadPaused(info);
+                break;
+            case OfflineItemState.FAILED:
+                mUi.notifyDownloadFailed(info);
                 break;
+        }
+    }
+
+    private boolean needsVisualsForUi(OfflineItem item) {
+        switch (item.state) {
+            case OfflineItemState.IN_PROGRESS:

[nyquist, 2017/04/13 05:08:24]

Often I want to ask the author to add . // Intentional fall through. at the end
of each line. But I think it's pretty clear it's intentional here.

[David Trainor- moved to gerrit, 2017/04/13 07:20:23]

Yeah I wasn't sure what looked nicer, but this looked cleaner than a long if
block.  I agree the fallthrough would probably be overkill in this case.

+            case OfflineItemState.PENDING:
+            case OfflineItemState.COMPLETE:
+            case OfflineItemState.INTERRUPTED:
+            case OfflineItemState.FAILED:
+            case OfflineItemState.PAUSED:
+                return true;
+            case OfflineItemState.CANCELLED:
+            default:
+                return false;
+        }
+    }
+
+    private boolean shouldPushNewItemToUi(OfflineItem item) {
+        switch (item.state) {
+            case OfflineItemState.IN_PROGRESS:
+                return true;
+            case OfflineItemState.PENDING:
+            case OfflineItemState.COMPLETE:
+            case OfflineItemState.INTERRUPTED:
+            case OfflineItemState.FAILED:
+            case OfflineItemState.PAUSED:
+            case OfflineItemState.CANCELLED:
+            default:
+                return false;
+        }
+    }
+
+    private boolean shouldCacheVisuals(OfflineItem item) {
+        switch (item.state) {
+            case OfflineItemState.IN_PROGRESS:
+            case OfflineItemState.PENDING:
+            case OfflineItemState.INTERRUPTED:
+            case OfflineItemState.PAUSED:
+                return true;
+            case OfflineItemState.FAILED:
+            case OfflineItemState.COMPLETE:
+            case OfflineItemState.CANCELLED:
             default:
-                assert false;
+                return false;
         }
     }
 }