Pop up the notification when the download is interrupted or completed

Pop up the notification when the download is interrupted or completed

Changes:
- Update notification with type = PROGRESS, so the content is updated
  even when the message center is opened.
- Popup the notification when the download is interrupted or completed
- Add browser tests
- Fix the string ID of interrupted download

BUG=446112, 468562
TEST=run browser_tests

Committed: https://crrev.com/46a900266f4dcda21aba0f29e1d8b6c03d581460
Cr-Commit-Position: refs/heads/master@{#330056}

[yoshiki, 2015-04-24 18:24:04]

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

[yoshiki, 2015-04-24 18:27:32]

Asanka, PTAL. Thanks.

[asanka, 2015-04-28 22:36:40]

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

https://codereview.chromium.org/1105953002/diff/20001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:190:
notification_->set_progress(100);
Isn't the progress and type already correct at this point? It seems redundant to
set it here.

Perhaps DCHECK instead?

This is more of a UI question, but shouldn't the appearance of the notification
change after completion?

[yoshiki, 2015-05-12 13:57:39]

Patchset #4 (id:60001) has been deleted

[yoshiki, 2015-05-12 14:47:10]

Patchset #4 (id:80001) has been deleted

[yoshiki, 2015-05-12 14:49:34]

Asanka, PTAL again. I added some code to pop up the notification after
completion. Because I think it's better to do them in one patch. Thanks.

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

https://codereview.chromium.org/1105953002/diff/20001/chrome/browser/download...
chrome/browser/download/notification/download_notification_item.cc:190:
notification_->set_progress(100);
On 2015/04/28 22:36:40, asanka wrote:
> Isn't the progress and type already correct at this point? It seems redundant
to
> set it here.

No. As for progress, before here, the progress, which has been set in the
previous call (STATE == IN_PROGRESS), is almost 100% but not 100%. We need to
set it to just 100%.

And I think it's better to set them here for downloads which is finished in a
very short time.

> Perhaps DCHECK instead?

Done.

> This is more of a UI question, but shouldn't the appearance of the
notification
> change after completion?

Sorry for lacking word, it's a kind of workaround for message center. As I wrote
in the comment, the content in non-progress notification is not updated when the
message center is visible, so we need to upload the notification once with type
== PROGRESS.

And I also updated the CL to remove and re-show the notification (patch set #4).

The new behavior is summarized as follows:
  step 1) Update with type=progress, progress=100% -> updated immediately
  step 2) Remove -> might be delayed when message center is visible
  step 3) Add -> might be delayed when message center is visible

https://codereview.chromium.org/1105953002/diff/100001/chrome/browser/downloa...
File chrome/browser/download/notification/download_notification_item.cc (right):

https://codereview.chromium.org/1105953002/diff/100001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_item.cc:184: if
(previous_download_state_ != content::DownloadItem::IN_PROGRESS) {
The semantics is same as the previous. Just rewrite with
|previous_download_state_| instead of |downloading_|.

https://codereview.chromium.org/1105953002/diff/100001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_item.cc:395:
IDS_DOWNLOAD_STATUS_DOWNLOAD_FAILED_TITLE, file_name);
just fixing wrong ID for failure download.

[asanka, 2015-05-14 21:36:17]

lgtm modulo nits

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
File chrome/browser/download/notification/download_notification_browsertest.cc
(right):

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:378:
// Waits for new notification is popped up.
// Waits for new notification.

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:378:
// Waits for new notification is popped up.
// Waits for new notification.

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:565:
// Waits that new notification is popped up.
// Waits for new notification.

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:591:
// Waits that new notification is popped up.
ditto

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
File chrome/browser/download/notification/download_notification_item.cc (right):

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_item.cc:165: const
std::string notification_id_in_message_center =
unused?

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_item.cc:299: //
notification once and re-show it immediately so it'll be poped up.
so it'll pop up.

[yoshiki, 2015-05-15 06:27:09]

Thank you for reviewing!

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
File chrome/browser/download/notification/download_notification_browsertest.cc
(right):

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:378:
// Waits for new notification is popped up.
On 2015/05/14 21:36:17, asanka wrote:
> // Waits for new notification.

Done.

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:565:
// Waits that new notification is popped up.
On 2015/05/14 21:36:17, asanka wrote:
> // Waits for new notification.

Done.

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_browsertest.cc:591:
// Waits that new notification is popped up.
On 2015/05/14 21:36:16, asanka wrote:
> ditto

Done.

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
File chrome/browser/download/notification/download_notification_item.cc (right):

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_item.cc:165: const
std::string notification_id_in_message_center =
On 2015/05/14 21:36:17, asanka wrote:
> unused?

Done.

https://codereview.chromium.org/1105953002/diff/130001/chrome/browser/downloa...
chrome/browser/download/notification/download_notification_item.cc:299: //
notification once and re-show it immediately so it'll be poped up.
On 2015/05/14 21:36:17, asanka wrote:
> so it'll pop up.

Done.

[yoshiki, 2015-05-15 06:27:14]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2015-05-15 06:27:15]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/1105953002/#ps150001
(title: "Addressed comments #7")

[yoshiki, 2015-05-15 06:27:17]

The CQ bit was unchecked by yoshiki@chromium.org

[yoshiki, 2015-05-15 06:28:00]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2015-05-15 06:28:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1105953002/150001

[commit-bot: I haz the power, 2015-05-15 07:15:08]

Committed patchset #7 (id:150001)

[commit-bot: I haz the power, 2015-05-15 07:15:49]

Patchset 7 (id:??) landed as
https://crrev.com/46a900266f4dcda21aba0f29e1d8b6c03d581460
Cr-Commit-Position: refs/heads/master@{#330056}