cros: Suspend media sessions with display off trigger by tablet power button

cros: Suspend media sessions with display off trigger by tablet power button

Changes:
Convertible/tablet device's side power button is described as tablet power button. It will set display forced-off or forced-on just like android phone. This CL is going to add extra functionality to tablet power button, which is suspending media sessions when display is forced-off by tablet power button.

BUG=694384
TEST=tested on kevin, with two youtube browser tabs and one youtube arc++ app. arc++ app's audio is not controlled by the tablet power button.
(1) browser tabs audio playing, pressing power button to set display off, audio will pause.
(2) browser tabs audio pausing, pressing power button to set display off will still pause audio, pressing again will keep audio paused.

Also added test coverage in TabletPowerButtonControllerTest.

Review-Url: https://codereview.chromium.org/2821303004
Cr-Commit-Position: refs/heads/master@{#476018}
Committed: https://chromium.googlesource.com/chromium/src/+/67a6d3fc1811bf403d56f59cf4099e3f88b68063

[Qiang(Joe) Xu, 2017-04-18 23:53:32]

Description was changed from

==========
[WIP] media session


media session base change

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

to

==========
[WIP] stop and resume media session for tablet power button

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

[Qiang(Joe) Xu, 2017-05-15 17:53:11]

Patchset #3 (id:40001) has been deleted

[Qiang(Joe) Xu, 2017-05-17 00:40:43]

Patchset #3 (id:60001) has been deleted

[Qiang(Joe) Xu, 2017-05-17 00:49:07]

Description was changed from

==========
[WIP] stop and resume media session for tablet power button

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

to

==========
cros: Suspend and resume media session for tablet power button

Changes:
Convertible/tablet device's side power button is described as tablet power
button. It will set display forced-off or forced-on just like android phone.
This CL is going to add extra functionality to tablet power button, which is
suspending/resuming media sessions.

BUG=694384
TEST=tested on kevin, with two youtube browser tabs and one youtube arc++ app.
arc++ app's audio is not controlled by the tablet power button.
(1) browser tabs audio playing, pressing power button to set display off, audios
will pause, pressing again will resume audios.
(2) browser tabs audio pausing, pressing power button to set display off will
still pause audio, pressing again will keep audios paused.

Note:
This CL is dependent on the crash fix CL:
https://codereview.chromium.org/2893473002/
==========

[Qiang(Joe) Xu, 2017-05-17 00:49:07]

warx@chromium.org changed reviewers:
+ derat@chromium.org, mlamouri@chromium.org

[Qiang(Joe) Xu, 2017-05-17 00:50:27]

Description was changed from

==========
cros: Suspend and resume media session for tablet power button

Changes:
Convertible/tablet device's side power button is described as tablet power
button. It will set display forced-off or forced-on just like android phone.
This CL is going to add extra functionality to tablet power button, which is
suspending/resuming media sessions.

BUG=694384
TEST=tested on kevin, with two youtube browser tabs and one youtube arc++ app.
arc++ app's audio is not controlled by the tablet power button.
(1) browser tabs audio playing, pressing power button to set display off, audios
will pause, pressing again will resume audios.
(2) browser tabs audio pausing, pressing power button to set display off will
still pause audio, pressing again will keep audios paused.

Note:
This CL is dependent on the crash fix CL:
https://codereview.chromium.org/2893473002/
==========

to

==========
cros: Suspend and resume media session for tablet power button

Changes:
Convertible/tablet device's side power button is described as tablet power
button. It will set display forced-off or forced-on just like android phone.
This CL is going to add extra functionality to tablet power button, which is
suspending/resuming media sessions.

BUG=694384
TEST=tested on kevin, with two youtube browser tabs and one youtube arc++ app.
arc++ app's audio is not controlled by the tablet power button.
(1) browser tabs audio playing, pressing power button to set display off, audio
will pause, pressing again will resume audio.
(2) browser tabs audio pausing, pressing power button to set display off will
still pause audio, pressing again will keep audio paused.

Note:
This CL is dependent on the crash fix CL:
https://codereview.chromium.org/2893473002/
==========

[Qiang(Joe) Xu, 2017-05-17 00:52:40]

Hi all, PTAL, thanks!

Note issue 716066 is not included in this CL, as I found there is still pending
issue of it. Let us do it separately.

[Daniel Erat, 2017-05-17 01:22:25]

https://codereview.chromium.org/2821303004/diff/80001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/2821303004/diff/80001/ash/shell_delegate.h#ne...
ash/shell_delegate.h:143: // Resumes/Suspends media sessions systematically.
could you document what media sessions are here? it's non-obvious to me what's
included.

https://codereview.chromium.org/2821303004/diff/80001/ash/system/power/tablet...
File ash/system/power/tablet_power_button_controller.cc (right):

https://codereview.chromium.org/2821303004/diff/80001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller.cc:227: :
delegate->ResumeMediaSessions();
please update unit tests to check that this happens as expected.

https://codereview.chromium.org/2821303004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2821303004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:919:
::switches::kEnableInternalMediaSession);
it seems strange to do this programmatically here. we usually pass switches from
session_manager, or perhaps better, just turn the feature on automatically
within chrome when OS_CHROMEOS is defined.

https://codereview.chromium.org/2821303004/diff/80001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/2821303004/diff/80001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate.cc:598:
content::MediaSession::SuspendType::SYSTEM);
is SYSTEM actually an appropriate type to use here? from looking at the
comments, UI seems like it might be better.

  enum class SuspendType {
    // Suspended by the system because a transient sound needs to be played.
    SYSTEM,
    // Suspended by the UI.
    UI,
    // Suspended by the page via script or user interaction.
    CONTENT,
  };

[mlamouri (slow - plz ping), 2017-05-17 12:52:27]

This lgtm. We might want to talk about the various edge cases but ideally, the
high level API usage shouldn't change :)

https://codereview.chromium.org/2821303004/diff/80001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/2821303004/diff/80001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate.cc:598:
content::MediaSession::SuspendType::SYSTEM);
On 2017/05/17 at 01:22:25, Daniel Erat wrote:
> is SYSTEM actually an appropriate type to use here? from looking at the
comments, UI seems like it might be better.
> 
>   enum class SuspendType {
>     // Suspended by the system because a transient sound needs to be played.
>     SYSTEM,
>     // Suspended by the UI.
>     UI,
>     // Suspended by the page via script or user interaction.
>     CONTENT,
>   };

System sounds like the right thing to do here. The comments, as with all
documentation, are slightly outdated and are focused on Chrome Android. In CrOS
case, it's fair to say that the behaviour you have is part of the system.

Note that it shouldn't matter much apart that suspend/resume should use the same
ID and UMAs will vary.

[Qiang(Joe) Xu, 2017-05-22 22:53:42]

Description was changed from

==========
cros: Suspend and resume media session for tablet power button

Changes:
Convertible/tablet device's side power button is described as tablet power
button. It will set display forced-off or forced-on just like android phone.
This CL is going to add extra functionality to tablet power button, which is
suspending/resuming media sessions.

BUG=694384
TEST=tested on kevin, with two youtube browser tabs and one youtube arc++ app.
arc++ app's audio is not controlled by the tablet power button.
(1) browser tabs audio playing, pressing power button to set display off, audio
will pause, pressing again will resume audio.
(2) browser tabs audio pausing, pressing power button to set display off will
still pause audio, pressing again will keep audio paused.

Note:
This CL is dependent on the crash fix CL:
https://codereview.chromium.org/2893473002/
==========

to

==========
cros: Suspend media sessions with display off trigger by tablet power button

Changes:
Convertible/tablet device's side power button is described as tablet power
button. It will set display forced-off or forced-on just like android phone.
This CL is going to add extra functionality to tablet power button, which is
suspending media sessions when display is forced-off by tablet power button.

BUG=694384
TEST=tested on kevin, with two youtube browser tabs and one youtube arc++ app.
arc++ app's audio is not controlled by the tablet power button.
(1) browser tabs audio playing, pressing power button to set display off, audio
will pause.
(2) browser tabs audio pausing, pressing power button to set display off will
still pause audio, pressing again will keep audio paused.

Also added test coverage in TabletPowerButtonControllerTest.

Note:
This CL is dependent on the crash fix CL:
https://codereview.chromium.org/2893473002/
==========

[Qiang(Joe) Xu, 2017-05-22 22:58:01]

Description was changed from

==========
cros: Suspend media sessions with display off trigger by tablet power button

Changes:
Convertible/tablet device's side power button is described as tablet power
button. It will set display forced-off or forced-on just like android phone.
This CL is going to add extra functionality to tablet power button, which is
suspending media sessions when display is forced-off by tablet power button.

BUG=694384
TEST=tested on kevin, with two youtube browser tabs and one youtube arc++ app.
arc++ app's audio is not controlled by the tablet power button.
(1) browser tabs audio playing, pressing power button to set display off, audio
will pause.
(2) browser tabs audio pausing, pressing power button to set display off will
still pause audio, pressing again will keep audio paused.

Also added test coverage in TabletPowerButtonControllerTest.

Note:
This CL is dependent on the crash fix CL:
https://codereview.chromium.org/2893473002/
==========

to

==========
cros: Suspend media sessions with display off trigger by tablet power button

Changes:
Convertible/tablet device's side power button is described as tablet power
button. It will set display forced-off or forced-on just like android phone.
This CL is going to add extra functionality to tablet power button, which is
suspending media sessions when display is forced-off by tablet power button.

BUG=694384
TEST=tested on kevin, with two youtube browser tabs and one youtube arc++ app.
arc++ app's audio is not controlled by the tablet power button.
(1) browser tabs audio playing, pressing power button to set display off, audio
will pause.
(2) browser tabs audio pausing, pressing power button to set display off will
still pause audio, pressing again will keep audio paused.

Also added test coverage in TabletPowerButtonControllerTest.
==========

[Qiang(Joe) Xu, 2017-05-22 22:58:47]

Hi all, PTAL thanks

https://codereview.chromium.org/2821303004/diff/80001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/2821303004/diff/80001/ash/shell_delegate.h#ne...
ash/shell_delegate.h:143: // Resumes/Suspends media sessions systematically.
On 2017/05/17 01:22:25, Daniel Erat wrote:
> could you document what media sessions are here? it's non-obvious to me what's
> included.

Done. Media session is a name/way manage media contents to allow resume/stop
managed players.

On Chrome android, media session is an API for notification media controller.

https://codereview.chromium.org/2821303004/diff/80001/ash/system/power/tablet...
File ash/system/power/tablet_power_button_controller.cc (right):

https://codereview.chromium.org/2821303004/diff/80001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller.cc:227: :
delegate->ResumeMediaSessions();
On 2017/05/17 01:22:25, Daniel Erat wrote:
> please update unit tests to check that this happens as expected.

Done.

https://codereview.chromium.org/2821303004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2821303004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:919:
::switches::kEnableInternalMediaSession);
On 2017/05/17 01:22:25, Daniel Erat wrote:
> it seems strange to do this programmatically here. we usually pass switches
from
> session_manager, or perhaps better, just turn the feature on automatically
> within chrome when OS_CHROMEOS is defined.

I moved it to ChromeShellDelegate::PreInit(), is that in line with "turn the
feature on automatically within chrome when OS_CHROMEOS is defined"?

https://codereview.chromium.org/2821303004/diff/80001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/2821303004/diff/80001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate.cc:598:
content::MediaSession::SuspendType::SYSTEM);
On 2017/05/17 12:52:26, mlamouri (OOO up to 23-05) wrote:
> On 2017/05/17 at 01:22:25, Daniel Erat wrote:
> > is SYSTEM actually an appropriate type to use here? from looking at the
> comments, UI seems like it might be better.
> > 
> >   enum class SuspendType {
> >     // Suspended by the system because a transient sound needs to be played.
> >     SYSTEM,
> >     // Suspended by the UI.
> >     UI,
> >     // Suspended by the page via script or user interaction.
> >     CONTENT,
> >   };
> 
> System sounds like the right thing to do here. The comments, as with all
> documentation, are slightly outdated and are focused on Chrome Android. In
CrOS
> case, it's fair to say that the behaviour you have is part of the system.
> 
> Note that it shouldn't matter much apart that suspend/resume should use the
same
> ID and UMAs will vary.

mlamouri@, since we are not actually entering suspended state for power button
pressed, it is just a screen brightness off with chrome still running, is SYSTEM
still preferred over UI?

[Daniel Erat, 2017-05-22 23:38:34]

https://codereview.chromium.org/2821303004/diff/100001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/2821303004/diff/100001/ash/shell_delegate.h#n...
ash/shell_delegate.h:143: // Suspends all webcontents associated media sessions
to stop managed players.
nit: "WebContents-associated"

https://codereview.chromium.org/2821303004/diff/100001/ash/system/power/table...
File ash/system/power/tablet_power_button_controller.cc (right):

https://codereview.chromium.org/2821303004/diff/100001/ash/system/power/table...
ash/system/power/tablet_power_button_controller.cc:227:
delegate->SuspendMediaSessions();
nit: i'd put this on a single line since you only use the delegate once:

  Shell::Get()->shell_delegate()->SuspendMediaSessions();

https://codereview.chromium.org/2821303004/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/2821303004/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.cc:473:
::switches::kEnableInternalMediaSession);
src/content/browser/media/session/media_session_controllers_manager.cc currently
contains the following:

bool IsMediaSessionEnabled() {
#if defined(OS_ANDROID)
  return true;
#else
  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
  return command_line->HasSwitch(switches::kEnableInternalMediaSession) ||
         command_line->HasSwitch(switches::kEnableAudioFocus);
#endif
}

i think it'd be cleaner to change the condition to also check
defined(OS_CHROMEOS), probably along with a comment explaining why this is
needed on chrome os.

that's assuming that the media session owners agree, though. :-)

[Qiang(Joe) Xu, 2017-05-23 01:16:46]

Patchset #5 (id:120001) has been deleted

[Qiang(Joe) Xu, 2017-05-23 01:19:34]

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

[commit-bot: I haz the power, 2017-05-23 01:20:00]

Dry run: CQ is trying da patch.

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

[Qiang(Joe) Xu, 2017-05-23 01:20:07]

Patchset #5 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-05-23 01:22:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Qiang(Joe) Xu, 2017-05-23 01:28:54]

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

[commit-bot: I haz the power, 2017-05-23 01:29:27]

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-05-23 02:20:53]

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

[commit-bot: I haz the power, 2017-05-23 02:20:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Qiang(Joe) Xu, 2017-05-23 17:54:46]

Hi all, PTAL thanks

https://codereview.chromium.org/2821303004/diff/100001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/2821303004/diff/100001/ash/shell_delegate.h#n...
ash/shell_delegate.h:143: // Suspends all webcontents associated media sessions
to stop managed players.
On 2017/05/22 23:38:34, Daniel Erat wrote:
> nit: "WebContents-associated"

Done.

https://codereview.chromium.org/2821303004/diff/100001/ash/system/power/table...
File ash/system/power/tablet_power_button_controller.cc (right):

https://codereview.chromium.org/2821303004/diff/100001/ash/system/power/table...
ash/system/power/tablet_power_button_controller.cc:227:
delegate->SuspendMediaSessions();
On 2017/05/22 23:38:34, Daniel Erat wrote:
> nit: i'd put this on a single line since you only use the delegate once:
> 
>   Shell::Get()->shell_delegate()->SuspendMediaSessions();

Done.

https://codereview.chromium.org/2821303004/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/2821303004/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.cc:473:
::switches::kEnableInternalMediaSession);
On 2017/05/22 23:38:34, Daniel Erat wrote:
> src/content/browser/media/session/media_session_controllers_manager.cc
currently
> contains the following:
> 
> bool IsMediaSessionEnabled() {
> #if defined(OS_ANDROID)
>   return true;
> #else
>   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
>   return command_line->HasSwitch(switches::kEnableInternalMediaSession) ||
>          command_line->HasSwitch(switches::kEnableAudioFocus);
> #endif
> }
> 
> i think it'd be cleaner to change the condition to also check
> defined(OS_CHROMEOS), probably along with a comment explaining why this is
> needed on chrome os.
> 
> that's assuming that the media session owners agree, though. :-)

Done. mlamouri@, do you agree on this?

[Daniel Erat, 2017-05-23 18:09:06]

lgtm

[Qiang(Joe) Xu, 2017-05-31 18:19:33]

friendly ping: mlamouri@, would you take another look on the new changes?
thanks!

[mlamouri (slow - plz ping), 2017-05-31 18:38:54]

On 2017/05/31 at 18:19:33, warx wrote:
> friendly ping: mlamouri@, would you take another look on the new changes?
thanks!

Sorry, I've completely missed the ping. Yes, that's reasonable. Another approach
would be to turn the flag on by default on cros and Android but this lgtm.

[Qiang(Joe) Xu, 2017-05-31 18:39:58]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-05-31 18:39:59]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2821303004/#ps190001
(title: "rebase")

[Qiang(Joe) Xu, 2017-05-31 18:40:52]

On 2017/05/31 18:38:54, mlamouri wrote:
> On 2017/05/31 at 18:19:33, warx wrote:
> > friendly ping: mlamouri@, would you take another look on the new changes?
> thanks!
> 
> Sorry, I've completely missed the ping. Yes, that's reasonable. Another
approach
> would be to turn the flag on by default on cros and Android but this lgtm.

no worries, thanks for the review!

[commit-bot: I haz the power, 2017-05-31 18:41:05]

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-05-31 21:00:23]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1496255998811670,
"parent_rev": "61e622b8b398999d4884231999375245c07dad01", "commit_rev":
"67a6d3fc1811bf403d56f59cf4099e3f88b68063"}

[commit-bot: I haz the power, 2017-05-31 21:00:30]

Description was changed from

==========
cros: Suspend media sessions with display off trigger by tablet power button

Changes:
Convertible/tablet device's side power button is described as tablet power
button. It will set display forced-off or forced-on just like android phone.
This CL is going to add extra functionality to tablet power button, which is
suspending media sessions when display is forced-off by tablet power button.

BUG=694384
TEST=tested on kevin, with two youtube browser tabs and one youtube arc++ app.
arc++ app's audio is not controlled by the tablet power button.
(1) browser tabs audio playing, pressing power button to set display off, audio
will pause.
(2) browser tabs audio pausing, pressing power button to set display off will
still pause audio, pressing again will keep audio paused.

Also added test coverage in TabletPowerButtonControllerTest.
==========

to

==========
cros: Suspend media sessions with display off trigger by tablet power button

Changes:
Convertible/tablet device's side power button is described as tablet power
button. It will set display forced-off or forced-on just like android phone.
This CL is going to add extra functionality to tablet power button, which is
suspending media sessions when display is forced-off by tablet power button.

BUG=694384
TEST=tested on kevin, with two youtube browser tabs and one youtube arc++ app.
arc++ app's audio is not controlled by the tablet power button.
(1) browser tabs audio playing, pressing power button to set display off, audio
will pause.
(2) browser tabs audio pausing, pressing power button to set display off will
still pause audio, pressing again will keep audio paused.

Also added test coverage in TabletPowerButtonControllerTest.

Review-Url: https://codereview.chromium.org/2821303004
Cr-Commit-Position: refs/heads/master@{#476018}
Committed:
https://chromium.googlesource.com/chromium/src/+/67a6d3fc1811bf403d56f59cf409...
==========

[commit-bot: I haz the power, 2017-05-31 21:00:32]

Committed patchset #7 (id:190001) as
https://chromium.googlesource.com/chromium/src/+/67a6d3fc1811bf403d56f59cf409...