Fix DownloadNotificationService foreground state

Fix DownloadNotificationService foreground state

There are some cases that keep the summary notification visible.  This
CL tries to address them (but does not catch *all* cases).  The
following scenarios are now handled:

- NotificationManager#notify() posts a task, which means querying
NotificationManager#getActiveNotifications() might not take into account
the addition.  Only hide the summary if there are no currently running
notifications.
- NotificationManager#cancel() also posts at ask which causes the same
issues.  Ignore the notification id that was passed to cancel when
checking the list state.

There are still some cases where NotificationManager#cancel() on the
summary doesn't dismiss it.  This needs to be investigated in a follow
up (or a failsafe added and the code refactored).

BUG=680819
Review-Url: https://codereview.chromium.org/2726423002
Cr-Commit-Position: refs/heads/master@{#454941}
Committed: https://chromium.googlesource.com/chromium/src/+/1f6307adb1b1ff13661af0432d735866f2531831

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

dtrainor@chromium.org changed reviewers:
+ qinmin@chromium.org

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

ptal :'(

https://codereview.chromium.org/2726423002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2726423002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:293:
// TODO(dtrainor): Post a task to make SURE the summary notification is gone...
I'll look into fixing this before I land the patch.

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

lgtm % nit, and please fix the issue in shutDownService

https://codereview.chromium.org/2726423002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2726423002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:401:
if (!BuildInfo.isAtLeastO()) return false;
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 05:52:45]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-04 05:52:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 06:31:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-04 06:31:37]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2726423002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2726423002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:293:
// TODO(dtrainor): Post a task to make SURE the summary notification is gone...
On 2017/03/03 20:11:22, David Trainor wrote:
> I'll look into fixing this before I land the patch.

I believe I fixed this without the posted task.

https://codereview.chromium.org/2726423002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:401:
if (!BuildInfo.isAtLeastO()) return false;
On 2017/03/03 20:48:40, qinmin wrote:
> nit: android version doesn't matter with this function, and you already have
> version check in hideSummaryNotification.

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

[David Trainor- moved to gerrit, 2017-03-04 21:44:05]

min can you give this another once-over?  i update a lot of the logic and want a
final review before landing.  thanks!

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

lgtm % nit

https://codereview.chromium.org/2726423002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2726423002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:301:
void stopForegroundInteral(boolean killNotification) {
nit:s/Interal/Internal/

[David Trainor- moved to gerrit, 2017-03-06 18:55:25]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 18:56:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2726423002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2726423002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:301:
void stopForegroundInteral(boolean killNotification) {
On 2017/03/05 06:09:29, qinmin wrote:
> nit:s/Interal/Internal/

Done.

[commit-bot: I haz the power, 2017-03-06 19:33:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-06 19:33:50]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2017-03-06 20:23:05]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-03-06 20:23:05]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2726423002/#ps40001
(title: "Fix spelling mistake")

[commit-bot: I haz the power, 2017-03-06 20:23:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 20:29:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488831785318990,
"parent_rev": "3ed5d998cc82797b116a949f75da144ec3465a6d", "commit_rev":
"1f6307adb1b1ff13661af0432d735866f2531831"}

[commit-bot: I haz the power, 2017-03-06 20:30:09]

Description was changed from

==========
Fix DownloadNotificationService foreground state

There are some cases that keep the summary notification visible.  This
CL tries to address them (but does not catch *all* cases).  The
following scenarios are now handled:

- NotificationManager#notify() posts a task, which means querying
NotificationManager#getActiveNotifications() might not take into account
the addition.  Only hide the summary if there are no currently running
notifications.
- NotificationManager#cancel() also posts at ask which causes the same
issues.  Ignore the notification id that was passed to cancel when
checking the list state.

There are still some cases where NotificationManager#cancel() on the
summary doesn't dismiss it.  This needs to be investigated in a follow
up (or a failsafe added and the code refactored).

BUG=680819
==========

to

==========
Fix DownloadNotificationService foreground state

There are some cases that keep the summary notification visible.  This
CL tries to address them (but does not catch *all* cases).  The
following scenarios are now handled:

- NotificationManager#notify() posts a task, which means querying
NotificationManager#getActiveNotifications() might not take into account
the addition.  Only hide the summary if there are no currently running
notifications.
- NotificationManager#cancel() also posts at ask which causes the same
issues.  Ignore the notification id that was passed to cancel when
checking the list state.

There are still some cases where NotificationManager#cancel() on the
summary doesn't dismiss it.  This needs to be investigated in a follow
up (or a failsafe added and the code refactored).

BUG=680819

Review-Url: https://codereview.chromium.org/2726423002
Cr-Commit-Position: refs/heads/master@{#454941}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f6307adb1b1ff13661af0432d73...
==========

[commit-bot: I haz the power, 2017-03-06 20:30:10]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1f6307adb1b1ff13661af0432d73...