Removing CustomLayout media notification logic

Removing CustomLayout media notification logic

Removed code and the Feature flag related to the
CustomLayout media notification logic since it was replaced with
MediaStyle notification in M52.

BUG=613564
Committed: https://crrev.com/ce18f330c76f5c43f85dc15543ba9d7e3316c4e5
Cr-Commit-Position: refs/heads/master@{#403885}

[kdsilva, 2016-07-05 14:20:15]

Description was changed from

==========
removed trailing whitespace


Removed unused imports


Working on resolving issue 613564.

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

to

==========
Working on resolving issue 613564.

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

[kdsilva, 2016-07-05 14:24:34]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 14:24:43]

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

[kdsilva, 2016-07-05 14:24:55]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-07-05 14:26:06]

Dry run: Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[commit-bot: I haz the power, 2016-07-05 14:59:04]

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

[commit-bot: I haz the power, 2016-07-05 14:59:07]

Dry run: This issue passed the CQ dry run.

[kdsilva, 2016-07-05 16:09:27]

Description was changed from

==========
Working on resolving issue 613564.

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

to

==========
Removed code and the Feature flag related to the CustomLayout media notification
logic since it was replaced with MediaStyle notification in M52.

BUG=613564
==========

[kdsilva, 2016-07-05 16:14:15]

kdsilva@google.com changed reviewers:
+ bauerb@chromium.org, mlamouri@chromium.org

[kdsilva, 2016-07-05 16:14:16]

bauerb:
  chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java
  chrome/browser/android/chrome_feature_list.cc
  chrome/browser/android/chrome_feature_list.h
mlamouri:
 
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
 
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
 
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
 
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java

[Bernhard Bauer, 2016-07-05 16:17:01]

LGTM, thanks!

[whywhat, 2016-07-05 16:45:45]

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

[whywhat, 2016-07-05 16:45:47]

+Zhiqiang who's more familiar with the code than any of us :)

Feel free to punt my suggestions to a follow up patch if they make sense.

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

https://codereview.chromium.org/2119353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:234:
* @param largeIcon An image associated with the media
nit: add a dot at the end to be consistent with the rest of the param
descriptions.

Is this an accidental change, btw? There's three more parameters that don't have
the description yet. Maybe leave for a separate cleanup change or add them too?
:)

Also, just to double check, are all of these fields still used for media style
notification?

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

https://codereview.chromium.org/2119353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:515:
setMediaStyleLayoutForNotificationBuilder(mNotificationBuilder);
nit: maybe rename to setupNotificationBuilder or something to make it shorter
and move some lines from below to the method too? It could even return the
notification builder and be unit tested.

Currently the logic to set up the builder is split this way:

setMediaStyleLayoutForNotificationBuilder calls:
- setLargeIcon
- setStyle
- setActions

updateNotification calls:
- setSmallIcon
- setAutoCancel
- setLocalOnly
- setOngoing
- setContentIntent
- setVisibility

It made sense before as the former part was different for two styles.

WDYT?

https://codereview.chromium.org/2119353003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java
(right):

https://codereview.chromium.org/2119353003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:199:
observedText = ((TextView) contentView.findViewById(R.id.title))
Seems like you left the else branch here?

[Zhiqiang Zhang (Slow), 2016-07-05 16:52:53]

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

https://codereview.chromium.org/2119353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:234:
* @param largeIcon An image associated with the media
On 2016/07/05 16:45:46, whywhat_OOO_until_July5 wrote:

> Also, just to double check, are all of these fields still used for media style
> notification?

Yes, the fields are still used.

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

https://codereview.chromium.org/2119353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:666:
new RemoteViews(mContext.getPackageName(), R.layout.playback_notification_bar);
You also need to remove the related layout xml (in java/res/layout) and resource
files (in java/res/drawable*).

Sorry I forgot to remind you of this.

[mlamouri (slow - plz ping), 2016-07-05 18:16:16]

lgtm! Thank you Krittika! :)

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

https://codereview.chromium.org/2119353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:515:
setMediaStyleLayoutForNotificationBuilder(mNotificationBuilder);
On 2016/07/05 at 16:45:46, whywhat_OOO_until_July5 wrote:
> nit: maybe rename to setupNotificationBuilder or something to make it shorter
and move some lines from below to the method too? It could even return the
notification builder and be unit tested.
> 
> Currently the logic to set up the builder is split this way:
> 
> setMediaStyleLayoutForNotificationBuilder calls:
> - setLargeIcon
> - setStyle
> - setActions
> 
> updateNotification calls:
> - setSmallIcon
> - setAutoCancel
> - setLocalOnly
> - setOngoing
> - setContentIntent
> - setVisibility
> 
> It made sense before as the former part was different for two styles.
> 
> WDYT?

To me, that sounds like something to think of for a follow-up.

https://codereview.chromium.org/2119353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:666:
new RemoteViews(mContext.getPackageName(), R.layout.playback_notification_bar);
On 2016/07/05 at 16:52:53, Zhiqiang Zhang wrote:
> You also need to remove the related layout xml (in java/res/layout) and
resource files (in java/res/drawable*).
> 
> Sorry I forgot to remind you of this.

Nice catch Zhiqiang! :)

I was hoping a lint would have found this problem though.

https://codereview.chromium.org/2119353003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java
(right):

https://codereview.chromium.org/2119353003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:199:
observedText = ((TextView) contentView.findViewById(R.id.title))
On 2016/07/05 at 16:45:46, whywhat_OOO_until_July5 wrote:
> Seems like you left the else branch here?

WDYM? It sounds good to me.

[mlamouri (slow - plz ping), 2016-07-05 18:16:51]

Description was changed from

==========
Removed code and the Feature flag related to the CustomLayout media notification
logic since it was replaced with MediaStyle notification in M52.

BUG=613564
==========

to

==========
Removing CustomLayout media notification logic 

Removed code and the Feature flag related to the
CustomLayout media notification logic since it was replaced with
MediaStyle notification in M52.

BUG=613564
==========

[mlamouri (slow - plz ping), 2016-07-05 18:17:56]

Notes:
- before landing, think of removing the xml file and apply other comments;
- I have reformatted your CL description to match the usual format.

[Zhiqiang Zhang (Slow), 2016-07-05 18:41:04]

https://codereview.chromium.org/2119353003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java
(right):

https://codereview.chromium.org/2119353003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:199:
observedText = ((TextView) contentView.findViewById(R.id.title))
On 2016/07/05 18:16:16, Mounir Lamouri wrote:
> On 2016/07/05 at 16:45:46, whywhat_OOO_until_July5 wrote:
> > Seems like you left the else branch here?
> 
> WDYM? It sounds good to me.

I think there's a problem here.

The reason is that when we use MediaStyle notification, the contentView can be
built either from the OS-version Notification or the SupportLibrary-version
NotificationCompat. In these two cases, the layout title id are different, one
is android.R.id.title, the other one is R.id.title (chromium scope). So we need
that null check.

Otherwise, the tests should not pass on devices with API level >= 21.

[kdsilva, 2016-07-06 09:57:42]

The CQ bit was checked by kdsilva@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-06 09:57:53]

Dry run: 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-07-06 10:47:28]

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

[commit-bot: I haz the power, 2016-07-06 10:47:29]

Dry run: This issue passed the CQ dry run.

[kdsilva, 2016-07-06 10:49:32]

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

https://codereview.chromium.org/2119353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:234:
* @param largeIcon An image associated with the media
On 2016/07/05 16:45:46, whywhat_OOO_until_July5 wrote:
> nit: add a dot at the end to be consistent with the rest of the param
> descriptions.
> 
> Is this an accidental change, btw? There's three more parameters that don't
have
> the description yet. Maybe leave for a separate cleanup change or add them
too?
> :)
> 
> Also, just to double check, are all of these fields still used for media style
> notification?


I add this line because I noticed that the CL that incorporated the MediaStyle
notification added the largeIcon parameter but removed a comment associated with
it.  I'll remove this line since it doesn't fit into the purpose of the CL and
then add comments for the parameters that don't have a description in another XS
CL.

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

https://codereview.chromium.org/2119353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:515:
setMediaStyleLayoutForNotificationBuilder(mNotificationBuilder);
On 2016/07/05 18:16:16, Mounir Lamouri wrote:
> On 2016/07/05 at 16:45:46, whywhat_OOO_until_July5 wrote:
> > nit: maybe rename to setupNotificationBuilder or something to make it
shorter
> and move some lines from below to the method too? It could even return the
> notification builder and be unit tested.
> > 
> > Currently the logic to set up the builder is split this way:
> > 
> > setMediaStyleLayoutForNotificationBuilder calls:
> > - setLargeIcon
> > - setStyle
> > - setActions
> > 
> > updateNotification calls:
> > - setSmallIcon
> > - setAutoCancel
> > - setLocalOnly
> > - setOngoing
> > - setContentIntent
> > - setVisibility
> > 
> > It made sense before as the former part was different for two styles.
> > 
> > WDYT?
> 
> To me, that sounds like something to think of for a follow-up. 

I agree. I think that sounds like a good idea for a follow-up CL!

https://codereview.chromium.org/2119353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:666:
new RemoteViews(mContext.getPackageName(), R.layout.playback_notification_bar);
On 2016/07/05 18:16:16, Mounir Lamouri wrote:
> On 2016/07/05 at 16:52:53, Zhiqiang Zhang wrote:
> > You also need to remove the related layout xml (in java/res/layout) and
> resource files (in java/res/drawable*).
> > 
> > Sorry I forgot to remind you of this.
> 
> Nice catch Zhiqiang! :)
> 
> I was hoping a lint would have found this problem though.

Done.

https://codereview.chromium.org/2119353003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java
(right):

https://codereview.chromium.org/2119353003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:199:
observedText = ((TextView) contentView.findViewById(R.id.title))
On 2016/07/05 18:41:04, Zhiqiang Zhang wrote:
> On 2016/07/05 18:16:16, Mounir Lamouri wrote:
> > On 2016/07/05 at 16:45:46, whywhat_OOO_until_July5 wrote:
> > > Seems like you left the else branch here?
> > 
> > WDYM? It sounds good to me.
> 
> I think there's a problem here.
> 
> The reason is that when we use MediaStyle notification, the contentView can be
> built either from the OS-version Notification or the SupportLibrary-version
> NotificationCompat. In these two cases, the layout title id are different, one
> is android.R.id.title, the other one is R.id.title (chromium scope). So we
need
> that null check.
> 
> Otherwise, the tests should not pass on devices with API level >= 21.

Done.

[Zhiqiang Zhang (Slow), 2016-07-06 10:53:45]

lgtm w/ comments applied:

There's one more entry in generated_resources.grd that need to be removed.

[kdsilva, 2016-07-06 11:08:50]

The CQ bit was checked by kdsilva@google.com

[kdsilva, 2016-07-06 11:08:51]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
mlamouri@chromium.org, zqzhang@chromium.org
Link to the patchset: https://codereview.chromium.org/2119353003/#ps60001
(title: "Review comments")

[commit-bot: I haz the power, 2016-07-06 11:09:11]

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-07-06 11:51:15]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-07-06 11:53:03]

Description was changed from

==========
Removing CustomLayout media notification logic 

Removed code and the Feature flag related to the
CustomLayout media notification logic since it was replaced with
MediaStyle notification in M52.

BUG=613564
==========

to

==========
Removing CustomLayout media notification logic

Removed code and the Feature flag related to the
CustomLayout media notification logic since it was replaced with
MediaStyle notification in M52.

BUG=613564

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

[commit-bot: I haz the power, 2016-07-06 11:53:05]

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