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 a51f90892d9d559d49c2e63dd03c923010ec340a..1118aac1d3a75f17569cf4a22523517802c3dd1e 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
@@ -214,10 +214,11 @@ public class DownloadNotificationService extends Service {
     @Override
     public void onTaskRemoved(Intent rootIntent) {
         super.onTaskRemoved(rootIntent);
-        // If we've lost all Activities, cancel the off the record downloads.
+        // If we've lost all Activities, cancel the off the record downloads and validate that we
+        // should still be showing any download notifications at all.
         if (ApplicationStatus.isEveryActivityDestroyed()) {
             cancelOffTheRecordDownloads();
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
         }
     }
 
@@ -244,10 +245,14 @@ public class DownloadNotificationService extends Service {
     @Override
     public int onStartCommand(final Intent intent, int flags, int startId) {
         if (intent == null) {
+            // Intent is only null during a process restart because of returning START_STICKY.  In
+            // this case cancel the off the record notifications and put the normal notifications
+            // into a pending state, then try to restart.  Finally validate that we are actually
+            // showing something.
             updateNotificationsForShutdown();
             handleDownloadOperation(
                     new Intent(DownloadNotificationService.ACTION_DOWNLOAD_RESUME_ALL));
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
         } else if (isDownloadOperationIntent(intent)) {
             handleDownloadOperation(intent);
             DownloadResumptionScheduler.getDownloadResumptionScheduler(mContext).cancelTask();
@@ -286,29 +291,24 @@ public class DownloadNotificationService extends Service {
     }
 
     /**
-     * Called when browser is killed. Schedule a resumption task and pause all the download
-     * notifications.
+     * On >= O Android releases, puts this service into a background state.
+     * @param killNotification Whether or not this call should kill the summary notification or not.
+     *                         Not killing it puts the service into the background, but leaves the
+     *                         download notifications visible.
      */
     @VisibleForTesting
-    void shutdownService() {
-        stopForeground();
-        for (Observer observer : mObservers) observer.onServiceShutdownRequested();
-        stopSelf();
-    }
-
-    private void stopForegroundIfNecessary() {
-        if (mDownloadsInProgress.size() == 0) stopForeground();
-    }
-
-    @VisibleForTesting
     @TargetApi(Build.VERSION_CODES.N)
-    void stopForeground() {
+    void stopForegroundInteral(boolean killNotification) {

[qinmin, 2017/03/05 06:09:29]

nit:s/Interal/Internal/

[David Trainor- moved to gerrit, 2017/03/06 18:56:08]

Done.

         if (!BuildInfo.isAtLeastO()) return;
-        stopForeground(STOP_FOREGROUND_DETACH);
+        stopForeground(killNotification ? STOP_FOREGROUND_REMOVE : STOP_FOREGROUND_DETACH);
     }
 
+    /**
+     * On >= O Android releases, puts this service into a foreground state, binding it to the
+     * {@link Notification} generated by {@link #getSummaryNotification(Context)}.
+     */
     @VisibleForTesting
-    void startForeground() {
+    void startForegroundInternal() {
         if (!BuildInfo.isAtLeastO()) return;
         Notification notification = getSummaryNotification(getApplicationContext());
         startForeground(NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY, notification);
@@ -369,24 +369,83 @@ public class DownloadNotificationService extends Service {
     }
 
     /**
-     * Track in-progress downloads here and, if on an Android version greater or equal to O, make
+     * Track in-progress downloads here and, if on an Android version >= O, make
      * this a foreground service.
      * @param downloadGuid The guid of the download that has been started and should be tracked.
      */
     private void startTrackingInProgressDownload(String downloadGuid) {
-        if (mDownloadsInProgress.size() == 0) startForeground();
+        if (mDownloadsInProgress.size() == 0) startForegroundInternal();
         if (!mDownloadsInProgress.contains(downloadGuid)) mDownloadsInProgress.add(downloadGuid);
     }
 
     /**
-     * Stop tracking the download represented by {@code downloadGuid}.  If on an Android version
-     * greater or equal to O, stop making this a foreground service.
+     * Stop tracking the download represented by {@code downloadGuid}.  If on an Android version >=
+     * O, stop making this a foreground service.
      * @param downloadGuid The guid of the download that has been paused or canceled and shouldn't
      *                     be tracked.
+     * @param allowStopForeground Whether or not this should check internal state and stop the
+     *                            foreground notification from showing.  This could be false if we
+     *                            plan on removing the notification in the near future.  We don't
+     *                            want to just detach here, because that will put us in a
+     *                            potentially bad state where we cannot dismiss the notification.
      */
-    private void stopTrackingInProgressDownload(String downloadGuid) {
+    private void stopTrackingInProgressDownload(String downloadGuid, boolean allowStopForeground) {
         mDownloadsInProgress.remove(downloadGuid);
-        stopForegroundIfNecessary();
+        if (allowStopForeground && mDownloadsInProgress.size() == 0) stopForegroundInteral(false);
+    }
+
+    /**
+     * Returns whether or not there are any download notifications showing that aren't the summary
+     * notification.
+     * @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;
+
+        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;
+    }
+
+    /**
+     * @return The summary {@link StatusBarNotification} if one is showing.
+     */
+    @VisibleForTesting
+    @TargetApi(Build.VERSION_CODES.M)
+    StatusBarNotification getSummaryNotification() {
+        if (!BuildInfo.isAtLeastO()) return null;
+
+        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;
+            if (isDownloadsGroup && isSummaryNotification) return notification;
+        }
+
+        return null;
+    }
+
+    /**
+     * Cancels the existing summary notification.  Moved to a helper method for test mocking.
+     */
+    @VisibleForTesting
+    void cancelSummaryNotification() {
+        mNotificationManager.cancel(NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY);
     }
 
     /**
@@ -395,27 +454,47 @@ public class DownloadNotificationService extends Service {
      * 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 Canceling 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;
-
-        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;
+        if (mDownloadsInProgress.size() > 0) return;
+
+        if (hasDownloadNotifications(notificationIdToIgnore)) return;
+
+        StatusBarNotification notification = getSummaryNotification();
+        if (notification != null) {
+            // We have a valid summary notification, but how we dismiss it depends on whether or not
+            // it is currently bound to this service via startForeground(...).
+            if ((notification.getNotification().flags & Notification.FLAG_FOREGROUND_SERVICE)
+                    != 0) {
+                // If we are a foreground service and we are hiding the notification, we have no
+                // other downloads notifications showing, so we need to remove the notification and
+                // unregister it from this service at the same time.
+                stopForegroundInteral(true);
+            } else {
+                // If we are not a foreground service, remove the notification via the
+                // NotificationManager.  The notification is not bound to this service, so any call
+                // to stopForeground() won't affect the notification.
+                cancelSummaryNotification();
             }
+        } else {
+            // If we don't have a valid summary, just guarantee that we aren't in the foreground for
+            // safety.
+            stopForegroundInteral(false);
         }
-        mNotificationManager.cancel(NotificationConstants.NOTIFICATION_ID_DOWNLOAD_SUMMARY);
 
-        // Added to shut down the service when everything is gone.
-        // TODO(dtrainor): Make sure this makes sense.
-        shutdownService();
+        // Notify all observers that we are requesting a chance to shut down.  This will let any
+        // observers unbind from us if necessary.
+        for (Observer observer : mObservers) observer.onServiceShutdownRequested();
+
+        // 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();
     }
 
     @Override
@@ -570,8 +649,12 @@ public class DownloadNotificationService extends Service {
     void cancelNotification(int notificationId, String downloadGuid) {
         mNotificationManager.cancel(NOTIFICATION_NAMESPACE, notificationId);
         mDownloadSharedPreferenceHelper.removeSharedPreferenceEntry(downloadGuid);
-        stopTrackingInProgressDownload(downloadGuid);
-        hideSummaryNotificationIfNecessary();
+
+        // Since we are about to go through the process of validating whether or not we can shut
+        // 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);
     }
 
     /**
@@ -607,7 +690,7 @@ public class DownloadNotificationService extends Service {
         if (isAutoResumable) {
             notifyDownloadPending(entry.downloadGuid, entry.fileName, entry.isOffTheRecord,
                     entry.canDownloadWhileMetered, entry.isOfflinePage());
-            stopTrackingInProgressDownload(entry.downloadGuid);
+            stopTrackingInProgressDownload(entry.downloadGuid, true);
             return;
         }
 
@@ -644,7 +727,7 @@ public class DownloadNotificationService extends Service {
                 new DownloadSharedPreferenceEntry(entry.notificationId, entry.isOffTheRecord,
                         entry.canDownloadWhileMetered, entry.downloadGuid, entry.fileName,
                         entry.itemType, isAutoResumable));
-        stopTrackingInProgressDownload(downloadGuid);
+        stopTrackingInProgressDownload(downloadGuid, true);
     }
 
     /**
@@ -687,7 +770,7 @@ public class DownloadNotificationService extends Service {
         }
         builder.setLargeIcon(mDownloadSuccessLargeIcon);
         updateNotification(notificationId, builder.build(), downloadGuid, isOfflinePage, null);
-        stopTrackingInProgressDownload(downloadGuid);
+        stopTrackingInProgressDownload(downloadGuid, true);
         return notificationId;
     }
 
@@ -713,7 +796,7 @@ public class DownloadNotificationService extends Service {
                 buildNotification(android.R.drawable.stat_sys_download_done, fileName,
                         mContext.getResources().getString(R.string.download_notification_failed));
         updateNotification(notificationId, builder.build(), downloadGuid, isOfflinePage, null);
-        stopTrackingInProgressDownload(downloadGuid);
+        stopTrackingInProgressDownload(downloadGuid, true);
     }
 
     /**
@@ -821,7 +904,7 @@ public class DownloadNotificationService extends Service {
         if (entry == null
                 && !(isOfflinePage && TextUtils.equals(intent.getAction(), ACTION_DOWNLOAD_OPEN))) {
             handleDownloadOperationForMissingNotification(intent);
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
             return;
         }
 
@@ -830,7 +913,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())) {
@@ -846,7 +929,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?
@@ -898,7 +981,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 {