Move low battery notification to Message Center.

Move low battery notification to Message Center.

The low battery notification is a TrayNotificationView. This CL makes it
into a message center notification instead, giving it updated icons.

This fixes issues caused by using the outdated tray notification style.

A new BatteryNotification class is added to show, hide and update
the MessageCenter Notification. TrayPower::PowerNotificationView is
removed. PowerStatusView and PowerStatusViewTest are simplified.

BUG=163120, 371364
R=skuhne@chromium.org, oshima@chromium.org

Committed: https://crrev.com/64461c4d9dab2a7f33b2611447d259a94976aca5
Cr-Commit-Position: refs/heads/master@{#321521}

[michaelpg, 2015-03-17 04:29:26]

michaelpg@chromium.org changed reviewers:
+ skuhne@chromium.org

[michaelpg, 2015-03-17 04:57:46]

Patchset #1 (id:1) has been deleted

[michaelpg, 2015-03-17 04:57:48]

Patchset #1 (id:20001) has been deleted

[michaelpg, 2015-03-17 05:00:36]

PTAL.

[Mr4D (OOO till 08-26), 2015-03-17 17:04:37]

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/battery_notification.cc (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/battery_notification.cc:33: resource_id =
IDR_AURA_NOTIFICATION_BATTERY_FLUCTUATING;
?? What is this? The USB charger has not enough power?

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/battery_notification.cc:58: if
(status.IsUsbChargerConnected()) {
Is the meaning of "IsUsbChargerConnected()" really more like
"IsLowPowerUsbChargerConnected"? If so - would it be possible to rename that?

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/battery_notification.cc:102: if
(message_center_->FindVisibleNotificationById(kBatteryNotificationId)) {
Don't need {}

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/power_status_view_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/power_status_view_unittest.cc:40: 
Could you add a comment here that this is only needed for tests - and / or mark
them with a postfix (..ForTest())?

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/tray_power.h (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/tray_power.h:93: scoped_ptr<BatteryNotification>
battery_notification_;
Practically you do not need a scoped_ptr here - couldn't you add the variable
directly? (BatteryNotification battery_notification_;)

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/tray_power_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/tray_power_unittest.cc:156:
EXPECT_EQ(TrayPower::NOTIFICATION_NONE, notification_state());
Instead of having here lots of EXPECT_EQ's - couldn't you pass
UpdateNotificationState(<property>, <expected add>, <expected remove>) and check
that the function incremented accordingly? That way it seems easier to follow
than it is now.

[michaelpg, 2015-03-17 18:32:46]

Some questions. Haven't uploaded changes yet. Thanks!

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/battery_notification.cc (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/battery_notification.cc:33: resource_id =
IDR_AURA_NOTIFICATION_BATTERY_FLUCTUATING;
On 2015/03/17 17:04:37, Mr4D wrote:
> ?? What is this? The USB charger has not enough power?

When a USB charger is connected we assume it probably only supports a low
charging rate -- this is usually the case. We only show this notification if the
battery level is low or critical.

So we know you have a low battery, and we're not sure how long it will last or
whether you're likely to charge or discharge (or both depending on load). We
show the fluctuating icon added to this CL for that (which is a larger version
of a similar status area icon).

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/battery_notification.cc:58: if
(status.IsUsbChargerConnected()) {
On 2015/03/17 17:04:37, Mr4D wrote:
> Is the meaning of "IsUsbChargerConnected()" really more like
> "IsLowPowerUsbChargerConnected"? If so - would it be possible to rename that?

No -- there's just a note in the comments that a USB charger is usually
low-power (but not always).

This is the same logic currently used for the system tray notification, it's
just being used for the notification here. This CL shouldn't result in
functional changes besides moving the notification.

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/battery_notification.cc:102: if
(message_center_->FindVisibleNotificationById(kBatteryNotificationId)) {
On 2015/03/17 17:04:37, Mr4D wrote:
> Don't need {}

Will do.

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/power_status_view_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/power_status_view_unittest.cc:40: 
On 2015/03/17 17:04:37, Mr4D wrote:
> Could you add a comment here that this is only needed for tests - and / or
mark
> them with a postfix (..ForTest())?

I'm not sure I understand -- this is already a test class, why do we need a
comment for these methods in particular?

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/tray_power.h (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/tray_power.h:93: scoped_ptr<BatteryNotification>
battery_notification_;
On 2015/03/17 17:04:37, Mr4D wrote:
> Practically you do not need a scoped_ptr here - couldn't you add the variable
> directly? (BatteryNotification battery_notification_;)

Hmm. I had trouble including battery_notification.h because it depends on this
file for the NotificationState enum. It doesn't look like we allow C++11 forward
declaration of enums. Am I missing a simple way of resolving the this dependency
issue?

I thought about moving the enum to BatteryNotification. In fact, maybe all the
logic about when to show a notification belongs in BatteryNotification as it
doesn't directly relate to the tray anymore. WDYT?

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/tray_power_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/tray_power_unittest.cc:156:
EXPECT_EQ(TrayPower::NOTIFICATION_NONE, notification_state());
On 2015/03/17 17:04:37, Mr4D wrote:
> Instead of having here lots of EXPECT_EQ's - couldn't you pass
> UpdateNotificationState(<property>, <expected add>, <expected remove>) and
check
> that the function incremented accordingly? That way it seems easier to follow
> than it is now.

Will do, good idea.

[Mr4D (OOO till 08-26), 2015-03-17 22:25:05]

See comments!

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/battery_notification.cc (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/battery_notification.cc:58: if
(status.IsUsbChargerConnected()) {
Okay. Somehow this is confusing however. Think about the Pixel2 for example. It
only charges through USB. So the name of the function is misleading. But okay -
we could possibly change it later.

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/power_status_view_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/power_status_view_unittest.cc:40: 
Argh. I looked up and down and landed here again. Ignore!

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/tray_power.h (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/tray_power.h:93: scoped_ptr<BatteryNotification>
battery_notification_;
That makes sense! Please do!

[michaelpg, 2015-03-17 22:28:40]

Patchset #2 (id:60001) has been deleted

[michaelpg, 2015-03-17 22:28:46]

Patchset #2 (id:80001) has been deleted

[michaelpg, 2015-03-17 23:10:39]

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/battery_notification.cc (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/battery_notification.cc:58: if
(status.IsUsbChargerConnected()) {
On 2015/03/17 22:25:04, Mr4D wrote:
> Okay. Somehow this is confusing however. Think about the Pixel2 for example.
It
> only charges through USB. So the name of the function is misleading. But okay
-
> we could possibly change it later.

derat@ is the person to talk to about this. Raiden ports are treated as
low-power USB unless they're connected to a USB-PD device like the Pixel
charger, then we consider that mains rather than USB charger (so the unreliable
charger notification doesn't show). But it's outside the scope of this CL.

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/battery_notification.cc:102: if
(message_center_->FindVisibleNotificationById(kBatteryNotificationId)) {
On 2015/03/17 18:32:45, michaelpg wrote:
> On 2015/03/17 17:04:37, Mr4D wrote:
> > Don't need {}
> 
> Will do.

Done.

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/tray_power.h (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/tray_power.h:93: scoped_ptr<BatteryNotification>
battery_notification_;
On 2015/03/17 22:25:04, Mr4D wrote:
> That makes sense! Please do!

Took another look. TrayPower uses NotificationState to determine when to update
the tray view. I wonder if maybe PowerStatus would be a better place, but I
don't want to add more UI specifics there.

However, maybe a scoped pointer actually works. Why should we keep the
BatteryNotification around when we're not using it? And why instantiate one on
startup unless the battery is low?

BatteryNotification is basically a wrapper around a help function for creating a
notification. Instead of using Show/Hide, we can just create/remove the
notification in the constructor/destructor. Take a look at let me know again
what you think - thanks!

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/tray_power_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/tray_power_unittest.cc:156:
EXPECT_EQ(TrayPower::NOTIFICATION_NONE, notification_state());
On 2015/03/17 17:04:37, Mr4D wrote:
> Instead of having here lots of EXPECT_EQ's - couldn't you pass
> UpdateNotificationState(<property>, <expected add>, <expected remove>) and
check
> that the function incremented accordingly? That way it seems easier to follow
> than it is now.

Done. Added SCOPED_TRACE so the failures will at least point to the right line.
This is easier to follow and IMO a lot cleaner, although it's 5 lines instead of
4 in most cases.

[oshima, 2015-03-19 21:06:44]

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
File ash/system/chromeos/power/power_status_view.h (right):

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
ash/system/chromeos/power/power_status_view.h:22: PowerStatusView(bool
default_view_right_align);
explicit

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
File ash/system/chromeos/power/power_status_view_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
ash/system/chromeos/power/power_status_view_unittest.cc:47: }
const methods?

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
File ash/system/chromeos/power/tray_power_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
ash/system/chromeos/power/tray_power_unittest.cc:165: SCOPED_TRACE("");
do you need these scoped_trace?

[michaelpg, 2015-03-19 21:38:50]

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
File ash/system/chromeos/power/tray_power_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
ash/system/chromeos/power/tray_power_unittest.cc:165: SCOPED_TRACE("");
On 2015/03/19 21:06:43, oshima wrote:
> do you need these scoped_trace?

Yes, otherwise it will be really difficult to tell which call to
UpdateNotificationState failed. Or should I print a stack trace in
UpdateNotificationState instead if a condition isn't met?

[oshima, 2015-03-19 21:42:00]

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
File ash/system/chromeos/power/tray_power_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
ash/system/chromeos/power/tray_power_unittest.cc:165: SCOPED_TRACE("");
On 2015/03/19 21:38:50, michaelpg wrote:
> On 2015/03/19 21:06:43, oshima wrote:
> > do you need these scoped_trace?
> 
> Yes, otherwise it will be really difficult to tell which call to
> UpdateNotificationState failed. Or should I print a stack trace in
> UpdateNotificationState instead if a condition isn't met?

Please set some meaningful name rather than empty strong then.

[michaelpg, 2015-03-20 00:16:34]

PTAL, thanks!

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
File ash/system/chromeos/power/power_status_view.h (right):

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
ash/system/chromeos/power/power_status_view.h:22: PowerStatusView(bool
default_view_right_align);
On 2015/03/19 21:06:43, oshima wrote:
> explicit

Done.

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
File ash/system/chromeos/power/power_status_view_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
ash/system/chromeos/power/power_status_view_unittest.cc:47: }
On 2015/03/19 21:06:43, oshima wrote:
> const methods?

Done.

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
File ash/system/chromeos/power/tray_power_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/100001/ash/system/chromeos/po...
ash/system/chromeos/power/tray_power_unittest.cc:165: SCOPED_TRACE("");
On 2015/03/19 21:42:00, oshima wrote:
> On 2015/03/19 21:38:50, michaelpg wrote:
> > On 2015/03/19 21:06:43, oshima wrote:
> > > do you need these scoped_trace?
> > 
> > Yes, otherwise it will be really difficult to tell which call to
> > UpdateNotificationState failed. Or should I print a stack trace in
> > UpdateNotificationState instead if a condition isn't met?
> 
> Please set some meaningful name rather than empty strong then.

Done.

[oshima, 2015-03-20 00:53:16]

lgtm with a nit.

https://codereview.chromium.org/1014753003/diff/120001/ash/system/chromeos/po...
File ash/system/chromeos/power/tray_power_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/120001/ash/system/chromeos/po...
ash/system/chromeos/power/tray_power_unittest.cc:165: SCOPED_TRACE("Expected no
notifications when no battery present");
you may omit "Expected to" part, but am ok to have them.

[michaelpg, 2015-03-20 00:55:47]

Thanks, just a quick question.

https://codereview.chromium.org/1014753003/diff/120001/ash/system/chromeos/po...
File ash/system/chromeos/power/tray_power_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/120001/ash/system/chromeos/po...
ash/system/chromeos/power/tray_power_unittest.cc:165: SCOPED_TRACE("Expected no
notifications when no battery present");
On 2015/03/20 00:53:15, oshima wrote:
> you may omit "Expected to" part, but am ok to have them. 

It seems ambiguous to me. Do you usually write the expected or the actual
condition as a message here?

A. "No notifications when no battery present" (meaning: We expected no
notifications but there was one")
B. "Notification when no battery present" (meaning: we actually saw a
notification, which is an error)

[Mr4D (OOO till 08-26), 2015-03-20 03:10:53]

lgtm

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
File ash/system/chromeos/power/tray_power.h (right):

https://codereview.chromium.org/1014753003/diff/40001/ash/system/chromeos/pow...
ash/system/chromeos/power/tray_power.h:93: scoped_ptr<BatteryNotification>
battery_notification_;
Okay. Now with your changes it makes sense. Thanks!

[michaelpg, 2015-03-20 03:54:30]

Patchset #4 (id:140001) has been deleted

[michaelpg, 2015-03-20 03:56:39]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2015-03-20 03:56:39]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/1014753003/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-03-20 03:57:08]

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

[michaelpg, 2015-03-20 03:57:12]

Thanks! Glad to get this in for 43.

https://codereview.chromium.org/1014753003/diff/120001/ash/system/chromeos/po...
File ash/system/chromeos/power/tray_power_unittest.cc (right):

https://codereview.chromium.org/1014753003/diff/120001/ash/system/chromeos/po...
ash/system/chromeos/power/tray_power_unittest.cc:165: SCOPED_TRACE("Expected no
notifications when no battery present");
On 2015/03/20 00:53:15, oshima wrote:
> you may omit "Expected to" part, but am ok to have them. 

Done.

[commit-bot: I haz the power, 2015-03-20 05:41:32]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2015-03-20 05:42:20]

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