Watching a video is not pleasant when the display times put every 30 seconds. when the user is playing a video in fullscreen we can be reasonably sure that its their primary activity

Agreed. Watching a video entails involves not touching the screen for long periods of time, but that doesn't mean you're not using the device.

Looks pretty straightforward from the Android side, http://developer.android.com/reference/android/os/PowerManager.html#SCREEN_BRIGHT_WAKE_LOCK Question is though where to do this from our code. We have a |MakeFullScreen| that's called when entering and exiting fullscreen, but I am assuming we only want to disable the screensaver when (1) fullscreen for *video* (and not other content), and (2) the video is *playing* - is that correct?

IMO, this shouldn't be tied to MakeFullScreen(). I think your assumptions are roughly correct, but more generally this is a decision that should be pushed out the front end. So we should just provide a scriptable method to trigger it. nsIScreen::PreventTimeout()/AllowTimeout()? We'll have to play the interface frozen dance here regardless.

What if the user is just watching a Flash video on a site like Youtube, non-full-screen? Shouldn't we be keeping the screen awake for that too?

This is functional, so seems like a good time to check if the interfaces etc. are agreeable. Overview: * WakeLockWidget is an abstract base class that manages a list of wake locks. Widgets that support wake locks should inherit from it, and implement SetWakeLevel(). * SetWakeLevel() is called with the current wake lock level. That is, implementations don't need to mess with the concept of several wake locks existing - SetWakeLevel() is called with the current actual wake level. * On Android, the implementation creates a wake lock of the proper level. That is, the OS also has a list of wake locks, which might seem unnecessary. However, other implementations might work in other ways (they might have a simple "disable screensaver" call, for example - that seems to be the case on GTK).

mobile-browser portion. This currently will lock the screen when in fullscreen video mode, regardless of whether playing or not, so it isn't ready yet. Posting it now since the other patch might be easier to read with this alongside.

Comment on attachment 498261 [details] [diff] [review] patch >@@ -1,9 +1,9 @@ >-/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- >+/** don't delete the mode line >@@ -1,9 +1,9 @@ >-/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- >+/** no... really, don't delete my mode lines > } > > public static void hideProgressDialog() { > if (GeckoApp.mAppContext.mProgressDialog != null) { > GeckoApp.mAppContext.mProgressDialog.dismiss(); > GeckoApp.mAppContext.mProgressDialog = null; > } > } >+ >+ public static void acquireWakeLock(int type) { >+ PowerManager pm = (PowerManager) GeckoApp.surfaceView.getContext().getSystemService(Context.POWER_SERVICE); use GeckoApp.mAppContext not GeckoApp.surfaceView.getContext() >+ long acquireWakeLock(in long type); I think this maps too directly onto the android functionality. I hadn't thought of the dim versus turn off though. Perhaps enableScreenMode with normal/dimmable/no-sleep. I'm not sure though, roc what do you think here? I'm gonna r- for now, I'd like to hear what roc thinks.

Working mobile-browser patch - keeps screen on only during playing fullscreen video (not paused). Not asking for review yet, since the wakelock API is in progress, but would be good to know if the mobile-browser bits are generally correct.

Comment on attachment 498274 [details] [diff] [review] mobile-browser patch > init: function fsv_init() { > messageManager.addMessageListener("Browser:FullScreenVideo:Start", this.show.bind(this)); > messageManager.addMessageListener("Browser:FullScreenVideo:Close", this.hide.bind(this)); >+ messageManager.addMessageListener("Browser:FullScreenVideo:Play", (function() { <snip> >+ }).bind(this)); >+ messageManager.addMessageListener("Browser:FullScreenVideo:Pause", (function() { <snip> >+ }).bind(this)); Should probably use real methods here, like this.show() and this.hide() >diff --git a/chrome/content/fullscreen-video.js b/chrome/content/fullscreen-video.js >-function closeFullScreen() { >- sendAsyncMessage("Browser:FullScreenVideo:Close"); >-} >- > addEventListener("click", function(aEvent) { > if (aEvent.target.id == "close") > closeFullScreen(); > }, false); closeFullScreen() is still called here Approach looks good to me

Comment on attachment 498261 [details] [diff] [review] patch Please check if http://developer.android.com/reference/android/view/View.html#setKeepScreenOn%28boolean%29 will be sufficient for our needs.

(In reply to comment #10) > Comment on attachment 498261 [details] [diff] [review] > patch > > Please check if > http://developer.android.com/reference/android/view/View.html#setKeepScreenOn%28boolean%29 > will be sufficient for our needs. Nice find, thanks! However it isn't clear to me from the docs whether this keeps the screen brightly lit or dimmed. So I prefer the current patch, which uses the Android API that explicitly keeps it bright. In any case the difference is just a few lines of Java code, so there isn't much cost to doing things as the patch does.

Just to further clarify the API in the current patch: 1. Internally, our UI scripts (and potentially other code, of course) use an acquireWakeLock/releaseWakeLock API. This API happens to look similar to the one in Android, but the only reason it does is that I think that kind of API is the minimal one that is sane and provides all the functionality (namely, it allows multiple components to acquire separate wake locks). 2. Each widget implementation just needs to implement a SetWakeLevel() function. In other words each widget code needs just to implement something much simpler than the API mentioned in 1. 3. In our Android widget implementation, we receive SetWakeLevel() and then call into Android's wake lock API, acquiring a single lock of the right sort. Again, this is similar to the API in 1, but this is just superficially. Other widget code might be very different (e.g. I believe that GTK's would use the DBUS screensaver stuff, which has no concept of multiple locks AFAICT). Summary: So, in our UI code we acquire locks as we need them. There can be any number of them. The widget-independent code manages those locks and sends out to widget-specific code a simple, single request for the current wake level.

Seems to me that the minimal API here would be something like (on nsIScreen): const long BRIGHTNESS_DIM = 1; const long BRIGHTNESS_FULL = 2; void lockMinimumBrightness(long brightness); void unlockMinimumBrightness(long brightness); where every call to lockMinimumBrightness(b) should be followed by a call to unlockMinimumBrightness(b) (eventually). Sound OK?

I like that API very much. Here's a patch with it.

Android bits, separated out for reviewing convenience.

Updated mobile-browser patch.

Comment on attachment 498883 [details] [diff] [review] android bits >diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java >--- a/embedding/android/GeckoAppShell.java >+++ b/embedding/android/GeckoAppShell.java >@@ -37,16 +37,17 @@ > > package org.mozilla.gecko; > > import java.io.*; > import java.util.*; > import java.util.zip.*; > import java.nio.*; > import java.lang.reflect.*; >+import java.lang.Runnable; IIRC java.lang.*; is autoimported for you. >@@ -633,9 +639,21 @@ class GeckoAppShell > } > > public static void hideProgressDialog() { > if (GeckoApp.mAppContext.mProgressDialog != null) { > GeckoApp.mAppContext.mProgressDialog.dismiss(); > GeckoApp.mAppContext.mProgressDialog = null; > } > } >+ >+ public static void setKeepScreenOn(final boolean on) { >+ class DoIt >+ implements Runnable >+ { >+ public void run() { >+ GeckoApp.surfaceView.setKeepScreenOn(on); >+ } >+ } >+ sTheHandler.post(new DoIt()); Use http://developer.android.com/reference/android/app/Activity.html#runOnUiThread%28java.lang.Runnable%29 and use an anonymous inner class for the Runnable. >diff --git a/widget/src/android/AndroidBridge.h b/widget/src/android/AndroidBridge.h >--- a/widget/src/android/AndroidBridge.h >+++ b/widget/src/android/AndroidBridge.h >@@ -206,16 +206,18 @@ public: > int mEntries; > }; > > /* See GLHelpers.java as to why this is needed */ > void *CallEglCreateWindowSurface(void *dpy, void *config, AndroidGeckoSurfaceView& surfaceView); > > bool GetStaticStringField(const char *classID, const char *field, nsAString &result); > >+ void SetKeepScreenOn(PRBool on); >+ bool on >diff --git a/widget/src/android/nsScreenManagerAndroid.cpp b/widget/src/android/nsScreenManagerAndroid.cpp >--- a/widget/src/android/nsScreenManagerAndroid.cpp >+++ b/widget/src/android/nsScreenManagerAndroid.cpp >@@ -33,18 +33,22 @@ > * and other provisions required by the GPL or the LGPL. If you do not delete > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "nsScreenManagerAndroid.h" > #include "nsWindow.h" >+#include "AndroidJavaWrappers.h" What's this include for? >+#include "AndroidBridge.h" > >-NS_IMPL_ISUPPORTS1(nsScreenAndroid, nsIScreen) >+using namespace mozilla; >+ >+NS_IMPL_ISUPPORTS2(nsScreenAndroid, nsIScreen, nsIScreen_MOZILLA_2_0_BRANCH) > > nsScreenAndroid::nsScreenAndroid(void *nativeScreen) > { > } > > nsScreenAndroid::~nsScreenAndroid() > { > } >@@ -83,16 +87,24 @@ nsScreenAndroid::GetPixelDepth(PRInt32 * > > > NS_IMETHODIMP > nsScreenAndroid::GetColorDepth(PRInt32 *aColorDepth) > { > return GetPixelDepth(aColorDepth); > } > >+void >+nsScreenAndroid::ApplyMinimumBrightness(PRUint32 aType) >+{ >+ AndroidBridge::Bridge()->SetKeepScreenOn(aType == BRIGHTNESS_FULL); >+} >+ >+// >+ remove the // and extra blank line.

Fixed android bits.

Comment on attachment 498910 [details] [diff] [review] android bits v2 >diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java >--- a/embedding/android/GeckoAppShell.java >+++ b/embedding/android/GeckoAppShell.java >@@ -1,9 +1,9 @@ >-/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- >+/* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*- If blassey added this modeline, he probably doesn't want you messing with it. I have no opinion here. >diff --git a/widget/src/android/nsScreenManagerAndroid.h b/widget/src/android/nsScreenManagerAndroid.h >--- a/widget/src/android/nsScreenManagerAndroid.h >+++ b/widget/src/android/nsScreenManagerAndroid.h >-class nsScreenAndroid : >- public nsIScreen >+class nsScreenAndroid >+ : public nsIScreen >+ , public mozilla::widget::BrightnessLockingWidget This part seems a little strange to me. Is BrightnessLockingWidget basically a base class for nsIScreen implementations that want to use brightness locking? If that's the case, it seems like base classes are usually implemented in files named nsBase* in xpwidgets and it might be better to follow that pattern. I'm not reviewing that part though, so r=me regardless.

Comment on attachment 498881 [details] [diff] [review] patch v2 We don't need BRIGHTNESS_OFF, right? It doesn't really make sense to set a minimum brightness of OFF, surely? :-) + virtual void ApplyMinimumBrightness(PRUint32 aType) = 0; Can't this just be protected?

(In reply to comment #19) > Comment on attachment 498910 [details] [diff] [review] > android bits v2 > > >diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java > >--- a/embedding/android/GeckoAppShell.java > >+++ b/embedding/android/GeckoAppShell.java > >@@ -1,9 +1,9 @@ > >-/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- > >+/* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*- > > If blassey added this modeline, he probably doesn't want you messing with it. I > have no opinion here. blassey, I just changed the tab-width, following our chat last week. Is that ok? (In reply to comment #20) > Comment on attachment 498881 [details] [diff] [review] > patch v2 > > We don't need BRIGHTNESS_OFF, right? It doesn't really make sense to set a > minimum brightness of OFF, surely? :-) I was using BRIGHTNESS_OFF internally for convenience. You are totally right, that has no business being in an interface. Removed it. > > + virtual void ApplyMinimumBrightness(PRUint32 aType) = 0; > > Can't this just be protected? Yes, very true. Changed.

Comment on attachment 498932 [details] [diff] [review] patch v3 sr=me, maybe Brad can review it?

m-c parts: http://hg.mozilla.org/mozilla-central/rev/d488f09f6f8d http://hg.mozilla.org/mozilla-central/rev/77956e4dd8f8

m-b part: http://hg.mozilla.org/mobile-browser/rev/40f2c3829452

need to verify this test. Alon, do you have automated tests for this?

There is no practical way to automatically test this that I can see - one needs to actually start a fullscreen video and wait a while to see that the screen doesn't dim. An automatic test can't detect the screen dimming.

I tried to remove the _MOZILLA_2_0 interface added here post-2.0, but it seems that not every screen implementation implements the new interface, only the android implementation does. If so, should this really be a unified interface? Or should this be nsIScreenBrightness and left as a separate interface?

I don't have an opinion about unified or separate, but I do think this would be useful in other places than Android. It would be nice to not have the display go to sleep on desktop too (like Flash video).

I think we should make everything implement the new interface, with a dummy implementation for non-Android.

VERIFIED FIXED on: Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110315 Firefox/4.0b13pre Fennec /4.0b6pre Device: HTC Desire