Change PowerStatus::GetBatteryImage to not use ExtractImageRep and scale

Change PowerStatus::GetBatteryImage to not use ExtractImageRep and scale

This changes PowerStatus::GetBatteryImage to take a scale parameter, and
to use GetBitmap() instead of ExtractImageRep() which copies pixels out
of the gfx::Canvas, since it uses a constructor that builds a bitmap.

In order to raster the correct scale, this changes TrayPower to make an
ImageSkiaSource instead of generating a bitmap off a fixed device
scale factor.

In the process, the return type had to change from ImageSkia to
ImageSkiaRep, so rather than updating the dead non-material design
path, I've removed that code.

R=estade@chromium.org, pkasting@chromium.org
BUG=671433, 614453
Review-Url: https://codereview.chromium.org/2774093002
Cr-Commit-Position: refs/heads/master@{#461484}
Committed: https://chromium.googlesource.com/chromium/src/+/df0952b6d946aeddee245b2085acba1a0e7ba654

[danakj, 2017-03-24 23:56:29]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 23:57:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-03-24 23:57:10]

Description was changed from

==========
Change PowerStatus::GetBatteryImage to not use ExtractImageRep and scale

This changes PowerStatus::GetBatteryImage to take a scale parameter, and
to use GetBitmap() instead of ExtractImageRep() which copies pixels out
of the gfx::Canvas, since it uses a constructor that builds a bitmap.

In order to raster the correct scale, this changes TrayPower to make an
ImageSkiaSource instead of generating a bitmap off a fixed device
scale factor.

R=estade@chromium.org, pkasting@chromium.org
BUG=671433
==========

to

==========
Change PowerStatus::GetBatteryImage to not use ExtractImageRep and scale

This changes PowerStatus::GetBatteryImage to take a scale parameter, and
to use GetBitmap() instead of ExtractImageRep() which copies pixels out
of the gfx::Canvas, since it uses a constructor that builds a bitmap.

In order to raster the correct scale, this changes TrayPower to make an
ImageSkiaSource instead of generating a bitmap off a fixed device
scale factor.

In the process, the return type had to change from ImageSkia to
ImageSkiaRep, so rather than updating the dead non-material design
path, I've removed that code.

R=estade@chromium.org, pkasting@chromium.org
BUG=671433
==========

[danakj, 2017-03-24 23:57:25]

Description was changed from

==========
Change PowerStatus::GetBatteryImage to not use ExtractImageRep and scale

This changes PowerStatus::GetBatteryImage to take a scale parameter, and
to use GetBitmap() instead of ExtractImageRep() which copies pixels out
of the gfx::Canvas, since it uses a constructor that builds a bitmap.

In order to raster the correct scale, this changes TrayPower to make an
ImageSkiaSource instead of generating a bitmap off a fixed device
scale factor.

In the process, the return type had to change from ImageSkia to
ImageSkiaRep, so rather than updating the dead non-material design
path, I've removed that code.

R=estade@chromium.org, pkasting@chromium.org
BUG=671433
==========

to

==========
Change PowerStatus::GetBatteryImage to not use ExtractImageRep and scale

This changes PowerStatus::GetBatteryImage to take a scale parameter, and
to use GetBitmap() instead of ExtractImageRep() which copies pixels out
of the gfx::Canvas, since it uses a constructor that builds a bitmap.

In order to raster the correct scale, this changes TrayPower to make an
ImageSkiaSource instead of generating a bitmap off a fixed device
scale factor.

In the process, the return type had to change from ImageSkia to
ImageSkiaRep, so rather than updating the dead non-material design
path, I've removed that code.

R=estade@chromium.org, pkasting@chromium.org
BUG=671433, 614453
==========

[danakj, 2017-03-24 23:59:18]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 23:59:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-03-24 23:59:41]

Patchset #1 (id:1) has been deleted

[Peter Kasting, 2017-03-25 00:22:31]

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

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:441: info.index *
kBatteryImageHeight, kBatteryImageWidth,
With this removal, |resource_id|, |offset|, and |index| are dead, so they should
be removed + all code transitively touching them.

If this also removes the last references to some bitmap resources, make sure to
remove those too.

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

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:393: return
gfx::Size(kBatteryCanvasSizeMd, kBatteryCanvasSizeMd);
Nit: Now that there is no non-MD path, remove "Md" from this constant.

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status_view.cc (left):

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status_view.cc:62:
PowerStatus::Get()->GetBatteryImageInfo(PowerStatus::ICON_DARK);
This was the only non-test use of ICON_DARK.  Based on that and the comment
above, looks like its entire enum should go away and code should assume the
equivalent of ICON_LIGHT everywhere.

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

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/tray_power.cc:94: class PowerTrayImageSource :
public gfx::ImageSkiaSource {
Nit: I'm sorta inclined to declare this class inside the private section of
PowerTrayView.

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/tray_power.cc:96: PowerTrayImageSource(const
PowerStatus::BatteryImageInfo& info)
Nit: explicit

Maybe also pass by value and move into the member?

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/tray_power.cc:108: };
Nit: DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/tray_power.cc:144: // gfx::ImageSkia takes
ownership of image_source_. It will stay alive until
Blargh, it'd sure be nice if it took a unique_ptr to obviate the need for a
comment like this.

[commit-bot: I haz the power, 2017-03-25 00:35:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-25 00:35:01]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-03-27 15:11:23]

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

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status.cc:441: info.index *
kBatteryImageHeight, kBatteryImageWidth,
On 2017/03/25 00:22:30, Peter Kasting wrote:
> With this removal, |resource_id|, |offset|, and |index| are dead, so they
should
> be removed + all code transitively touching them.
> 
> If this also removes the last references to some bitmap resources, make sure
to
> remove those too.

that's all done here: https://codereview.chromium.org/2770953003/

Dana, you might want to wait till that one lands and rebase, but there are
changes here that will still be relevant.

[danakj, 2017-03-31 20:49:05]

PTAL

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status_view.cc (left):

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status_view.cc:62:
PowerStatus::Get()->GetBatteryImageInfo(PowerStatus::ICON_DARK);
On 2017/03/25 00:22:30, Peter Kasting wrote:
> This was the only non-test use of ICON_DARK.  Based on that and the comment
> above, looks like its entire enum should go away and code should assume the
> equivalent of ICON_LIGHT everywhere.

Done.

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

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/tray_power.cc:94: class PowerTrayImageSource :
public gfx::ImageSkiaSource {
On 2017/03/25 00:22:30, Peter Kasting wrote:
> Nit: I'm sorta inclined to declare this class inside the private section of
> PowerTrayView.

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

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/tray_power.cc:96: PowerTrayImageSource(const
PowerStatus::BatteryImageInfo& info)
On 2017/03/25 00:22:30, Peter Kasting wrote:
> Nit: explicit

Done.

> Maybe also pass by value and move into the member?

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).

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/tray_power.cc:108: };
On 2017/03/25 00:22:30, Peter Kasting wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/tray_power.cc:144: // gfx::ImageSkia takes
ownership of image_source_. It will stay alive until
On 2017/03/25 00:22:30, Peter Kasting wrote:
> Blargh, it'd sure be nice if it took a unique_ptr to obviate the need for a
> comment like this.

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

[danakj, 2017-03-31 20:49:10]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 20:49:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 21:35:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 21:35:29]

Dry run: This issue passed the CQ dry run.

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

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

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/tray_power.cc:96: PowerTrayImageSource(const
PowerStatus::BatteryImageInfo& info)
On 2017/03/31 20:49:04, danakj wrote:
> On 2017/03/25 00:22:30, Peter Kasting wrote:
> > Nit: explicit
> 
> Done.
> 
> > Maybe also pass by value and move into the member?
> 
> 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).

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.)

https://codereview.chromium.org/2774093002/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status_view.cc (left):

https://codereview.chromium.org/2774093002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status_view.cc:63: if (info !=
previous_battery_image_info_) {
Looks like this member variable is now dead and should be removed.

https://codereview.chromium.org/2774093002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status_view.cc:64:
icon_->SetImage(PowerStatus::Get()->GetBatteryImage(info));
Looks like we no longer need to track this pointer as a member variable, since
it's never read; we can just AddChildView() directly in the places that
currently set it.

[Peter Kasting, 2017-04-01 05:55:41]

LGTM BTW, I trust you to resolve the above issues in some appropriate way

[danakj, 2017-04-03 14:43:14]

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

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/tray_power.cc:96: PowerTrayImageSource(const
PowerStatus::BatteryImageInfo& info)
On 2017/03/31 22:07:20, Peter Kasting wrote:
> On 2017/03/31 20:49:04, danakj wrote:
> > On 2017/03/25 00:22:30, Peter Kasting wrote:
> > > Nit: explicit
> > 
> > Done.
> > 
> > > Maybe also pass by value and move into the member?
> > 
> > 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).
> 
> 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.)

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?

https://codereview.chromium.org/2774093002/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/power/power_status_view.cc (left):

https://codereview.chromium.org/2774093002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status_view.cc:63: if (info !=
previous_battery_image_info_) {
On 2017/03/31 22:07:20, Peter Kasting wrote:
> Looks like this member variable is now dead and should be removed.

Done.

https://codereview.chromium.org/2774093002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/power/power_status_view.cc:64:
icon_->SetImage(PowerStatus::Get()->GetBatteryImage(info));
On 2017/03/31 22:07:20, Peter Kasting wrote:
> Looks like we no longer need to track this pointer as a member variable, since
> it's never read; we can just AddChildView() directly in the places that
> currently set it.

Done.

[danakj, 2017-04-03 14:43:46]

danakj@chromium.org changed reviewers:
+ oshima@chromium.org

[danakj, 2017-04-03 14:43:47]

+oshima for OWNERS

[danakj, 2017-04-03 14:44:23]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 14:44:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 15:05:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 15:05:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oshima, 2017-04-03 17:45:36]

lgtm

[danakj, 2017-04-03 17:52:38]

The CQ bit was checked by danakj@chromium.org

[danakj, 2017-04-03 17:52:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2774093002/#ps80001
(title: "powericon: removedeadunittestcode")

[commit-bot: I haz the power, 2017-04-03 17:53:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 18:38:43]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491241958936790,
"parent_rev": "56a8a01d454f2a1fe3dbe86c931bc513d88c7327", "commit_rev":
"df0952b6d946aeddee245b2085acba1a0e7ba654"}

[commit-bot: I haz the power, 2017-04-03 18:39:36]

Description was changed from

==========
Change PowerStatus::GetBatteryImage to not use ExtractImageRep and scale

This changes PowerStatus::GetBatteryImage to take a scale parameter, and
to use GetBitmap() instead of ExtractImageRep() which copies pixels out
of the gfx::Canvas, since it uses a constructor that builds a bitmap.

In order to raster the correct scale, this changes TrayPower to make an
ImageSkiaSource instead of generating a bitmap off a fixed device
scale factor.

In the process, the return type had to change from ImageSkia to
ImageSkiaRep, so rather than updating the dead non-material design
path, I've removed that code.

R=estade@chromium.org, pkasting@chromium.org
BUG=671433, 614453
==========

to

==========
Change PowerStatus::GetBatteryImage to not use ExtractImageRep and scale

This changes PowerStatus::GetBatteryImage to take a scale parameter, and
to use GetBitmap() instead of ExtractImageRep() which copies pixels out
of the gfx::Canvas, since it uses a constructor that builds a bitmap.

In order to raster the correct scale, this changes TrayPower to make an
ImageSkiaSource instead of generating a bitmap off a fixed device
scale factor.

In the process, the return type had to change from ImageSkia to
ImageSkiaRep, so rather than updating the dead non-material design
path, I've removed that code.

R=estade@chromium.org, pkasting@chromium.org
BUG=671433, 614453

Review-Url: https://codereview.chromium.org/2774093002
Cr-Commit-Position: refs/heads/master@{#461484}
Committed:
https://chromium.googlesource.com/chromium/src/+/df0952b6d946aeddee245b2085ac...
==========

[commit-bot: I haz the power, 2017-04-03 18:39:37]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/df0952b6d946aeddee245b2085ac...

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

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

https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/power/tray_power.cc:96: PowerTrayImageSource(const
PowerStatus::BatteryImageInfo& info)
On 2017/04/03 14:43:13, danakj wrote:
> I think everything I wrote above is accurate but I might have something wrong
> too. Does this agree with what you read?

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.

[danakj, 2017-04-03 21:30:55]

On 2017/04/03 20:32:51, Peter Kasting wrote:
>
https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
> File ash/common/system/chromeos/power/tray_power.cc (right):
> 
>
https://codereview.chromium.org/2774093002/diff/20001/ash/common/system/chrom...
> ash/common/system/chromeos/power/tray_power.cc:96: PowerTrayImageSource(const
> PowerStatus::BatteryImageInfo& info)
> On 2017/04/03 14:43:13, danakj wrote:
> > I think everything I wrote above is accurate but I might have something
wrong
> > too. Does this agree with what you read?
> 
> 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.

Oh this sounds like RVO, I guess that can apply when the item is a native
rvalue.

CallingThing(ThisIsRVOYay());

That would require no copy or move from the caller, and just a move inside the
method to assign to the member. So that'd be a case where pass-by-value wins
clearly.

Whereas a const& would need an lvalue, so it would destroy RVO. I think that's
what I was thinking of when I mentioned native rvalues, without realizing I was
talking about RVO.

So pass-by-value is:
- 2 moves if you had an lvalue and casted
- 1 copy + 1 move if you had an lvalue and didn't cast
- 1 move if you had an rvalue and RVO worked
- 2 moves if you had an rvalue and RVO didn't work

And pass-by-constref is:
- 1 copy if you had an lvalue
- 2 copies if you had a rvalue cuz you can't RVO

So by-value optimizes for rvalues but hurts lvalues, and by-constref optimizes
for
lvalues but hurts rvalues... but by-value takes advantage of move being cheaper
when relevant to the type. To me then, the easy rule here is const-ref unless
move is cheaper than copy (or required) for the type. This is consistent with
our
historical practices, and the only time by-value is better (when move == copy)
is
when you had an rvalue and you're getting RVO assistance.

> 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.