[MediaSession] Keep MediaSession controllable when losing audio focus

[MediaSession] Keep MediaSession controllable when losing audio focus

This is for keeping the media notification more persistent when the
audio focus is lost. The rationale is to reduce the effort to resume
audio. Otherwise, the user go back to Chrome and find the tab and click
play.

BUG=678225
Review-Url: https://codereview.chromium.org/2613143003
Cr-Commit-Position: refs/heads/master@{#442587}
Committed: https://chromium.googlesource.com/chromium/src/+/0c63aa9cb9891cc2d5376e5cec4c4b808ebdb3da

[Zhiqiang Zhang (Slow), 2017-01-09 14:28:20]

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

[Zhiqiang Zhang (Slow), 2017-01-09 14:28:21]

PTAL

[mlamouri (slow - plz ping), 2017-01-09 14:33:37]

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

[mlamouri (slow - plz ping), 2017-01-09 14:33:37]

https://codereview.chromium.org/2613143003/diff/1/content/browser/media/sessi...
File content/browser/media/session/audio_focus_delegate_android.cc (left):

https://codereview.chromium.org/2613143003/diff/1/content/browser/media/sessi...
content/browser/media/session/audio_focus_delegate_android.cc:67:
media_session_->Stop(MediaSession::SuspendType::SYSTEM);
Do we still use `Stop()` somewhere?

https://codereview.chromium.org/2613143003/diff/1/content/browser/media/sessi...
File content/browser/media/session/audio_focus_delegate_android.cc (right):

https://codereview.chromium.org/2613143003/diff/1/content/browser/media/sessi...
content/browser/media/session/audio_focus_delegate_android.cc:60: // See
https://crbug.com/539998
I guess your change would deprecate this comment, can you remove it? :)

https://codereview.chromium.org/2613143003/diff/1/content/browser/media/sessi...
content/browser/media/session/audio_focus_delegate_android.cc:64: // Suspending
the session instead of stopping regardless of |temporary| to
Could you remove `temporary` from the method if it's not used? :)

[whywhat, 2017-01-09 16:03:16]

lgtm modulo Mounir's comments

do we have tests to update for this change?

[Zhiqiang Zhang (Slow), 2017-01-09 18:10:59]

Patchset #2 (id:20001) has been deleted

[Zhiqiang Zhang (Slow), 2017-01-09 18:11:59]

On 2017/01/09 16:03:16, whywhat wrote:
> do we have tests to update for this change?

Just did :)

https://codereview.chromium.org/2613143003/diff/1/content/browser/media/sessi...
File content/browser/media/session/audio_focus_delegate_android.cc (left):

https://codereview.chromium.org/2613143003/diff/1/content/browser/media/sessi...
content/browser/media/session/audio_focus_delegate_android.cc:67:
media_session_->Stop(MediaSession::SuspendType::SYSTEM);
On 2017/01/09 14:33:37, mlamouri wrote:
> Do we still use `Stop()` somewhere?

Yes, but Stop() is now only called with SuspendType::UI (from the notification).
Maybe we should also remove the SuspendType argument from Stop().

https://codereview.chromium.org/2613143003/diff/1/content/browser/media/sessi...
File content/browser/media/session/audio_focus_delegate_android.cc (right):

https://codereview.chromium.org/2613143003/diff/1/content/browser/media/sessi...
content/browser/media/session/audio_focus_delegate_android.cc:60: // See
https://crbug.com/539998
On 2017/01/09 14:33:37, mlamouri wrote:
> I guess your change would deprecate this comment, can you remove it? :)

Done.

https://codereview.chromium.org/2613143003/diff/1/content/browser/media/sessi...
content/browser/media/session/audio_focus_delegate_android.cc:64: // Suspending
the session instead of stopping regardless of |temporary| to
On 2017/01/09 14:33:37, mlamouri wrote:
> Could you remove `temporary` from the method if it's not used? :)

Done.

[Zhiqiang Zhang (Slow), 2017-01-09 18:12:13]

Description was changed from

==========
[MediaSession] Keep MediaSession controllable when losing audio focus

This is for keeping the media notification more persistent when the
audio focus is lost. The rationale is to reduce the effort to resume
audio. Otherwise, the user go back to Chrome and find the tab and click
play.

BUG=678225
TEST=Manual
==========

to

==========
[MediaSession] Keep MediaSession controllable when losing audio focus

This is for keeping the media notification more persistent when the
audio focus is lost. The rationale is to reduce the effort to resume
audio. Otherwise, the user go back to Chrome and find the tab and click
play.

BUG=678225
==========

[Zhiqiang Zhang (Slow), 2017-01-09 18:12:22]

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

[commit-bot: I haz the power, 2017-01-09 18:13:08]

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, 2017-01-09 19:35:23]

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

[commit-bot: I haz the power, 2017-01-09 19:35:24]

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

[Zhiqiang Zhang (Slow), 2017-01-09 21:10:12]

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

[commit-bot: I haz the power, 2017-01-09 21:10:56]

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, 2017-01-09 22:23:10]

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

[commit-bot: I haz the power, 2017-01-09 22:23:11]

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

[Zhiqiang Zhang (Slow), 2017-01-10 14:16:16]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-01-10 14:16:16]

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/2613143003/#ps80001
(title: "suppress FB warnings")

[commit-bot: I haz the power, 2017-01-10 14:16:27]

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

[mlamouri (slow - plz ping), 2017-01-10 14:21:33]

lgtm

[commit-bot: I haz the power, 2017-01-10 14:57:42]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484057776384010,
"parent_rev": "cb43cd81f24e5bdeff02302ce9c86bb6c88d9c0d", "commit_rev":
"0c63aa9cb9891cc2d5376e5cec4c4b808ebdb3da"}

[commit-bot: I haz the power, 2017-01-10 14:58:12]

Description was changed from

==========
[MediaSession] Keep MediaSession controllable when losing audio focus

This is for keeping the media notification more persistent when the
audio focus is lost. The rationale is to reduce the effort to resume
audio. Otherwise, the user go back to Chrome and find the tab and click
play.

BUG=678225
==========

to

==========
[MediaSession] Keep MediaSession controllable when losing audio focus

This is for keeping the media notification more persistent when the
audio focus is lost. The rationale is to reduce the effort to resume
audio. Otherwise, the user go back to Chrome and find the tab and click
play.

BUG=678225

Review-Url: https://codereview.chromium.org/2613143003
Cr-Commit-Position: refs/heads/master@{#442587}
Committed:
https://chromium.googlesource.com/chromium/src/+/0c63aa9cb9891cc2d5376e5cec4c...
==========

[commit-bot: I haz the power, 2017-01-10 14:58:13]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0c63aa9cb9891cc2d5376e5cec4c...