Don't show the group notification when there is only one in-progress download

Don't show the group notification when there is only one in-progress download

See the issue. This patch makes the group notification count only in-progress downloads.

BUG=516459
TEST=added test passes

Committed: https://crrev.com/25f4575629ef634eadf89c9c5ea45f228d4f3b85
Cr-Commit-Position: refs/heads/master@{#342060}

[yoshiki, 2015-08-04 18:53:33]

yoshiki@chromium.org changed reviewers:
+ asanka@chromium.org

[yoshiki, 2015-08-04 18:56:20]

Patchset #1 (id:1) has been deleted

[yoshiki, 2015-08-04 19:23:13]

Asanka, PTAL. Thanks.

[asanka, 2015-08-04 21:55:10]

https://codereview.chromium.org/1271693006/diff/20001/chrome/browser/download...
File chrome/browser/download/notification/download_group_notification.cc
(right):

https://codereview.chromium.org/1271693006/diff/20001/chrome/browser/download...
chrome/browser/download/notification/download_group_notification.cc:103: for
(auto it = items_.begin(); it != items_.end(); it++) {
for (const auto& item : items_) {
  if (!item->IsDone() && ++inprogress_download_count == 2) {
    Show();
    break;
  }
}

https://codereview.chromium.org/1271693006/diff/20001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_browsertest.cc
(right):

https://codereview.chromium.org/1271693006/diff/20001/chrome/browser/download...
chrome/browser/download/notification/download_notification_browsertest.cc:789:
CreateDownload();
I think the fact that this call changes global state for the test fixture makes
it a bit hard to read these tests. We should clean this up a bit.

For example, the notification(), download_item() etc. calls are referring to the
download and notifications created here. And then the test has to manually
create a second set of values by manually initiating a download. It would be
great if there was a single CreateDownload() call that is reusable.

https://codereview.chromium.org/1271693006/diff/20001/chrome/browser/download...
chrome/browser/download/notification/download_notification_browsertest.cc:817:
// Confirms that there is two notification.
.. are two notifications...

[yoshiki, 2015-08-05 08:55:30]

Patchset #3 (id:60001) has been deleted

[yoshiki, 2015-08-05 08:55:35]

Patchset #2 (id:40001) has been deleted

[yoshiki, 2015-08-05 09:07:59]

Patchset #2 (id:80001) has been deleted

[yoshiki, 2015-08-05 09:09:32]

Asanka, PTAL. Thanks.

https://codereview.chromium.org/1271693006/diff/20001/chrome/browser/download...
File chrome/browser/download/notification/download_group_notification.cc
(right):

https://codereview.chromium.org/1271693006/diff/20001/chrome/browser/download...
chrome/browser/download/notification/download_group_notification.cc:103: for
(auto it = items_.begin(); it != items_.end(); it++) {
On 2015/08/04 21:55:09, asanka wrote:
> for (const auto& item : items_) {
>   if (!item->IsDone() && ++inprogress_download_count == 2) {
>     Show();
>     break;
>   }
> }

Done.

https://codereview.chromium.org/1271693006/diff/20001/chrome/browser/download...
File chrome/browser/download/notification/download_notification_browsertest.cc
(right):

https://codereview.chromium.org/1271693006/diff/20001/chrome/browser/download...
chrome/browser/download/notification/download_notification_browsertest.cc:789:
CreateDownload();
On 2015/08/04 21:55:09, asanka wrote:
> I think the fact that this call changes global state for the test fixture
makes
> it a bit hard to read these tests. We should clean this up a bit.
> 
> For example, the notification(), download_item() etc. calls are referring to
the
> download and notifications created here. And then the test has to manually
> create a second set of values by manually initiating a download. It would be
> great if there was a single CreateDownload() call that is reusable.

I added some comments, and introduced variables for each downloads instead of
using the methods in the class.

https://codereview.chromium.org/1271693006/diff/20001/chrome/browser/download...
chrome/browser/download/notification/download_notification_browsertest.cc:817:
// Confirms that there is two notification.
On 2015/08/04 21:55:09, asanka wrote:
> .. are two notifications...

Done.

[asanka, 2015-08-05 14:07:08]

lgtm

[yoshiki, 2015-08-05 14:19:54]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2015-08-05 14:20:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1271693006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1271693006/100001

[commit-bot: I haz the power, 2015-08-05 17:08:01]

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

[commit-bot: I haz the power, 2015-08-05 17:08:02]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yoshiki, 2015-08-06 04:16:51]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2015-08-06 04:17:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1271693006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1271693006/100001

[commit-bot: I haz the power, 2015-08-06 05:08:11]

Committed patchset #2 (id:100001)

[commit-bot: I haz the power, 2015-08-06 05:08:53]

Patchset 2 (id:??) landed as
https://crrev.com/25f4575629ef634eadf89c9c5ea45f228d4f3b85
Cr-Commit-Position: refs/heads/master@{#342060}