Update the download summary icon

Update the download summary icon

Add support for dynamically setting the download summary icon based on
the icons of the other downloads.  Currently the summary icon stays as a
single icon type, but it should update to reflect the combined state of
the downloads it represents.  This CL updates the summary notification
as the notifications are updated, depending on whether or not the icon
needs to change.

BUG=680819
Review-Url: https://codereview.chromium.org/2730243002
Cr-Commit-Position: refs/heads/master@{#455006}
Committed: https://chromium.googlesource.com/chromium/src/+/31f3265e09220f484c417341ca34acacf191600e

[David Trainor- moved to gerrit, 2017-03-04 23:10:53]

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

[David Trainor- moved to gerrit, 2017-03-04 23:10:53]

min ptal.  updates the summary icon with the notifications.  thanks.

[qinmin, 2017-03-05 06:48:26]

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

https://codereview.chromium.org/2730243002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:205:
if (addedNotification != null && addedNotification.first ==
notification.getId())
if addedNotification is not null, this for loop skips the notification that is
the addedNotification.
However, after the for loop, there is an if statement to consider the icon for
this addedNotification.

if (addedNotification != null) {
  ......
 }

So why cannot we remove the latter if statement?

[David Trainor- moved to gerrit, 2017-03-06 16:29:54]

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

https://codereview.chromium.org/2730243002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:205:
if (addedNotification != null && addedNotification.first ==
notification.getId())
On 2017/03/05 06:48:26, qinmin wrote:
> if addedNotification is not null, this for loop skips the notification that is
> the addedNotification.
> However, after the for loop, there is an if statement to consider the icon for
> this addedNotification.
> 
> if (addedNotification != null) {
>   ......
>  }
> 
> So why cannot we remove the latter if statement?

The added notification might not be shown yet, so it might not be in the list of
StatusBarNotifications that we query.

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

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:08]

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-06 19:21:53]

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

[commit-bot: I haz the power, 2017-03-06 19:21:54]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[David Trainor- moved to gerrit, 2017-03-06 19:41:28]

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

[commit-bot: I haz the power, 2017-03-06 19:42:27]

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-06 20:14:40]

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

[commit-bot: I haz the power, 2017-03-06 20:14:40]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[David Trainor- moved to gerrit, 2017-03-06 20:30:08]

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

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

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-06 21:15:25]

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

[commit-bot: I haz the power, 2017-03-06 21:15:26]

Dry run: This issue passed the CQ dry run.

[qinmin, 2017-03-06 21:43:43]

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

https://codereview.chromium.org/2730243002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:205:
if (addedNotification != null && addedNotification.first ==
notification.getId())
On 2017/03/06 16:29:54, David Trainor wrote:
> On 2017/03/05 06:48:26, qinmin wrote:
> > if addedNotification is not null, this for loop skips the notification that
is
> > the addedNotification.
> > However, after the for loop, there is an if statement to consider the icon
for
> > this addedNotification.
> > 
> > if (addedNotification != null) {
> >   ......
> >  }
> > 
> > So why cannot we remove the latter if statement?
> 
> The added notification might not be shown yet, so it might not be in the list
of
> StatusBarNotifications that we query.

i see, in that case, can we just initialize those booleans to those values from
the addedNotification if it is non null?
It looks ugly to have a lot of redudant code here.

[qinmin, 2017-03-06 21:47:14]

lgtm % nit

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

https://codereview.chromium.org/2730243002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:566:
return;
nit: return can go to the previous line

[David Trainor- moved to gerrit, 2017-03-06 23:47:30]

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

https://codereview.chromium.org/2730243002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:566:
return;
On 2017/03/06 21:47:14, qinmin wrote:
> nit: return can go to the previous line

Done.

[David Trainor- moved to gerrit, 2017-03-06 23:47:34]

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

[commit-bot: I haz the power, 2017-03-06 23:48:27]

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-07 00:26:03]

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

[commit-bot: I haz the power, 2017-03-07 00:26:04]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2017-03-07 00:33:30]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-03-07 00:33:31]

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/2730243002/#ps120001
(title: "Addressed nit")

[commit-bot: I haz the power, 2017-03-07 00:34:44]

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-07 00:40:05]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488846810886440,
"parent_rev": "09d4b5a586b0701cf794aa34b86b352196b481a5", "commit_rev":
"31f3265e09220f484c417341ca34acacf191600e"}

[commit-bot: I haz the power, 2017-03-07 00:40:47]

Description was changed from

==========
Update the download summary icon

Add support for dynamically setting the download summary icon based on
the icons of the other downloads.  Currently the summary icon stays as a
single icon type, but it should update to reflect the combined state of
the downloads it represents.  This CL updates the summary notification
as the notifications are updated, depending on whether or not the icon
needs to change.

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

to

==========
Update the download summary icon

Add support for dynamically setting the download summary icon based on
the icons of the other downloads.  Currently the summary icon stays as a
single icon type, but it should update to reflect the combined state of
the downloads it represents.  This CL updates the summary notification
as the notifications are updated, depending on whether or not the icon
needs to change.

BUG=680819

Review-Url: https://codereview.chromium.org/2730243002
Cr-Commit-Position: refs/heads/master@{#455006}
Committed:
https://chromium.googlesource.com/chromium/src/+/31f3265e09220f484c417341ca34...
==========

[commit-bot: I haz the power, 2017-03-07 00:40:48]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/31f3265e09220f484c417341ca34...