Render Ash material design battery image icon without PNGs

ash/common/system/chromeos/power/power_status.cc

Index: ash/common/system/chromeos/power/power_status.cc
diff --git a/ash/common/system/chromeos/power/power_status.cc b/ash/common/system/chromeos/power/power_status.cc
index 806db347d2dda4f038931ac4472640a3d6fb9ab3..a6da2e4ffbb8476b3ea0244265a06e806c3bb9a0 100644
--- a/ash/common/system/chromeos/power/power_status.cc
+++ b/ash/common/system/chromeos/power/power_status.cc
@@ -17,9 +17,14 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/l10n/time_format.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/display/display.h"
+#include "ui/display/screen.h"
+#include "ui/gfx/canvas.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia_operations.h"
+#include "ui/gfx/paint_vector_icon.h"
+#include "ui/gfx/vector_icons_public.h"
 
 namespace ash {
 namespace {
@@ -93,6 +98,23 @@ int PowerSourceToMessageID(
   return 0;
 }
 
+gfx::VectorIconId VectorIconIdForIconBadge(PowerStatus::IconBadge icon_badge) {
+  switch (icon_badge) {
+    case PowerStatus::ICON_BADGE_NONE:
+      return gfx::VectorIconId::VECTOR_ICON_NONE;
+    case PowerStatus::ICON_BADGE_ALERT:
+      return gfx::VectorIconId::SYSTEM_TRAY_BATTERY_ALERT;
+    case PowerStatus::ICON_BADGE_BOLT:
+      return gfx::VectorIconId::SYSTEM_TRAY_BATTERY_BOLT;
+    case PowerStatus::ICON_BADGE_X:
+      return gfx::VectorIconId::SYSTEM_TRAY_BATTERY_X;
+    case PowerStatus::ICON_BADGE_UNRELIABLE:
+      return gfx::VectorIconId::SYSTEM_TRAY_BATTERY_UNRELIABLE;
+  }
+  NOTREACHED();
+  return gfx::VectorIconId::VECTOR_ICON_NONE;
+}
+
 static PowerStatus* g_power_status = NULL;
 
 // Minimum battery percentage rendered in UI.
@@ -105,6 +127,30 @@ const int kBatteryImageWidth = 25;
 // Number of different power states.
 const int kNumPowerImages = 15;
 
+// The height of the battery icon in material design (as measured from the
+// user-visible bottom of the icon to the user-visible top of the icon).
+const int kBatteryImageHeightMD = 12;

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

nit: s/MD/Md

[tdanderson, 2016/06/21 22:42:28]

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

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

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.

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

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

[tdanderson, 2016/06/22 15:25:00]

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

+
+// The dimensions of the canvas containing the material design battery icon.
+const int kBatteryCanvasSizeMD = 16;
+
+// The empty background color of the battery icon in the system tray. Used
+// for material design.
+// TODO(tdanderson): Move these constants to a shared location if they are
+// shared by more than one material design system icon.
+const SkColor kBatteryBaseColor = SkColorSetARGB(0x4C, 0xFF, 0xFF, 0xFF);

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

nit: imo easier to read as SkColorSetA(SK_ColorWHITE, 0x4C);

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

Done.

+
+// The background color of the filled region of the battery in the system
+// tray. Used for material design.
+const SkColor kBatteryFillColor = SkColorSetARGB(0xFF, 0xFF, 0xFF, 0xFF);

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

just SK_ColorWHITE

[tdanderson, 2016/06/21 22:42:28]

Done.

+
+// The color of the battery's badge (bolt, unreliable, X).
+const SkColor kBatteryBadgeColor = SkColorSetARGB(0xB2, 0x00, 0x00, 0x00);

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

SkColorSetA(SK_ColorBLACK, 0xB2)

[tdanderson, 2016/06/21 22:42:28]

Done.

+
+// The color used for the battery's badge and fill color when the battery
+// fill level is critically low.
+const SkColor kBatteryAlertColor = SkColorSetARGB(0xFF, 0xDA, 0x27, 0x12);

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

just SkColorSetRGB

[tdanderson, 2016/06/21 22:42:28]

Done.

+
 }  // namespace
 
 const int PowerStatus::kMaxBatteryTimeToDisplaySec = 24 * 60 * 60;
@@ -260,35 +306,111 @@ std::string PowerStatus::GetCurrentPowerSourceID() const {
 PowerStatus::BatteryImageInfo PowerStatus::GetBatteryImageInfo(
     IconSet icon_set) const {
   BatteryImageInfo info;
+  CalculateBatteryImageInfoMD(&info);
+  CalculateBatteryImageInfoNonMD(&info, icon_set);
+  return info;
+}
 
+void PowerStatus::CalculateBatteryImageInfoMD(BatteryImageInfo* info) const {
+  if (!IsUsbChargerConnected() && !IsBatteryPresent()) {
+    info->icon_badge = ICON_BADGE_X;
+    info->fill_level = 0;
+    return;
+  }
+
+  if (IsUsbChargerConnected()) {
+    info->icon_badge = ICON_BADGE_UNRELIABLE;
+  } else if (IsLinePowerConnected()) {
+    info->icon_badge = ICON_BADGE_BOLT;
+  } else {
+    info->icon_badge = ICON_BADGE_NONE;
+  }

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

nit: I'd make this a series of nested ternaries. If you hate that then I'd still
remove the curlies here.

[tdanderson, 2016/06/21 22:42:28]

Done.

+
+  // |fill_state| is a value between 0 and kBatteryImageHeightMD representing
+  // the number of pixels the battery image should be filled. The exception
+  // is when |fill_state| is 0 (a critically-low battery); in this case, still
+  // draw 1px of fill.
+  int fill_state =
+      static_cast<int>(GetBatteryPercent() / 100.0 * kBatteryImageHeightMD);
+  fill_state = std::max(std::min(fill_state, kBatteryImageHeightMD), 0);
+  info->fill_level = fill_state ? fill_state : 1;

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

implictly casting int to bool is confusing imo. Can't you just change the 0 on
the previous line to 1?

[tdanderson, 2016/06/21 22:42:28]

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.

+
+  // Use ICON_BADGE_ALERT if the battery is critically low and does not already
+  // have a badge assigned.
+  if (!fill_state && info->icon_badge == ICON_BADGE_NONE)

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

I'd say 1 is pretty low so it's ok to change !fill_state to fill_level == 1

[tdanderson, 2016/06/21 22:42:28]

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.

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

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:00]

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.

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

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]

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]

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.

+    info->icon_badge = ICON_BADGE_ALERT;
+}
+
+void PowerStatus::CalculateBatteryImageInfoNonMD(
+    BatteryImageInfo* info,
+    const IconSet& icon_set) const {
   if (IsUsbChargerConnected()) {
-    info.resource_id =
+    info->resource_id =
         (icon_set == ICON_DARK)
             ? IDR_AURA_UBER_TRAY_POWER_SMALL_CHARGING_UNRELIABLE_DARK
             : IDR_AURA_UBER_TRAY_POWER_SMALL_CHARGING_UNRELIABLE;
   } else {
-    info.resource_id = (icon_set == ICON_DARK)
-                           ? IDR_AURA_UBER_TRAY_POWER_SMALL_DARK
-                           : IDR_AURA_UBER_TRAY_POWER_SMALL;
+    info->resource_id = (icon_set == ICON_DARK)
+                            ? IDR_AURA_UBER_TRAY_POWER_SMALL_DARK
+                            : IDR_AURA_UBER_TRAY_POWER_SMALL;
   }
 
-  info.offset = IsUsbChargerConnected() ? 0 : (IsLinePowerConnected() ? 1 : 0);
+  info->offset = IsUsbChargerConnected() ? 0 : (IsLinePowerConnected() ? 1 : 0);
 
   if (GetBatteryPercent() >= 100.0) {
-    info.index = kNumPowerImages - 1;
+    info->index = kNumPowerImages - 1;
   } else if (!IsBatteryPresent()) {
-    info.index = kNumPowerImages;
+    info->index = kNumPowerImages;
   } else {
-    info.index =
+    info->index =
         static_cast<int>(GetBatteryPercent() / 100.0 * (kNumPowerImages - 1));
-    info.index = std::max(std::min(info.index, kNumPowerImages - 2), 0);
+    info->index = std::max(std::min(info->index, kNumPowerImages - 2), 0);
+  }
+}
+
+gfx::ImageSkia PowerStatus::GetBatteryImage(
+    const BatteryImageInfo& info) const {
+  if (!MaterialDesignController::UseMaterialDesignSystemIcons())
+    return GetBatteryImageNonMD(info);
+
+  const bool use_alert_color =
+      (info.fill_level == 1 && info.icon_badge != ICON_BADGE_BOLT);
+  const SkColor badge_color =
+      use_alert_color ? kBatteryAlertColor : kBatteryBadgeColor;
+  const SkColor fill_color =
+      use_alert_color ? kBatteryAlertColor : kBatteryFillColor;
+  gfx::Canvas canvas(
+      gfx::Size(kBatteryCanvasSizeMD, kBatteryCanvasSizeMD),
+      display::Screen::GetScreen()->GetPrimaryDisplay().device_scale_factor(),
+      true);
+
+  // Paint the battery's base (background) color.
+  PaintVectorIcon(&canvas, gfx::VectorIconId::SYSTEM_TRAY_BATTERY,
+                  kBatteryCanvasSizeMD, kBatteryBaseColor);
+  canvas.Save();

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

nit: put right above clip call

[tdanderson, 2016/06/21 22:42:28]

Done.

+
+  // Paint the filled portion of the battery. Note that |fill_height| adjusts
+  // for the 2px of padding between the bottom of the battery icon and the

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

nit: here and elsewhere you say px but you mean dp (because it would double at
2x dsf)

[tdanderson, 2016/06/21 22:42:28]

Done.

+  // bottom edge of |canvas|.
+  const int fill_height = info.fill_level + 2;
+  gfx::Rect clip_rect(0, kBatteryCanvasSizeMD - fill_height,
+                      kBatteryCanvasSizeMD, fill_height);
+  canvas.ClipRect(clip_rect);
+  PaintVectorIcon(&canvas, gfx::VectorIconId::SYSTEM_TRAY_BATTERY,
+                  kBatteryCanvasSizeMD, fill_color);
+  canvas.Restore();
+
+  // Paint the badge over top of the battery, if applicable.
+  if (info.icon_badge != ICON_BADGE_NONE) {
+    PaintVectorIcon(&canvas, VectorIconIdForIconBadge(info.icon_badge),
+                    kBatteryCanvasSizeMD, badge_color);
   }
 
-  return info;
+  return gfx::ImageSkia(canvas.ExtractImageRep());
 }
 
-gfx::ImageSkia PowerStatus::GetBatteryImage(IconSet icon_set) const {
-  const BatteryImageInfo info = GetBatteryImageInfo(icon_set);
+gfx::ImageSkia PowerStatus::GetBatteryImageNonMD(
+    const BatteryImageInfo& info) const {
   gfx::Image all;
   all = ui::ResourceBundle::GetSharedInstance().GetImageNamed(info.resource_id);
   gfx::Rect region(info.offset * kBatteryImageWidth,