Fix system notifications incorrectly marked as type WEB_PAGE

Fix system notifications incorrectly marked as type WEB_PAGE

Some system components were incorrectly calling the Notification
constructor that was intended only for Web Notifications (though this
wasn't clearly documented). I've fixed the incorrect calls, and removed
the confusing constructor to prevent this reoccuring.

Other system components were explicitly constructing a NotifierId using
the constructor that takes a single GURL. I've fixed these incorrect
calls.

Finally, now that Web Notifications can be reliably detected using
`notification->notifier_id().type == NotifierId::WEB_PAGE` I've removed
the obsolete is_web_notification flag from message_center::Notification.

BUG=542232
Committed: https://crrev.com/81256f554f024571955f7f82b7fe4c7064afd238
Cr-Commit-Position: refs/heads/master@{#353529}

[johnme, 2015-10-09 10:24:05]

johnme@chromium.org changed reviewers:
+ peter@chromium.org

[johnme, 2015-10-09 10:24:57]

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

[commit-bot: I haz the power, 2015-10-09 10:26:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1395093002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1395093002/1

[Peter Beverloo, 2015-10-09 10:47:30]

Thanks! This looks great, let me confirm one of my comments before I sign off.

https://codereview.chromium.org/1395093002/diff/1/chrome/browser/status_icons...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

https://codereview.chromium.org/1395093002/diff/1/chrome/browser/status_icons...
chrome/browser/status_icons/desktop_notification_balloon.cc:74: void
DesktopNotificationBalloon::DisplayBalloon(
Heh, this class only seems to be used for background mode, yet has a lot of
plumbing to support it. That makes it a nice clean-up target for the future :).

https://codereview.chromium.org/1395093002/diff/1/chrome/browser/status_icons...
chrome/browser/status_icons/desktop_notification_balloon.cc:92: kNotifierId),
This is not great. Theoretically the desktop notification balloon class can be
used by lots of other features, among which status tray icons, all of which
would not fall under a single system component id.

However, also given my earlier comment, I think this is OK for now as long as we
follow up with a clean-up patch. I'll take a stab this afternoon to see how
feasible it is.

Please, at least, call it out explicitly with a comment/TODO.

https://codereview.chromium.org/1395093002/diff/1/ui/message_center/popup_tim...
File ui/message_center/popup_timer.cc (right):

https://codereview.chromium.org/1395093002/diff/1/ui/message_center/popup_tim...
ui/message_center/popup_timer.cc:22: return
base::TimeDelta::FromSeconds(kAutocloseWebNotificationDelaySeconds);
nit: this would now be more appropriately named as
kAutocloseWebPageDelaySeconds.

[commit-bot: I haz the power, 2015-10-09 11:08:41]

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

[commit-bot: I haz the power, 2015-10-09 11:08:42]

Dry run: This issue passed the CQ dry run.

[johnme, 2015-10-09 11:21:02]

Addressed Peter's review comments - thanks :)

https://codereview.chromium.org/1395093002/diff/1/chrome/browser/status_icons...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

https://codereview.chromium.org/1395093002/diff/1/chrome/browser/status_icons...
chrome/browser/status_icons/desktop_notification_balloon.cc:92: kNotifierId),
On 2015/10/09 10:47:30, Peter Beverloo wrote:
> This is not great. Theoretically the desktop notification balloon class can be
> used by lots of other features, among which status tray icons, all of which
> would not fall under a single system component id.
> 
> However, also given my earlier comment, I think this is OK for now as long as
we
> follow up with a clean-up patch. I'll take a stab this afternoon to see how
> feasible it is.
> 
> Please, at least, call it out explicitly with a comment/TODO.

Added TODO explaining this.

https://codereview.chromium.org/1395093002/diff/1/ui/message_center/popup_tim...
File ui/message_center/popup_timer.cc (right):

https://codereview.chromium.org/1395093002/diff/1/ui/message_center/popup_tim...
ui/message_center/popup_timer.cc:22: return
base::TimeDelta::FromSeconds(kAutocloseWebNotificationDelaySeconds);
On 2015/10/09 10:47:30, Peter Beverloo wrote:
> nit: this would now be more appropriately named as
> kAutocloseWebPageDelaySeconds.

Done.

[Peter Beverloo, 2015-10-09 12:55:22]

peter@chromium.org changed reviewers:
+ dewittj@chromium.org

[Peter Beverloo, 2015-10-09 12:55:23]

lgtm % comments

I'd like Justin to sign off on this as well given that he reviewed the initial
patch and has a better understanding of the history here. I personally think
that getting rid of yet another constructor is a Good Thing - these certainly
were bugs.

https://codereview.chromium.org/1395093002/diff/20001/chrome/browser/backgrou...
File chrome/browser/background/background_contents_service.cc (right):

https://codereview.chromium.org/1395093002/diff/20001/chrome/browser/backgrou...
chrome/browser/background/background_contents_service.cc:73: const char
kNotifierId[] = "app.background.crashed";
micro nit: alphabetise (you do it elsewhere).

https://codereview.chromium.org/1395093002/diff/20001/chrome/browser/status_i...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

https://codereview.chromium.org/1395093002/diff/20001/chrome/browser/status_i...
chrome/browser/status_icons/desktop_notification_balloon.cc:35: const char
kNotifierId[] = "status-icons.desktop-notification-balloon";
micro nit: alphabetise.

https://codereview.chromium.org/1395093002/diff/20001/chrome/browser/status_i...
chrome/browser/status_icons/desktop_notification_balloon.cc:91: // TODO(johnme):
In theory the desktop notification balloon class can be
Please place this comment above the call, not in the call (it makes the full
statement much harder to parse). Also please refer the bug.

https://codereview.chromium.org/1395093002/diff/20001/ui/message_center/messa...
File ui/message_center/message_center_style.cc (right):

https://codereview.chromium.org/1395093002/diff/20001/ui/message_center/messa...
ui/message_center/message_center_style.cc:59: // consider replacing
kAutocloseDefaultDelaySeconds with this.
nit: Please refer to a bug.

[johnme, 2015-10-09 14:20:42]

Addressed Peter's review nits - thanks :)

https://codereview.chromium.org/1395093002/diff/20001/chrome/browser/backgrou...
File chrome/browser/background/background_contents_service.cc (right):

https://codereview.chromium.org/1395093002/diff/20001/chrome/browser/backgrou...
chrome/browser/background/background_contents_service.cc:73: const char
kNotifierId[] = "app.background.crashed";
On 2015/10/09 12:55:23, Peter Beverloo wrote:
> micro nit: alphabetise (you do it elsewhere).

Done.

https://codereview.chromium.org/1395093002/diff/20001/chrome/browser/status_i...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

https://codereview.chromium.org/1395093002/diff/20001/chrome/browser/status_i...
chrome/browser/status_icons/desktop_notification_balloon.cc:35: const char
kNotifierId[] = "status-icons.desktop-notification-balloon";
On 2015/10/09 12:55:23, Peter Beverloo wrote:
> micro nit: alphabetise.

Done.

https://codereview.chromium.org/1395093002/diff/20001/chrome/browser/status_i...
chrome/browser/status_icons/desktop_notification_balloon.cc:91: // TODO(johnme):
In theory the desktop notification balloon class can be
On 2015/10/09 12:55:23, Peter Beverloo wrote:
> Please place this comment above the call, not in the call (it makes the full
> statement much harder to parse). Also please refer the bug.

Done (I linked directly to https://codereview.chromium.org/1387383004).

https://codereview.chromium.org/1395093002/diff/20001/ui/message_center/messa...
File ui/message_center/message_center_style.cc (right):

https://codereview.chromium.org/1395093002/diff/20001/ui/message_center/messa...
ui/message_center/message_center_style.cc:59: // consider replacing
kAutocloseDefaultDelaySeconds with this.
On 2015/10/09 12:55:23, Peter Beverloo wrote:
> nit: Please refer to a bug.

Done.

[dewittj, 2015-10-09 15:28:38]

lgtm, thanks for the cleanup!

https://codereview.chromium.org/1395093002/diff/20002/chrome/browser/status_i...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

https://codereview.chromium.org/1395093002/diff/20002/chrome/browser/status_i...
chrome/browser/status_icons/desktop_notification_balloon.cc:92: // see
https://codereview.chromium.org/1387383004.
optional nit: I'd prefer a bug reference to a codereview reference

[johnme, 2015-10-12 13:54:13]

Done, thanks :)

https://codereview.chromium.org/1395093002/diff/20002/chrome/browser/status_i...
File chrome/browser/status_icons/desktop_notification_balloon.cc (right):

https://codereview.chromium.org/1395093002/diff/20002/chrome/browser/status_i...
chrome/browser/status_icons/desktop_notification_balloon.cc:92: // see
https://codereview.chromium.org/1387383004.
On 2015/10/09 15:28:38, dewittj wrote:
> optional nit: I'd prefer a bug reference to a codereview reference

Done.

[johnme, 2015-10-12 14:45:07]

johnme@chromium.org changed reviewers:
+ jochen@chromium.org

[johnme, 2015-10-12 14:45:07]

jochen@chromium.org: Please give owners approval for changes in chrome/browser/
(except chrome/browser/notifications/, which already has it). Thanks :)

[jochen (gone - plz use gerrit), 2015-10-12 14:53:25]

lgtm

[johnme, 2015-10-12 14:59:35]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-10-12 14:59:36]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1395093002/#ps70001
(title: "Fix typo")

[commit-bot: I haz the power, 2015-10-12 15:04:20]

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

[commit-bot: I haz the power, 2015-10-12 16:19:26]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2015-10-12 16:20:31]

Patchset 5 (id:??) landed as
https://crrev.com/81256f554f024571955f7f82b7fe4c7064afd238
Cr-Commit-Position: refs/heads/master@{#353529}