Scaling large notification icons for MediaNotification

Scaling large notification icons for MediaNotification

Previously MediaNotification large icons are passed to Android
NotificationManager without any scaling, which may hit OOMs when the
icons are too large. This CL scales the icons and probably solves the
OOM issue.

BUG=601912
Committed: https://crrev.com/ca81e7b91523f6c67508def3db471c3b68d1b8b2
Cr-Commit-Position: refs/heads/master@{#417257}

[Zhiqiang Zhang (Slow), 2016-09-05 16:00:35]

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

[mlamouri (slow - plz ping), 2016-09-06 10:41:07]

https://codereview.chromium.org/2313753002/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/2313753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:368:
android.R.dimen.notification_large_icon_height);
I'm not entirely sure this is correct. Based on the design doc, it seems that we
have multiple max sizes:
https://docs.google.com/document/d/11Db8idKH9zVfyy8YsPcWVZ0UQkIHx64nGPKke5Voj...

On N, max would be 94dp, see media_notification_expanded_image_max_size in
https://android.googlesource.com/platform/frameworks/base/+/master/core/res/r...

Before N, max would be 128dp (using
notification_template_material_big_media_narrow.xml style).

This might not be correct but I think for sure the media notification might not
match the notification icon size.

[Zhiqiang Zhang (Slow), 2016-09-06 12:56:00]

PTAL

https://codereview.chromium.org/2313753002/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/2313753002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:368:
android.R.dimen.notification_large_icon_height);
On 2016/09/06 10:41:07, mlamouri (slow) wrote:
> I'm not entirely sure this is correct. Based on the design doc, it seems that
we
> have multiple max sizes:
>
https://docs.google.com/document/d/11Db8idKH9zVfyy8YsPcWVZ0UQkIHx64nGPKke5Voj...
> 
> On N, max would be 94dp, see media_notification_expanded_image_max_size in
>
https://android.googlesource.com/platform/frameworks/base/+/master/core/res/r...
> 
> Before N, max would be 128dp (using
> notification_template_material_big_media_narrow.xml style).
> 
> This might not be correct but I think for sure the media notification might
not
> match the notification icon size.

Thanks for catching this. The code was pretty much the same as the push
notifications, but it should be different for MediaStyle notifications.

Now using the correct way to compute the size.

[mlamouri (slow - plz ping), 2016-09-07 08:51:25]

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

https://codereview.chromium.org/2313753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:50:
private static final int PRE_N_LARGE_ICON_SIZE_DP = 128;
I would prefer if we could have a way of not hard coding this but I'm not 100%
sure where to find this. The two layouts are
notification_template_material_big_media_narrow.xml and
notification_template_material_big_media.xml. Looking at the files again, I'm
not 100% what are the sizes used for the icons. I wonder if it is
notification_large_icon_width or 128dp. Can you double-check?

https://codereview.chromium.org/2313753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:52:
private static final int N_LARGE_ICON_SIZE_DP = 94;
I think you should use `media_notification_expanded_image_max_size` on N because
it's more flexible than hard coding the value. It's not define before N though.

https://codereview.chromium.org/2313753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:410:
// TODO(zqzhang): update this when N is released.
I think N is released :)

You should be able to use Build.VERSION_CODES.N, see:
https://developer.android.com/reference/android/os/Build.VERSION_CODES.html#N

[Zhiqiang Zhang (Slow), 2016-09-07 10:13:26]

PTAL

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

https://codereview.chromium.org/2313753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:50:
private static final int PRE_N_LARGE_ICON_SIZE_DP = 128;
On 2016/09/07 08:51:25, mlamouri (slow) wrote:
> I would prefer if we could have a way of not hard coding this but I'm not 100%
> sure where to find this. The two layouts are
> notification_template_material_big_media_narrow.xml and
> notification_template_material_big_media.xml. Looking at the files again, I'm
> not 100% what are the sizes used for the icons. I wonder if it is
> notification_large_icon_width or 128dp. Can you double-check?

OK, both the layouts could be used.
notification_template_material_big_media_narrow.xml is used when there are at
most 3 buttons. Otherwise notification_template_material_big_media.xml is used.
The former one has 128dp icon size, while the later one has much smaller icon.
The size 128dp is hard-coded in the xml so I guess I have to hard code here.

https://codereview.chromium.org/2313753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:52:
private static final int N_LARGE_ICON_SIZE_DP = 94;
On 2016/09/07 08:51:25, mlamouri (slow) wrote:
> I think you should use `media_notification_expanded_image_max_size` on N
because
> it's more flexible than hard coding the value. It's not define before N
though.

We haven't yet rolled SDK to level 24, see https://crbug.com/623989
Added TODO

https://codereview.chromium.org/2313753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:410:
// TODO(zqzhang): update this when N is released.
On 2016/09/07 08:51:25, mlamouri (slow) wrote:
> I think N is released :)
> 
> You should be able to use Build.VERSION_CODES.N, see:
> https://developer.android.com/reference/android/os/Build.VERSION_CODES.html#N

We haven't yet rolled SDK to level 24, see https://crbug.com/623989.
Added TODO

[mlamouri (slow - plz ping), 2016-09-08 10:04:36]

lgtm

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

https://codereview.chromium.org/2313753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:53:
//SDK is rolled to level 24.
style: add space after //

https://codereview.chromium.org/2313753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:366:
* TODO(zqzhang): Maybe move to a some common place for utilities?
Do we still need this TODO? it's quite Media related. Not the worse place.

https://codereview.chromium.org/2313753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:412:
// TODO(zqzhang): Use Build.VERSION_CODES.N when Android SDK is rolled to level
24.
Add bug #

[Zhiqiang Zhang (Slow), 2016-09-08 10:36:08]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-09-08 10:36:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2313753002/#ps60001
(title: "addressed Mounir's comments")

[commit-bot: I haz the power, 2016-09-08 10:36: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, 2016-09-08 11:39:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-08 11:41:54]

Description was changed from

==========
Scaling large notification icons for MediaNotification

Previously MediaNotification large icons are passed to Android
NotificationManager without any scaling, which may hit OOMs when the
icons are too large. This CL scales the icons and probably solves the
OOM issue.

BUG=601912
==========

to

==========
Scaling large notification icons for MediaNotification

Previously MediaNotification large icons are passed to Android
NotificationManager without any scaling, which may hit OOMs when the
icons are too large. This CL scales the icons and probably solves the
OOM issue.

BUG=601912

Committed: https://crrev.com/ca81e7b91523f6c67508def3db471c3b68d1b8b2
Cr-Commit-Position: refs/heads/master@{#417257}
==========

[commit-bot: I haz the power, 2016-09-08 11:41:55]

Patchset 4 (id:??) landed as
https://crrev.com/ca81e7b91523f6c67508def3db471c3b68d1b8b2
Cr-Commit-Position: refs/heads/master@{#417257}