Change PowerStatus::GetBatteryImage to not use ExtractImageRep and scale

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/common/system/chromeos/power/tray_power.h"
 
 #include <utility>
 
 #include "ash/common/accessibility_delegate.h"
 #include "ash/common/ash_switches.h"
 #include "ash/common/system/chromeos/devicetype_utils.h"
 #include "ash/common/system/chromeos/power/battery_notification.h"
 #include "ash/common/system/chromeos/power/dual_role_notification.h"
 #include "ash/common/system/date/date_view.h"
 #include "ash/common/system/system_notifier.h"
 #include "ash/common/system/tray/system_tray_delegate.h"
 #include "ash/common/system/tray/tray_constants.h"
 #include "ash/common/system/tray/tray_item_view.h"
 #include "ash/common/system/tray/tray_utils.h"
 #include "ash/resources/grit/ash_resources.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #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"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/view.h"
 
 using message_center::MessageCenter;
 using message_center::Notification;
 
 namespace ash {
@@ skipping 45 matching lines @@
                             int remaining_minutes) {
   LOG(WARNING) << "Showing " << GetNotificationStateString(state)
                << " notification. No charger connected."
                << " Remaining time: " << remaining_minutes << " minutes.";
 }
 
 }  // namespace
 
 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:
   explicit PowerTrayView(SystemTrayItem* owner) : TrayItemView(owner) {
     CreateImageView();
     UpdateImage();
   }
 
   ~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;
   }
 
   void UpdateStatus(bool battery_alert) {
     UpdateImage();
     SetVisible(PowerStatus::Get()->IsBatteryPresent());
 
     if (battery_alert) {
       accessible_name_ = PowerStatus::Get()->GetAccessibleNameString(true);
       NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true);
     }
   }
 
  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);
 };
 
 }  // namespace tray
 
 const int TrayPower::kCriticalMinutes = 5;
 const int TrayPower::kLowPowerMinutes = 15;
 const int TrayPower::kNoWarningMinutes = 30;
 const int TrayPower::kCriticalPercentage = 5;
@@ skipping 202 matching lines @@
   }
   NOTREACHED();
   return false;
 }
 
 void TrayPower::NotifyUsbNotificationClosedByUser() {
   usb_notification_dismissed_ = true;
 }
 
 }  // namespace ash