Refactor Clank cast code to use MediaNotificationManager

Refactor Clank cast code to use MediaNotificationManager

This CL removes cast's own notification and lock screen classes, and
replaces them with a wrapper of MediaNotificationManager.

BUG=579919
Committed: https://crrev.com/f6db886968fd5493172f2941d6466a021e520a4e
Cr-Commit-Position: refs/heads/master@{#373536}

[aberent, 2016-02-01 17:53:31]

aberent@chromium.org changed reviewers:
+ avayvod@chromium.org, bauerb@chromium.org

[aberent, 2016-02-01 17:53:31]

avayvod@ - Please review whole CL.
bauberb@ - Please give owner signoff on chrome/android changes.

[aberent, 2016-02-01 17:54:28]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-01 17:56:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1652163002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1652163002/1

[commit-bot: I haz the power, 2016-02-01 18:13:14]

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

[commit-bot: I haz the power, 2016-02-01 18:13:15]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[aberent, 2016-02-01 18:41:39]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-01 18:42:13]

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

[aberent, 2016-02-01 18:42:32]

aberent@chromium.org changed reviewers:
+ tedchoc@chromium.org

[aberent, 2016-02-01 18:42:33]

tedchoc@chromium.org: Please review changes to resources
(chrome/android/java/res) as OWNER

[commit-bot: I haz the power, 2016-02-01 19:29:39]

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

[commit-bot: I haz the power, 2016-02-01 19:29:39]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[whywhat, 2016-02-01 20:54:15]

overall lgtm with nits and questions (and I think some tests are still to be
fixed according to CQ) :)

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

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:26:
MediaNotificationListener, AudioManager.OnAudioFocusChangeListener {
Do you need to handle any audio focus changes?
How does it work if one casts a video and then plays another video locally (that
would request audio focus for the remote playback first, then for the local
playback)?
I guess it should be fine since we had this behavior before.

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ExpandedControllerActivity.java
(right):

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ExpandedControllerActivity.java:34:
* TODO(cimamoglu): Refactor to merge some common logic with {@link
CastNotificationControl}.
I doubt Cihat will ever fix this, maybe change the name to yours?

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java
(right):

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:46:
private CastNotificationControl mNotificationControl;
It's probably concern the whole MediaNotification* classes family (what other
name could we use? SystemMediaControls?) but since they're handling the lock
screen as well and other things, maybe leave a comment above clarifying that?

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:278:
return mNotificationControl;
when does this return null?

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/remote/CastTestBase.java
(right):

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/media/remote/CastTestBase.java:91:
private CountDownLatch mLatch;
I never heard of a latch in programming :( You should tell me what it is so I
learn.

[Ted C, 2016-02-01 21:41:58]

On 2016/02/01 18:42:33, aberent wrote:
> mailto:tedchoc@chromium.org: Please review changes to resources
> (chrome/android/java/res) as OWNER

lgtm - chrome/android/java/res

[aberent, 2016-02-02 14:30:11]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-02 14:31:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1652163002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1652163002/40001

[aberent, 2016-02-02 14:31:11]

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

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:26:
MediaNotificationListener, AudioManager.OnAudioFocusChangeListener {
On 2016/02/01 20:54:14, whywhat wrote:
> Do you need to handle any audio focus changes?
> How does it work if one casts a video and then plays another video locally
(that
> would request audio focus for the remote playback first, then for the local
> playback)?
> I guess it should be fine since we had this behavior before.

It seems that the most recent item gets audio focus. The whole audio focus
concept seems a bit of a mess when one has multiple audio outputs, but that is
an Android problem.

As you say, this is no different from the the previous behaviour.

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ExpandedControllerActivity.java
(right):

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ExpandedControllerActivity.java:34:
* TODO(cimamoglu): Refactor to merge some common logic with {@link
CastNotificationControl}.
On 2016/02/01 20:54:14, whywhat wrote:
> I doubt Cihat will ever fix this, maybe change the name to yours?

Done, assuming we don't get rid of this completely.

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java
(right):

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:46:
private CastNotificationControl mNotificationControl;
On 2016/02/01 20:54:14, whywhat wrote:
> It's probably concern the whole MediaNotification* classes family (what other
> name could we use? SystemMediaControls?) but since they're handling the lock
> screen as well and other things, maybe leave a comment above clarifying that?

The class comment already mentions the lock screen, so I think a comment here is
unnecessary, especially since modern versions of Android tightly link the two
together.

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:278:
return mNotificationControl;
On 2016/02/01 20:54:14, whywhat wrote:
> when does this return null?

I don't think it can be, but I am not absolutely certain, and I have only done
the minimum possible changes to RemoteMediaPlayerController in this CL. In fact
RemoteMediaPlayerController is quite messy, but I will sort this out in a
different CL.

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/remote/CastTestBase.java
(right):

https://codereview.chromium.org/1652163002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/media/remote/CastTestBase.java:91:
private CountDownLatch mLatch;
On 2016/02/01 20:54:14, whywhat wrote:
> I never heard of a latch in programming :( You should tell me what it is so I
> learn.

I only learnt about them recently. They are quite nice because, unlike most
other concurrency primitives they only block the receiver, not the sender. As
such they are quite nice for this sort of situation, where one doesn't want to
fundamentally change the behaviour of the sender.

I think one can do cleverer things with them than I am here, such as having
multiple threads wait for the same events; but even with the simple use here
they are quite useful.

[commit-bot: I haz the power, 2016-02-02 15:14:23]

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

[commit-bot: I haz the power, 2016-02-02 15:14:23]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-02-02 16:02:52]

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

[mlamouri (slow - plz ping), 2016-02-02 16:02:53]

some drive-by comments

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

https://codereview.chromium.org/1652163002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:97:
AudioManager.AUDIOFOCUS_GAIN);
Why do we take audio focus if we don't do anything with it? (ie. we don't play
on the device and don't stop if asked to)

If this is an Android requirement to have something on the lock screen,
shouldn't we take audio focus back as soon as the Chrome activity is visible?
Otherwise, something might take audio focus and lose it. Taking it will make the
remote playback to lose audio focus then we will end up with no more audio focus
on the system, thus no lock screen integration?

https://codereview.chromium.org/1652163002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:208:
static CastNotificationControl getIfExists() {
style: getForTests()

https://codereview.chromium.org/1652163002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:213:
boolean isShowing() {
style: isShowingForTests()

[aberent, 2016-02-02 21:34:06]

avayvod@ PTAL

https://codereview.chromium.org/1652163002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/remote/CastNotificationTest.java
(left):

https://codereview.chromium.org/1652163002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/media/remote/CastNotificationTest.java:74:
public void testNotificationSelect() throws InterruptedException,
TimeoutException {
Since the cast code no longer gets involved in starting the expanded controller
activity  (when the notification is tapped) it is no longer useful to test this
at this level. Doing so would simply test Android Intent handling.

[aberent, 2016-02-02 21:52:18]

On 2016/02/02 16:02:53, Mounir Lamouri wrote:
> some drive-by comments
> 
>
https://codereview.chromium.org/1652163002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java
> (right):
> 
>
https://codereview.chromium.org/1652163002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:97:
> AudioManager.AUDIOFOCUS_GAIN);
> Why do we take audio focus if we don't do anything with it? (ie. we don't play
> on the device and don't stop if asked to)
> 
> If this is an Android requirement to have something on the lock screen,
> shouldn't we take audio focus back as soon as the Chrome activity is visible?
> Otherwise, something might take audio focus and lose it. Taking it will make
the
> remote playback to lose audio focus then we will end up with no more audio
focus
> on the system, thus no lock screen integration?
> 
>
https://codereview.chromium.org/1652163002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:208:
> static CastNotificationControl getIfExists() {
> style: getForTests()
> 
>
https://codereview.chromium.org/1652163002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:213:
> boolean isShowing() {
> style: isShowingForTests()

Missed these comments in my new patchset. Will reply (and possibly update)
tomorrow.

[aberent, 2016-02-02 21:52:38]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-02 21:56:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1652163002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1652163002/80001

[commit-bot: I haz the power, 2016-02-02 22:49:31]

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

[commit-bot: I haz the power, 2016-02-02 22:49:32]

Dry run: This issue passed the CQ dry run.

[aberent, 2016-02-03 11:58:48]

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

https://codereview.chromium.org/1652163002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:97:
AudioManager.AUDIOFOCUS_GAIN);
On 2016/02/02 16:02:53, Mounir Lamouri wrote:
> Why do we take audio focus if we don't do anything with it? (ie. we don't play
> on the device and don't stop if asked to)
> 
> If this is an Android requirement to have something on the lock screen,
> shouldn't we take audio focus back as soon as the Chrome activity is visible?
> Otherwise, something might take audio focus and lose it. Taking it will make
the
> remote playback to lose audio focus then we will end up with no more audio
focus
> on the system, thus no lock screen integration?

I admit I don't fully understand how audio focus works on Android or in Chrome;
however this reproduces the previous behaviour. I tried removing this, but doing
so had two effects:
1 - The local playback notification didn't go away when switching to remote
playback.
2 - If we had switched from local to remote playback (rather than starting
remote playback directly) we didn't get a lock screen.

You are right that, if we play a video in a different app (e.g. YouTube), we
lose the lock screen, and don't regain it when Chrome comes back into foreground
(although we do if the user presses play on Chrome), so there are things to be
fixed here. It isn't, however, obvious how to wire through the onResume() event
to here.

As such I left it as it is for now but have added a TODO to investigate in more
detail.

https://codereview.chromium.org/1652163002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:208:
static CastNotificationControl getIfExists() {
On 2016/02/02 16:02:53, Mounir Lamouri wrote:
> style: getForTests()

Done.

https://codereview.chromium.org/1652163002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:213:
boolean isShowing() {
On 2016/02/02 16:02:53, Mounir Lamouri wrote:
> style: isShowingForTests()

Done.

[aberent, 2016-02-03 11:59:33]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-03 12:00:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1652163002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1652163002/100001

[commit-bot: I haz the power, 2016-02-03 12:50:22]

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

[commit-bot: I haz the power, 2016-02-03 12:50:23]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-02-03 14:47:18]

lgtm with comments applied

https://codereview.chromium.org/1652163002/diff/100001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1652163002/diff/100001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:679: </service>
You are adding these classes in the other CL. Shouldn't they be in this CL if
you only use them here?

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

https://codereview.chromium.org/1652163002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:107:
.setActions(MediaNotificationInfo.ACTION_PLAY_PAUSE)
`.setActions(MediaNotificationInfo.ACTION_PLAY_PAUSE |
MediaNotificationInfo.ACTION_STOP)`

https://codereview.chromium.org/1652163002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:124:
mNotificationBuilder.setPaused(!(mState == PlayerState.PLAYING));
`mState != PlayerState.PLAYING`

[aberent, 2016-02-04 14:36:47]

https://codereview.chromium.org/1652163002/diff/100001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1652163002/diff/100001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:679: </service>
On 2016/02/03 14:47:18, Mounir Lamouri (slow) wrote:
> You are adding these classes in the other CL. Shouldn't they be in this CL if
> you only use them here?

Maybe should have been, but the other one is now landed.

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

https://codereview.chromium.org/1652163002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:107:
.setActions(MediaNotificationInfo.ACTION_PLAY_PAUSE)
On 2016/02/03 14:47:18, Mounir Lamouri (slow) wrote:
> `.setActions(MediaNotificationInfo.ACTION_PLAY_PAUSE |
> MediaNotificationInfo.ACTION_STOP)`

Removed, since the actions are set (correctly!) by updateNotification().

https://codereview.chromium.org/1652163002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:124:
mNotificationBuilder.setPaused(!(mState == PlayerState.PLAYING));
On 2016/02/03 14:47:18, Mounir Lamouri (slow) wrote:
> `mState != PlayerState.PLAYING`

Done.

[aberent, 2016-02-04 14:36:57]

The CQ bit was checked by aberent@chromium.org

[aberent, 2016-02-04 14:36:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, avayvod@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1652163002/#ps120001
(title: "Respond to final batch of nits")

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

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

[commit-bot: I haz the power, 2016-02-04 15:23:16]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-04 15:24:37]

Description was changed from

==========
Refactor Clank cast code to use MediaNotificationManager

This CL removes cast's own notification and lock screen classes, and
replaces them with a wrapper of MediaNotificationManager.

BUG=579919
==========

to

==========
Refactor Clank cast code to use MediaNotificationManager

This CL removes cast's own notification and lock screen classes, and
replaces them with a wrapper of MediaNotificationManager.

BUG=579919

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

[commit-bot: I haz the power, 2016-02-04 15:24:38]

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