Plumb NotifierId to status icon balloons

Plumb NotifierId to status icon balloons

When constructing Notifications, a NotifierId must be passed indicating which component of Chrome is responsible for the notification.

Previously, as of https://codereview.chromium.org/1395093002, a status icon DesktopNotificationBalloon would use a fixed string for the NotifierId.

But DesktopNotificationBalloons is just a middle-man, that could actually be used by various components within Chrome. Thus, this patch adds a NotifierId parameter to StatusIcon::DisplayBalloon, which is provided by the calling component.

BUG=542232
Committed: https://crrev.com/9295617d1839f851becebee59a3c64cdadf46164
Cr-Commit-Position: refs/heads/master@{#354286}

[Peter Beverloo, 2015-10-10 16:35:26]

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

[Peter Beverloo, 2015-10-10 16:35:27]

+dewittj, atwilson

Could you please amend the CL's description to describe the *why* in one or two
sentences, rather than just linking to discussion in another CL? That'll make it
much clearer to readers what's going on.

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

https://codereview.chromium.org/1387383004/diff/20001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager_win.cc:26: const char
kAppInstalledNotifierId[] = "background-mode.app-installed";
nit: no need for the anonymous namespace here, constant values default to
internal linkage.

https://codereview.chromium.org/1387383004/diff/20001/chrome/browser/status_i...
File chrome/browser/status_icons/status_icon.h (right):

https://codereview.chromium.org/1387383004/diff/20001/chrome/browser/status_i...
chrome/browser/status_icons/status_icon.h:44: const message_center::NotifierId&
notifier_id) = 0;
I'm a bit torn on what type to use here.

Yes, mc::NotifierId certainly is applicable since that's what it will be used
as, but it's entirely unrelated to what any platform-native (e.g. Windows)
implementation would be using, unless you think of it strictly as a data
carrier.

On the other end, using something like "const char* source" is not super obvious
either, and would require one to read this comment (when amended) for context.

I don't feel strongly one way or another, but perhaps Justin or Andrew will have
an opinion.

[Andrew T Wilson (Slow), 2015-10-12 10:37:04]

LGTM - no strong opinion about NotifierId, so I think it's probably OK as-is.

[johnme, 2015-10-12 14:34:09]

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

https://codereview.chromium.org/1387383004/diff/20001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager_win.cc:26: const char
kAppInstalledNotifierId[] = "background-mode.app-installed";
On 2015/10/10 16:35:27, Peter Beverloo wrote:
> nit: no need for the anonymous namespace here, constant values default to
> internal linkage.

Done.

https://codereview.chromium.org/1387383004/diff/20001/chrome/browser/status_i...
File chrome/browser/status_icons/status_icon.h (right):

https://codereview.chromium.org/1387383004/diff/20001/chrome/browser/status_i...
chrome/browser/status_icons/status_icon.h:44: const message_center::NotifierId&
notifier_id) = 0;
On 2015/10/10 16:35:27, Peter Beverloo wrote:
> I'm a bit torn on what type to use here.
> 
> Yes, mc::NotifierId certainly is applicable since that's what it will be used
> as, but it's entirely unrelated to what any platform-native (e.g. Windows)
> implementation would be using, unless you think of it strictly as a data
> carrier.
> 
> On the other end, using something like "const char* source" is not super
obvious
> either, and would require one to read this comment (when amended) for context.
> 
> I don't feel strongly one way or another, but perhaps Justin or Andrew will
have
> an opinion.

Since this is a method for displaying "a notification balloon", it doesn't seem
unreasonable for it to always take a NotifierId, even on platforms where it uses
a native balloon rather than Chrome's notifications.

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

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

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

pkasting@chromium.org: Please give owners approval for changes in
chrome/browser/ui/. Thanks!

[Peter Beverloo, 2015-10-12 17:09:29]

lgtm

[dewittj, 2015-10-12 17:50:59]

It seems better to me to just remove the entire DisplayBalloon method and make
background_mode_manager just create a notification directly, since that appears
to be the only consumer of this method.

[Peter Kasting, 2015-10-12 23:40:58]

LGTM, feel free to TBR OWNERS for trivial mechanical changes in their area

[johnme, 2015-10-13 13:16:41]

On 2015/10/12 17:50:59, dewittj wrote:
> It seems better to me to just remove the entire DisplayBalloon method and make
> background_mode_manager just create a notification directly, since that
appears
> to be the only consumer of this method.

Peter and I discussed this, and found that at least on Windows, the
DisplayBalloon method has superior UI to notifications: it shows a balloon that
is visibly connected to the systray status icon, rather than a notification that
is disconnected. So it doesn't seem we can simply remove DisplayBalloon, at
least without UX feedback.

Does this work for you?

[dewittj, 2015-10-14 21:04:41]

sure, lgtm

[johnme, 2015-10-15 11:07:44]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-10-15 11:07:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1387383004/#ps40001
(title: "Rebase and remove unnecessary anonymous namespace")

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

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

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

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

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

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[johnme, 2015-10-15 16:13:07]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-10-15 16:13:07]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
atwilson@chromium.org, pkasting@chromium.org, dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1387383004/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-15 16:13:13]

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

[commit-bot: I haz the power, 2015-10-15 16:54:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-15 16:55:36]

Patchset 4 (id:??) landed as
https://crrev.com/9295617d1839f851becebee59a3c64cdadf46164
Cr-Commit-Position: refs/heads/master@{#354286}