Update the download summary icon

chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java

Index: chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java b/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
index 1118aac1d3a75f17569cf4a22523517802c3dd1e..d2d00103bd68d927feb26003bd1ff1a6ede9dc18 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
@@ -23,9 +23,11 @@ import android.graphics.Rect;
 import android.graphics.drawable.shapes.OvalShape;
 import android.os.Binder;
 import android.os.Build;
+import android.os.Bundle;
 import android.os.IBinder;
 import android.service.notification.StatusBarNotification;
 import android.text.TextUtils;
+import android.util.Pair;
 
 import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.ApplicationStatus;
@@ -59,7 +61,7 @@ import java.util.concurrent.TimeUnit;
  *
  * On O and above, this service will receive {@link Service#startForeground(int, Notification)}
  * calls when containing active downloads.  The foreground notification will be the summary
- * notification generated by {@link DownloadNotificationService#getSummaryNotification(Context)}.
+ * notification generated by {@link DownloadNotificationService#buildSummaryNotification(Context)}.
  * The service will receive a {@link Service#stopForeground(boolean)} call when all active downloads
  * are paused.  The summary notification will be hidden when there are no other notifications in the
  * {@link NotificationConstants#GROUP_DOWNLOADS} group.  This gets checked after every notification
@@ -85,6 +87,8 @@ public class DownloadNotificationService extends Service {
             "org.chromium.chrome.browser.download.DOWNLOAD_RESUME_ALL";
     public static final String ACTION_DOWNLOAD_OPEN =
             "org.chromium.chrome.browser.download.DOWNLOAD_OPEN";
+    public static final String ACTION_UPDATE_SUMMARY_ICON =
+            "org.chromium.chrome.browser.download.UPDATE_SUMMARY_ICON";
 
     static final String NOTIFICATION_NAMESPACE = "DownloadNotificationService";
     private static final String TAG = "DownloadNotification";
@@ -93,6 +97,8 @@ public class DownloadNotificationService extends Service {
 
     /** Notification Id starting value, to avoid conflicts from IDs used in prior versions. */
 
+    private static final String EXTRA_NOTIFICATION_BUNDLE_ICON_ID =
+            "Chrome.NotificationBundleIconIdExtra";
     private static final int STARTING_NOTIFICATION_ID = 1000000;
     private static final int MAX_RESUMPTION_ATTEMPT_LEFT = 5;
     @VisibleForTesting static final long SECONDS_PER_MINUTE = TimeUnit.MINUTES.toSeconds(1);
@@ -145,7 +151,7 @@ public class DownloadNotificationService extends Service {
         intent.setComponent(new ComponentName(context, DownloadNotificationService.class));
 
         if (BuildInfo.isAtLeastO()) {
-            Notification notification = getSummaryNotification(context);
+            Notification notification = buildSummaryNotification(context);
 
             AppHooks.get().startServiceWithNotification(
                     intent, NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY, notification);
@@ -155,16 +161,101 @@ public class DownloadNotificationService extends Service {
     }
 
     /**
-     * Builds a summary notification that represents downloads.  This is the notification passed to
-     * {@link #startForeground(int, Notification)}, which keeps this service in the foreground.
-     * @param context The context used to build the notification and pull specific resources.
-     * @return The {@link Notification} to show for the summary.  Meant to be used by
-     *         {@link NotificationManager#notify(int, Notification)}.
+     * Calculates the suggested icon for the summary notification based on the other notifications
+     * currently showing.
+     * @param context A context to use to query Android-specific information (NotificationManager).
+     * @param removedNotificationId The id of a notification that is currently closing and should be
+     *                              ignored.  -1 if no notifications are being closed.
+     * @param addedNotification     A {@link Pair} of <id, Notification> of a notification that is
+     *                              currently being added and should be used in addition to or in
+     *                              place of the existing icons.
+     * @return                      A {@link Pair} that represents both whether or not the new icon
+     *                              is different from the old one and the icon id itself.
+     */
+    @TargetApi(Build.VERSION_CODES.M)
+    private static Pair<Boolean, Integer> getSummaryIcon(Context context, int removedNotificationId,
+            Pair<Integer, Notification> addedNotification) {
+        if (!BuildInfo.isAtLeastO()) return new Pair<Boolean, Integer>(false, -1);
+        boolean progress = false;
+        boolean paused = false;
+        boolean pending = false;
+        boolean completed = false;
+        boolean failed = false;
+
+        final int progressIcon = android.R.drawable.stat_sys_download;
+        final int pausedIcon = R.drawable.ic_download_pause;
+        final int pendingIcon = R.drawable.ic_download_pending;
+        final int completedIcon = R.drawable.offline_pin;
+        final int failedIcon = android.R.drawable.stat_sys_download_done;
+
+        NotificationManager manager =
+                (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE);
+        StatusBarNotification[] notifications = manager.getActiveNotifications();
+
+        int oldIcon = -1;
+        for (StatusBarNotification notification : notifications) {
+            boolean isDownloadsGroup = TextUtils.equals(notification.getNotification().getGroup(),
+                    NotificationConstants.GROUP_DOWNLOADS);
+            if (!isDownloadsGroup) continue;
+            if (notification.getId() == removedNotificationId) continue;
+
+            boolean isSummaryNotification =
+                    notification.getId() == NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY;
+
+            if (addedNotification != null && addedNotification.first == notification.getId())

[qinmin, 2017/03/05 06:48:26]

if addedNotification is not null, this for loop skips the notification that is
the addedNotification.
However, after the for loop, there is an if statement to consider the icon for
this addedNotification.

if (addedNotification != null) {
  ......
 }

So why cannot we remove the latter if statement?

[David Trainor- moved to gerrit, 2017/03/06 16:29:54]

The added notification might not be shown yet, so it might not be in the list of
StatusBarNotifications that we query.

[qinmin, 2017/03/06 21:43:43]

i see, in that case, can we just initialize those booleans to those values from
the addedNotification if it is non null?
It looks ugly to have a lot of redudant code here.

+                continue;
+
+            int icon =
+                    notification.getNotification().extras.getInt(EXTRA_NOTIFICATION_BUNDLE_ICON_ID);
+            if (isSummaryNotification) {
+                oldIcon = icon;
+                continue;
+            }
+
+            progress |= icon == progressIcon;
+            paused |= icon == pausedIcon;
+            pending |= icon == pendingIcon;
+            completed |= icon == completedIcon;
+            failed |= icon == failedIcon;
+        }
+
+        if (addedNotification != null) {
+            int icon = addedNotification.second.extras.getInt(EXTRA_NOTIFICATION_BUNDLE_ICON_ID);
+
+            progress |= icon == progressIcon;
+            paused |= icon == pausedIcon;
+            pending |= icon == pendingIcon;
+            completed |= icon == completedIcon;
+            failed |= icon == failedIcon;
+        }
+
+        int newIcon = android.R.drawable.stat_sys_download_done;
+        if (progress) {
+            newIcon = android.R.drawable.stat_sys_download;
+        } else if (paused) {
+            newIcon = R.drawable.ic_download_pause;
+        } else if (pending) {
+            newIcon = R.drawable.ic_download_pending;
+        } else if (completed) {
+            newIcon = R.drawable.offline_pin;
+        } else if (failed) {
+            newIcon = android.R.drawable.stat_sys_download_done;
+        }
+        return new Pair<Boolean, Integer>(newIcon != oldIcon, newIcon);
+    }
+
+    /**
+     * Builds a summary notification that represents all downloads.
+     * {@see #buildSummaryNotification(Context)}.
+     * @param context A context used to query Android strings and resources.
+     * @param iconId  The id of an icon to use for the notification.
+     * @return        a {@link Notification} that represents the summary icon for all downloads.
      */
-    private static Notification getSummaryNotification(Context context) {
+    private static Notification buildSummaryNotificationWithIcon(Context context, int iconId) {
         String title =
                 context.getResources().getString(R.string.download_notification_summary_title,
                         DownloadSharedPreferenceHelper.getInstance().getEntries().size());
+
         ChromeNotificationBuilder builder =
                 AppHooks.get()
                         .createChromeNotificationBuilder(true /* preferCompat */,
@@ -173,10 +264,13 @@ public class DownloadNotificationService extends Service {
                                 NotificationConstants.CATEGORY_GROUP_ID_GENERAL,
                                 context.getString(R.string.notification_category_group_general))
                         .setContentTitle(title)
-                        .setSmallIcon(android.R.drawable.stat_sys_download_done)
+                        .setSmallIcon(iconId)
                         .setLocalOnly(true)
                         .setGroup(NotificationConstants.GROUP_DOWNLOADS)
                         .setGroupSummary(true);
+        Bundle extras = new Bundle();
+        extras.putInt(EXTRA_NOTIFICATION_BUNDLE_ICON_ID, iconId);
+        builder.addExtras(extras);
 
         // This notification should not actually be shown.  But if it is, set the click intent to
         // open downloads home.
@@ -190,6 +284,18 @@ public class DownloadNotificationService extends Service {
     }
 
     /**
+     * Builds a summary notification that represents downloads.  This is the notification passed to
+     * {@link #startForeground(int, Notification)}, which keeps this service in the foreground.
+     * @param context The context used to build the notification and pull specific resources.
+     * @return The {@link Notification} to show for the summary.  Meant to be used by
+     *         {@link NotificationManager#notify(int, Notification)}.
+     */
+    private static Notification buildSummaryNotification(Context context) {
+        Pair<Boolean, Integer> icon = getSummaryIcon(context, -1, null);
+        return buildSummaryNotificationWithIcon(context, icon.second);
+    }
+
+    /**
      * @return Whether or not there are any current resumable downloads being tracked.  These
      *         tracked downloads may not currently be showing notifications.
      */
@@ -268,6 +374,9 @@ public class DownloadNotificationService extends Service {
                 mNumAutoResumptionAttemptLeft = MAX_RESUMPTION_ATTEMPT_LEFT;
                 clearResumptionAttemptLeft();
             }
+        } else if (ACTION_UPDATE_SUMMARY_ICON.equals(intent.getAction())) {
+            updateSummaryIcon(-1, null);
+            hideSummaryNotificationIfNecessary(null);
         }
         // This should restart the service after Chrome gets killed. However, this
         // doesn't work on Android 4.4.2.
@@ -305,12 +414,12 @@ public class DownloadNotificationService extends Service {
 
     /**
      * On >= O Android releases, puts this service into a foreground state, binding it to the
-     * {@link Notification} generated by {@link #getSummaryNotification(Context)}.
+     * {@link Notification} generated by {@link #buildSummaryNotification(Context)}.
      */
     @VisibleForTesting
     void startForegroundInternal() {
         if (!BuildInfo.isAtLeastO()) return;
-        Notification notification = getSummaryNotification(getApplicationContext());
+        Notification notification = buildSummaryNotification(getApplicationContext());
         startForeground(NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY, notification);
     }
 
@@ -441,6 +550,32 @@ public class DownloadNotificationService extends Service {
     }
 
     /**
+     * Updates the notification summary with a new icon, if necessary.
+     * @param removedNotificationId The id of a notification that is currently closing and should be
+     *                              ignored.  -1 if no notifications are being closed.
+     * @param addedNotification     A {@link Pair} of <id, Notification> of a notification that is
+     *                              currently being added and should be used in addition to or in
+     *                              place of the existing icons.
+     */
+    @VisibleForTesting
+    void updateSummaryIcon(
+            int removedNotificationId, Pair<Integer, Notification> addedNotification) {
+        if (!BuildInfo.isAtLeastO()) return;
+
+        Pair<Boolean, Integer> icon =
+                getSummaryIcon(mContext, removedNotificationId, addedNotification);
+
+        // Avoid rebuilding the summary notification if the icon hasn't changed or we have (or are
+        // about to have) no active downloads.
+        if (!icon.first || !hasDownloadNotifications(removedNotificationId)) {
+            return;
+        }
+
+        mNotificationManager.notify(NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY,
+                buildSummaryNotificationWithIcon(mContext, icon.second));
+    }
+
+    /**
      * Cancels the existing summary notification.  Moved to a helper method for test mocking.
      */
     @VisibleForTesting
@@ -460,11 +595,11 @@ public class DownloadNotificationService extends Service {
      *                               Or pass {@code null} if no notification fits that criteria.
      */
     @TargetApi(Build.VERSION_CODES.M)
-    void hideSummaryNotificationIfNecessary(Integer notificationIdToIgnore) {
-        if (!BuildInfo.isAtLeastO()) return;
-        if (mDownloadsInProgress.size() > 0) return;
+    boolean hideSummaryNotificationIfNecessary(Integer notificationIdToIgnore) {
+        if (!BuildInfo.isAtLeastO()) return false;
+        if (mDownloadsInProgress.size() > 0) return false;
 
-        if (hasDownloadNotifications(notificationIdToIgnore)) return;
+        if (hasDownloadNotifications(notificationIdToIgnore)) return false;
 
         StatusBarNotification notification = getSummaryNotification();
         if (notification != null) {
@@ -495,6 +630,7 @@ public class DownloadNotificationService extends Service {
         // Stop the service which should start the destruction process.  At this point we should be
         // (1) a background service and (2) unbound from any clients.
         stopSelf();
+        return true;
     }
 
     @Override
@@ -654,7 +790,9 @@ public class DownloadNotificationService extends Service {
         // down, don't stop foreground if we have no download notifications left to show.  Hiding
         // the summary will take care of that for us.
         stopTrackingInProgressDownload(downloadGuid, hasDownloadNotifications(notificationId));
-        hideSummaryNotificationIfNecessary(notificationId);
+        if (!hideSummaryNotificationIfNecessary(notificationId)) {
+            updateSummaryIcon(notificationId, null);
+        }
     }
 
     /**
@@ -769,6 +907,7 @@ public class DownloadNotificationService extends Service {
             mDownloadSuccessLargeIcon = getLargeNotificationIcon(bitmap);
         }
         builder.setLargeIcon(mDownloadSuccessLargeIcon);
+        builder.setDeleteIntent(buildIconUpdateIntent(notificationId));
         updateNotification(notificationId, builder.build(), downloadGuid, isOfflinePage, null);
         stopTrackingInProgressDownload(downloadGuid, true);
         return notificationId;
@@ -795,6 +934,7 @@ public class DownloadNotificationService extends Service {
         ChromeNotificationBuilder builder =
                 buildNotification(android.R.drawable.stat_sys_download_done, fileName,
                         mContext.getResources().getString(R.string.download_notification_failed));
+        builder.setDeleteIntent(buildIconUpdateIntent(notificationId));
         updateNotification(notificationId, builder.build(), downloadGuid, isOfflinePage, null);
         stopTrackingInProgressDownload(downloadGuid, true);
     }
@@ -810,6 +950,21 @@ public class DownloadNotificationService extends Service {
     }
 
     /**
+     * Helper method to build a {@link PendingIntent} that will wrap an {@link Intent} that will
+     * update the summary notification icon.
+     * @param notificationId ID of the notification.
+     */
+    // ---- BEFORE LANDING NOTE ----
+    // TODO(dtrainor): This is not firing for the complete notification.  Test or remove this if we
+    // don't need it.
+    // ---- BEFORE LANDING NOTE ----
+    private PendingIntent buildIconUpdateIntent(int notificationId) {
+        Intent intent = new Intent(mContext, DownloadBroadcastReceiver.class);
+        intent.setAction(ACTION_UPDATE_SUMMARY_ICON);
+        return buildPendingIntent(intent, notificationId);
+    }
+
+    /**
      * Helper method to build an download action Intent from the provided information.
      * @param context {@link Context} to pull resources from.
      * @param action Download action to perform.
@@ -838,6 +993,9 @@ public class DownloadNotificationService extends Service {
      */
     private ChromeNotificationBuilder buildNotification(
             int iconId, String title, String contentText) {
+        Bundle extras = new Bundle();
+        extras.putInt(EXTRA_NOTIFICATION_BUNDLE_ICON_ID, iconId);
+
         ChromeNotificationBuilder builder =
                 AppHooks.get()
                         .createChromeNotificationBuilder(true /* preferCompat */,
@@ -851,7 +1009,8 @@ public class DownloadNotificationService extends Service {
                         .setLocalOnly(true)
                         .setAutoCancel(true)
                         .setContentText(contentText)
-                        .setGroup(NotificationConstants.GROUP_DOWNLOADS);
+                        .setGroup(NotificationConstants.GROUP_DOWNLOADS)
+                        .addExtras(extras);
         return builder;
     }
 
@@ -1061,6 +1220,7 @@ public class DownloadNotificationService extends Service {
         } else {
             mDownloadSharedPreferenceHelper.removeSharedPreferenceEntry(downloadGuid);
         }
+        updateSummaryIcon(-1, new Pair<Integer, Notification>(id, notification));
     }
 
     private void trackNotificationUma(boolean isOfflinePage, String downloadGuid) {