CrOS MD Network icon badging: layout

ui/chromeos/network/network_icon.cc

Index: ui/chromeos/network/network_icon.cc
diff --git a/ui/chromeos/network/network_icon.cc b/ui/chromeos/network/network_icon.cc
index 70ea9347bf793dc74e8a085902c7c13ee7fc78b6..c6a55c2e88a2eb7c1457aa08bb8d6105d0c09a55 100644
--- a/ui/chromeos/network/network_icon.cc
+++ b/ui/chromeos/network/network_icon.cc
@@ -45,6 +45,19 @@ namespace network_icon {
 
 namespace {
 
+// Constants for offseting the badge displayed on top of the signal strength
+// icon. The badge will extend outside of the base icon bounds by these amounts.
+// Only used for MD. All values are in dp.
+
+// The badge offsets are different depending on whether the icon is in the tray
+// or menu.
+const int kTrayIconBadgeOffset = 3;
+const int kMenuIconBadgeOffset = 2;
+
+bool UseMd() {
+  return md_icon_controller::UseMaterialDesignNetworkIcons();
+}
+
 //------------------------------------------------------------------------------
 // Struct to pass icon badges to NetworkIconImageSource.
 struct Badges {
@@ -189,11 +202,6 @@ SkColor GetBaseColorForIconType(IconType icon_type) {
   return icon_type == ICON_TYPE_TRAY ? SK_ColorWHITE : gfx::kChromeIconGrey;
 }
 
-gfx::Size GetSizeForIconType(IconType icon_type) {
-  // TODO(estade): use kTrayIconSize and kMenuIconSize.
-  return icon_type == ICON_TYPE_TRAY ? gfx::Size(16, 16) : gfx::Size(20, 20);
-}
-
 bool IconTypeIsDark(IconType icon_type) {
   return (icon_type != ICON_TYPE_TRAY);
 }
@@ -202,91 +210,109 @@ bool IconTypeHasVPNBadge(IconType icon_type) {
   return (icon_type != ICON_TYPE_LIST);
 }
 
-//------------------------------------------------------------------------------
-// Classes for generating scaled images.
-
-const SkBitmap GetEmptyBitmap(const gfx::Size pixel_size) {
-  typedef std::pair<int, int> SizeKey;
-  typedef std::map<SizeKey, SkBitmap> SizeBitmapMap;
-  static SizeBitmapMap* s_empty_bitmaps = new SizeBitmapMap;
-
-  SizeKey key(pixel_size.width(), pixel_size.height());
-
-  SizeBitmapMap::iterator iter = s_empty_bitmaps->find(key);
-  if (iter != s_empty_bitmaps->end())
-    return iter->second;
-
-  SkBitmap empty;
-  empty.allocN32Pixels(key.first, key.second);
-  empty.eraseARGB(0, 0, 0, 0);
-  (*s_empty_bitmaps)[key] = empty;
-  return empty;
-}
-
-class EmptyImageSource: public gfx::ImageSkiaSource {
+// This defines how we assemble a network icon.
+class NetworkIconImageSource : public gfx::CanvasImageSource {

[tdanderson, 2016/09/08 19:11:58]

optional: add a TODO to remove this class and reference crbug.com/614453 

[Evan Stade, 2016/09/08 19:31:34]

I don't think it's necessary because it's guarded by UseMd(), so when we remove
UseMd() it should be no longer used and obvious that we can remove it.

  public:
-  explicit EmptyImageSource(const gfx::Size& size)
-      : size_(size) {
-  }
+  NetworkIconImageSource(const gfx::ImageSkia& icon, const Badges& badges)
+      : CanvasImageSource(icon.size(), false), icon_(icon), badges_(badges) {}
+  ~NetworkIconImageSource() override {}
+
+  // TODO(pkotwicz): Figure out what to do when a new image resolution becomes
+  // available.
+  void Draw(gfx::Canvas* canvas) override {
+    canvas->DrawImageInt(icon_, 0, 0);
 
-  gfx::ImageSkiaRep GetImageForScale(float scale) override {
-    gfx::Size pixel_size = gfx::ScaleToFlooredSize(size_, scale);
-    SkBitmap empty_bitmap = GetEmptyBitmap(pixel_size);
-    return gfx::ImageSkiaRep(empty_bitmap, scale);
+    if (badges_.top_left)
+      canvas->DrawImageInt(*badges_.top_left, 0, 0);
+    if (badges_.top_right)
+      canvas->DrawImageInt(*badges_.top_right,
+                           icon_.width() - badges_.top_right->width(), 0);
+    if (badges_.bottom_left) {
+      canvas->DrawImageInt(*badges_.bottom_left, 0,
+                           icon_.height() - badges_.bottom_left->height());
+    }
+    if (badges_.bottom_right) {
+      canvas->DrawImageInt(*badges_.bottom_right,
+                           icon_.width() - badges_.bottom_right->width(),
+                           icon_.height() - badges_.bottom_right->height());
+    }
   }
 
  private:
-  const gfx::Size size_;
+  const gfx::ImageSkia icon_;
+  const Badges badges_;
 
-  DISALLOW_COPY_AND_ASSIGN(EmptyImageSource);
+  DISALLOW_COPY_AND_ASSIGN(NetworkIconImageSource);
 };
 
 // This defines how we assemble a network icon.

[tdanderson, 2016/09/08 19:11:58]

nit: "...in material design"

[Evan Stade, 2016/09/08 19:31:34]

Do you think the class name makes that apparent?

-class NetworkIconImageSource : public gfx::ImageSkiaSource {
+class NetworkIconImageSourceMd : public gfx::CanvasImageSource {
  public:
-  NetworkIconImageSource(const gfx::ImageSkia& icon, const Badges& badges)
-      : icon_(icon),
-        badges_(badges) {
+  static gfx::ImageSkia CreateImage(const gfx::ImageSkia& icon,
+                                    const Badges& badges) {
+    auto source = new NetworkIconImageSourceMd(icon, badges);
+    return gfx::ImageSkia(source, source->size());
   }
-  ~NetworkIconImageSource() override {}
 
-  // TODO(pkotwicz): Figure out what to do when a new image resolution becomes
-  // available.
-  gfx::ImageSkiaRep GetImageForScale(float scale) override {
-    gfx::Canvas canvas(icon_.size(), scale, false);
-    canvas.DrawImageInt(icon_, 0, 0);
+  // gfx::CanvasImageSource:
+  void Draw(gfx::Canvas* canvas) override {
+    int width = size().width();

[tdanderson, 2016/09/08 19:11:58]

int: const for width and height

+    int height = size().height();
+
+    // The base icon is centered in both dimensions.
+    const int icon_y = (height - icon_.height()) / 2;
+    canvas->DrawImageInt(icon_, (width - icon_.width()) / 2, icon_y);
 
+    // The badges are flush against the edges of the canvas, except at the top,
+    // where the badge is only 1dp higher than the base image.
+    const int top_badge_y = icon_y - 1;
     if (badges_.top_left)
-      canvas.DrawImageInt(*badges_.top_left, 0, 0);
-    if (badges_.top_right)
-      canvas.DrawImageInt(*badges_.top_right,
-                          icon_.width() - badges_.top_right->width(), 0);
+      canvas->DrawImageInt(*badges_.top_left, 0, top_badge_y);
+    if (badges_.top_right) {
+      canvas->DrawImageInt(*badges_.top_right,
+                           width - badges_.top_right->width(), top_badge_y);
+    }
     if (badges_.bottom_left) {
-      canvas.DrawImageInt(*badges_.bottom_left,
-                          0, icon_.height() - badges_.bottom_left->height());
+      canvas->DrawImageInt(*badges_.bottom_left, 0,
+                           height - badges_.bottom_left->height());
     }
     if (badges_.bottom_right) {
-      canvas.DrawImageInt(*badges_.bottom_right,
-                          icon_.width() - badges_.bottom_right->width(),
-                          icon_.height() - badges_.bottom_right->height());
+      canvas->DrawImageInt(*badges_.bottom_right,
+                           width - badges_.bottom_right->width(),
+                           height - badges_.bottom_right->height());
     }
-    return canvas.ExtractImageRep();
   }
 
+  bool HasRepresentationAtAllScales() const override { return true; }
+
  private:
+  NetworkIconImageSourceMd(const gfx::ImageSkia& icon, const Badges& badges)
+      : CanvasImageSource(GetSizeForBaseIconSize(icon.size()), false),
+        icon_(icon),
+        badges_(badges) {}
+  ~NetworkIconImageSourceMd() override {}
+
+  static gfx::Size GetSizeForBaseIconSize(const gfx::Size& base_icon_size) {
+    gfx::Size size = base_icon_size;
+    int badge_offset = base_icon_size.width() == 16 ? kTrayIconBadgeOffset

[tdanderson, 2016/09/08 19:11:58]

nit: const

[Evan Stade, 2016/09/08 19:31:34]

why? there are lots of local variables where we could apply const but don't. I
think it adds clutter most of the time.

I just went up and looked up the c++ style guide. My reading is that
constexpr/const should be used for compile-time constants (these are not that)
and const should be used for function parameters, data members, and methods.
Local variables are none of these.

+                                                    : kMenuIconBadgeOffset;
+    size.Enlarge(badge_offset * 2, badge_offset * 2);
+    return size;
+  }
+
   const gfx::ImageSkia icon_;
   const Badges badges_;
 
-  DISALLOW_COPY_AND_ASSIGN(NetworkIconImageSource);
+  DISALLOW_COPY_AND_ASSIGN(NetworkIconImageSourceMd);
 };
 
 // Depicts a given signal strength using arcs (e.g. for WiFi connections) or
 // bars (e.g. for cell connections).
-class NetworkIconImageSourceMd : public gfx::CanvasImageSource {
+class SignalStrengthImageSource : public gfx::CanvasImageSource {
  public:
-  NetworkIconImageSourceMd(ImageType image_type,
-                           IconType icon_type,
-                           int signal_strength)
+  SignalStrengthImageSource(ImageType image_type,
+                            IconType icon_type,
+                            int signal_strength)
       : CanvasImageSource(GetSizeForIconType(icon_type), false),
         image_type_(image_type),
         icon_type_(icon_type),
@@ -297,7 +323,7 @@ class NetworkIconImageSourceMd : public gfx::CanvasImageSource {
     DCHECK_GE(signal_strength, 0);
     DCHECK_LT(signal_strength, kNumNetworkImages);
   }
-  ~NetworkIconImageSourceMd() override {}
+  ~SignalStrengthImageSource() override {}
 
   // gfx::CanvasImageSource:
   void Draw(gfx::Canvas* canvas) override {
@@ -306,14 +332,17 @@ class NetworkIconImageSourceMd : public gfx::CanvasImageSource {
     else
       DrawBars(canvas);
   }
+
   bool HasRepresentationAtAllScales() const override {
-    // This image source can handle any scale factor. TODO(estade): investigate
-    // why this doesn't seem to be respected. (Perhaps because we compose
-    // multiple images that don't have representations at all scales?)
     return true;
   }
 
  private:
+  static gfx::Size GetSizeForIconType(IconType icon_type) {
+    // TODO(estade): use kTrayIconSize and kMenuIconSize.

[tdanderson, 2016/09/08 19:11:58]

nit: can you define 16 and 20 as constants in this file, change lines 297 and
343, and move the TODO to above the constants? Also reference crbug.com/623987
in your TODO.

[Evan Stade, 2016/09/08 19:31:34]

done (more or less)

+    return icon_type == ICON_TYPE_TRAY ? gfx::Size(16, 16) : gfx::Size(20, 20);
+  }
+
   void DrawArcs(gfx::Canvas* canvas) {
     gfx::RectF oval_bounds((gfx::Rect(size())));
     oval_bounds.Inset(gfx::Insets(kIconInset));
@@ -409,7 +438,7 @@ class NetworkIconImageSourceMd : public gfx::CanvasImageSource {
   // See crbug.com/623987 and crbug.com/632827
   static constexpr int kBgAlpha = 0x4D;
 
-  DISALLOW_COPY_AND_ASSIGN(NetworkIconImageSourceMd);
+  DISALLOW_COPY_AND_ASSIGN(SignalStrengthImageSource);
 };
 
 //------------------------------------------------------------------------------
@@ -440,9 +469,9 @@ ImageType ImageTypeForNetworkType(const std::string& type) {
 gfx::ImageSkia GetImageForIndex(ImageType image_type,
                                 IconType icon_type,
                                 int index) {
-  if (md_icon_controller::UseMaterialDesignNetworkIcons()) {
+  if (UseMd()) {
     gfx::CanvasImageSource* source =
-        new NetworkIconImageSourceMd(image_type, icon_type, index);
+        new SignalStrengthImageSource(image_type, icon_type, index);
     return gfx::ImageSkia(source, source->size());
   }
 
@@ -496,12 +525,11 @@ gfx::ImageSkia* ConnectingWirelessImage(ImageType image_type,
     images = dark ? s_arcs_images_dark : s_arcs_images_light;
   if (!images[index]) {
     // Lazily cache images.
+    // TODO(estade): should the alpha be applied in SignalStrengthImageSource?
     gfx::ImageSkia source = GetImageForIndex(image_type, icon_type, index + 1);
-    images[index] = new gfx::ImageSkia(
-        gfx::ImageSkiaOperations::CreateBlendedImage(
-            gfx::ImageSkia(new EmptyImageSource(source.size()), source.size()),
-            source,
-            kConnectingImageAlpha));
+    images[index] =
+        new gfx::ImageSkia(gfx::ImageSkiaOperations::CreateTransparentImage(
+            source, kConnectingImageAlpha));
   }
   return images[index];
 }
@@ -514,10 +542,7 @@ gfx::ImageSkia* ConnectingVpnImage(double animation) {
     ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
     gfx::ImageSkia* icon = rb.GetImageSkiaNamed(IDR_AURA_UBER_TRAY_NETWORK_VPN);
     s_vpn_images[index] = new gfx::ImageSkia(
-        gfx::ImageSkiaOperations::CreateBlendedImage(
-            gfx::ImageSkia(new EmptyImageSource(icon->size()), icon->size()),
-            *icon,
-            animation));
+        gfx::ImageSkiaOperations::CreateTransparentImage(*icon, animation));
   }
   return s_vpn_images[index];
 }
@@ -528,15 +553,10 @@ gfx::ImageSkia* ConnectingVpnBadge(double animation) {
   if (!s_vpn_badges[index]) {
     // Lazily cache images.
     ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-    gfx::ImageSkia* icon =
-        rb.GetImageSkiaNamed(IDR_AURA_UBER_TRAY_NETWORK_WIRED);  // For size
     gfx::ImageSkia* badge =
         rb.GetImageSkiaNamed(IDR_AURA_UBER_TRAY_NETWORK_VPN_BADGE);
     s_vpn_badges[index] = new gfx::ImageSkia(
-        gfx::ImageSkiaOperations::CreateBlendedImage(
-            gfx::ImageSkia(new EmptyImageSource(icon->size()), icon->size()),
-            *badge,
-            animation));
+        gfx::ImageSkiaOperations::CreateTransparentImage(*badge, animation));
   }
   return s_vpn_badges[index];
 }
@@ -600,11 +620,6 @@ const gfx::ImageSkia* BadgeForNetworkTechnology(const NetworkState* network,
     return ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(id);
 }
 
-const gfx::ImageSkia* BadgeForVPN(IconType icon_type) {
-  return ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
-      IDR_AURA_UBER_TRAY_NETWORK_VPN_BADGE);
-}
-
 gfx::ImageSkia GetIcon(const NetworkState* network,
                        IconType icon_type,
                        int strength_index) {
@@ -640,12 +655,14 @@ gfx::ImageSkia GetConnectingVpnImage(IconType icon_type) {
     gfx::ImageSkia icon = GetImageForNetwork(connected_network, icon_type);
     Badges badges;
     badges.bottom_left = ConnectingVpnBadge(animation);
-    return gfx::ImageSkia(
-        new NetworkIconImageSource(icon, badges), icon.size());
+    return UseMd() ? NetworkIconImageSourceMd::CreateImage(icon, badges)
+                   : gfx::ImageSkia(new NetworkIconImageSource(icon, badges),
+                                    icon.size());
   } else {
     gfx::ImageSkia* icon = ConnectingVpnImage(animation);
-    return gfx::ImageSkia(
-        new NetworkIconImageSource(*icon, Badges()), icon->size());
+    return UseMd() ? NetworkIconImageSourceMd::CreateImage(*icon, Badges())
+                   : gfx::ImageSkia(new NetworkIconImageSource(*icon, Badges()),
+                                    icon->size());
   }
 }
 
@@ -659,8 +676,9 @@ gfx::ImageSkia GetConnectingImage(IconType icon_type,
 
   gfx::ImageSkia* icon = ConnectingWirelessImage(
       image_type, icon_type, animation);
-  return gfx::ImageSkia(
-      new NetworkIconImageSource(*icon, Badges()), icon->size());
+  return UseMd() ? NetworkIconImageSourceMd::CreateImage(*icon, Badges())
+                 : gfx::ImageSkia(new NetworkIconImageSource(*icon, Badges()),
+                                  icon->size());
 }
 
 }  // namespace
@@ -755,7 +773,8 @@ bool NetworkIconImpl::UpdateVPNBadge() {
   const NetworkState* vpn = NetworkHandler::Get()->network_state_handler()->
       ConnectedNetworkByType(NetworkTypePattern::VPN());
   if (vpn && vpn_badge_ == NULL) {
-    vpn_badge_ = BadgeForVPN(icon_type_);
+    vpn_badge_ = ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
+        IDR_AURA_UBER_TRAY_NETWORK_VPN_BADGE);
     return true;
   } else if (!vpn && vpn_badge_ != NULL) {
     vpn_badge_ = NULL;
@@ -815,8 +834,9 @@ void NetworkIconImpl::GenerateImage(const NetworkState* network) {
   gfx::ImageSkia icon = GetIcon(network, icon_type_, strength_index_);
   Badges badges;
   GetBadges(network, &badges);
-  image_ = gfx::ImageSkia(
-      new NetworkIconImageSource(icon, badges), icon.size());
+  image_ = UseMd() ? NetworkIconImageSourceMd::CreateImage(icon, badges)
+                   : gfx::ImageSkia(new NetworkIconImageSource(icon, badges),
+                                    icon.size());
 }
 
 namespace {