Refactoring media notification

Refactoring media notification

Some refactoring in the media notification.

Media notification will no longer updates itself. When after a control
message is received, it only sends command to the underlying
MediaSession. Then when MediaSession has changed its state, it will tell
the media notification to update accordingly.

Besides, a lot of redundant code has been cleaned up.

BUG=546981
Committed: https://crrev.com/134cb4ede9e4d62fe675424eb5827767e2165963
Cr-Commit-Position: refs/heads/master@{#364364}

[Zhiqiang Zhang (Slow), 2015-12-02 14:21:08]

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

[whywhat, 2015-12-02 14:41:09]

https://codereview.chromium.org/1491943002/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/1491943002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:419:
updateNotification();
I think the idea was to replace onPlay and onPause with a simple
listener.onPlay/Pause the same way you did onStop().
Thus removing relying on the mNotificationInfoBuilder here.

https://codereview.chromium.org/1491943002/diff/1/content/browser/media/andro...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1491943002/diff/1/content/browser/media/andro...
content/browser/media/android/media_session.cc:156: UpdateWebContents();
It seems like now UpdateWebContents could be moved into
OnResumeInternal/OnSuspendInternal since it's always called after the call.

[Zhiqiang Zhang (Slow), 2015-12-03 16:34:59]

PTAL. I've done some more refactoring with respect to the notification id. As
well, please see the following replies.

https://codereview.chromium.org/1491943002/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/1491943002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:419:
updateNotification();
On 2015/12/02 14:41:09, whywhat_OOO_Dec3_Dec8 wrote:
> I think the idea was to replace onPlay and onPause with a simple
> listener.onPlay/Pause the same way you did onStop().
> Thus removing relying on the mNotificationInfoBuilder here.

Done.

https://codereview.chromium.org/1491943002/diff/1/content/browser/media/andro...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1491943002/diff/1/content/browser/media/andro...
content/browser/media/android/media_session.cc:156: UpdateWebContents();
On 2015/12/02 14:41:09, whywhat_OOO_Dec3_Dec8 wrote:
> It seems like now UpdateWebContents could be moved into
> OnResumeInternal/OnSuspendInternal since it's always called after the call.

I'm not sure whether it is OK for Stop(), because it first calls
OnSuspendInternal, then clears the players, and then call
AbandonSystemAudioFocusIfNeeded (which calls UpdateWebContents).

If we put UpdateWebContents into OnSuspendInternal, then Stop() will call
UpdatedWebContents twice.

[Zhiqiang Zhang (Slow), 2015-12-07 15:24:06]

PTAL.
I've fixed some media_session_browsertests, and moved UpdateWebContents into
OnSuspendInternal/OnResumeInternal.

[mlamouri (slow - plz ping), 2015-12-07 18:38:03]

Looks very good. Very exciting to see so much code cleanup! :)

https://codereview.chromium.org/1491943002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaButtonReceiver.java
(right):

https://codereview.chromium.org/1491943002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaButtonReceiver.java:18:
public abstract int getNotificationId();
I think you can remove getNotificationId()

https://codereview.chromium.org/1491943002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(left):

https://codereview.chromium.org/1491943002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:65:
MediaNotificationManager.clear(R.id.media_playback_notification);
Could you make sure that avayvod@ has a look at this. He might know why we used
to call clear().

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
content/browser/media/android/media_session.cc:129: :
MediaSessionSuspendedSource::SystemPermanent);
I will need to double-check that but do you think you could move this call to
|OnSuspendInternal|?

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
content/browser/media/android/media_session.cc:219: UpdateWebContents();
nit: leave an empty line before `UpdateWebContents()`

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
File content/browser/media/android/media_session.h (right):

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
content/browser/media/android/media_session.h:139: void
OnSuspendInternal(SuspendType type, bool temporary);
I wonder if we could have something better than a `bool` here. Maybe the new
state or an enum with two values?

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
File content/browser/media/android/media_session_browsertest.cc (right):

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
content/browser/media/android/media_session_browsertest.cc:770:
MediaSessionStateChanged(true, true)).After(showControls);
nit: maybe it would be easier to read if you had:
```
Expectation showControls = [...];
Expectation paused = [...];

EXPECT_CALL(paused).After(showControls);
```
?

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
content/browser/media/android/media_session_browsertest.cc:789:
MediaSessionStateChanged(true, false)).After(pauseControls);
ditto

[Zhiqiang Zhang (Slow), 2015-12-08 16:28:16]

PTAL.

Changes:
1. Removed getNotificationId() from ListenerService and MediaButtonReceiver;
2. Moved UpdateWebContents into OnSuspendInternal/OnResumeInternal;
3. Added new tests for MediaSession::OnStop().

Also, see the replies below:

https://codereview.chromium.org/1491943002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaButtonReceiver.java
(right):

https://codereview.chromium.org/1491943002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaButtonReceiver.java:18:
public abstract int getNotificationId();
On 2015/12/07 18:38:02, Mounir Lamouri wrote:
> I think you can remove getNotificationId()

Done. Now MediaNotificationManager stores NotificationId in itself, so we can
remove getNotificationId() in many places. Previously, the notificatioId is only
in mMediaNotificationInfo, which could be null, thus it was hard to get the
notificationId from the manager.

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
content/browser/media/android/media_session.cc:129: :
MediaSessionSuspendedSource::SystemPermanent);
On 2015/12/07 18:38:02, Mounir Lamouri wrote:
> I will need to double-check that but do you think you could move this call to
> |OnSuspendInternal|?

Done.

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
content/browser/media/android/media_session.cc:219: UpdateWebContents();
On 2015/12/07 18:38:02, Mounir Lamouri wrote:
> nit: leave an empty line before `UpdateWebContents()`

Moved UpdateWebContents() into OnSuspendInternal/OnResumeInternal, with an empty
line before the call.

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
File content/browser/media/android/media_session.h (right):

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
content/browser/media/android/media_session.h:139: void
OnSuspendInternal(SuspendType type, bool temporary);
On 2015/12/07 18:38:02, Mounir Lamouri wrote:
> I wonder if we could have something better than a `bool` here. Maybe the new
> state or an enum with two values?

Now uses the new State. But I feel a little bit awkward.

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
File content/browser/media/android/media_session_browsertest.cc (right):

https://codereview.chromium.org/1491943002/diff/60001/content/browser/media/a...
content/browser/media/android/media_session_browsertest.cc:770:
MediaSessionStateChanged(true, true)).After(showControls);
On 2015/12/07 18:38:03, Mounir Lamouri wrote:
> nit: maybe it would be easier to read if you had:
> ```
> Expectation showControls = [...];
> Expectation paused = [...];
> 
> EXPECT_CALL(paused).After(showControls);
> ```
> ?

No. I tried, but gmock seems does not allow this.

[mlamouri (slow - plz ping), 2015-12-09 14:31:37]

This looks great! :)

lgtm but please wait for avayvod@ before landing.

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

https://codereview.chromium.org/1491943002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:199:
protected MediaNotificationManager getManager() {
I didn't want you to remove getNotificationId() from ListenerService but only
from MediaButtonReceiver. You can keep this as is or put getNotificationId()
back. Up to you.

https://codereview.chromium.org/1491943002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:347:
private MediaNotificationInfo mMediaNotificationInfo;
nit:
// When not null, |mMediaNotificationINfo.id| must be the same as
|mNotificationId|.

[whywhat, 2015-12-09 16:59:27]

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

https://codereview.chromium.org/1491943002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaButtonReceiver.java:23:
context.startService(intent);
Why is setClassName() equivalent to setComponent() then? Why do we get rid of
the checks above?

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

https://codereview.chromium.org/1491943002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:346:
// |mMediaNotificationInfo|.|id| must be the same with |mNotificationId| when it
is not null.
why can't we just use mMediaNotificationInfo.id then?

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

https://codereview.chromium.org/1491943002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:80:
if (mTab == null) {
Shouldn't we move this logic into hideNotification() ?
Is the question whether mTab can never be null in case we get
WebContentsObserver.destroy() call? It seems to depend on the order of
TabObserver.onTabDestroyed() and WebContentsObserver.destroy().

Because if it is null and we will not hide the notification, the user will be
upset.

https://codereview.chromium.org/1491943002/diff/120001/content/browser/media/...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1491943002/diff/120001/content/browser/media/...
content/browser/media/android/media_session.cc:90: 
nit: accidental change?

https://codereview.chromium.org/1491943002/diff/120001/content/browser/media/...
content/browser/media/android/media_session.cc:203: assert(new_state ==
State::SUSPENDED || new_state == State::INACTIVE);
Don't we use DCHECKs in C++ code outside Blink?

https://codereview.chromium.org/1491943002/diff/120001/content/browser/media/...
File content/browser/media/android/media_session_browsertest.cc (right):

https://codereview.chromium.org/1491943002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_browsertest.cc:783:
.After(hideControls);
So the test didn't change but the behavior did change?

[Zhiqiang Zhang (Slow), 2015-12-09 18:02:56]

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

https://codereview.chromium.org/1491943002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaButtonReceiver.java:23:
context.startService(intent);
On 2015/12/09 16:59:27, whywhat wrote:
> Why is setClassName() equivalent to setComponent() then? Why do we get rid of
> the checks above?

I checked the Android reference. setClassName() is a convenience way of calling
setComponent(). Because we know that PlaybackMediaButtonReceiver will only
forward to PlaybackListenerService (and the same with Presentation), directly
setting the service name should not be a problem.

The check above was incorrect (see LISTENER_SERVICE_CLASS_NAME). The assertions
don't fail on many devices. So I think we don't need that check. If the service
cannot be started, we can found it in adb logcat.

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

https://codereview.chromium.org/1491943002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:346:
// |mMediaNotificationInfo|.|id| must be the same with |mNotificationId| when it
is not null.
On 2015/12/09 16:59:27, whywhat wrote:
> why can't we just use mMediaNotificationInfo.id then?

mMediaNotificationInfo could be null. I'm not sure it will be valid at all
call-sites.

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

https://codereview.chromium.org/1491943002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:80:
if (mTab == null) {
On 2015/12/09 16:59:27, whywhat wrote:
> Shouldn't we move this logic into hideNotification() ?
> Is the question whether mTab can never be null in case we get
> WebContentsObserver.destroy() call? It seems to depend on the order of
> TabObserver.onTabDestroyed() and WebContentsObserver.destroy().
> 
> Because if it is null and we will not hide the notification, the user will be
> upset.

The reason I removed clear() is that sometimes if a WebContents is destroyed,
even the notification of another Tab will be destroyed, because the clear()
function only takes the notification id as argument. WDYT?

https://codereview.chromium.org/1491943002/diff/120001/content/browser/media/...
File content/browser/media/android/media_session_browsertest.cc (right):

https://codereview.chromium.org/1491943002/diff/120001/content/browser/media/...
content/browser/media/android/media_session_browsertest.cc:783:
.After(hideControls);
On 2015/12/09 16:59:27, whywhat wrote:
> So the test didn't change but the behavior did change?

Sorry, my fault. hideControls should correspond to
MediaSessionStateChanged(false, true).
I shouldn't changed the variable names.

[whywhat, 2015-12-09 20:46:12]

lgtm (extra bonus lgtm if you can get rid of mNotificationId as discussed
offline). Thanks for the updates!

[Zhiqiang Zhang (Slow), 2015-12-09 21:07:01]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2015-12-09 21:07:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1491943002/#ps140001
(title: "small fixes")

[commit-bot: I haz the power, 2015-12-09 21:07:51]

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

[commit-bot: I haz the power, 2015-12-09 21:49:15]

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

[commit-bot: I haz the power, 2015-12-09 21:49:16]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Zhiqiang Zhang (Slow), 2015-12-10 10:33:54]

Patchset #9 (id:160001) has been deleted

[Zhiqiang Zhang (Slow), 2015-12-10 10:37:12]

Description was changed from

==========
Making synchronized control and update in media notification

Media notification will no longer updates itself. When after a control
message is received, it only sends command to the underlying
MediaSession. Then when MediaSession has changed its state, it will tell
the media notification to update accordingly.

BUG=546981
==========

to

==========
Refactoring media notification

Some refactoring in the media notification.

Media notification will no longer updates itself. When after a control
message is received, it only sends command to the underlying
MediaSession. Then when MediaSession has changed its state, it will tell
the media notification to update accordingly.

Besides, a lot of redundant code has been cleaned up.

BUG=546981
==========

[Zhiqiang Zhang (Slow), 2015-12-10 11:04:21]

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

[Zhiqiang Zhang (Slow), 2015-12-10 11:04:22]

+miguelg@ because we don't own PauseOnHeadsetUnplugTest.java. 

Perhaps consider adding an OWNER file to .../javatests/.../media/ui/ in another
CL.

Also, I changed the CL description a little bit.

[mlamouri (slow - plz ping), 2015-12-10 11:20:00]

I've sent https://codereview.chromium.org/1520463002 for review. If miguelg@ can
review that. we will be able to land this CL.

[Zhiqiang Zhang (Slow), 2015-12-10 14:12:06]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2015-12-10 14:12:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1491943002/#ps180001
(title: "removed manager.mNotificationId, some small fixes")

[commit-bot: I haz the power, 2015-12-10 14:12:12]

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

[commit-bot: I haz the power, 2015-12-10 15:30:55]

Description was changed from

==========
Refactoring media notification

Some refactoring in the media notification.

Media notification will no longer updates itself. When after a control
message is received, it only sends command to the underlying
MediaSession. Then when MediaSession has changed its state, it will tell
the media notification to update accordingly.

Besides, a lot of redundant code has been cleaned up.

BUG=546981
==========

to

==========
Refactoring media notification

Some refactoring in the media notification.

Media notification will no longer updates itself. When after a control
message is received, it only sends command to the underlying
MediaSession. Then when MediaSession has changed its state, it will tell
the media notification to update accordingly.

Besides, a lot of redundant code has been cleaned up.

BUG=546981
==========

[commit-bot: I haz the power, 2015-12-10 15:30:56]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-12-10 15:32:05]

Description was changed from

==========
Refactoring media notification

Some refactoring in the media notification.

Media notification will no longer updates itself. When after a control
message is received, it only sends command to the underlying
MediaSession. Then when MediaSession has changed its state, it will tell
the media notification to update accordingly.

Besides, a lot of redundant code has been cleaned up.

BUG=546981
==========

to

==========
Refactoring media notification

Some refactoring in the media notification.

Media notification will no longer updates itself. When after a control
message is received, it only sends command to the underlying
MediaSession. Then when MediaSession has changed its state, it will tell
the media notification to update accordingly.

Besides, a lot of redundant code has been cleaned up.

BUG=546981

Committed: https://crrev.com/134cb4ede9e4d62fe675424eb5827767e2165963
Cr-Commit-Position: refs/heads/master@{#364364}
==========

[commit-bot: I haz the power, 2015-12-10 15:32:06]

Patchset 9 (id:??) landed as
https://crrev.com/134cb4ede9e4d62fe675424eb5827767e2165963
Cr-Commit-Position: refs/heads/master@{#364364}