[Media>UI] Don't scale the icon if it's smaller than the ideal size

[Media>UI] Don't scale the icon if it's smaller than the ideal size

Previously we scale favicons or media artwork images to the ideal
size regardless of their original size. This increases the memory
usage as we create another copy of the original image and the
size will also be increased if the original image size is smaller
than the ideal size.

In this CL, if the original image size is smaller than the ideal
size, we will reuse the original image and won't scale it
anymore.

BUG=682701
Review-Url: https://codereview.chromium.org/2641133004
Cr-Commit-Position: refs/heads/master@{#445147}
Committed: https://chromium.googlesource.com/chromium/src/+/c3a579962e733b26ef12f564732260844d558a56

[Zhiqiang Zhang (Slow), 2017-01-20 13:58:23]

zqzhang@chromium.org changed reviewers:
+ mlamouri@chromium.org

[whywhat, 2017-01-20 15:43:22]

avayvod@chromium.org changed reviewers:
+ avayvod@chromium.org

[whywhat, 2017-01-20 15:43:23]

I'm a little confused by the change description. It indicates two problems:

1. we scale some favicons up to the ideal size increasing the memory usage
2. we scale the images even if we're not playing

For me the second issue is higher priority - we shouldn't do extra work until
there's a notification. If we fixed that, would scaling favicons up to the ideal
size be justified?
Agnostic to that, should we try to find a balance and don't scale the favicon up
if it's more than X% smaller than the ideal size (if it's close to the ideal
size, maybe the memory increase is not that big? although we might argue that
such icons look good as they are...)

https://codereview.chromium.org/2641133004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/2641133004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:492:
public static Bitmap scaleIconForDisplayIfNeeded(@Nullable Bitmap icon) {
nit: I think you could keep it as scaleIconToIdealSizeIfNeeded. Having display
in the name might add unnecessary context to the method.

[whywhat, 2017-01-20 15:44:12]

https://codereview.chromium.org/2641133004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/2641133004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:492:
public static Bitmap scaleIconForDisplayIfNeeded(@Nullable Bitmap icon) {
On 2017/01/20 at 15:43:23, whywhat wrote:
> nit: I think you could keep it as scaleIconToIdealSizeIfNeeded. Having display
in the name might add unnecessary context to the method.

Or even rename to downscaleToIdealSize since you don't upscale anymore, no?

[Zhiqiang Zhang (Slow), 2017-01-20 16:42:55]

Description was changed from

==========
[Media>UI] Don't scale the icon if it's smaller than the ideal size

Previously, we scale the icons to the ideal size regardless to the
original size. Even if websites don't play media, we scale its favicon
to the ideal size which is a waste of memory. In this CL the icons will
not be scaled if they are smaller than the ideal size, and original icon
will be returned.

BUG=682701
==========

to

==========
[Media>UI] Don't scale the icon if it's smaller than the ideal size

Previously we scale favicons or media artwork images to the ideal
size regardless of their original size. This increases the memory
usage as we create another copy of the original image and the
size will also be increased if the original image size is smaller
than the ideal size.

In this CL, if the original image size is smaller than the ideal
size, we will reuse the original image and won't scale it
anymore.

BUG=682701
==========

[Zhiqiang Zhang (Slow), 2017-01-20 16:53:35]

PTAL, changed method name and updated the CL description.

For Favicon, I think we are the only client that needs to store it. It's OK to
create a downscaled copy and abandon the original one. Also it should be rare
that favicons are larger than 256.

https://codereview.chromium.org/2641133004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/2641133004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:492:
public static Bitmap scaleIconForDisplayIfNeeded(@Nullable Bitmap icon) {
On 2017/01/20 15:44:12, whywhat wrote:
> On 2017/01/20 at 15:43:23, whywhat wrote:
> > nit: I think you could keep it as scaleIconToIdealSizeIfNeeded. Having
display
> in the name might add unnecessary context to the method.
> 
> Or even rename to downscaleToIdealSize since you don't upscale anymore, no?

Done.

[Zhiqiang Zhang (Slow), 2017-01-20 17:02:39]

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

[commit-bot: I haz the power, 2017-01-20 17:02:54]

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

[whywhat, 2017-01-20 17:25:53]

lgtm

[commit-bot: I haz the power, 2017-01-20 17:38:12]

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

[commit-bot: I haz the power, 2017-01-20 17:38:14]

Dry run: This issue passed the CQ dry run.

[Zhiqiang Zhang (Slow), 2017-01-20 17:55:00]

The CQ bit was checked by zqzhang@chromium.org

[commit-bot: I haz the power, 2017-01-20 17:55:13]

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-01-20 17:56:52]

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

[commit-bot: I haz the power, 2017-01-20 17:56:53]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Zhiqiang Zhang (Slow), 2017-01-20 20:25:48]

The CQ bit was checked by zqzhang@chromium.org

[commit-bot: I haz the power, 2017-01-20 20:26:31]

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-01-20 20:31:02]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484943948920550,
"parent_rev": "f1cbfff18b19ff99310ffbfb94ca75462da7a859", "commit_rev":
"c3a579962e733b26ef12f564732260844d558a56"}

[commit-bot: I haz the power, 2017-01-20 20:33:38]

Description was changed from

==========
[Media>UI] Don't scale the icon if it's smaller than the ideal size

Previously we scale favicons or media artwork images to the ideal
size regardless of their original size. This increases the memory
usage as we create another copy of the original image and the
size will also be increased if the original image size is smaller
than the ideal size.

In this CL, if the original image size is smaller than the ideal
size, we will reuse the original image and won't scale it
anymore.

BUG=682701
==========

to

==========
[Media>UI] Don't scale the icon if it's smaller than the ideal size

Previously we scale favicons or media artwork images to the ideal
size regardless of their original size. This increases the memory
usage as we create another copy of the original image and the
size will also be increased if the original image size is smaller
than the ideal size.

In this CL, if the original image size is smaller than the ideal
size, we will reuse the original image and won't scale it
anymore.

BUG=682701

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

[commit-bot: I haz the power, 2017-01-20 20:33:41]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/c3a579962e733b26ef12f5647322...