Fix DownloadNotificationService foreground state

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 29ce08ba0c8c2bb34ef07da7873e916638cbc63e..c63f1e67b12d30efdd930d796190eb34e9817cee 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
@@ -216,7 +216,7 @@ public class DownloadNotificationService extends Service {
         // If we've lost all Activities, cancel the off the record downloads.
         if (ApplicationStatus.isEveryActivityDestroyed()) {
             cancelOffTheRecordDownloads();
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
         }
     }
 
@@ -246,7 +246,7 @@ public class DownloadNotificationService extends Service {
             updateNotificationsForShutdown();
             handleDownloadOperation(
                     new Intent(DownloadNotificationService.ACTION_DOWNLOAD_RESUME_ALL));
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
         } else if (isDownloadOperationIntent(intent)) {
             handleDownloadOperation(intent);
             DownloadResumptionScheduler.getDownloadResumptionScheduler(mContext).cancelTask();
@@ -290,6 +290,7 @@ public class DownloadNotificationService extends Service {
      */
     @VisibleForTesting
     void shutdownService() {
+        // TODO(dtrainor): Post a task to make SURE the summary notification is gone...

[David Trainor- moved to gerrit, 2017/03/03 20:11:22]

I'll look into fixing this before I land the patch.

[David Trainor- moved to gerrit, 2017/03/04 06:46:01]

I believe I fixed this without the posted task.

         stopForeground();
         for (Observer observer : mObservers) observer.onServiceShutdownRequested();
         stopSelf();
@@ -389,31 +390,55 @@ public class DownloadNotificationService extends Service {
     }
 
     /**
+     * @see #hideSummaryNotificationIfNecessary(Integer)
+     * @param notificationIdToIgnore If not {@code null}, the id of a notification to ignore and
+     *                               assume is closing or about to be closed.
+     * @return Whether or not there are valid download notifications currently visible.
+     */
+    @VisibleForTesting
+    @TargetApi(Build.VERSION_CODES.M)
+    boolean hasDownloadNotifications(Integer notificationIdToIgnore) {
+        if (!BuildInfo.isAtLeastO()) return false;

[qinmin, 2017/03/03 20:48:40]

nit: android version doesn't matter with this function, and you already have
version check in hideSummaryNotification.

[David Trainor- moved to gerrit, 2017/03/04 06:46:01]

I figured it's a good safety catch in case someone else calls it.

+
+        StatusBarNotification[] notifications = mNotificationManager.getActiveNotifications();
+        for (StatusBarNotification notification : notifications) {
+            boolean isDownloadsGroup = TextUtils.equals(notification.getNotification().getGroup(),
+                    NotificationConstants.GROUP_DOWNLOADS);
+            boolean isSummaryNotification =
+                    notification.getId() == NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY;
+            boolean isIgnoredNotification = notificationIdToIgnore != null
+                    && notificationIdToIgnore == notification.getId();
+            if (isDownloadsGroup && !isSummaryNotification && !isIgnoredNotification) return true;
+        }
+
+        return false;
+    }
+
+    @VisibleForTesting
+    void cancelSummaryNotification() {
+        mNotificationManager.cancel(NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY);
+    }
+
+    /**
      * Check all current notifications and hide the summary notification if we have no downloads
      * notifications left.  On Android if the user swipes away the last download notification the
      * summary will be dismissed.  But if the last downloads notification is dismissed via
      * {@link NotificationManager#cancel(int)}, the summary will remain, so we need to check and
      * manually remove it ourselves.
+     * @param notificationIdToIgnore Cancelling a notification and querying for the current list of
+     *                               active notifications isn't synchronous.  Pass a notification id
+     *                               here if there is a notification that should be assumed gone.
+     *                               Or pass {@code null} if no notification fits that criteria.
      */
-    @TargetApi(Build.VERSION_CODES.M)
-    void hideSummaryNotificationIfNecessary() {
+    void hideSummaryNotificationIfNecessary(Integer notificationIdToIgnore) {
         if (!BuildInfo.isAtLeastO()) return;
+        if (mDownloadsInProgress.size() > 0) return;
 
         stopForegroundIfNecessary();
-
-        StatusBarNotification[] notifications = mNotificationManager.getActiveNotifications();
-        for (StatusBarNotification notification : notifications) {
-            if (TextUtils.equals(notification.getNotification().getGroup(),
-                        NotificationConstants.GROUP_DOWNLOADS)
-                    && notification.getId()
-                            != NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY) {
-                return;
-            }
-        }
-        mNotificationManager.cancel(NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY);
+        if (hasDownloadNotifications(notificationIdToIgnore)) return;
+        cancelSummaryNotification();
 
         // Added to shut down the service when everything is gone.
-        // TODO(dtrainor): Make sure this makes sense.
         shutdownService();
     }
 
@@ -572,7 +597,7 @@ public class DownloadNotificationService extends Service {
         mNotificationManager.cancel(NOTIFICATION_NAMESPACE, notificationId);
         mDownloadSharedPreferenceHelper.removeSharedPreferenceEntry(downloadGuid);
         stopTrackingInProgressDownload(downloadGuid);
-        hideSummaryNotificationIfNecessary();
+        hideSummaryNotificationIfNecessary(notificationId);
     }
 
     /**
@@ -825,7 +850,7 @@ public class DownloadNotificationService extends Service {
         if (entry == null
                 && !(isOfflinePage && TextUtils.equals(intent.getAction(), ACTION_DOWNLOAD_OPEN))) {
             handleDownloadOperationForMissingNotification(intent);
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
             return;
         }
 
@@ -834,7 +859,7 @@ public class DownloadNotificationService extends Service {
             // nothing in that case.
             if (!DownloadManagerService.hasDownloadManagerService()) {
                 notifyDownloadPaused(entry.downloadGuid, !entry.isOffTheRecord, false);
-                hideSummaryNotificationIfNecessary();
+                hideSummaryNotificationIfNecessary(null);
                 return;
             }
         } else if (ACTION_DOWNLOAD_RESUME.equals(intent.getAction())) {
@@ -850,7 +875,7 @@ public class DownloadNotificationService extends Service {
         } else if (ACTION_DOWNLOAD_RESUME_ALL.equals(intent.getAction())
                 && (mDownloadSharedPreferenceHelper.getEntries().isEmpty()
                         || DownloadManagerService.hasDownloadManagerService())) {
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
             return;
         } else if (ACTION_DOWNLOAD_OPEN.equals(intent.getAction())) {
             // TODO(fgorski): Do we even need to do anything special here, before we launch Chrome?
@@ -902,7 +927,10 @@ public class DownloadNotificationService extends Service {
                 if (!ACTION_DOWNLOAD_OPEN.equals(intent.getAction())) {
                     downloadServiceDelegate.destroyServiceDelegate();
                 }
-                hideSummaryNotificationIfNecessary();
+
+                hideSummaryNotificationIfNecessary(ACTION_DOWNLOAD_CANCEL.equals(intent.getAction())
+                                ? entry.notificationId
+                                : null);
             }
         };
         try {