deprecate notificationId for downloads

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 6650be754492f4e727a3611fb3330abf78a109ae..5b0bed4729bb4d507e85507eb646d6575fb15734 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
@@ -53,14 +53,18 @@ public class DownloadNotificationService extends Service {
     static final int INVALID_DOWNLOAD_PERCENTAGE = -1;
     @VisibleForTesting
     static final String PENDING_DOWNLOAD_NOTIFICATIONS = "PendingDownloadNotifications";
-    private static final String NOTIFICATION_NAMESPACE = "DownloadNotificationService";
+    static final String NOTIFICATION_NAMESPACE = "DownloadNotificationService";

[Ted C, 2016/05/20 00:02:13]

rename DEPRECATED?

[qinmin, 2016/05/23 23:03:14]

There is no need to deprecate it. See reasons below

     private static final String TAG = "DownloadNotification";
+    private static final String NEXT_DOWNLOAD_NOTIFICATION_ID = "NextDownloadNotificationId";

[Ted C, 2016/05/20 00:02:13]

Instead of Next, I would use V2

[qinmin, 2016/05/23 23:03:13]

Since I am keeping the namespace, V2 is probably not very suitable.

[Ted C, 2016/05/23 23:15:23]

got it...I was thinking it was a namespace...do'h

+    // Notification Id starting value, to avoid conflicts from IDs used in prior versions.
+    private static final int STARTING_NOTIFICATION_ID = 1000000;

[Ted C, 2016/05/20 00:02:13]

any reason we don't use a different notification namespace?  couldn't we just
clear all notifications in the previous namespace and then add new ones w/o
having to worry about the id collision possibilities?

[qinmin, 2016/05/23 23:03:14]

If we clear all existing notifications, we have to recreate these notifications
with new namespace so that user won't lose paused downloads.
However,recreate notification requires us to read the intent from the existing
notification. That requires API 18, and BIND_NOTIFICATION_LISTENER_SERVICE
permission, which is not desirable.

     private final IBinder mBinder = new LocalBinder();
     private final List<DownloadSharedPreferenceEntry> mDownloadSharedPreferenceEntries =
             new ArrayList<DownloadSharedPreferenceEntry>();
     private NotificationManager mNotificationManager;
     private SharedPreferences mSharedPrefs;
     private Context mContext;
+    private int mNextNotificationId;
 
     /**
      * Class for clients to access.
@@ -98,6 +102,9 @@ public class DownloadNotificationService extends Service {
             pauseAllDownloads();
             stopSelf();
         }
+        mNextNotificationId = mSharedPrefs.getInt(
+            NEXT_DOWNLOAD_NOTIFICATION_ID, STARTING_NOTIFICATION_ID);

[Ted C, 2016/05/20 00:02:13]

+4 indent

[qinmin, 2016/05/23 23:03:14]

Done.

+
     }
 
     @Override
@@ -118,7 +125,6 @@ public class DownloadNotificationService extends Service {
 
     /**
      * Add a in-progress download notification.
-     * @param notificationId Notification ID of the download.
      * @param downloadGuid GUID of the download.
      * @param fileName File name of the download.
      * @param percentage Percentage completed. Value should be between 0 to 100 if
@@ -128,8 +134,8 @@ public class DownloadNotificationService extends Service {
      * @param isResumable Whether the download can be resumed.
      * @param canDownloadWhileMetered Whether the download can happen in metered network.
      */
-    public void notifyDownloadProgress(int notificationId, String downloadGuid, String fileName,
-            int percentage, long timeRemainingInMillis, long startTime, boolean isResumable,
+    public void notifyDownloadProgress(String downloadGuid, String fileName, int percentage,
+            long timeRemainingInMillis, long startTime, boolean isResumable,
             boolean canDownloadWhileMetered) {
         boolean indeterminate = percentage == INVALID_DOWNLOAD_PERCENTAGE;
         NotificationCompat.Builder builder = buildNotification(
@@ -142,6 +148,7 @@ public class DownloadNotificationService extends Service {
             String duration = getDurationString(timeRemainingInMillis);
             builder.setContentText(duration).setContentInfo(percentText);
         }
+        int notificationId = getNotificationId(downloadGuid);
         addOrReplaceSharedPreferenceEntry(new DownloadSharedPreferenceEntry(
                 notificationId, isResumable, canDownloadWhileMetered, downloadGuid, fileName));
         if (startTime > 0) builder.setWhen(startTime);
@@ -169,15 +176,26 @@ public class DownloadNotificationService extends Service {
 
     /**
      * Cancel a download notification.
-     * @param notificationId Notification ID of the download.
+     * @notificationId Notification ID of the download
      * @param downloadGuid GUID of the download.
      */
-    public void cancelNotification(int notificationId, String downloadGuid) {
-        mNotificationManager.cancel(NOTIFICATION_NAMESPACE, notificationId);
+    @VisibleForTesting
+    void cancelNotification(int notificaitonId, String downloadGuid) {
+        mNotificationManager.cancel(NOTIFICATION_NAMESPACE, notificaitonId);
         removeSharedPreferenceEntry(downloadGuid);
     }
 
     /**
+     * Called when a download is canceled.
+     * @param downloadGuid GUID of the download.
+     */
+    public void notifyDownloadCanceled(String downloadGuid) {
+        DownloadSharedPreferenceEntry entry = getDownloadSharedPreferenceEntry(downloadGuid);
+        if (entry == null) return;
+        cancelNotification(entry.notificationId, downloadGuid);
+    }
+
+    /**
      * Change a download notification to paused state.
      * @param downloadGuid GUID of the download.
      * @param isAutoResumable whether download is can be resumed automatically.
@@ -210,13 +228,14 @@ public class DownloadNotificationService extends Service {
 
     /**
      * Add a download successful notification.
-     * @param notificationId Notification ID of the download.
      * @param downloadGuid GUID of the download.
      * @param fileName GUID of the download.
      * @param intent Intent to launch when clicking the notification.
+     * @return ID of the successful download notification. Used for removing the notification when
+     *         user click on the snackbar.
      */
-    public void notifyDownloadSuccessful(int notificationId, String downloadGuid, String fileName,
-            Intent intent) {
+    public int notifyDownloadSuccessful(String downloadGuid, String fileName, Intent intent) {
+        int notificationId = getNotificationId(downloadGuid);
         NotificationCompat.Builder builder = buildNotification(
                 android.R.drawable.stat_sys_download_done, fileName,
                 mContext.getResources().getString(R.string.download_notification_completed));
@@ -226,15 +245,16 @@ public class DownloadNotificationService extends Service {
         }
         updateNotification(notificationId, builder.build());
         removeSharedPreferenceEntry(downloadGuid);
+        return notificationId;
     }
 
     /**
      * Add a download failed notification.
-     * @param notificationId Notification ID of the download.
      * @param downloadGuid GUID of the download.
      * @param fileName GUID of the download.
      */
-    public void notifyDownloadFailed(int notificationId, String downloadGuid, String fileName) {
+    public void notifyDownloadFailed(String downloadGuid, String fileName) {
+        int notificationId = getNotificationId(downloadGuid);
         NotificationCompat.Builder builder = buildNotification(
                 android.R.drawable.stat_sys_download_done, fileName,
                 mContext.getResources().getString(R.string.download_notification_failed));
@@ -342,9 +362,8 @@ public class DownloadNotificationService extends Service {
                         break;
                     case ACTION_DOWNLOAD_RESUME:
                         assert item != null;
-                        notifyDownloadProgress(notificationId, guid, fileName,
-                                INVALID_DOWNLOAD_PERCENTAGE, 0, 0, true,
-                                canDownloadWhileMetered);
+                        notifyDownloadProgress(guid, fileName, INVALID_DOWNLOAD_PERCENTAGE, 0, 0,
+                                true, canDownloadWhileMetered);
                         service.resumeDownload(item, true);
                         break;
                     default:
@@ -451,8 +470,8 @@ public class DownloadNotificationService extends Service {
         for (int i = 0; i < mDownloadSharedPreferenceEntries.size(); ++i) {
             DownloadSharedPreferenceEntry entry = mDownloadSharedPreferenceEntries.get(i);
             if (!entry.canDownloadWhileMetered && isNetworkMetered) continue;
-            notifyDownloadProgress(entry.notificationId, entry.downloadGuid, entry.fileName,
-                    INVALID_DOWNLOAD_PERCENTAGE, 0, 0, true, entry.canDownloadWhileMetered);
+            notifyDownloadProgress(entry.downloadGuid, entry.fileName, INVALID_DOWNLOAD_PERCENTAGE,
+                    0, 0, true, entry.canDownloadWhileMetered);
             service.resumeDownload(entry.buildDownloadItem(), false);
         }
     }
@@ -500,4 +519,20 @@ public class DownloadNotificationService extends Service {
         DownloadManagerService.storeDownloadInfo(
                 mSharedPrefs, PENDING_DOWNLOAD_NOTIFICATIONS, entries);
     }
+
+    /**
+     * Return the notification ID for the given download GUID.
+     * @return notification ID to be used.
+     */
+    private int getNotificationId(String downloadGuid) {
+        DownloadSharedPreferenceEntry entry = getDownloadSharedPreferenceEntry(downloadGuid);
+        if (entry != null) return entry.notificationId;
+        int notificationId = mNextNotificationId;
+        mNextNotificationId = mNextNotificationId == Integer.MAX_VALUE
+                ? STARTING_NOTIFICATION_ID : mNextNotificationId + 1;
+        SharedPreferences.Editor editor = mSharedPrefs.edit();
+        editor.putInt(NEXT_DOWNLOAD_NOTIFICATION_ID, mNextNotificationId);
+        editor.apply();
+        return notificationId;
+    }
 }