Keep screen on when there is an active WebRTC session on Android.

Keep screen on when there is an active WebRTC session on Android.

When WebRTC session has video channel, the screen is kept on.

R=bulach@chromium.org, joth@chromium.org, qinmin@chromium.org, sky@chromium.org

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

[mkosiba (inactive), 2013-05-17 09:34:46]

https://chromiumcodereview.appspot.com/15035013/diff/1/content/browser/power_...
File content/browser/power_save_blocker_android.cc (right):

https://chromiumcodereview.appspot.com/15035013/diff/1/content/browser/power_...
content/browser/power_save_blocker_android.cc:59: BrowserThread::PostTask(
rather than spamming the UI thread maybe make the increment and decrement atomic
and post to the UI thread only if you need to call KeepScreenOn.

also, you can check the type_ before posting.

[wjia(left Chromium), 2013-05-17 18:04:43]

https://chromiumcodereview.appspot.com/15035013/diff/1/content/browser/power_...
File content/browser/power_save_blocker_android.cc (right):

https://chromiumcodereview.appspot.com/15035013/diff/1/content/browser/power_...
content/browser/power_save_blocker_android.cc:59: BrowserThread::PostTask(
On 2013/05/17 09:34:47, mkosiba wrote:
> rather than spamming the UI thread maybe make the increment and decrement
atomic
> and post to the UI thread only if you need to call KeepScreenOn.
> 
> also, you can check the type_ before posting.

Done.

[wjia(left Chromium), 2013-05-20 21:00:19]

ping.

[joth, 2013-05-20 22:10:36]

seems gtm, but +bulach to approve from power usage correctness side.

+michaelbai FYI - your other ContentVideoViewDelegate refactor will need another
rebase....

https://chromiumcodereview.appspot.com/15035013/diff/10001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java
(right):

https://chromiumcodereview.appspot.com/15035013/diff/10001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java:50:
mActivity.getWindow().clearFlags(WindowManager.LayoutParams.FLAG_KEEP_SCREEN_ON);
nit: use { } in java (unless it all fits on one line, no else clause)

[wjia(left Chromium), 2013-05-20 22:18:03]

braces have been added in patch set 4.

https://chromiumcodereview.appspot.com/15035013/diff/10001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java
(right):

https://chromiumcodereview.appspot.com/15035013/diff/10001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java:50:
mActivity.getWindow().clearFlags(WindowManager.LayoutParams.FLAG_KEEP_SCREEN_ON);
On 2013/05/20 22:10:36, joth wrote:
> nit: use { } in java (unless it all fits on one line, no else clause)

Done.

[bulach, 2013-05-21 06:58:18]

lgtm, but qinmin owns the media module. I'm curious how regular videos keep the
screen on and why is it different for the webrtc case?

[qinmin, 2013-05-21 14:49:56]

https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
File content/browser/power_save_blocker_android.cc (right):

https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
content/browser/power_save_blocker_android.cc:56: if (count == 1) {
hmm...I think you don't need the global here. You can just use a static member
variable in Delegate, and check if it is 0 or 1 when applyblock() and
removeblock() are called. Delegate always live on the UI thread and it is
threadsafe, so there will be no race conditions.

[wjia(left Chromium), 2013-05-21 16:13:51]

On 2013/05/21 14:49:56, qinmin wrote:
>
https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
> File content/browser/power_save_blocker_android.cc (right):
> 
>
https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
> content/browser/power_save_blocker_android.cc:56: if (count == 1) {
> hmm...I think you don't need the global here. You can just use a static member
> variable in Delegate, and check if it is 0 or 1 when applyblock() and
> removeblock() are called. Delegate always live on the UI thread and it is
> threadsafe, so there will be no race conditions.

The first patch did all calculation on UI thread. After comment #2, the
calculation was moved into ctor/dtor in order to avoid unnecessary task posting
onto UI thread.

[qinmin, 2013-05-21 17:12:58]

Then the problem becomes: if thread A calls increment, and see the value is 1,
now thread B calls decrement, and see the value is 0, and thread B calls
removeBlock(), and then thread A calls applyblock().

Now the screen lock is held by A, and the global value is 0. If someone calls
increment again, it will try to get the screen lock again.
 
On 2013/05/21 16:13:51, wjia wrote:
> On 2013/05/21 14:49:56, qinmin wrote:
> >
>
https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
> > File content/browser/power_save_blocker_android.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
> > content/browser/power_save_blocker_android.cc:56: if (count == 1) {
> > hmm...I think you don't need the global here. You can just use a static
member
> > variable in Delegate, and check if it is 0 or 1 when applyblock() and
> > removeblock() are called. Delegate always live on the UI thread and it is
> > threadsafe, so there will be no race conditions.
> 
> The first patch did all calculation on UI thread. After comment #2, the
> calculation was moved into ctor/dtor in order to avoid unnecessary task
posting
> onto UI thread.

[qinmin, 2013-05-21 17:18:40]

https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
File content/browser/power_save_blocker_android.cc (right):

https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
content/browser/power_save_blocker_android.cc:34: const std::string reason_;
nit: you can remove this, not being used

[wjia(left Chromium), 2013-05-21 17:48:06]

On 2013/05/21 17:12:58, qinmin wrote:
> Then the problem becomes: if thread A calls increment, and see the value is 1,
> now thread B calls decrement, and see the value is 0, and thread B calls
> removeBlock(), and then thread A calls applyblock().

That wouldn't happen because increment happens only in ctor and decrement only
in dtor. When thread B calls decrement, there must be a corresponding increment
when that object is created.


> 
> Now the screen lock is held by A, and the global value is 0. If someone calls
> increment again, it will try to get the screen lock again.
>  
> On 2013/05/21 16:13:51, wjia wrote:
> > On 2013/05/21 14:49:56, qinmin wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
> > > File content/browser/power_save_blocker_android.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
> > > content/browser/power_save_blocker_android.cc:56: if (count == 1) {
> > > hmm...I think you don't need the global here. You can just use a static
> member
> > > variable in Delegate, and check if it is 0 or 1 when applyblock() and
> > > removeblock() are called. Delegate always live on the UI thread and it is
> > > threadsafe, so there will be no race conditions.
> > 
> > The first patch did all calculation on UI thread. After comment #2, the
> > calculation was moved into ctor/dtor in order to avoid unnecessary task
> posting
> > onto UI thread.

[wjia(left Chromium), 2013-05-21 17:51:03]

On 2013/05/21 17:18:40, qinmin wrote:
>
https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
> File content/browser/power_save_blocker_android.cc (right):
> 
>
https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
> content/browser/power_save_blocker_android.cc:34: const std::string reason_;
> nit: you can remove this, not being used

done.

[qinmin, 2013-05-21 17:54:53]

what is thread A calls increment, and then it calls decrement, the value becomes
0. Right before it calls RemoveBlock（）， thread B comes and call increment, the
value becomes 1, and it calls applyBlock(). Now thread A calls RemoveBlock().
Now the value is 1, and the screenlock is held by no one.

On 2013/05/21 17:48:06, wjia wrote:
> On 2013/05/21 17:12:58, qinmin wrote:
> > Then the problem becomes: if thread A calls increment, and see the value is
1,
> > now thread B calls decrement, and see the value is 0, and thread B calls
> > removeBlock(), and then thread A calls applyblock().
> 
> That wouldn't happen because increment happens only in ctor and decrement only
> in dtor. When thread B calls decrement, there must be a corresponding
increment
> when that object is created.
> 
> 
> > 
> > Now the screen lock is held by A, and the global value is 0. If someone
calls
> > increment again, it will try to get the screen lock again.
> >  
> > On 2013/05/21 16:13:51, wjia wrote:
> > > On 2013/05/21 14:49:56, qinmin wrote:
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
> > > > File content/browser/power_save_blocker_android.cc (right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
> > > > content/browser/power_save_blocker_android.cc:56: if (count == 1) {
> > > > hmm...I think you don't need the global here. You can just use a static
> > member
> > > > variable in Delegate, and check if it is 0 or 1 when applyblock() and
> > > > removeblock() are called. Delegate always live on the UI thread and it
is
> > > > threadsafe, so there will be no race conditions.
> > > 
> > > The first patch did all calculation on UI thread. After comment #2, the
> > > calculation was moved into ctor/dtor in order to avoid unnecessary task
> > posting
> > > onto UI thread.

[wjia(left Chromium), 2013-05-21 18:16:39]

On 2013/05/21 17:54:53, qinmin wrote:
> what is thread A calls increment, and then it calls decrement, the value
becomes
> 0. Right before it calls RemoveBlock（）， thread B comes and call increment, the
> value becomes 1, and it calls applyBlock(). Now thread A calls RemoveBlock().
> Now the value is 1, and the screenlock is held by no one.
> 

You are right for this case. Basically, the atomic operation should applied to
both increment/decrement and ApplyBlock/RemoveBlock. They shouldn't be split.
Now go back to patch #1.

I will add support for audio soon. So I don't check type in ctor/dtor.

> On 2013/05/21 17:48:06, wjia wrote:
> > On 2013/05/21 17:12:58, qinmin wrote:
> > > Then the problem becomes: if thread A calls increment, and see the value
is
> 1,
> > > now thread B calls decrement, and see the value is 0, and thread B calls
> > > removeBlock(), and then thread A calls applyblock().
> > 
> > That wouldn't happen because increment happens only in ctor and decrement
only
> > in dtor. When thread B calls decrement, there must be a corresponding
> increment
> > when that object is created.
> > 
> > 
> > > 
> > > Now the screen lock is held by A, and the global value is 0. If someone
> calls
> > > increment again, it will try to get the screen lock again.
> > >  
> > > On 2013/05/21 16:13:51, wjia wrote:
> > > > On 2013/05/21 14:49:56, qinmin wrote:
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
> > > > > File content/browser/power_save_blocker_android.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/15035013/diff/30001/content/browser/po...
> > > > > content/browser/power_save_blocker_android.cc:56: if (count == 1) {
> > > > > hmm...I think you don't need the global here. You can just use a
static
> > > member
> > > > > variable in Delegate, and check if it is 0 or 1 when applyblock() and
> > > > > removeblock() are called. Delegate always live on the UI thread and it
> is
> > > > > threadsafe, so there will be no race conditions.
> > > > 
> > > > The first patch did all calculation on UI thread. After comment #2, the
> > > > calculation was moved into ctor/dtor in order to avoid unnecessary task
> > > posting
> > > > onto UI thread.

[qinmin, 2013-05-21 19:42:15]

or you can protect the ctor and dtor with a global lock, so that all the task
are posted onto the UI thread with the right order.
But I would avoid guarding a ctor and dtor with lock.

[wjia(left Chromium), 2013-05-21 21:15:33]

On 2013/05/21 19:42:15, qinmin wrote:
> or you can protect the ctor and dtor with a global lock, so that all the task
> are posted onto the UI thread with the right order.
> But I would avoid guarding a ctor and dtor with lock.

You are right. That also requires a global lock which is not recommended. Could
you take a look at patch set 6?

[qinmin, 2013-05-21 21:40:24]

lgtm%nits

https://chromiumcodereview.appspot.com/15035013/diff/56001/content/browser/po...
File content/browser/power_save_blocker_android.cc (right):

https://chromiumcodereview.appspot.com/15035013/diff/56001/content/browser/po...
content/browser/power_save_blocker_android.cc:20: : type_(type) {}
nit: can move to a single line

https://chromiumcodereview.appspot.com/15035013/diff/56001/content/browser/po...
content/browser/power_save_blocker_android.cc:37: if (type_ ==
kPowerSaveBlockPreventDisplaySleep) {
nit: remove nested loops.
if (type_ != kPowerSaveBlockPreventDisplaySleep)
  return;
if (blocker_count == 0)
  ContentVideoView::KeepScreenOn(true);
++blocker_count;

https://chromiumcodereview.appspot.com/15035013/diff/56001/content/browser/po...
content/browser/power_save_blocker_android.cc:46: if (type_ ==
kPowerSaveBlockPreventDisplaySleep) {
ditto

[wjia(left Chromium), 2013-05-21 21:50:24]

For owener's stamp:
sky@ -> content/browser/
joth@ -> android_webview/

https://chromiumcodereview.appspot.com/15035013/diff/56001/content/browser/po...
File content/browser/power_save_blocker_android.cc (right):

https://chromiumcodereview.appspot.com/15035013/diff/56001/content/browser/po...
content/browser/power_save_blocker_android.cc:20: : type_(type) {}
On 2013/05/21 21:40:24, qinmin wrote:
> nit: can move to a single line

Done.

https://chromiumcodereview.appspot.com/15035013/diff/56001/content/browser/po...
content/browser/power_save_blocker_android.cc:37: if (type_ ==
kPowerSaveBlockPreventDisplaySleep) {
On 2013/05/21 21:40:24, qinmin wrote:
> nit: remove nested loops.
> if (type_ != kPowerSaveBlockPreventDisplaySleep)
>   return;
> if (blocker_count == 0)
>   ContentVideoView::KeepScreenOn(true);
> ++blocker_count;

Done.

https://chromiumcodereview.appspot.com/15035013/diff/56001/content/browser/po...
content/browser/power_save_blocker_android.cc:46: if (type_ ==
kPowerSaveBlockPreventDisplaySleep) {
On 2013/05/21 21:40:24, qinmin wrote:
> ditto

Done.

[sky, 2013-05-21 21:57:48]

https://chromiumcodereview.appspot.com/15035013/diff/72001/content/browser/po...
File content/browser/power_save_blocker_android.cc (right):

https://chromiumcodereview.appspot.com/15035013/diff/72001/content/browser/po...
content/browser/power_save_blocker_android.cc:13: int blocker_count = 0;
Description? Also, since this is scoped to PowerSaveBlockerImpl::Delegate can
you make it a static member? That way ownership if more clearly defined.

https://chromiumcodereview.appspot.com/15035013/diff/72001/content/browser/po...
content/browser/power_save_blocker_android.cc:29: PowerSaveBlockerType type_;
const

[wjia(left Chromium), 2013-05-21 22:17:03]

PTAL.

https://chromiumcodereview.appspot.com/15035013/diff/72001/content/browser/po...
File content/browser/power_save_blocker_android.cc (right):

https://chromiumcodereview.appspot.com/15035013/diff/72001/content/browser/po...
content/browser/power_save_blocker_android.cc:13: int blocker_count = 0;
On 2013/05/21 21:57:48, sky wrote:
> Description? Also, since this is scoped to PowerSaveBlockerImpl::Delegate can
> you make it a static member? That way ownership if more clearly defined.

Done.

https://chromiumcodereview.appspot.com/15035013/diff/72001/content/browser/po...
content/browser/power_save_blocker_android.cc:29: PowerSaveBlockerType type_;
On 2013/05/21 21:57:48, sky wrote:
> const

Done.

[sky, 2013-05-21 23:54:38]

LGTM

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/browser/po...
File content/browser/power_save_blocker_android.cc (right):

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/browser/po...
content/browser/power_save_blocker_android.cc:16: Delegate(PowerSaveBlockerType
type) : type_(type) {}
nit: explicit

[joth, 2013-05-21 23:56:07]

lgtm % nit

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/browser/po...
File content/browser/power_save_blocker_android.cc (right):

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/browser/po...
content/browser/power_save_blocker_android.cc:42:
ContentVideoView::KeepScreenOn(true);
Having PowerSaveBlocker depend on ContentVideoView seems a  random dependency.
And as this is static, it relies on the (possibly null?) static instance inside
ContentVideoView to get to the activity to find the window is awkward; it would
be better if PowerSaveBlocker instances could be associated with a specific
android view, and then you would  add/remove View.KEEP_SCREEN_ON flag to that
view only.
That's a pretty major change to PowerSaveBlocker though I imagine, and I can't
think of anything less brittle to suggest.

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:598:
sDelegate.keepScreenOn(screenOn);
nit: in java one line if must be all on one line.

[wjia(left Chromium), 2013-05-22 02:17:51]

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/browser/po...
File content/browser/power_save_blocker_android.cc (right):

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/browser/po...
content/browser/power_save_blocker_android.cc:16: Delegate(PowerSaveBlockerType
type) : type_(type) {}
On 2013/05/21 23:54:38, sky wrote:
> nit: explicit

Done.

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/browser/po...
content/browser/power_save_blocker_android.cc:42:
ContentVideoView::KeepScreenOn(true);
On 2013/05/21 23:56:08, joth wrote:
> Having PowerSaveBlocker depend on ContentVideoView seems a  random dependency.
> And as this is static, it relies on the (possibly null?) static instance
inside
> ContentVideoView to get to the activity to find the window is awkward; it
would
> be better if PowerSaveBlocker instances could be associated with a specific
> android view, and then you would  add/remove View.KEEP_SCREEN_ON flag to that
> view only.
> That's a pretty major change to PowerSaveBlocker though I imagine, and I can't
> think of anything less brittle to suggest.

Yeah, we could iterate later.

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:598:
sDelegate.keepScreenOn(screenOn);
On 2013/05/21 23:56:08, joth wrote:
> nit: in java one line if must be all on one line.

Done.

[wjia(left Chromium), 2013-05-22 02:21:48]

Committed patchset #10 manually as r201424 (presubmit successful).

[joth, 2013-05-24 18:06:59]

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/browser/po...
File content/browser/power_save_blocker_android.cc (right):

https://chromiumcodereview.appspot.com/15035013/diff/77001/content/browser/po...
content/browser/power_save_blocker_android.cc:42:
ContentVideoView::KeepScreenOn(true);
On 2013/05/22 02:17:51, wjia wrote:
> On 2013/05/21 23:56:08, joth wrote:
> > Having PowerSaveBlocker depend on ContentVideoView seems a  random
dependency.
> > And as this is static, it relies on the (possibly null?) static instance
> inside
> > ContentVideoView to get to the activity to find the window is awkward; it
> would
> > be better if PowerSaveBlocker instances could be associated with a specific
> > android view, and then you would  add/remove View.KEEP_SCREEN_ON flag to
that
> > view only.
> > That's a pretty major change to PowerSaveBlocker though I imagine, and I
can't
> > think of anything less brittle to suggest.
> 
> Yeah, we could iterate later.

I realized part of the answer - the screen_on state should be handled via
ViewAndroid and/or WindowAndroid. In terms of correctness, perhaps the nicest
thing would be every time a ApplyBlock() call is made we create a small View
instance with the KEEP_SCREEN_ON flag and attach to the window, and
RemoveBlock() releases that View. This way it plays nicely with any other usage
of the flags that an application may have (i.e. we don't want to remove the flag
from the window if the application is also keeping it on for another reason we
don't know about)

The only remaining question then is from here (effectively, from the
PowerSaveBlocker c'tor) how do we find the appropriate WindowAndroid to attach
to? I think that implies a cross platform change to always pass the window (or
view) into said c'tor.