Fix DownloadNotificationService foreground state

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser.download;
 
 import android.annotation.TargetApi;
 import android.app.DownloadManager;
 import android.app.Notification;
 import android.app.NotificationManager;
@@ skipping 198 matching lines @@
             return DownloadNotificationService.this;
         }
     }
 
     @Override
     public void onTaskRemoved(Intent rootIntent) {
         super.onTaskRemoved(rootIntent);
         // If we've lost all Activities, cancel the off the record downloads.
         if (ApplicationStatus.isEveryActivityDestroyed()) {
             cancelOffTheRecordDownloads();
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
         }
     }
 
     @Override
     public void onCreate() {
         mContext = ContextUtils.getApplicationContext();
         mNotificationManager = (NotificationManager) mContext.getSystemService(
                 Context.NOTIFICATION_SERVICE);
         mSharedPrefs = ContextUtils.getAppSharedPreferences();
         mNumAutoResumptionAttemptLeft = mSharedPrefs.getInt(KEY_AUTO_RESUMPTION_ATTEMPT_LEFT,
                 MAX_RESUMPTION_ATTEMPT_LEFT);
         mDownloadSharedPreferenceHelper = DownloadSharedPreferenceHelper.getInstance();
         mNextNotificationId = mSharedPrefs.getInt(
                 KEY_NEXT_DOWNLOAD_NOTIFICATION_ID, STARTING_NOTIFICATION_ID);
     }
 
     @Override
     public void onDestroy() {
         updateNotificationsForShutdown();
         rescheduleDownloads();
         super.onDestroy();
     }
 
     @Override
     public int onStartCommand(final Intent intent, int flags, int startId) {
         if (intent == null) {
             updateNotificationsForShutdown();
             handleDownloadOperation(
                     new Intent(DownloadNotificationService.ACTION_DOWNLOAD_RESUME_ALL));
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
         } else if (isDownloadOperationIntent(intent)) {
             handleDownloadOperation(intent);
             DownloadResumptionScheduler.getDownloadResumptionScheduler(mContext).cancelTask();
             // Limit the number of auto resumption attempts in case Chrome falls into a vicious
             // cycle.
             if (ACTION_DOWNLOAD_RESUME_ALL.equals(intent.getAction())) {
                 if (mNumAutoResumptionAttemptLeft > 0) {
                     mNumAutoResumptionAttemptLeft--;
                     updateResumptionAttemptLeft();
                 }
@@ skipping 23 matching lines @@
     public void removeObserver(Observer observer) {
         mObservers.removeObserver(observer);
     }
 
     /**
      * Called when browser is killed. Schedule a resumption task and pause all the download
      * notifications.
      */
     @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();
     }
 
     private void stopForegroundIfNecessary() {
         if (mDownloadsInProgress.size() == 0) stopForeground();
     }
 
     @VisibleForTesting
@@ skipping 79 matching lines @@
      * greater or equal to 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.
      */
     private void stopTrackingInProgressDownload(String downloadGuid) {
         mDownloadsInProgress.remove(downloadGuid);
         stopForegroundIfNecessary();
     }
 
     /**
+     * @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();
     }
 
     @Override
     public IBinder onBind(Intent intent) {
         return mBinder;
     }
 
     /**
      * Helper method to update the remaining number of background resumption attempts left.
@@ skipping 138 matching lines @@
     /**
      * Cancel a download notification.
      * @param notificationId Notification ID of the download
      * @param downloadGuid GUID of the download.
      */
     @VisibleForTesting
     void cancelNotification(int notificationId, String downloadGuid) {
         mNotificationManager.cancel(NOTIFICATION_NAMESPACE, notificationId);
         mDownloadSharedPreferenceHelper.removeSharedPreferenceEntry(downloadGuid);
         stopTrackingInProgressDownload(downloadGuid);
-        hideSummaryNotificationIfNecessary();
+        hideSummaryNotificationIfNecessary(notificationId);
     }
 
     /**
      * Called when a download is canceled.
      * @param downloadGuid GUID of the download.
      */
     @VisibleForTesting
     public void notifyDownloadCanceled(String downloadGuid) {
         DownloadSharedPreferenceEntry entry =
                 mDownloadSharedPreferenceHelper.getDownloadSharedPreferenceEntry(downloadGuid);
@@ skipping 232 matching lines @@
      * @param intent Intent with the download operation.
      */
     private void handleDownloadOperation(final Intent intent) {
         // TODO(qinmin): Figure out how to properly handle this case.
         boolean isOfflinePage =
                 IntentUtils.safeGetBooleanExtra(intent, EXTRA_IS_OFFLINE_PAGE, false);
         final DownloadSharedPreferenceEntry entry = getDownloadEntryFromIntent(intent);
         if (entry == null
                 && !(isOfflinePage && TextUtils.equals(intent.getAction(), ACTION_DOWNLOAD_OPEN))) {
             handleDownloadOperationForMissingNotification(intent);
-            hideSummaryNotificationIfNecessary();
+            hideSummaryNotificationIfNecessary(null);
             return;
         }
 
         if (ACTION_DOWNLOAD_PAUSE.equals(intent.getAction())) {
             // If browser process already goes away, the download should have already paused. Do
             // 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())) {
             // If user manually resumes a download, update the network type if it
             // is not metered previously.
             boolean canDownloadWhileMetered = entry.canDownloadWhileMetered
                     || DownloadManagerService.isActiveNetworkMetered(mContext);
             // Update the SharedPreference entry.
             mDownloadSharedPreferenceHelper.addOrReplaceSharedPreferenceEntry(
                     new DownloadSharedPreferenceEntry(entry.notificationId, entry.isOffTheRecord,
                             canDownloadWhileMetered, entry.downloadGuid, entry.fileName,
                             entry.itemType, true));
         } 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?
         }
 
         BrowserParts parts = new EmptyBrowserParts() {
             @Override
             public boolean shouldStartGpuProcess() {
                 return false;
             }
@@ skipping 31 matching lines @@
                         resumeAllPendingDownloads();
                 } else if (ACTION_DOWNLOAD_OPEN.equals(intent.getAction())) {
                         OfflinePageDownloadBridge.openDownloadedPage(
                                 IntentUtils.safeGetStringExtra(intent, EXTRA_DOWNLOAD_GUID));
                 } else {
                         Log.e(TAG, "Unrecognized intent action.", intent);
                 }
                 if (!ACTION_DOWNLOAD_OPEN.equals(intent.getAction())) {
                     downloadServiceDelegate.destroyServiceDelegate();
                 }
-                hideSummaryNotificationIfNecessary();
+
+                hideSummaryNotificationIfNecessary(ACTION_DOWNLOAD_CANCEL.equals(intent.getAction())
+                                ? entry.notificationId
+                                : null);
             }
         };
         try {
             ChromeBrowserInitializer.getInstance(mContext).handlePreNativeStartup(parts);
             ChromeBrowserInitializer.getInstance(mContext).handlePostNativeStartup(true, parts);
         } catch (ProcessInitException e) {
             Log.e(TAG, "Unable to load native library.", e);
             ChromeApplication.reportStartupErrorAndExit(e);
         }
     }
@@ skipping 169 matching lines @@
             return context.getString(R.string.remaining_duration_minutes, minutes);
         } else if (minutes > 0) {
             return context.getString(R.string.remaining_duration_one_minute);
         } else if (seconds == 1) {
             return context.getString(R.string.remaining_duration_one_second);
         } else {
             return context.getString(R.string.remaining_duration_seconds, seconds);
         }
     }
 }