[WebView] Create PowerSaveBlocker for fullscreen video.

[WebView] Create PowerSaveBlocker for fullscreen video.

BUG=417623
Committed: https://crrev.com/ec9b8860c670221d8bfcfbe628f0597a5b6bd4f6
Cr-Commit-Position: refs/heads/master@{#296738}

[Ignacio Solla, 2014-09-25 10:36:02]

https://codereview.chromium.org/606633002/diff/1/content/browser/android/cont...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/606633002/diff/1/content/browser/android/cont...
content/browser/android/content_video_view.cc:265: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
Some context: kDisableOverlayFullscreenVideoSubtitle was used by embedders that
did not support html5 video controls. Both Clank and WebView now support html5
video controls, so I will be removing this flag very soon. The problem is that
during the transition in the WebView we inadvertently broke power save in
fullscreen, because the fullscreen power-save feature was incorrectly associated
with the html5 controls feature.

[Ignacio Solla, 2014-09-25 10:38:44]

igsolla@chromium.org changed reviewers:
+ avi@chromium.org, mkosiba@chromium.org

[Ignacio Solla, 2014-09-25 10:38:44]

mkosiba@chromium.org: Please review changes in android_webview/

avi@chromium.org: Please review changes in content/

[mkosiba (inactive), 2014-09-25 14:42:33]

lgtm

[boliu, 2014-09-25 14:58:39]

drive by. Can you find a way to *not* add a command line switch? That's the
nuclear method of passing around options.

[Ignacio Solla, 2014-09-25 15:15:54]

On 2014/09/25 14:58:39, boliu wrote:
> drive by. Can you find a way to *not* add a command line switch? That's the
> nuclear method of passing around options.

We will need to change this again when we add support for fullscreen for
non-media elements as we will need to create a PowerSaveBlocker too for videos
contained in a fullscreen div so I don't think that doing something more
complicated is worth the effort for now. But we need this fix now to merge into
Android.

[Ignacio Solla, 2014-09-25 15:19:05]

igsolla@chromium.org changed reviewers:
- avi@chromium.org

[Ignacio Solla, 2014-09-25 15:19:21]

igsolla@chromium.org changed reviewers:
+ jam@chromium.org

[Ignacio Solla, 2014-09-25 15:19:22]

jam@chromium.org: Please review changes in content/

[boliu, 2014-09-25 15:22:55]

On 2014/09/25 15:15:54, Ignacio Solla wrote:
> On 2014/09/25 14:58:39, boliu wrote:
> > drive by. Can you find a way to *not* add a command line switch? That's the
> > nuclear method of passing around options.
> 
> We will need to change this again when we add support for fullscreen for
> non-media elements as we will need to create a PowerSaveBlocker too for videos
> contained in a fullscreen div so I don't think that doing something more
> complicated is worth the effort for now. But we need this fix now to merge
into
> Android.

How about my propsal in
https://code.google.com/p/chromium/issues/detail?id=417623#c3 ?

[Ignacio Solla, 2014-09-25 15:23:21]

On 2014/09/25 15:22:55, boliu wrote:
> On 2014/09/25 15:15:54, Ignacio Solla wrote:
> > On 2014/09/25 14:58:39, boliu wrote:
> > > drive by. Can you find a way to *not* add a command line switch? That's
the
> > > nuclear method of passing around options.
> > 
> > We will need to change this again when we add support for fullscreen for
> > non-media elements as we will need to create a PowerSaveBlocker too for
videos
> > contained in a fullscreen div so I don't think that doing something more
> > complicated is worth the effort for now. But we need this fix now to merge
> into
> > Android.
> 
> How about my propsal in
> https://code.google.com/p/chromium/issues/detail?id=417623#c3 ?

Please see my response.

[Avi (use Gerrit), 2014-09-25 15:24:58]

avi@chromium.org changed reviewers:
+ avi@chromium.org
- jam@chromium.org

[Avi (use Gerrit), 2014-09-25 15:24:59]

Small fix needed.

https://codereview.chromium.org/606633002/diff/1/content/public/common/conten...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/606633002/diff/1/content/public/common/conten...
content/public/common/content_switches.cc:386:
"enable-content-video-view-power-save-blocker";
Platform-specific switches go at the end inside the OS_ANDROID block that
already exists.

https://codereview.chromium.org/606633002/diff/1/content/public/common/conten...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/606633002/diff/1/content/public/common/conten...
content/public/common/content_switches.h:119: CONTENT_EXPORT extern const char
kEnableContentVideoViewPowerSaveBlocker[];
Platform-specific switches go at the end inside the OS_ANDROID block that
already exists.

[boliu, 2014-09-25 15:30:56]

On 2014/09/25 15:23:21, Ignacio Solla wrote:
> On 2014/09/25 15:22:55, boliu wrote:
> > On 2014/09/25 15:15:54, Ignacio Solla wrote:
> > > On 2014/09/25 14:58:39, boliu wrote:
> > > > drive by. Can you find a way to *not* add a command line switch? That's
> the
> > > > nuclear method of passing around options.
> > > 
> > > We will need to change this again when we add support for fullscreen for
> > > non-media elements as we will need to create a PowerSaveBlocker too for
> videos
> > > contained in a fullscreen div so I don't think that doing something more
> > > complicated is worth the effort for now. But we need this fix now to merge
> > into
> > > Android.
> > 
> > How about my propsal in
> > https://code.google.com/p/chromium/issues/detail?id=417623#c3 ?
> 
> Please see my response.

Which part?

I really don't see how it needs to be more complicated. You add a call to
keepScreenOn(true) in FullScreenView.onAttachedToWindow, and
keepScreenOff(false) in detach. And it's all in android webview code so it's
easier for merging etc. And it's a *lot* simpler than this.

[Ignacio Solla, 2014-09-25 15:37:18]

On 2014/09/25 15:30:56, boliu wrote:
> On 2014/09/25 15:23:21, Ignacio Solla wrote:
> > On 2014/09/25 15:22:55, boliu wrote:
> > > On 2014/09/25 15:15:54, Ignacio Solla wrote:
> > > > On 2014/09/25 14:58:39, boliu wrote:
> > > > > drive by. Can you find a way to *not* add a command line switch?
That's
> > the
> > > > > nuclear method of passing around options.
> > > > 
> > > > We will need to change this again when we add support for fullscreen for
> > > > non-media elements as we will need to create a PowerSaveBlocker too for
> > videos
> > > > contained in a fullscreen div so I don't think that doing something more
> > > > complicated is worth the effort for now. But we need this fix now to
merge
> > > into
> > > > Android.
> > > 
> > > How about my propsal in
> > > https://code.google.com/p/chromium/issues/detail?id=417623#c3 ?
> > 
> > Please see my response.
> 
> Which part?
> 
> I really don't see how it needs to be more complicated. You add a call to
> keepScreenOn(true) in FullScreenView.onAttachedToWindow, and
> keepScreenOff(false) in detach. And it's all in android webview code so it's
> easier for merging etc. And it's a *lot* simpler than this.

If the fullscreenView is attached but the video is paused or has ended we don't
want to block the power save. I meant my response in the bug.

[Ignacio Solla, 2014-09-25 15:38:03]

https://codereview.chromium.org/606633002/diff/1/content/public/common/conten...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/606633002/diff/1/content/public/common/conten...
content/public/common/content_switches.cc:386:
"enable-content-video-view-power-save-blocker";
On 2014/09/25 15:24:58, Avi wrote:
> Platform-specific switches go at the end inside the OS_ANDROID block that
> already exists.

Done.

https://codereview.chromium.org/606633002/diff/1/content/public/common/conten...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/606633002/diff/1/content/public/common/conten...
content/public/common/content_switches.h:119: CONTENT_EXPORT extern const char
kEnableContentVideoViewPowerSaveBlocker[];
On 2014/09/25 15:24:59, Avi wrote:
> Platform-specific switches go at the end inside the OS_ANDROID block that
> already exists.

Done.

[boliu, 2014-09-25 15:39:55]

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

[boliu, 2014-09-25 15:39:56]

https://codereview.chromium.org/606633002/diff/20001/content/browser/android/...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/606633002/diff/20001/content/browser/android/...
content/browser/android/content_video_view.cc:266:
switches::kEnableContentVideoViewPowerSaveBlocker)) {
nit: So if I'm reading this correctly, ignoring the added comment, you are just
changing kDisableOverlayFullscreenVideoSubtitle to
kEnableContentVideoViewPowerSaveBlocker, right? If so can you code it that way
and not change the structure of the function?

At this point I don't know what either switch does :( Maybe get Min to look over
this?

[Avi (use Gerrit), 2014-09-25 15:41:19]

If everything in the file is in alphabetical order, please follow.

https://codereview.chromium.org/606633002/diff/20001/content/public/common/co...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/606633002/diff/20001/content/public/common/co...
content/public/common/content_switches.h:260: CONTENT_EXPORT extern const char
kEnableContentVideoViewPowerSaveBlocker[];
e comes before r.

[Ignacio Solla, 2014-09-25 15:43:38]

https://codereview.chromium.org/606633002/diff/20001/content/browser/android/...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/606633002/diff/20001/content/browser/android/...
content/browser/android/content_video_view.cc:266:
switches::kEnableContentVideoViewPowerSaveBlocker)) {
On 2014/09/25 15:39:56, boliu wrote:
> nit: So if I'm reading this correctly, ignoring the added comment, you are
just
> changing kDisableOverlayFullscreenVideoSubtitle to
> kEnableContentVideoViewPowerSaveBlocker, right? If so can you code it that way
> and not change the structure of the function?
> 
> At this point I don't know what either switch does :( Maybe get Min to look
over
> this?

Bo, with this change we will create the blocker in the WebView regardless of
whether we use html5 video controls or not.

Without this change we are not creating the blocker because we have enabled
html5 video controls.

Please see my follow-up path to remove the
kEnableContentVideoViewPowerSaveBlocker setting
https://codereview.chromium.org/597523003/

[Ignacio Solla, 2014-09-25 15:46:57]

https://codereview.chromium.org/606633002/diff/20001/content/public/common/co...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/606633002/diff/20001/content/public/common/co...
content/public/common/content_switches.h:260: CONTENT_EXPORT extern const char
kEnableContentVideoViewPowerSaveBlocker[];
On 2014/09/25 15:41:19, Avi wrote:
> e comes before r.

Done.

[boliu, 2014-09-25 15:50:34]

https://codereview.chromium.org/606633002/diff/20001/content/browser/android/...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/606633002/diff/20001/content/browser/android/...
content/browser/android/content_video_view.cc:266:
switches::kEnableContentVideoViewPowerSaveBlocker)) {
On 2014/09/25 15:43:38, Ignacio Solla wrote:
> On 2014/09/25 15:39:56, boliu wrote:
> > nit: So if I'm reading this correctly, ignoring the added comment, you are
> just
> > changing kDisableOverlayFullscreenVideoSubtitle to
> > kEnableContentVideoViewPowerSaveBlocker, right? If so can you code it that
way
> > and not change the structure of the function?
> > 
> > At this point I don't know what either switch does :( Maybe get Min to look
> over
> > this?
> 
> Bo, with this change we will create the blocker in the WebView regardless of
> whether we use html5 video controls or not.
> 
> Without this change we are not creating the blocker because we have enabled
> html5 video controls.
> 
> Please see my follow-up path to remove the
> kEnableContentVideoViewPowerSaveBlocker setting
> https://codereview.chromium.org/597523003/

You removed the kDisableOverlayFullscreenVideoSubtitle, not this one :p

Anyway, I just had that nit. I'm not owner here or anything.

[boliu, 2014-09-25 16:31:23]

Btw I'm happy, in case Avi is waiting for my complaints to be resolved.

[Avi (use Gerrit), 2014-09-25 17:39:54]

LGTM

[Ignacio Solla, 2014-09-25 17:40:41]

The CQ bit was checked by igsolla@chromium.org

[commit-bot: I haz the power, 2014-09-25 17:41:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/606633002/40001

[commit-bot: I haz the power, 2014-09-25 17:46:30]

Committed patchset #3 (id:40001) as fc9387fe8d66ba58f9058a7ca72069e024b7da90

[commit-bot: I haz the power, 2014-09-25 17:47:33]

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