Use ContentViewCore's container view to keep screen on

The window's KEEP_SCREEN_ON flag is more like controlling the screen from an Activity level, and we shouldn't use it instead, view's KEEP_SCREEN_ON flag should be used.

This patch attaches a zero size view to ContentViewCore's mContainerView instead of view tree, so once the ContainerView is invisible, the screen can turned off even PowerSaveBlocker not removed.


BUG=270903
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216840

[michaelbai, 2013-07-24 23:54:28]

I still not comfortable with attaching a new view, but it seems the better way.
I would like to apply KEEP_SCREEN_ON directly to ContainerView, but it will
screw up the WebView once application also use WebView's KEEP_SCREEN_ON bit.

[wjia(left Chromium), 2013-07-25 00:41:17]

On 2013/07/24 23:54:28, michaelbai wrote:
> I still not comfortable with attaching a new view, but it seems the better
way.
> I would like to apply KEEP_SCREEN_ON directly to ContainerView, but it will
> screw up the WebView once application also use WebView's KEEP_SCREEN_ON bit.

cc'ing Ray (punyabrata@chromium.org) to confirm WebRTC use case.

At high level, WebRTC needs ScreenOn as long as there is a tab containing one
<video>.
This can serve as a reminder for the user that camera/mic is still in use. That
means
we need the new view attached to the Window, instead of ViewGroup.

Previously, PowerSaveBlocker is managed by RenderViewHost. Now it's in
WebContentsImpl
which has to keep a map indexed by RenderViewHost. I think previous location is
better
for PowerSaveBlocker.

[michaelbai, 2013-07-25 03:14:16]

On 2013/07/25 00:41:17, wjia wrote:
> On 2013/07/24 23:54:28, michaelbai wrote:
> > I still not comfortable with attaching a new view, but it seems the better
> way.
> > I would like to apply KEEP_SCREEN_ON directly to ContainerView, but it will
> > screw up the WebView once application also use WebView's KEEP_SCREEN_ON bit.
> 
> cc'ing Ray (mailto:punyabrata@chromium.org) to confirm WebRTC use case.
> 
> At high level, WebRTC needs ScreenOn as long as there is a tab containing one
> <video>.
> This can serve as a reminder for the user that camera/mic is still in use.
That
> means
> we need the new view attached to the Window, instead of ViewGroup.
> 

This behavior is different from video playback. We might need to 2 type of
locker 


> Previously, PowerSaveBlocker is managed by RenderViewHost. Now it's in
> WebContentsImpl
> which has to keep a map indexed by RenderViewHost. I think previous location
is
> better
> for PowerSaveBlocker.

The reason to move it to WebContentsImpl is RenderViewHost shouldn't know
anything about WebContents, though you access WebContents inside
PowerSaveBlockerImpl, it is still not OK.

[wjia(left Chromium), 2013-07-25 16:52:41]

On 2013/07/25 03:14:16, michaelbai wrote:
> The reason to move it to WebContentsImpl is RenderViewHost shouldn't know
> anything about WebContents, though you access WebContents inside
> PowerSaveBlockerImpl, it is still not OK.

How should WebContents::FromRenderViewHost(rvh) be used? I have seen it used in
content/ and chrome/ quite often:
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...

[michaelbai, 2013-07-25 17:25:55]

On 2013/07/25 16:52:41, wjia wrote:
> On 2013/07/25 03:14:16, michaelbai wrote:
> > The reason to move it to WebContentsImpl is RenderViewHost shouldn't know
> > anything about WebContents, though you access WebContents inside
> > PowerSaveBlockerImpl, it is still not OK.
> 
> How should WebContents::FromRenderViewHost(rvh) be used? I have seen it used
in
> content/ and chrome/ quite often:
>
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...

As to the Window level blocker, it will not meet WebRTC's need, the screen will
be turned off once clank is not in the foreground.

The Android already has the icon to show the video and audio input is used by
Chrome. When GPS is used, the screen could be turn off, and system show the GPS
icon to warning the user, you might not need to keep screen in this case. This
is just my thought.

Location, we only need WebContents or NativeWindow as param of
InitDisplaySleepBlocker, so we have to get it before the PowerSaveBlocker is
created, You can see there is no one calling FromRenderViewHost(rvh) in
RenderViewHostImpl.cc.

[punyabrata1, 2013-07-25 18:23:19]

On 2013/07/25 17:25:55, michaelbai wrote:
> On 2013/07/25 16:52:41, wjia wrote:
> > On 2013/07/25 03:14:16, michaelbai wrote:
> > > The reason to move it to WebContentsImpl is RenderViewHost shouldn't know
> > > anything about WebContents, though you access WebContents inside
> > > PowerSaveBlockerImpl, it is still not OK.
> > 
> > How should WebContents::FromRenderViewHost(rvh) be used? I have seen it used
> in
> > content/ and chrome/ quite often:
> >
>
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...
> 
> As to the Window level blocker, it will not meet WebRTC's need, the screen
will
> be turned off once clank is not in the foreground.
> 
> The Android already has the icon to show the video and audio input is used by
> Chrome. When GPS is used, the screen could be turn off, and system show the
GPS
> icon to warning the user, you might not need to keep screen in this case. This
> is just my thought.
> 
> Location, we only need WebContents or NativeWindow as param of
> InitDisplaySleepBlocker, so we have to get it before the PowerSaveBlocker is
> created, You can see there is no one calling FromRenderViewHost(rvh) in
> RenderViewHostImpl.cc.

Hi,
I worry that the video being on and transmitting is slightly more dangerous than
the GPS in terms of privacy. Is there nothing that can be done to keep the
screen alive so that the user can at least get a chance to see the camera icon
in the Android system tray? On desktops, the camera light at least turns on but
this is not an option for mobile devices (we have discussed trying to use the
LED but it has been turned down for now).
Thanks,
Ray

[michaelbai, 2013-07-26 19:50:04]

On 2013/07/25 18:23:19, punyabrata1 wrote:
> On 2013/07/25 17:25:55, michaelbai wrote:
> > On 2013/07/25 16:52:41, wjia wrote:
> > > On 2013/07/25 03:14:16, michaelbai wrote:
> > > > The reason to move it to WebContentsImpl is RenderViewHost shouldn't
know
> > > > anything about WebContents, though you access WebContents inside
> > > > PowerSaveBlockerImpl, it is still not OK.
> > > 
> > > How should WebContents::FromRenderViewHost(rvh) be used? I have seen it
used
> > in
> > > content/ and chrome/ quite often:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...
> > 
> > As to the Window level blocker, it will not meet WebRTC's need, the screen
> will
> > be turned off once clank is not in the foreground.
> > 
> > The Android already has the icon to show the video and audio input is used
by
> > Chrome. When GPS is used, the screen could be turn off, and system show the
> GPS
> > icon to warning the user, you might not need to keep screen in this case.
This
> > is just my thought.
> > 
> > Location, we only need WebContents or NativeWindow as param of
> > InitDisplaySleepBlocker, so we have to get it before the PowerSaveBlocker is
> > created, You can see there is no one calling FromRenderViewHost(rvh) in
> > RenderViewHostImpl.cc.
> 
> Hi,
> I worry that the video being on and transmitting is slightly more dangerous
than
> the GPS in terms of privacy. Is there nothing that can be done to keep the
> screen alive so that the user can at least get a chance to see the camera icon
> in the Android system tray? On desktops, the camera light at least turns on
but
> this is not an option for mobile devices (we have discussed trying to use the
> LED but it has been turned down for now).
> Thanks,
> Ray




Hi Jia,

Do you still have any concern about this patch? If not, let's get it checked in.

[wjia(left Chromium), 2013-07-26 21:10:27]

How about Ray's comment?

Wei


On Fri, Jul 26, 2013 at 12:50 PM, <michaelbai@chromium.org> wrote:

> On 2013/07/25 18:23:19, punyabrata1 wrote:
>
>> On 2013/07/25 17:25:55, michaelbai wrote:
>> > On 2013/07/25 16:52:41, wjia wrote:
>> > > On 2013/07/25 03:14:16, michaelbai wrote:
>> > > > The reason to move it to WebContentsImpl is RenderViewHost shouldn't
>>
> know
>
>> > > > anything about WebContents, though you access WebContents inside
>> > > > PowerSaveBlockerImpl, it is still not OK.
>> > >
>> > > How should WebContents::**FromRenderViewHost(rvh) be used? I have
>> seen it
>>
> used
>
>> > in
>> > > content/ and chrome/ quite often:
>> > >
>> >
>>
>
> https://code.google.com/p/**chromium/codesearch#search/&**
>
sq=package:chromium&type=cs&q=**WebContents::**FromRenderViewHost<https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&type=cs&q=WebContents::FromRenderViewHost>
>
>> >
>> > As to the Window level blocker, it will not meet WebRTC's need, the
>> screen
>> will
>> > be turned off once clank is not in the foreground.
>> >
>> > The Android already has the icon to show the video and audio input is
>> used
>>
> by
>
>> > Chrome. When GPS is used, the screen could be turn off, and system show
>> the
>> GPS
>> > icon to warning the user, you might not need to keep screen in this
>> case.
>>
> This
>
>> > is just my thought.
>> >
>> > Location, we only need WebContents or NativeWindow as param of
>> > InitDisplaySleepBlocker, so we have to get it before the
>> PowerSaveBlocker is
>> > created, You can see there is no one calling FromRenderViewHost(rvh) in
>> > RenderViewHostImpl.cc.
>>
>
>  Hi,
>> I worry that the video being on and transmitting is slightly more
>> dangerous
>>
> than
>
>> the GPS in terms of privacy. Is there nothing that can be done to keep the
>> screen alive so that the user can at least get a chance to see the camera
>> icon
>> in the Android system tray? On desktops, the camera light at least turns
>> on
>>
> but
>
>> this is not an option for mobile devices (we have discussed trying to use
>> the
>> LED but it has been turned down for now).
>> Thanks,
>> Ray
>>
>
>
>
>
> Hi Jia,
>
> Do you still have any concern about this patch? If not, let's get it
> checked in.
>
>
https://codereview.chromium.**org/20125004/<https://codereview.chromium.org/2...
>

[michaelbai, 2013-07-26 21:49:58]

I don't know whether there is any API can keep screen on globally. Why the LED
light wouldn't work?

[wjia(left Chromium), 2013-07-26 23:34:22]

On 2013/07/26 21:49:58, michaelbai wrote:
> I don't know whether there is any API can keep screen on globally. Why the LED
> light wouldn't work?

There is no LED indicator for camera on mobile devices. It's different from
webcam.

[michaelbai, 2013-07-26 23:51:35]

On 2013/07/26 23:34:22, wjia wrote:
> On 2013/07/26 21:49:58, michaelbai wrote:
> > I don't know whether there is any API can keep screen on globally. Why the
LED
> > light wouldn't work?
> 
> There is no LED indicator for camera on mobile devices. It's different from
> webcam.

I meant Notification LED,
http://developer.android.com/design/patterns/notifications.html

[wjia(left Chromium), 2013-07-27 01:51:07]

We are fine with this patch since video will not be delivered to the remote
once the tab goes background.

Thanks for fixing this!

Wei


On Fri, Jul 26, 2013 at 4:51 PM, <michaelbai@chromium.org> wrote:

> On 2013/07/26 23:34:22, wjia wrote:
>
>> On 2013/07/26 21:49:58, michaelbai wrote:
>> > I don't know whether there is any API can keep screen on globally. Why
>> the
>>
> LED
>
>> > light wouldn't work?
>>
>
>  There is no LED indicator for camera on mobile devices. It's different
>> from
>> webcam.
>>
>
> I meant Notification LED,
>
http://developer.android.com/**design/patterns/notifications.**html<http://de...
>
>
https://codereview.chromium.**org/20125004/<https://codereview.chromium.org/2...
>

[joth, 2013-08-06 18:40:15]

Sorry -  I wrote feedback on this but failed to hit 'send'

https://codereview.chromium.org/20125004/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/20125004/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2925:
public void setKeepScreenOn(boolean keepScreenOn) {
the aim is to reduce the amount of peripheral stuff like this in ContentViewCore
rather than add more.


I understand you don't want to add the view to the WindowAndroid (i.e. to the
ViewRootImpl) as that will not respect the parent view's visibility. Sounds
reasonable.
In that case, can these methods be plumbed out via ViewAndroid /
ViewAndroidDelegate instead? That stops PowerSaveBlocker having to be
WebContentsImpl & ContentViewCore specific.

It may even be that the existing ViewAndroidDelegate.acquireAnchorView() is
appropriate to use for the purpose you have here. Just request an anchor view
and (if not null) set the KEEP_SCREEN_ON bit on it. Then remove the anchor view
when you're done.

[michaelbai, 2013-08-06 22:10:57]

PTAL

https://codereview.chromium.org/20125004/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/20125004/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2925:
public void setKeepScreenOn(boolean keepScreenOn) {
On 2013/08/06 18:40:15, joth wrote:
> the aim is to reduce the amount of peripheral stuff like this in
ContentViewCore
> rather than add more.
> 
> 
> I understand you don't want to add the view to the WindowAndroid (i.e. to the
> ViewRootImpl) as that will not respect the parent view's visibility. Sounds
> reasonable.
> In that case, can these methods be plumbed out via ViewAndroid /
> ViewAndroidDelegate instead? That stops PowerSaveBlocker having to be
> WebContentsImpl & ContentViewCore specific.
> 
> It may even be that the existing ViewAndroidDelegate.acquireAnchorView() is
> appropriate to use for the purpose you have here. Just request an anchor view
> and (if not null) set the KEEP_SCREEN_ON bit on it. Then remove the anchor
view
> when you're done. 

Done.

[joth, 2013-08-08 21:48:57]

lgtm with a couple nits

Thanks!

https://codereview.chromium.org/20125004/diff/21001/content/browser/power_sav...
File content/browser/power_save_blocker_impl.h (right):

https://codereview.chromium.org/20125004/diff/21001/content/browser/power_sav...
content/browser/power_save_blocker_impl.h:14: class WebContents;
not needed now

https://codereview.chromium.org/20125004/diff/21001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ViewAndroid.java (right):

https://codereview.chromium.org/20125004/diff/21001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ViewAndroid.java:73: } else {
suggest: assert mKeepScreenOnCount > 0

[michaelbai, 2013-08-09 17:46:51]

jam, content/browser/
Yaron, ui/android, content/public/android.

Thanks

https://codereview.chromium.org/20125004/diff/21001/content/browser/power_sav...
File content/browser/power_save_blocker_impl.h (right):

https://codereview.chromium.org/20125004/diff/21001/content/browser/power_sav...
content/browser/power_save_blocker_impl.h:14: class WebContents;
On 2013/08/08 21:48:57, joth wrote:
> not needed now

Done.

https://codereview.chromium.org/20125004/diff/21001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ViewAndroid.java (right):

https://codereview.chromium.org/20125004/diff/21001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ViewAndroid.java:73: } else {
On 2013/08/08 21:48:57, joth wrote:
> suggest: assert mKeepScreenOnCount > 0

Done.

[Yaron, 2013-08-09 17:56:32]

This should have a BUG=
content is trivially looks good.
+ newt for ui/

[michaelbai, 2013-08-09 18:36:51]

On 2013/08/09 17:56:32, Yaron wrote:
> This should have a BUG=
> content is trivially looks good.
> + newt for ui/

Bug have been added.

[jam, 2013-08-09 20:32:58]

lgtm

[newt (away), 2013-08-09 20:57:21]

https://codereview.chromium.org/20125004/diff/27001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ViewAndroid.java (right):

https://codereview.chromium.org/20125004/diff/27001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ViewAndroid.java:66: public void
setKeepScreenOn(boolean keepScreenOn) {
I think it should be more clear that this method is *not* a traditional setter.
Perhaps it could be split into two functions named
"incrementKeepScreenOnCount()" and "decrementKeepScreenOnCount()". The javadoc
should also be clear about this.

[michaelbai, 2013-08-09 23:06:50]

https://codereview.chromium.org/20125004/diff/27001/ui/android/java/src/org/c...
File ui/android/java/src/org/chromium/ui/ViewAndroid.java (right):

https://codereview.chromium.org/20125004/diff/27001/ui/android/java/src/org/c...
ui/android/java/src/org/chromium/ui/ViewAndroid.java:66: public void
setKeepScreenOn(boolean keepScreenOn) {
It was named by Android's convention, but yes, the increment/decrement are
better. 


On 2013/08/09 20:57:21, newt wrote:
> I think it should be more clear that this method is *not* a traditional
setter.
> Perhaps it could be split into two functions named
> "incrementKeepScreenOnCount()" and "decrementKeepScreenOnCount()". The javadoc
> should also be clear about this.

[newt (away), 2013-08-10 00:14:22]

thanks. lgtm!

[joth, 2013-08-10 01:31:06]

lgtm

[commit-bot: I haz the power, 2013-08-10 01:34:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/20125004/35001

[commit-bot: I haz the power, 2013-08-10 13:48:42]

Change committed as 216840