Change PowerStatus::GetBatteryImage to not use ExtractImageRep and scale

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

Index: ash/common/system/chromeos/power/tray_power.cc
diff --git a/ash/common/system/chromeos/power/tray_power.cc b/ash/common/system/chromeos/power/tray_power.cc
index 8e694e15c41542f9c14ca7a21cdbd7323a2d7ac2..f280906a0a9a981358013bcbcbe0e05931e7b78b 100644
--- a/ash/common/system/chromeos/power/tray_power.cc
+++ b/ash/common/system/chromeos/power/tray_power.cc
@@ -25,6 +25,7 @@
 #include "base/time/time.h"
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/image/image_skia_source.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/notification.h"
 #include "ui/message_center/notification_delegate.h"
@@ -90,6 +91,22 @@ void LogBatteryForNoCharger(TrayPower::NotificationState state,
 
 namespace tray {
 
+class PowerTrayImageSource : public gfx::ImageSkiaSource {

[Peter Kasting, 2017/03/25 00:22:30]

Nit: I'm sorta inclined to declare this class inside the private section of
PowerTrayView.

[danakj, 2017/03/31 20:49:04]

More nesting :/ and it's in the .cc anyways. But done.

+ public:
+  PowerTrayImageSource(const PowerStatus::BatteryImageInfo& info)

[Peter Kasting, 2017/03/25 00:22:30]

Nit: explicit

Maybe also pass by value and move into the member?

[danakj, 2017/03/31 20:49:04]

Done.
I don't think that's right here. That'd require two shallow copies which isn't
better than one deep copy here (shallow == deep copy here).

[Peter Kasting, 2017/03/31 22:07:20]

Wouldn't these both be at worst one deep copy?  In the copy-then-move route
you'd get the copy at the callsite, here you'd get it in the initializer list. 
But with the copy-then-move route the compiler can sometimes optimize the first
case better.

At least I think that's what would happen?  Please explain more deeply if I'm
mistaken, since I feel like my understanding of best practices for copy and move
semantics around constructors is poor.  (I consulted stackoverflow a bunch
before writing the original suggestion.)

[danakj, 2017/04/03 14:43:13]

If we pass by const&
- the caller doesn't invoke any constructors if it has an lvalue.
- if the caller has an rvalue, it will construct a temporary to pass to here,
invoking a copy constructor.
- inside this method, the assignment will invoke the copy constructor

If we pass by value
- the caller will invoke a copy constructor (or move constructor if it's an
rvalue (either natively or casted by std::move()). in this case both
constructors are the same.
- the assignment will invoke a copy constructor (or move constructor if it's an
rvalue, either natively or casted by std::move().
- assume the caller and assignment use std::move() then, we'd use 2 move
constructors, which are the same as the copy constructor here.

The optimization you're thinking of is when u have a native rvalue, I think. It
allows the move constructor to be used, whereas the const& requires copy
constructors. Since they are the exact same constructor here, it's not a good
trade off because we force 2 move constructors instead of 1-2 copy constructors.
When the move constructor is cheaper, or required, then pass-by-value is better.

I think everything I wrote above is accurate but I might have something wrong
too. Does this agree with what you read?

[Peter Kasting, 2017/04/03 20:32:51]

It sounds like pass-by-value + move-into-place is either 2 moves or 1 move + 1
copy (depending on caller), while pass-by-const-ref is 1-2 copies plus possibly
forcing the arg to the stack (you didn't mention this but I think it's true?). 
If move and copy are the same cost, then pass-by-const-ref is usually better?

I thought in some cases the compiler could optimize move constructors away
entirely, though, if it was able to move/copy the argument directly into the
right place.  That's the case where pass-by-value might be a win.

Anyway, it definitely doesn't matter here, so I'm happy whichever way.  I'm more
trying to figure this out so I know what best practices are for future code.

+      : info_(info) {}
+
+  // gfx::ImageSkiaSource implementation.
+  gfx::ImageSkiaRep GetImageForScale(float scale) override {
+    return PowerStatus::Get()->GetBatteryImage(info_, scale);
+  }
+
+  const PowerStatus::BatteryImageInfo& info() const { return info_; }
+
+ private:
+  PowerStatus::BatteryImageInfo info_;
+};

[Peter Kasting, 2017/03/25 00:22:30]

Nit: DISALLOW_COPY_AND_ASSIGN?

[danakj, 2017/03/31 20:49:04]

Done.

+
 // This view is used only for the tray.
 class PowerTrayView : public TrayItemView {
  public:
@@ -100,7 +117,7 @@ class PowerTrayView : public TrayItemView {
 
   ~PowerTrayView() override {}
 
-  // Overriden from views::View.
+  // Overridden from views::View.
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
     node_data->SetName(accessible_name_);
     node_data->role = ui::AX_ROLE_BUTTON;
@@ -118,19 +135,20 @@ class PowerTrayView : public TrayItemView {
 
  private:
   void UpdateImage() {
-    const PowerStatus::BatteryImageInfo info =
+    const PowerStatus::BatteryImageInfo& info =
         PowerStatus::Get()->GetBatteryImageInfo(PowerStatus::ICON_LIGHT);
-    if (info != previous_image_info_) {
-      image_view()->SetImage(PowerStatus::Get()->GetBatteryImage(info));
-      previous_image_info_ = info;
-    }
+    // Only change the image when the info changes. http://crbug.com/589348
+    if (image_source_ && image_source_->info() == info)
+      return;
+    image_source_ = new PowerTrayImageSource(info);
+    // gfx::ImageSkia takes ownership of image_source_. It will stay alive until

[Peter Kasting, 2017/03/25 00:22:30]

Blargh, it'd sure be nice if it took a unique_ptr to obviate the need for a
comment like this.

[danakj, 2017/03/31 20:49:04]

Yeeeeep. I also had a crash until I realized this cuz I was deleting it twice.

+    // we call SetImage() again, which only happens here.
+    image_view()->SetImage(
+        gfx::ImageSkia(image_source_, PowerStatus::GetBatteryImageSizeInDip()));
   }
 
   base::string16 accessible_name_;
-
-  // Information about the last-used image. Cached to avoid unnecessary updates
-  // (http://crbug.com/589348).
-  PowerStatus::BatteryImageInfo previous_image_info_;
+  PowerTrayImageSource* image_source_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(PowerTrayView);
 };