Scale large notification icons according to device settings.

Scale large notification icons according to device settings.

Previously we would directly pass developer-provided icons on to the
Android NotificationManager, but this has led to OOM errors on devices
with less memory when developers provide, supposedly, large icons.

Starting from this CL, we ensure that icons are at most the size at
which they will be presented to the user.

TBR=mvanouwerkerk (reviewed in CL 1362943002)
BUG=529980
Committed: https://crrev.com/1e5de1b0d2ba982a3be66b1f19d6a815c5fd462f
Cr-Commit-Position: refs/heads/master@{#351111}

[Peter Beverloo, 2015-09-28 17:31:58]

peter@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Peter Beverloo, 2015-09-28 17:31:58]

TBR=mvanouwerkerk

This is identical to https://codereview.chromium.org/1362943002/ with two
asserts on image size removed in the testShowNotificationWithIcon test.

This test is now nonsensical because we can't distinguish the image based on its
size anymore, and have to find a more reliable way of doing so.

Since this patch has been merged, I will update the test later.

[Peter Beverloo, 2015-09-28 17:32:23]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-09-28 17:32:25]

The CQ bit was unchecked by peter@chromium.org

[Miguel Garcia, 2015-09-28 17:47:12]

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

[Miguel Garcia, 2015-09-28 17:47:13]

lgtm % nits

https://codereview.chromium.org/1372163002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
(right):

https://codereview.chromium.org/1372163002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:97:
* Returns the current instance of the NotificationUIManager. Should only be used
by tests.
Doesn't the @VisibleForTesting annotation already imply that this is only is
supposed to be private + test visibility?

Also consider making it package private since the test will presumably live in
the same package.

https://codereview.chromium.org/1372163002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:132:

nit can you initialize density after line 137 so it matches the order in which
they are declared above?

https://codereview.chromium.org/1372163002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:134:
mLargeIconWidthPx =
I assume neither of this can ever return 0? Otherwise createScaledBitmap in #506
will explode.

https://codereview.chromium.org/1372163002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:506:
return icon.createScaledBitmap(icon, mLargeIconWidthPx, mLargeIconHeightPx,
false);
can you add /* not filtered */ right after the last parameter?

[Peter Beverloo, 2015-09-28 18:03:03]

Thank you!

https://codereview.chromium.org/1372163002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
(right):

https://codereview.chromium.org/1372163002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:97:
* Returns the current instance of the NotificationUIManager. Should only be used
by tests.
On 2015/09/28 17:47:12, Miguel Garcia wrote:
> Doesn't the @VisibleForTesting annotation already imply that this is only is
> supposed to be private + test visibility?
> 
> Also consider making it package private since the test will presumably live in
> the same package.

Done.

https://codereview.chromium.org/1372163002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:132:

On 2015/09/28 17:47:13, Miguel Garcia wrote:
> nit can you initialize density after line 137 so it matches the order in which
> they are declared above?

Done.

https://codereview.chromium.org/1372163002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:134:
mLargeIconWidthPx =
On 2015/09/28 17:47:13, Miguel Garcia wrote:
> I assume neither of this can ever return 0? Otherwise createScaledBitmap in
#506
> will explode.

Stack Overflow would have exploded already if that were the case :).

https://codereview.chromium.org/1372163002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:506:
return icon.createScaledBitmap(icon, mLargeIconWidthPx, mLargeIconHeightPx,
false);
On 2015/09/28 17:47:12, Miguel Garcia wrote:
> can you add /* not filtered */ right after the last parameter?

Done.

[Peter Beverloo, 2015-09-28 18:03:20]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-09-28 18:03:20]

The patchset sent to the CQ was uploaded after l-g-t-m from miguelg@chromium.org
Link to the patchset: https://codereview.chromium.org/1372163002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-09-28 18:07:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372163002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372163002/20001

[commit-bot: I haz the power, 2015-09-28 19:29:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-28 19:31:06]

Patchset 2 (id:??) landed as
https://crrev.com/1e5de1b0d2ba982a3be66b1f19d6a815c5fd462f
Cr-Commit-Position: refs/heads/master@{#351111}