Render Ash material design battery image icon without PNGs

Render Ash material design battery image icon without PNGs

Render the battery icon used in the Ash material
design system tray using a vector icon instead
of a PNG. The battery charge level is drawn at runtime,
and battery badges (alert, lightning bolt, X, and
unreliable charging) are drawn using separate
vector icons.

BUG=617298
TEST=PowerStatusTest.*

Committed: https://crrev.com/7da397af40d827b9d8252001c3c944ed09f4e9ea
Cr-Commit-Position: refs/heads/master@{#401378}

[tdanderson, 2016-06-13 01:06:56]

tdanderson@chromium.org changed reviewers:
+ estade@chromium.org

[tdanderson, 2016-06-13 01:06:56]

Evan, here is a WIP to illustrate how I'd like to draw the MD battery icon in
Ash. Can you please give feedback on the general approach only (in particular
PowerStatus::GetBatteryImage()) ?

[Evan Stade, 2016-06-13 17:37:20]

https://codereview.chromium.org/2063633002/diff/1/ash/system/chromeos/power/p...
File ash/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/1/ash/system/chromeos/power/p...
ash/system/chromeos/power/power_status.cc:352: const SkColor kFillColor =
this is kind of confusing as a name, because to me "fill" is opposed to
"stroke", not "background".

https://codereview.chromium.org/2063633002/diff/1/ash/system/chromeos/power/p...
ash/system/chromeos/power/power_status.cc:354: const SkColor kBadgeColor =
is there just one badge color? It isn't one color above the fill and a different
one below the fill?

https://codereview.chromium.org/2063633002/diff/1/ash/system/chromeos/power/p...
ash/system/chromeos/power/power_status.cc:376: // Mask the drawing of the
background fill level to the shape of the battery.
this masking idea seems like a fine approach but I wonder if it's the simplest
or most straightforward?

In my head I had imagined something like:

PaintVectorIcon(canvas, BATTERY, size, bg_color);
canvas.Save();
canvas.ClipRect(fill_rect);
PaintVectorIcon(canvas, BATTERY, size, fg_color);
canvas.Restore();
if (badge)
  PaintVectorIcon(canvas, badge, size, badge_color);
return gfx::ImageSkia(canvas.ExtractImageRep());

Or if you need two different badge colors, something like:

PaintVectorIcon(canvas, BATTERY, size, bg_color);
if (badge)
  PaintVectorIcon(canvas, badge, size, badge_color1);
canvas.ClipRect(fill_rect);
PaintVectorIcon(canvas, BATTERY, size, fg_color);
if (badge)
  PaintVectorIcon(canvas, badge, size, badge_color2);
return gfx::ImageSkia(canvas.ExtractImageRep());

[tdanderson, 2016-06-20 23:01:04]

Description was changed from

==========
[WIP] Render Ash material design battery image icon without PNGs

Incomplete WIP, not for review.

BUG=617298
==========

to

==========
Render Ash material design battery image icon without PNGs

Render the battery icon used in the Ash material
design system tray using a vector icon instead
of a PNG. The battery fill level is drawn at runtime,
and battery badges (alert, lightning bolt, X, and
unreliable charging) are drawn using separate
vector icons.

BUG=617298
TEST=in progress
==========

[tdanderson, 2016-06-20 23:06:53]

tdanderson@chromium.org changed reviewers:
+ jamescook@chromium.org

[tdanderson, 2016-06-20 23:06:53]

James and Evan, can you please take a look? Note that I still have to fix up one
or more existing tests and I'd like to add a new test, but otherwise this is
ready for review.

https://codereview.chromium.org/2063633002/diff/1/ash/system/chromeos/power/p...
File ash/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/1/ash/system/chromeos/power/p...
ash/system/chromeos/power/power_status.cc:352: const SkColor kFillColor =
On 2016/06/13 17:37:20, Evan Stade wrote:
> this is kind of confusing as a name, because to me "fill" is opposed to
> "stroke", not "background".

I picked the terminology that is being used in the specs folder. Have a look at
the changes in the next patch set and see if there are still changes you'd like
me to make in the naming.

https://codereview.chromium.org/2063633002/diff/1/ash/system/chromeos/power/p...
ash/system/chromeos/power/power_status.cc:354: const SkColor kBadgeColor =
On 2016/06/13 17:37:20, Evan Stade wrote:
> is there just one badge color? It isn't one color above the fill and a
different
> one below the fill?

Clarified in next patch set.

https://codereview.chromium.org/2063633002/diff/1/ash/system/chromeos/power/p...
ash/system/chromeos/power/power_status.cc:376: // Mask the drawing of the
background fill level to the shape of the battery.
On 2016/06/13 17:37:20, Evan Stade wrote:
> this masking idea seems like a fine approach but I wonder if it's the simplest
> or most straightforward?
> 
> In my head I had imagined something like:
> 
> PaintVectorIcon(canvas, BATTERY, size, bg_color);
> canvas.Save();
> canvas.ClipRect(fill_rect);
> PaintVectorIcon(canvas, BATTERY, size, fg_color);
> canvas.Restore();
> if (badge)
>   PaintVectorIcon(canvas, badge, size, badge_color);
> return gfx::ImageSkia(canvas.ExtractImageRep());
> 
> Or if you need two different badge colors, something like:
> 
> PaintVectorIcon(canvas, BATTERY, size, bg_color);
> if (badge)
>   PaintVectorIcon(canvas, badge, size, badge_color1);
> canvas.ClipRect(fill_rect);
> PaintVectorIcon(canvas, BATTERY, size, fg_color);
> if (badge)
>   PaintVectorIcon(canvas, badge, size, badge_color2);
> return gfx::ImageSkia(canvas.ExtractImageRep());

Yes, this is much better, thanks. Changed to this in the next patch set.

[Evan Stade, 2016-06-21 02:55:48]

https://codereview.chromium.org/2063633002/diff/1/ash/system/chromeos/power/p...
File ash/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/1/ash/system/chromeos/power/p...
ash/system/chromeos/power/power_status.cc:352: const SkColor kFillColor =
On 2016/06/20 23:06:53, tdanderson wrote:
> On 2016/06/13 17:37:20, Evan Stade wrote:
> > this is kind of confusing as a name, because to me "fill" is opposed to
> > "stroke", not "background".
> 
> I picked the terminology that is being used in the specs folder. Have a look
at
> the changes in the next patch set and see if there are still changes you'd
like
> me to make in the naming.

yes, I still don't like "fill". I'm not surprised that it has a different
connotation in a design context compared to code. Perhaps instead of
fill/fill_height/fill_level we can use "charge"?

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:132: const int
kBatteryImageHeightMD = 12;
nit: s/MD/Md

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:141: const SkColor
kBatteryBaseColor = SkColorSetARGB(0x4C, 0xFF, 0xFF, 0xFF);
nit: imo easier to read as SkColorSetA(SK_ColorWHITE, 0x4C);

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:145: const SkColor
kBatteryFillColor = SkColorSetARGB(0xFF, 0xFF, 0xFF, 0xFF);
just SK_ColorWHITE

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:148: const SkColor
kBatteryBadgeColor = SkColorSetARGB(0xB2, 0x00, 0x00, 0x00);
SkColorSetA(SK_ColorBLACK, 0xB2)

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:152: const SkColor
kBatteryAlertColor = SkColorSetARGB(0xFF, 0xDA, 0x27, 0x12);
just SkColorSetRGB

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:327: }
nit: I'd make this a series of nested ternaries. If you hate that then I'd still
remove the curlies here.

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:336: info->fill_level =
fill_state ? fill_state : 1;
implictly casting int to bool is confusing imo. Can't you just change the 0 on
the previous line to 1?

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:340: if (!fill_state &&
info->icon_badge == ICON_BADGE_NONE)
I'd say 1 is pretty low so it's ok to change !fill_state to fill_level == 1

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:390: canvas.Save();
nit: put right above clip call

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:393: // for the 2px of padding
between the bottom of the battery icon and the
nit: here and elsewhere you say px but you mean dp (because it would double at
2x dsf)

[James Cook, 2016-06-21 16:13:00]

I'll take a look after you address Evan's comments and the compile failures.
Feel free to ping me on hangouts.

[James Cook, 2016-06-21 16:13:29]

er, test failures I mean.

[tdanderson, 2016-06-21 22:35:23]

Description was changed from

==========
Render Ash material design battery image icon without PNGs

Render the battery icon used in the Ash material
design system tray using a vector icon instead
of a PNG. The battery fill level is drawn at runtime,
and battery badges (alert, lightning bolt, X, and
unreliable charging) are drawn using separate
vector icons.

BUG=617298
TEST=in progress
==========

to

==========
Render Ash material design battery image icon without PNGs

Render the battery icon used in the Ash material
design system tray using a vector icon instead
of a PNG. The battery charge level is drawn at runtime,
and battery badges (alert, lightning bolt, X, and
unreliable charging) are drawn using separate
vector icons.

BUG=617298
TEST=PowerStatusTest.*
==========

[tdanderson, 2016-06-21 22:42:29]

James and Evan, I have addressed the last round of comments and added tests. Can
you please take a look?

https://codereview.chromium.org/2063633002/diff/1/ash/system/chromeos/power/p...
File ash/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/1/ash/system/chromeos/power/p...
ash/system/chromeos/power/power_status.cc:352: const SkColor kFillColor =
On 2016/06/21 02:55:48, Evan Stade wrote:
> On 2016/06/20 23:06:53, tdanderson wrote:
> > On 2016/06/13 17:37:20, Evan Stade wrote:
> > > this is kind of confusing as a name, because to me "fill" is opposed to
> > > "stroke", not "background".
> > 
> > I picked the terminology that is being used in the specs folder. Have a look
> at
> > the changes in the next patch set and see if there are still changes you'd
> like
> > me to make in the naming.
> 
> yes, I still don't like "fill". I'm not surprised that it has a different
> connotation in a design context compared to code. Perhaps instead of
> fill/fill_height/fill_level we can use "charge"?

fill -> charge changed everywhere

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:132: const int
kBatteryImageHeightMD = 12;
On 2016/06/21 02:55:48, Evan Stade wrote:
> nit: s/MD/Md

Shouldn't we keep it as MD since 'M' and 'D' are the first letters of two
separate words?

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:141: const SkColor
kBatteryBaseColor = SkColorSetARGB(0x4C, 0xFF, 0xFF, 0xFF);
On 2016/06/21 02:55:48, Evan Stade wrote:
> nit: imo easier to read as SkColorSetA(SK_ColorWHITE, 0x4C);

Done.

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:145: const SkColor
kBatteryFillColor = SkColorSetARGB(0xFF, 0xFF, 0xFF, 0xFF);
On 2016/06/21 02:55:48, Evan Stade wrote:
> just SK_ColorWHITE

Done.

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:148: const SkColor
kBatteryBadgeColor = SkColorSetARGB(0xB2, 0x00, 0x00, 0x00);
On 2016/06/21 02:55:48, Evan Stade wrote:
> SkColorSetA(SK_ColorBLACK, 0xB2)

Done.

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:152: const SkColor
kBatteryAlertColor = SkColorSetARGB(0xFF, 0xDA, 0x27, 0x12);
On 2016/06/21 02:55:48, Evan Stade wrote:
> just SkColorSetRGB

Done.

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:327: }
On 2016/06/21 02:55:48, Evan Stade wrote:
> nit: I'd make this a series of nested ternaries. If you hate that then I'd
still
> remove the curlies here.

Done.

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:336: info->fill_level =
fill_state ? fill_state : 1;
On 2016/06/21 02:55:48, Evan Stade wrote:
> implictly casting int to bool is confusing imo. Can't you just change the 0 on
> the previous line to 1?

I will change to std::max(charge_state, 1). I can't change the line above for
the reason outlined in my reply on line 340.

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:340: if (!fill_state &&
info->icon_badge == ICON_BADGE_NONE)
On 2016/06/21 02:55:48, Evan Stade wrote:
> I'd say 1 is pretty low so it's ok to change !fill_state to fill_level == 1

Sebastien has asked that that we keep the same appearance as pre-MD here, which
for a |fill_state| of 0 is an exclamation mark, but not for a |fill_state| of 1.
Note that for both cases we do draw a 1dp red charge level.

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:390: canvas.Save();
On 2016/06/21 02:55:48, Evan Stade wrote:
> nit: put right above clip call

Done.

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:393: // for the 2px of padding
between the bottom of the battery icon and the
On 2016/06/21 02:55:48, Evan Stade wrote:
> nit: here and elsewhere you say px but you mean dp (because it would double at
> 2x dsf)

Done.

[James Cook, 2016-06-21 23:19:00]

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:132: const int
kBatteryImageHeightMD = 12;
On 2016/06/21 22:42:28, tdanderson wrote:
> On 2016/06/21 02:55:48, Evan Stade wrote:
> > nit: s/MD/Md
> 
> Shouldn't we keep it as MD since 'M' and 'D' are the first letters of two
> separate words?

In general, it's OsHttpUrl over OSHTTPURL. I haven't been complaining because it
looks like the MD code has standardized on capital MD before I started reviewing
it. Up to you whether you want to start following the Md convention in new code.

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:131: // user-visible bottom of
the icon to the user-visible top of the icon).
nice comment

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:310:
CalculateBatteryImageInfoNonMD(&info, icon_set);
Do you need to do both of these when in the non-MD state?

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:376: (info.charge_level == 1 &&
info.icon_badge != ICON_BADGE_BOLT);
Is this the same "1" as in line 335? I would either define some sort of "min
visual charge level" constant or do <= 1 here.

nit: Also, it might be clearer to test IsLinePowerConnected() rather than
checking for the bolt badge.

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status.h (right):

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.h:28: // Types of badges which can
be drawn over top of a battery icon.
nit: "on top of a" or "over a"

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.h:88: if
(ash::MaterialDesignController::UseMaterialDesignSystemIcons())
please define this function in the .cc file so you can avoid the #include for
material_design_controller.h in this header

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status_unittest.cc (right):

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status_unittest.cc:76: ,
nit: comment why this is blank

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status_unittest.cc:301: // A charge level
of 1 when the battery is up to 16% full, and a level of 2
optional: maybe also test battery at 0%? I think we can get into that state?

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status_unittest.cc:335: }
Nice test suite, very thorough.

[Evan Stade, 2016-06-21 23:25:42]

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:132: const int
kBatteryImageHeightMD = 12;
On 2016/06/21 22:42:28, tdanderson wrote:
> On 2016/06/21 02:55:48, Evan Stade wrote:
> > nit: s/MD/Md
> 
> Shouldn't we keep it as MD since 'M' and 'D' are the first letters of two
> separate words?

google style guide says "StartRpc(), not StartRPC()". Chrome is admittedly bad
about following this convention.

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:340: if (!fill_state &&
info->icon_badge == ICON_BADGE_NONE)
On 2016/06/21 22:42:28, tdanderson wrote:
> On 2016/06/21 02:55:48, Evan Stade wrote:
> > I'd say 1 is pretty low so it's ok to change !fill_state to fill_level == 1
> 
> Sebastien has asked that that we keep the same appearance as pre-MD here,
which
> for a |fill_state| of 0 is an exclamation mark, but not for a |fill_state| of
1.
> Note that for both cases we do draw a 1dp red charge level.

But you're not keeping the same behavior because before you had to have less
than ~1/30th charge to get a ! and now you need to have less than ~1/24th. It
seems like the ! should stop depending on a relatively unrelated graphical
detail (dp height of battery icon) and instead depend on a specific value, e.g.

if (GetBatteryPercent() < 2.f && info->icon_badge == ICON_BADGE_NONE)
  badge = alert

[tdanderson, 2016-06-22 15:25:01]

James and Evan, please take another look.

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:132: const int
kBatteryImageHeightMD = 12;
On 2016/06/21 23:25:42, Evan Stade wrote:
> On 2016/06/21 22:42:28, tdanderson wrote:
> > On 2016/06/21 02:55:48, Evan Stade wrote:
> > > nit: s/MD/Md
> > 
> > Shouldn't we keep it as MD since 'M' and 'D' are the first letters of two
> > separate words?
> 
> google style guide says "StartRpc(), not StartRPC()". Chrome is admittedly bad
> about following this convention.

I will use Md here and in new code going forward.

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:340: if (!fill_state &&
info->icon_badge == ICON_BADGE_NONE)
On 2016/06/21 23:25:42, Evan Stade wrote:
> On 2016/06/21 22:42:28, tdanderson wrote:
> > On 2016/06/21 02:55:48, Evan Stade wrote:
> > > I'd say 1 is pretty low so it's ok to change !fill_state to fill_level ==
1
> > 
> > Sebastien has asked that that we keep the same appearance as pre-MD here,
> which
> > for a |fill_state| of 0 is an exclamation mark, but not for a |fill_state|
of
> 1.
> > Note that for both cases we do draw a 1dp red charge level.
> 
> But you're not keeping the same behavior because before you had to have less
> than ~1/30th charge to get a ! and now you need to have less than ~1/24th. It
> seems like the ! should stop depending on a relatively unrelated graphical
> detail (dp height of battery icon) and instead depend on a specific value,
e.g.
> 
> if (GetBatteryPercent() < 2.f && info->icon_badge == ICON_BADGE_NONE)
>   badge = alert
>   

You are correct that the heights are different, which changes the number of
possible fill levels that can be shown. The bottom level is (0%,7%] pre-MD and
(0%,8%] in MD. The second-from-bottom level is [8%,14%] pre-MD and [9%,16%] in
MD.

Despite this, the request was to keep the ! for the bottom level only, which is
what I have done in this CL.

However I see and agree with your point about the alert state being tied to a
graphical detail rather than the icon height. I have reached out to Sebastien to
ask what he thinks that percentage should be and will update accordingly.

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:131: // user-visible bottom of
the icon to the user-visible top of the icon).
On 2016/06/21 23:19:00, James Cook wrote:
> nice comment

Thanks

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:310:
CalculateBatteryImageInfoNonMD(&info, icon_set);
On 2016/06/21 23:19:00, James Cook wrote:
> Do you need to do both of these when in the non-MD state?

This was to guarantee that all members of BatteryImageInfo had a sensible value
regardless of mode, but I suppose it is better to only initialize the parts that
are needed per mode. Changed.

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:376: (info.charge_level == 1 &&
info.icon_badge != ICON_BADGE_BOLT);
On 2016/06/21 23:19:00, James Cook wrote:
> Is this the same "1" as in line 335? I would either define some sort of "min
> visual charge level" constant or do <= 1 here.

Done.

> 
> nit: Also, it might be clearer to test IsLinePowerConnected() rather than
> checking for the bolt badge.

Done.

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status.h (right):

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.h:28: // Types of badges which can
be drawn over top of a battery icon.
On 2016/06/21 23:19:00, James Cook wrote:
> nit: "on top of a" or "over a"

Done.

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.h:88: if
(ash::MaterialDesignController::UseMaterialDesignSystemIcons())
On 2016/06/21 23:19:00, James Cook wrote:
> please define this function in the .cc file so you can avoid the #include for
> material_design_controller.h in this header

Done.

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status_unittest.cc (right):

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status_unittest.cc:76: ,
On 2016/06/21 23:19:00, James Cook wrote:
> nit: comment why this is blank

Done.

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status_unittest.cc:301: // A charge level
of 1 when the battery is up to 16% full, and a level of 2
On 2016/06/21 23:19:00, James Cook wrote:
> optional: maybe also test battery at 0%? I think we can get into that state?

Done.

https://codereview.chromium.org/2063633002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status_unittest.cc:335: }
On 2016/06/21 23:19:00, James Cook wrote:
> Nice test suite, very thorough.

Thank you

[James Cook, 2016-06-22 15:48:23]

LGTM

[Evan Stade, 2016-06-22 16:49:23]

lgtm modulo ! badge issue

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:340: if (!fill_state &&
info->icon_badge == ICON_BADGE_NONE)
On 2016/06/22 15:25:00, tdanderson wrote:
> On 2016/06/21 23:25:42, Evan Stade wrote:
> > On 2016/06/21 22:42:28, tdanderson wrote:
> > > On 2016/06/21 02:55:48, Evan Stade wrote:
> > > > I'd say 1 is pretty low so it's ok to change !fill_state to fill_level
==
> 1
> > > 
> > > Sebastien has asked that that we keep the same appearance as pre-MD here,
> > which
> > > for a |fill_state| of 0 is an exclamation mark, but not for a |fill_state|
> of
> > 1.
> > > Note that for both cases we do draw a 1dp red charge level.
> > 
> > But you're not keeping the same behavior because before you had to have less
> > than ~1/30th charge to get a ! and now you need to have less than ~1/24th.
It
> > seems like the ! should stop depending on a relatively unrelated graphical
> > detail (dp height of battery icon) and instead depend on a specific value,
> e.g.
> > 
> > if (GetBatteryPercent() < 2.f && info->icon_badge == ICON_BADGE_NONE)
> >   badge = alert
> >   
> 
> You are correct that the heights are different, which changes the number of
> possible fill levels that can be shown. The bottom level is (0%,7%] pre-MD and
> (0%,8%] in MD. The second-from-bottom level is [8%,14%] pre-MD and [9%,16%] in
> MD.
> 
> Despite this, the request was to keep the ! for the bottom level only, which
is
> what I have done in this CL.
> 
> However I see and agree with your point about the alert state being tied to a
> graphical detail rather than the icon height. I have reached out to Sebastien
to
> ask what he thinks that percentage should be and will update accordingly.

If you don't want to block this CL on his response, you can choose an arbitrary
number (5%?) and update it later.

[Daniel Erat, 2016-06-22 17:22:43]

derat@chromium.org changed reviewers:
+ derat@chromium.org

[Daniel Erat, 2016-06-22 17:22:44]

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:340: if (!fill_state &&
info->icon_badge == ICON_BADGE_NONE)
On 2016/06/22 16:49:23, Evan Stade wrote:
> On 2016/06/22 15:25:00, tdanderson wrote:
> > On 2016/06/21 23:25:42, Evan Stade wrote:
> > > On 2016/06/21 22:42:28, tdanderson wrote:
> > > > On 2016/06/21 02:55:48, Evan Stade wrote:
> > > > > I'd say 1 is pretty low so it's ok to change !fill_state to fill_level
> ==
> > 1
> > > > 
> > > > Sebastien has asked that that we keep the same appearance as pre-MD
here,
> > > which
> > > > for a |fill_state| of 0 is an exclamation mark, but not for a
|fill_state|
> > of
> > > 1.
> > > > Note that for both cases we do draw a 1dp red charge level.
> > > 
> > > But you're not keeping the same behavior because before you had to have
less
> > > than ~1/30th charge to get a ! and now you need to have less than ~1/24th.
> It
> > > seems like the ! should stop depending on a relatively unrelated graphical
> > > detail (dp height of battery icon) and instead depend on a specific value,
> > e.g.
> > > 
> > > if (GetBatteryPercent() < 2.f && info->icon_badge == ICON_BADGE_NONE)
> > >   badge = alert
> > >   
> > 
> > You are correct that the heights are different, which changes the number of
> > possible fill levels that can be shown. The bottom level is (0%,7%] pre-MD
and
> > (0%,8%] in MD. The second-from-bottom level is [8%,14%] pre-MD and [9%,16%]
in
> > MD.
> > 
> > Despite this, the request was to keep the ! for the bottom level only, which
> is
> > what I have done in this CL.
> > 
> > However I see and agree with your point about the alert state being tied to
a
> > graphical detail rather than the icon height. I have reached out to
Sebastien
> to
> > ask what he thinks that percentage should be and will update accordingly.
> 
> If you don't want to block this CL on his response, you can choose an
arbitrary
> number (5%?) and update it later.

the chrome os power management perspective: it'd instead be possible to trigger
this off the estimated time until the system will shut down (which is how chrome
chooses when to display notifications about low charge, if i recall correctly).

that's better, in that "5%" can mean vastly different things in terms of
remaining time on different hardware.

it's also worse, in that time-until-empty estimates bounce around depending on
the current system load, while the percentage only decreases while the system is
discharging.

sticking with some random percentage for now seems fine to me.

[tdanderson, 2016-06-22 17:27:28]

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status.cc (right):

https://codereview.chromium.org/2063633002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:340: if (!fill_state &&
info->icon_badge == ICON_BADGE_NONE)
On 2016/06/22 17:22:43, Daniel Erat wrote:
> On 2016/06/22 16:49:23, Evan Stade wrote:
> > On 2016/06/22 15:25:00, tdanderson wrote:
> > > On 2016/06/21 23:25:42, Evan Stade wrote:
> > > > On 2016/06/21 22:42:28, tdanderson wrote:
> > > > > On 2016/06/21 02:55:48, Evan Stade wrote:
> > > > > > I'd say 1 is pretty low so it's ok to change !fill_state to
fill_level
> > ==
> > > 1
> > > > > 
> > > > > Sebastien has asked that that we keep the same appearance as pre-MD
> here,
> > > > which
> > > > > for a |fill_state| of 0 is an exclamation mark, but not for a
> |fill_state|
> > > of
> > > > 1.
> > > > > Note that for both cases we do draw a 1dp red charge level.
> > > > 
> > > > But you're not keeping the same behavior because before you had to have
> less
> > > > than ~1/30th charge to get a ! and now you need to have less than
~1/24th.
> > It
> > > > seems like the ! should stop depending on a relatively unrelated
graphical
> > > > detail (dp height of battery icon) and instead depend on a specific
value,
> > > e.g.
> > > > 
> > > > if (GetBatteryPercent() < 2.f && info->icon_badge == ICON_BADGE_NONE)
> > > >   badge = alert
> > > >   
> > > 
> > > You are correct that the heights are different, which changes the number
of
> > > possible fill levels that can be shown. The bottom level is (0%,7%] pre-MD
> and
> > > (0%,8%] in MD. The second-from-bottom level is [8%,14%] pre-MD and
[9%,16%]
> in
> > > MD.
> > > 
> > > Despite this, the request was to keep the ! for the bottom level only,
which
> > is
> > > what I have done in this CL.
> > > 
> > > However I see and agree with your point about the alert state being tied
to
> a
> > > graphical detail rather than the icon height. I have reached out to
> Sebastien
> > to
> > > ask what he thinks that percentage should be and will update accordingly.
> > 
> > If you don't want to block this CL on his response, you can choose an
> arbitrary
> > number (5%?) and update it later.
> 
> the chrome os power management perspective: it'd instead be possible to
trigger
> this off the estimated time until the system will shut down (which is how
chrome
> chooses when to display notifications about low charge, if i recall
correctly).
> 
> that's better, in that "5%" can mean vastly different things in terms of
> remaining time on different hardware.
> 
> it's also worse, in that time-until-empty estimates bounce around depending on
> the current system load, while the percentage only decreases while the system
is
> discharging.
> 
> sticking with some random percentage for now seems fine to me.

Thanks for taking a look. I'll leave as-is with the constant of 5%; please let
me know if this needs to change in the future.

[tdanderson, 2016-06-22 17:27:41]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2016-06-22 17:27:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2063633002/#ps100001
(title: "add battery percentage constant")

[commit-bot: I haz the power, 2016-06-22 17:28:10]

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

[commit-bot: I haz the power, 2016-06-22 19:06:51]

Description was changed from

==========
Render Ash material design battery image icon without PNGs

Render the battery icon used in the Ash material
design system tray using a vector icon instead
of a PNG. The battery charge level is drawn at runtime,
and battery badges (alert, lightning bolt, X, and
unreliable charging) are drawn using separate
vector icons.

BUG=617298
TEST=PowerStatusTest.*
==========

to

==========
Render Ash material design battery image icon without PNGs

Render the battery icon used in the Ash material
design system tray using a vector icon instead
of a PNG. The battery charge level is drawn at runtime,
and battery badges (alert, lightning bolt, X, and
unreliable charging) are drawn using separate
vector icons.

BUG=617298
TEST=PowerStatusTest.*
==========

[commit-bot: I haz the power, 2016-06-22 19:06:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-22 19:09:50]

Description was changed from

==========
Render Ash material design battery image icon without PNGs

Render the battery icon used in the Ash material
design system tray using a vector icon instead
of a PNG. The battery charge level is drawn at runtime,
and battery badges (alert, lightning bolt, X, and
unreliable charging) are drawn using separate
vector icons.

BUG=617298
TEST=PowerStatusTest.*
==========

to

==========
Render Ash material design battery image icon without PNGs

Render the battery icon used in the Ash material
design system tray using a vector icon instead
of a PNG. The battery charge level is drawn at runtime,
and battery badges (alert, lightning bolt, X, and
unreliable charging) are drawn using separate
vector icons.

BUG=617298
TEST=PowerStatusTest.*

Committed: https://crrev.com/7da397af40d827b9d8252001c3c944ed09f4e9ea
Cr-Commit-Position: refs/heads/master@{#401378}
==========

[commit-bot: I haz the power, 2016-06-22 19:09:51]

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