[Media, UI] New default large icon in MediaNotification

[Media, UI] New default large icon in MediaNotification

Updated the default large icon.
Refined logic for determining if running on Android N.

BUG=600396
Committed: https://crrev.com/721197354d78ec06bb5dfbf0018347faa789cff9
Cr-Commit-Position: refs/heads/master@{#394214}

[Zhiqiang Zhang (Slow), 2016-05-13 13:33:08]

Patchset #1 (id:1) has been deleted

[Zhiqiang Zhang (Slow), 2016-05-13 13:40:13]

zqzhang@chromium.org changed reviewers:
+ avayvod@chromium.org, mlamouri@chromium.org, tedchoc@chromium.org

[Zhiqiang Zhang (Slow), 2016-05-13 13:40:14]

mlamouri/avayvod: Please review changes in
chrome/android/java/src/.../media/*

tedchoc: Please review changes in 
chrome/android/java/res/*
(I'm optimizing the png files right now, will upload the optimized pngs later)

[whywhat, 2016-05-13 20:10:25]

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java
(right):

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:33:
private static Bitmap sDefaultLargeIcon = null;
you already have one static variable that's initialized only once - why not make
mDefaultLargeIcon and initialize it in the CastNotificationControl ctor?

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastSessionImpl.java
(right):

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastSessionImpl.java:48:
private static Bitmap sDefaultLargeIcon = null;
Are you using this variable anywhere?

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

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:58:
private static Bitmap sDefaultLargeIcon;
Do we need this variable or should we just always set the large icon in
MediaSessionTabHelper and CastSessionImpl, CastNotificationControl? Can't we
just set it as the large icon (instead of having two - default and non-default)?

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:303:
if (sDefaultLargeIcon == null) {
nit: Do we care about loading the resource only once?
If so, this code repeats in this change three times, maybe it could be
generalized as some kind of BitmapResourceCache that keeps a map from resource
ids to loaded bitmaps? tedchoc@ might have more thoughts on this.

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

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:143:
.setDefaultLargeIcon(sDefaultLargeIcon)
I mean, can't we just setLargeIcon to sDefaultLargeIcon here if mFavicon is
null?

[Zhiqiang Zhang (Slow), 2016-05-16 15:41:10]

Patchset #3 (id:60001) has been deleted

[Zhiqiang Zhang (Slow), 2016-05-16 15:47:47]

Patchset #3 (id:80001) has been deleted

[Zhiqiang Zhang (Slow), 2016-05-16 15:51:55]

PTAL. Addressed avayvod's comments. Added BitmapResourceCache to cache the
Bitmaps.

tedchoc@, please also review:
BitmapResourceCache.java
chrome/android/java_sources.gni

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java
(right):

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:33:
private static Bitmap sDefaultLargeIcon = null;
On 2016/05/13 20:10:25, whywhat wrote:
> you already have one static variable that's initialized only once - why not
make
> mDefaultLargeIcon and initialize it in the CastNotificationControl ctor?

Done.

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastSessionImpl.java
(right):

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastSessionImpl.java:48:
private static Bitmap sDefaultLargeIcon = null;
On 2016/05/13 20:10:25, whywhat wrote:
> Are you using this variable anywhere?

Done. Added statements for setting the default large icon.

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

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:58:
private static Bitmap sDefaultLargeIcon;
On 2016/05/13 20:10:25, whywhat wrote:
> Do we need this variable or should we just always set the large icon in
> MediaSessionTabHelper and CastSessionImpl, CastNotificationControl? Can't we
> just set it as the large icon (instead of having two - default and
non-default)?

Done.

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:303:
if (sDefaultLargeIcon == null) {
On 2016/05/13 20:10:25, whywhat wrote:
> nit: Do we care about loading the resource only once?
> If so, this code repeats in this change three times, maybe it could be
> generalized as some kind of BitmapResourceCache that keeps a map from resource
> ids to loaded bitmaps? tedchoc@ might have more thoughts on this.

Done. Added BitmapesourceCache to cache the Bitmaps.

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

https://codereview.chromium.org/1978833002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:143:
.setDefaultLargeIcon(sDefaultLargeIcon)
On 2016/05/13 20:10:25, whywhat wrote:
> I mean, can't we just setLargeIcon to sDefaultLargeIcon here if mFavicon is
> null?

Done.

[whywhat, 2016-05-16 17:00:29]

lgtm with nits. looks much better now!

https://codereview.chromium.org/1978833002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java
(right):

https://codereview.chromium.org/1978833002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:87:
mNotificationBuilder.setLargeIcon((mMediaRouteController.getPoster() != null)
nit: although very short, checking poster for null repeats three times so maybe
extract it into a short local method. I also prefer if-s to ?: if it doesn't fit
all in one line.

https://codereview.chromium.org/1978833002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/1978833002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:184:
? mFavicon : BitmapResourceCache.getBitmap(R.drawable.audio_playing_square));
nit: ditto extracting a method (I'd even use it below when we're sure the
favicon is null for simplicity/consistency).

[Zhiqiang Zhang (Slow), 2016-05-16 19:14:15]

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

[Zhiqiang Zhang (Slow), 2016-05-16 19:14:16]

PTAL. Addressed nits.

I see tedchoc@ is sheriffing, so +dfalcantara@ to look at:
*.png
BitmapResourceCache.java
java_sources.gni

https://codereview.chromium.org/1978833002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java
(right):

https://codereview.chromium.org/1978833002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:87:
mNotificationBuilder.setLargeIcon((mMediaRouteController.getPoster() != null)
On 2016/05/16 17:00:29, whywhat wrote:
> nit: although very short, checking poster for null repeats three times so
maybe
> extract it into a short local method. I also prefer if-s to ?: if it doesn't
fit
> all in one line.

Done.

https://codereview.chromium.org/1978833002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/1978833002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:184:
? mFavicon : BitmapResourceCache.getBitmap(R.drawable.audio_playing_square));
On 2016/05/16 17:00:29, whywhat wrote:
> nit: ditto extracting a method (I'd even use it below when we're sure the
> favicon is null for simplicity/consistency).

Done.

[gone, 2016-05-16 19:56:41]

Instead of adding duplicated icons that differ only by background and foreground
color to the APK, you should use TintedDrawables:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

See here for an example of how to use it:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

[gone, 2016-05-16 19:57:23]

Oh, never mind you're deleting them and changing their color.

[gone, 2016-05-16 20:06:49]

I'm confused by this CL.  Is there any benefit to caching these Drawables?  I
can think of much more frequently shown UI widgets that don't cache their
Bitmaps and don't "permanently" hold onto resources that aren't being displayed.

[Zhiqiang Zhang (Slow), 2016-05-16 20:30:24]

On 2016/05/16 20:06:49, dfalcantara wrote:
> I'm confused by this CL.  Is there any benefit to caching these Drawables?  I
> can think of much more frequently shown UI widgets that don't cache their
> Bitmaps and don't "permanently" hold onto resources that aren't being
displayed.

Yes, that's a good point. If we cache the Bitmaps, they will not be garbage
collected event if we don't use it any more.

My major concern is MediaSessionTabHelper, which has one instance per tab.
MediaSessionTabHelper will need to load the Bitmap if the page plays some media.
I think loading the same Bitmap multiple times will waste CPU and memory, so I
decided to have a static variable to store the Bitmap (or having a Bitmap cache
behaving just the same).

The Bitmap is only loaded when the tab starts to play some media, so I think
it's not that frequent. I'll revert this change.

[Zhiqiang Zhang (Slow), 2016-05-16 21:03:02]

PTAL, addressed Dan's comments (reverting Bitmap Caching).

[gone, 2016-05-16 21:09:43]

https://codereview.chromium.org/1978833002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java
(right):

https://codereview.chromium.org/1978833002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:96:
return mDefaultLargeIcon;
If you only use mDefaultLargeIcon here, does it make sense to decode it at this
point instead of the constructor?  This prevents decoding and storing it at all
if it's not necessary.

https://codereview.chromium.org/1978833002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/1978833002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:317:
return mDefaultLargeIcon;
Same as above comment about mDefaultLargeIcon being decoded here instead.

[Zhiqiang Zhang (Slow), 2016-05-17 11:58:08]

PTAL.

Now I'm decoding the Bitmaps where they are needed, and not storing it as
mDefaultLargeIcon.

For MediaNotificationManager, I'm keeping mDefaultLargeIcon, but I delayed the
loading to where it's needed.

https://codereview.chromium.org/1978833002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java
(right):

https://codereview.chromium.org/1978833002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:96:
return mDefaultLargeIcon;
On 2016/05/16 21:09:42, dfalcantara wrote:
> If you only use mDefaultLargeIcon here, does it make sense to decode it at
this
> point instead of the constructor?  This prevents decoding and storing it at
all
> if it's not necessary.

Done.

https://codereview.chromium.org/1978833002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/1978833002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:317:
return mDefaultLargeIcon;
On 2016/05/16 21:09:42, dfalcantara wrote:
> Same as above comment about mDefaultLargeIcon being decoded here instead.

Done.

[Zhiqiang Zhang (Slow), 2016-05-17 11:58:28]

Patchset #6 (id:160001) has been deleted

[whywhat, 2016-05-17 12:57:18]

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

https://codereview.chromium.org/1978833002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:624:
mContext.getResources(), R.drawable.audio_playing_square);
Actually, this logic seems to belong to MediaSessionTabHelper as this drawable
is unusable for Cast notifications.

[Zhiqiang Zhang (Slow), 2016-05-17 14:02:09]

Patchset #7 (id:200001) has been deleted

[Zhiqiang Zhang (Slow), 2016-05-17 14:07:04]

PTAL. After talking to avayvod, we decided to move all the logic into
MediaNotificationManager.

All classes using MediaNotificationManager just pass the default largeIcon
resource id to MediaNotificationManager, and let it handle all the logic. Now it
loads the Bitmap when needed, but we want to keep a copy in
MediaNotificationManager since it is reused in updateNotification().

[gone, 2016-05-17 17:53:46]

https://chromiumcodereview.appspot.com/1978833002/diff/220001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://chromiumcodereview.appspot.com/1978833002/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:53:
private int mDefaultLargeIcon = -1;
mDefaultLargeIconResourceId?  Same with mIcon above.  All invalid resource IDs
should be set to 0:

https://developer.android.com/reference/android/content/res/Resources.html#ge...

https://chromiumcodereview.appspot.com/1978833002/diff/220001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://chromiumcodereview.appspot.com/1978833002/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:623:
int resourceId = (mMediaNotificationInfo.defaultLargeIcon != -1)
Put this inside the conditional.

[Zhiqiang Zhang (Slow), 2016-05-17 18:27:20]

Patchset #8 (id:240001) has been deleted

[Zhiqiang Zhang (Slow), 2016-05-17 18:27:47]

PTAL. Use 0 as invalid resource id.

https://codereview.chromium.org/1978833002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/1978833002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:53:
private int mDefaultLargeIcon = -1;
On 2016/05/17 17:53:46, dfalcantara wrote:
> mDefaultLargeIconResourceId?  Same with mIcon above.  All invalid resource IDs
> should be set to 0:
> 
>
https://developer.android.com/reference/android/content/res/Resources.html#ge...

Done.

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

https://codereview.chromium.org/1978833002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:623:
int resourceId = (mMediaNotificationInfo.defaultLargeIcon != -1)
On 2016/05/17 17:53:46, dfalcantara wrote:
> Put this inside the conditional.

Done.

[gone, 2016-05-17 18:31:42]

lgtm

[Zhiqiang Zhang (Slow), 2016-05-17 19:29:45]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-05-17 19:29:45]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1978833002/#ps260001
(title: "use 0 as invalid resource id")

[commit-bot: I haz the power, 2016-05-17 19:30:17]

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

[commit-bot: I haz the power, 2016-05-17 20:26:39]

Committed patchset #8 (id:260001)

[commit-bot: I haz the power, 2016-05-17 20:30:09]

Description was changed from

==========
[Media, UI] New default large icon in MediaNotification

Updated the default large icon.
Refined logic for determining if running on Android N.

BUG=600396
==========

to

==========
[Media, UI] New default large icon in MediaNotification

Updated the default large icon.
Refined logic for determining if running on Android N.

BUG=600396

Committed: https://crrev.com/721197354d78ec06bb5dfbf0018347faa789cff9
Cr-Commit-Position: refs/heads/master@{#394214}
==========

[commit-bot: I haz the power, 2016-05-17 20:30:10]

Patchset 8 (id:??) landed as
https://crrev.com/721197354d78ec06bb5dfbf0018347faa789cff9
Cr-Commit-Position: refs/heads/master@{#394214}