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 { |