[Media, UI] Change MediaNotification style to MediaStyle

[Media, UI] Change MediaNotification style to MediaStyle

This patch changes the MediaNotification style from a custom layout into
the MediaStyle using NotificationCompat.

BUG=599937
Committed: https://crrev.com/6fe77672d634a7b0adb11d2c11f166be282da012
Cr-Commit-Position: refs/heads/master@{#387302}

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

Description was changed from

==========
[Media, UI] Change MediaNotification style to MediaStyle

This patch changes the MediaNotification style from a custom layout into
the MediaStyle using NotificationCompat.

BUG=
==========

to

==========
[Media, UI] Change MediaNotification style to MediaStyle

This patch changes the MediaNotification style from a custom layout into
the MediaStyle using NotificationCompat.

BUG=599937
==========

[Zhiqiang Zhang (Slow), 2016-04-04 14:10:27]

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

[Zhiqiang Zhang (Slow), 2016-04-04 14:10:28]

I worked this patch based on Mounir's prototype. Made some changes about getting
the favicon.

[mlamouri (slow - plz ping), 2016-04-06 09:16:50]

On 2016/04/04 at 14:10:28, zqzhang wrote:
> I worked this patch based on Mounir's prototype. Made some changes about
getting the favicon.

Zhiqiang, could you evaluate how complex it would be to have the feature behind
a finch experiment?

[Zhiqiang Zhang (Slow), 2016-04-06 16:43:38]

PTAL, made the new notification behind a flag, which is disabled by default.

I'm worried about the NotificationTitleUpdatedTest, since we need to test both
with the flag on and off.

[mlamouri (slow - plz ping), 2016-04-07 19:47:35]

It looks good and the finch experiment doesn't make the code that complex :)

Why are you worried about the tests?

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java
(right):

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java:35:
public static final String USE_NEW_MEDIA_NOTIFICATION =
"UseNewMediaNotification";
"MediaStyleNotifications" might be a better name because self explanatory.

(applies here and below)

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

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:50:
private static final int WEARABLE_NOTIFICATION_BACKGROUND_HEIGHT = 400;
Where did you find these numbers?

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:506:
// better than the default image.
Should we try to use METADATA_KEY_ART with the largeIcon?

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:647:
if (mMediaSession != null) {
Is such a thing even possible?

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:655:
// or use subText() for origin and setContentText for artist + album.
After we ship the new style, we should see how to present the metadata.artist
and metadata.album.

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:667:
if (mMediaNotificationInfo.supportsPlayPause()) {
Add a TODO: if it doesn't supports play/pause, we shouldn't use a MediaStyle
notification (but basic style, still no custom layout).

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

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:161:
updateFavicon(icon);
Maybe `updateFavicon` could return a boolean saying whether the favicon was
actually updated so we can early return if it wasn't?

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:188:
mNotificationInfoBuilder.setLargeIcon(mFavicon);
Shouldn't you call mNotificationInfoBuilder.setOrigin(mOrigin)?

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:278:
if (icon.getWidth() < 112 || icon.getHeight() < 112) return;
Why 112px instead of the values from the design doc?

https://codereview.chromium.org/1847063005/diff/120001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1847063005/diff/120001/chrome/app/generated_r...
chrome/app/generated_resources.grd:5606: +        New media notification style
Media Style Notifications

https://codereview.chromium.org/1847063005/diff/120001/chrome/app/generated_r...
chrome/app/generated_resources.grd:5609: +        Better looking notification
when Chrome plays media.
Use Android MediaStyle notifications for Chrome media notifications.

https://codereview.chromium.org/1847063005/diff/120001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1847063005/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1838: {"use-new-media-notification",
mediastyle-notifications

[Zhiqiang Zhang (Slow), 2016-04-11 09:26:28]

PTAL, addressed Mounir's comments.
The MediaStyle tests are failing, need to investigate why.

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java
(right):

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java:35:
public static final String USE_NEW_MEDIA_NOTIFICATION =
"UseNewMediaNotification";
On 2016/04/07 19:47:34, Mounir Lamouri wrote:
> "MediaStyleNotifications" might be a better name because self explanatory.
> 
> (applies here and below)

Done.

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

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:50:
private static final int WEARABLE_NOTIFICATION_BACKGROUND_HEIGHT = 400;
On 2016/04/07 19:47:34, Mounir Lamouri wrote:
> Where did you find these numbers?

http://developer.android.com/training/wearables/notifications/creating.html

Added comments

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:506:
// better than the default image.
On 2016/04/07 19:47:34, Mounir Lamouri wrote:
> Should we try to use METADATA_KEY_ART with the largeIcon?

Done. Also I merged MediaNotificationInfo.image and .largeIcon since they are
actually the same thing.

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:647:
if (mMediaSession != null) {
On 2016/04/07 19:47:34, Mounir Lamouri wrote:
> Is such a thing even possible?

Done. Merged all logic when mNotificationInfo.supportsPlayPause() is true.

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:655:
// or use subText() for origin and setContentText for artist + album.
On 2016/04/07 19:47:34, Mounir Lamouri wrote:
> After we ship the new style, we should see how to present the metadata.artist
> and metadata.album.

Done.

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:667:
if (mMediaNotificationInfo.supportsPlayPause()) {
On 2016/04/07 19:47:34, Mounir Lamouri wrote:
> Add a TODO: if it doesn't supports play/pause, we shouldn't use a MediaStyle
> notification (but basic style, still no custom layout).

The logic is already that. Added comments.

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

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:161:
updateFavicon(icon);
On 2016/04/07 19:47:34, Mounir Lamouri wrote:
> Maybe `updateFavicon` could return a boolean saying whether the favicon was
> actually updated so we can early return if it wasn't?

Done.

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:188:
mNotificationInfoBuilder.setLargeIcon(mFavicon);
On 2016/04/07 19:47:34, Mounir Lamouri wrote:
> Shouldn't you call mNotificationInfoBuilder.setOrigin(mOrigin)?

I think not. mNotificationInfoBuilder can be null if the notification is hidden,
but we still want to keep track of the origin in case a media starts to play
after the page is loaded.

https://codereview.chromium.org/1847063005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:278:
if (icon.getWidth() < 112 || icon.getHeight() < 112) return;
On 2016/04/07 19:47:34, Mounir Lamouri wrote:
> Why 112px instead of the values from the design doc?

Done.

https://codereview.chromium.org/1847063005/diff/120001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1847063005/diff/120001/chrome/app/generated_r...
chrome/app/generated_resources.grd:5606: +        New media notification style
On 2016/04/07 19:47:34, Mounir Lamouri wrote:
> Media Style Notifications

Done.

https://codereview.chromium.org/1847063005/diff/120001/chrome/app/generated_r...
chrome/app/generated_resources.grd:5609: +        Better looking notification
when Chrome plays media.
On 2016/04/07 19:47:34, Mounir Lamouri wrote:
> Use Android MediaStyle notifications for Chrome media notifications.

Done.

https://codereview.chromium.org/1847063005/diff/120001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1847063005/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1838: {"use-new-media-notification",
On 2016/04/07 19:47:34, Mounir Lamouri wrote:
> mediastyle-notifications

Done.

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

https://codereview.chromium.org/1847063005/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:288:
float displayDensity = context.getResources().getDisplayMetrics().density;
remove

[mlamouri (slow - plz ping), 2016-04-11 14:37:38]

lgtm with comments applied

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

https://codereview.chromium.org/1847063005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:544:
// TODO: smallIcon set to Chrome's icon?
I think you can remove the TODO because if we don't do anything, it might stay
forever :) We should ask feedback from rolfe@ after she tries the feature and
changes if needed.

https://codereview.chromium.org/1847063005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:643:
// metadata.artist and metadata.album.
typo: "see"
nit: add a bug number :)

https://codereview.chromium.org/1847063005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:647:
// large icon?
Maybe rephrase to "Update the default icon when a new one in provided." and
point to the bug number?
Also, assign this to you or me :)

https://codereview.chromium.org/1847063005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:670:
builder.setStyle(style);
Is there a "X" in the UI before L?

https://codereview.chromium.org/1847063005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:742:
* Scales the Bitmap to have better looking on Wearable devices.
"to have better looking" sounds odd. Also, should you explain a bit more what's
happening here?

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

https://codereview.chromium.org/1847063005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:192:
MediaNotificationManager.show(
So here you update the notification using the new mNotificationBuilder info but
you did not update mOrigin even tough you know it just changed. I'm not sure I
understand why. Shouldn't it be updated at the same time as
`.setLargeIcon(mFavicon)`.

[Zhiqiang Zhang (Slow), 2016-04-11 20:44:53]

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

[Zhiqiang Zhang (Slow), 2016-04-11 20:44:55]

Addressed Mounir's comments with replies.
+nyquist@ to take a look at:
proguard.flags
ChromeFeatureList.java
chrome_feature_list.*

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

https://codereview.chromium.org/1847063005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:544:
// TODO: smallIcon set to Chrome's icon?
On 2016/04/11 14:37:38, Mounir Lamouri wrote:
> I think you can remove the TODO because if we don't do anything, it might stay
> forever :) We should ask feedback from rolfe@ after she tries the feature and
> changes if needed.

Done.

https://codereview.chromium.org/1847063005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:643:
// metadata.artist and metadata.album.
On 2016/04/11 14:37:38, Mounir Lamouri wrote:
> typo: "see"
> nit: add a bug number :)

Done.

https://codereview.chromium.org/1847063005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:647:
// large icon?
On 2016/04/11 14:37:38, Mounir Lamouri wrote:
> Maybe rephrase to "Update the default icon when a new one in provided." and
> point to the bug number?
> Also, assign this to you or me :)

Done.

https://codereview.chromium.org/1847063005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:670:
builder.setStyle(style);
On 2016/04/11 14:37:38, Mounir Lamouri wrote:
> Is there a "X" in the UI before L?

Fixed, finally :/

https://codereview.chromium.org/1847063005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:742:
* Scales the Bitmap to have better looking on Wearable devices.
On 2016/04/11 14:37:38, Mounir Lamouri wrote:
> "to have better looking" sounds odd. Also, should you explain a bit more
what's
> happening here?

Done.

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

https://codereview.chromium.org/1847063005/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:192:
MediaNotificationManager.show(
On 2016/04/11 14:37:38, Mounir Lamouri wrote:
> So here you update the notification using the new mNotificationBuilder info
but
> you did not update mOrigin even tough you know it just changed. I'm not sure I
> understand why. Shouldn't it be updated at the same time as
> `.setLargeIcon(mFavicon)`.

Fixed by adding `mNOtificationInfoBuilder.setOrigin(origin)`

[Zhiqiang Zhang (Slow), 2016-04-13 11:39:19]

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

[Zhiqiang Zhang (Slow), 2016-04-13 11:39:20]

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

[Zhiqiang Zhang (Slow), 2016-04-13 11:39:20]

+bauerb for approval of:
proguard.flags
CastNotificationControl.java
ChromeFeatureList.java
chrome_feature_list.*

[Zhiqiang Zhang (Slow), 2016-04-13 11:39:20]

+bauerb for approval of:
proguard.flags
CastNotificationControl.java
ChromeFeatureList.java
chrome_feature_list.*

[Bernhard Bauer, 2016-04-14 12:58:20]

LGTM w/ nits:

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

https://codereview.chromium.org/1847063005/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:694:
|| mMediaNotificationInfo.supportsStop()) {
I don't quite see how this code matches the comment. Should the
supportsSwipeAway() condition be negated?

https://codereview.chromium.org/1847063005/diff/300001/chrome/browser/android...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/1847063005/diff/300001/chrome/browser/android...
chrome/browser/android/chrome_feature_list.cc:41: const base::Feature
kMediaStyleNotification{
Space before opening brace.

[Zhiqiang Zhang (Slow), 2016-04-14 13:09:17]

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

https://codereview.chromium.org/1847063005/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:694:
|| mMediaNotificationInfo.supportsStop()) {
On 2016/04/14 12:58:20, Bernhard Bauer wrote:
> I don't quite see how this code matches the comment. Should the
> supportsSwipeAway() condition be negated?

MediaNotificationInfo.supportsSwipeAway() means the notification can be
dismissed by swiping away, but on pre-L, it is not possible because of the
service issue, so we have to show a stop button so the user can dismiss the
notification.

BTW, it's from the old code, and should be removed eventually.

https://codereview.chromium.org/1847063005/diff/300001/chrome/browser/android...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/1847063005/diff/300001/chrome/browser/android...
chrome/browser/android/chrome_feature_list.cc:41: const base::Feature
kMediaStyleNotification{
On 2016/04/14 12:58:20, Bernhard Bauer wrote:
> Space before opening brace.

Done.

[Zhiqiang Zhang (Slow), 2016-04-14 13:19:58]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-04-14 13:19:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1847063005/#ps340001
(title: "fixed nits")

[commit-bot: I haz the power, 2016-04-14 13:20:19]

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

[commit-bot: I haz the power, 2016-04-14 14:02:28]

Description was changed from

==========
[Media, UI] Change MediaNotification style to MediaStyle

This patch changes the MediaNotification style from a custom layout into
the MediaStyle using NotificationCompat.

BUG=599937
==========

to

==========
[Media, UI] Change MediaNotification style to MediaStyle

This patch changes the MediaNotification style from a custom layout into
the MediaStyle using NotificationCompat.

BUG=599937
==========

[commit-bot: I haz the power, 2016-04-14 14:02:30]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-04-14 14:03:32]

Description was changed from

==========
[Media, UI] Change MediaNotification style to MediaStyle

This patch changes the MediaNotification style from a custom layout into
the MediaStyle using NotificationCompat.

BUG=599937
==========

to

==========
[Media, UI] Change MediaNotification style to MediaStyle

This patch changes the MediaNotification style from a custom layout into
the MediaStyle using NotificationCompat.

BUG=599937

Committed: https://crrev.com/6fe77672d634a7b0adb11d2c11f166be282da012
Cr-Commit-Position: refs/heads/master@{#387302}
==========

[commit-bot: I haz the power, 2016-04-14 14:03:33]

Patchset 18 (id:??) landed as
https://crrev.com/6fe77672d634a7b0adb11d2c11f166be282da012
Cr-Commit-Position: refs/heads/master@{#387302}