Content ought to be able to keep the device's screen from turning off. I don't think we should extend this privilege to all pages. Users coming from other platforms expect that the phone will manage the display, within most applications. OTOH, it's kind of a bummer that a movie-watching app would need to explicitly ask for permission to keep the display on. If we made this facility available to all pages, it wouldn't need to ask. We have [1]; I'm not sure what the status of this API is. But I also don't like this API. It should just be lock() and unlock() with no args. We'll need a separate, more privileged API for Gaia itself to manage the current brightness. [1] https://mxr.mozilla.org/mozilla-central/source/widget/public/nsIScreen.idl

I definitely agree we need this API. On phones it would prevent the phone from turning off, and on desktop it would prevent the screensaver from appearing. For installed webapps I definitely think we could enable the API without any warnings etc. Likewise I think it would be ok to do for pages that had gone into fullscreen mode. For plain (non-fullscreen'ed) webpages I feel like we could almost do the same. We could do it if we at the same time enabled a mechanism which allowed the user turn off the disable-screensaver mode. Or we could simply show a permission doorhanger, that might be fine too :)

> For plain (non-fullscreen'ed) webpages I feel like we could almost do the same. We could do it if > we at the same time enabled a mechanism which allowed the user turn off the disable-screensaver > mode. Could you rephrase this?

Oh, I think I understand. We can let normal pages disable screensaver without a prompt, so long as we give the user a way to re-enable the screensaver without leaving the page. How would we do that without being...annoying? Also, Chris, can you give me an idea of this bug's priority relative to other current B2G work?

Being able to turn the display on/off and lock/unlock input is higher priority for B2G than API to allow content to prevent sleeping/screen-off. I don't 100% understand how the display on/off works in android yet, so that'd be the best place to start. I assume you want to do this in bug 681009?

> I assume you want to do this in bug 681009? Yes. :)

As far as API goes here. I think something like this would work: interface Navigator { Request disableScreenSaver(Boolean); } interface Request : EventTarget { Function onsuccess; } This makes for a clean API for most use cases where the page doesn't really care if the request succeeded or not. All you'd do is: navigator.disableScreenSaver(true); or navigator.disableScreenSaver(false); For pages which do want to display a warning to users where it's not able to disable the screensaver it can use the success event handler. Note that for our standard doorhanger UI we can't tell when the user denies the request (since simply ignoring the doorhanger or clicking outside it isn't equivalent to "no", but rather "I'll decide later"). I don't know for sure that we'll ever end up using a doorhanger UI here. But with the above API we keep our options open.

Android has a nice (conceptual) solution for this, which is the notion of a "wake lock" that prevents the display from turning off (~screensaver). What's nice about this compared to a binary on/off API is that it composes well across multiple callers, and is extensible to locking other kinds of power-saving mechanisms (like low-power CPU idle). I would be in favor of an API like that, but I'm not sure what a good DOM-y solution would look like.

Can we do disableScreenSaver, enableScreenSaver ? Functions which take magic booleans are a pet peeve of mine. (I guess this boolean isn't so magic, but I don't like disableScreenSaver(false) is still unfortunate.) I'm not sure we need a reentrant solution like the Android wake lock -- an author can easily code that up around the API if necessary, right? I'm also not sure pages need to be able to warn the user if they can't disable the screensaver; do you have a use-case, Jonas? I'm afraid of "you must disable your screensaver in order to use this app" kind of BS.

(In reply to Justin Lebar [:jlebar] from comment #8) > I'm not sure we need a reentrant solution like the Android wake lock -- an > author can easily code that up around the API if necessary, right? > Not with nested iframes, etc, or if something like the status bar and a game are trying to change power-save state. Note that the "lock" API (or something like it) is also extensible, which is nice to have and reduces API surface area.

I was thinking that internally, we'd maintain such a data structure. Obviously an iframe shouldn't be able to stomp its parent's disable-screensaver request. The question is whether we need an API like that within a document. I can imagine two components (one of which is third-party, so you can't change it) in a document both trying to manage the screen's lock/unlock state, but is that really a common case we should design around? (People made the same argument with history.pushState -- we should do something which allows two independent pieces of code to call pushState without stomping on each other -- but I haven't heard about the simpler design we chose being a problem in practice.)

I like disableScreenSaver/enableScreenSaver. The way I was thinking it'd work is similar to the vibrator API. We'd track separately for each page if it wants to have the screensaver enabled/disabled. Then we'd use the state of whichever page is the currently active page. I don't really see any use cases for a background page to disable the screen saver. Can anyone else? I'm not terribly worried about pages saying "you must disable your screensaver in order to use this app". I suspect most users would just move on with their life :) I'm more worried about netflix type pages which would want to inform the user that they are about to get a sub-par experience unless the user is ok with disabling the screen saver.

(In reply to Justin Lebar [:jlebar] from comment #10) > I was thinking that internally, we'd maintain such a data structure. > Obviously an iframe shouldn't be able to stomp its parent's > disable-screensaver request. The question is whether we need an API like > that within a document. > It's not obvious to me why an iframe shouldn't be able to stomp its parent's disableScreenSaver() request. Who gets to win, the power-wasters or power-savers? Why? The API doesn't make that clear. > I can imagine two components (one of which is third-party, so you can't > change it) in a document both trying to manage the screen's lock/unlock > state, but is that really a common case we should design around? > > (People made the same argument with history.pushState -- we should do > something which allows two independent pieces of code to call pushState > without stomping on each other -- but I haven't heard about the simpler > design we chose being a problem in practice.) I don't know about the pushState() discussion, but in this case I don't think a wake-lock style API is more complicated at all. I totally agree with you about preferring simplicity over generalization to possibly-uncommon cases, but in this case wake-lock isn't any more complex IMHO, is clearer about effects and more extensible. Where's the tradeoff.

(In reply to Jonas Sicking (:sicking) from comment #11) > I like disableScreenSaver/enableScreenSaver. > > The way I was thinking it'd work is similar to the vibrator API. We'd track > separately for each page if it wants to have the screensaver > enabled/disabled. Then we'd use the state of whichever page is the currently > active page. > Agreed, except in this case I think we'd want to save and restore lock state for windows that go visible --> hidden --> visible. I think that's what you're proposing. We don't do that for vibrator because it's too hard. > I don't really see any use cases for a background page to disable the screen > saver. Can anyone else? > Nope. There are use cases for background windows disabling other power-saving features, though.

Maybe we're actually talking about the same thing, Chris. In what I've been thinking, each document may "disableScreenSaver", and if any visible document has done this, then the screensaver doesn't show. So "disableScreenSaver" in some sense "locks out" the screensaver. This is similar to the lock-based API you proposed (I'll call this the reentrant API, since it acts like a reentrant lock). Each document may choose to lock out the screensaver, and if any visible document has done this, the screensaver doesn't show. The only difference AIUI between these two is that in the reentrant case, the document's 'lock the screensaver' bit becomes unset only if you call unlock() as many times as you've called lock(). In the non-reentrant case, a document's 'lock the screensaver' bit is unset as soon as you call enableScreenSaver once. The interaction with other windows and iframes is exactly the same in both cases. I think the non-reentrant solution is simpler for web authors. They don't have to worry about calling disable-screensaver more than once, and they don't have to worry about calling enable-screensaver too many times. Locks are hard; let's go shopping. The only case I can think of where the reentrant style helps is when I'm trying to manage the screensaver bit and a piece of third-party code is also trying to manage the bit. I just question whether that's a real use-case.

(In reply to Justin Lebar [:jlebar] from comment #14) > Maybe we're actually talking about the same thing, Chris. In what I've been > thinking, each document may "disableScreenSaver", and if any visible > document has done this, then the screensaver doesn't show. So > "disableScreenSaver" in some sense "locks out" the screensaver. > Yes, I don't think they're terribly different fundamentally. The difference is that a window is allowed (AFAICT) to call enableScreenSaver() before disableScreenSaver(), and we have to arbitrarily define preferring the screen-saver being disabled to enabled. > This is similar to the lock-based API you proposed (I'll call this the > reentrant API, since it acts like a reentrant lock). Each document may > choose to lock out the screensaver, and if any visible document has done > this, the screensaver doesn't show. > Gotcha, but to my eyes that's not too clear from the API. If my app successfully calls enableScreenSaver(), I would expect the power-save to be enabled. But that's not the case; I've only released my window's claim on preventing power-save. > I think the non-reentrant solution is simpler for web authors. They don't > have to worry about calling disable-screensaver more than once, and they > don't have to worry about calling enable-screensaver too many times. Locks > are hard; let's go shopping. > Let's compare use cases. I have in mind an API like |lock = navigator.requestWakeState("screenOn")| paired with |lock.release()|. The simplest and almost certainly most common use case would be J. Webauthor locking the screen the whole time her game is loaded. In that case, it's navigator.requestWakeState("screenOn") vs. navigator.disableScreenSaver() I'd say no difference. The next simplest case would be locking/unlocking on video play/pause, say. Then it's gLock = navigator.requestWakeState("screenOn") ... gLock.release() vs. navigator.disableScreenSaver() ... navigator.enableScreenSaver() I'd say a win for disable/enable, but IMHO requestWakeState isn't much added complexity. Next up is the re-entrancy case that may not be terribly common. But one use case for that is a phone app that locks the screen whenever there's a call session. I would implement that by having some per call-session state. session1.lock = navigator.requestWakeState("screenOn") ... session2.lock = navigator.requestWakeState("screenOn") ... session2.release(); ... session1.lock.release() vs. navigator.disableScreenSaver() ++gNumDisables ... if (!--gNumDisables) navigator.disableScreenSaver() ... if (!--gNumDisables) navigator.disableScreenSaver() I would say a win for requestWakeState. > The only case I can think of where the reentrant style helps is when I'm > trying to manage the screensaver bit and a piece of third-party code is also > trying to manage the bit. I just question whether that's a real use-case. I think we mostly agree on this. I can imagine some use cases, but my concern is it's not sanely possible to compose disable/enable, but fairly natural to compose requestWakeState. IMHO disable/enable isn't really simpler than requestWakeState in the cases above, but in exchange for potential slight simplification we give up composability and extensibility. I don't think that's a good trade.

Would this proposed API interact with full-screen at all? I haven't followed that spec.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13) > > I don't really see any use cases for a background page to disable the screen > > saver. Can anyone else? > > > > Nope. There are use cases for background windows disabling other > power-saving features, though. What specific power-saving features do you have in mind? And what type of app would want to disable them? (In reply to Chris Jones [:cjones] [:warhammer] from comment #16) > Would this proposed API interact with full-screen at all? I haven't > followed that spec. For the screen saver use case I think it would make sense to allow disabling screen-saver without any prompting.

(In reply to Jonas Sicking (:sicking) from comment #17) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #13) > > > I don't really see any use cases for a background page to disable the screen > > > saver. Can anyone else? > > > > > > > Nope. There are use cases for background windows disabling other > > power-saving features, though. > > What specific power-saving features do you have in mind? And what type of > app would want to disable them? > The influence is http://developer.android.com/reference/android/os/PowerManager.html . Android allows locking the wake states EVERYTHING FULL ON, just screen set to full brightness, screen on but necessarily full brightness, and CPU on. The last one is needed for background services to prevent the device from sleeping on them, like Sync e.g.

Another wake state needed for background apps is a wifi lock, to prevent the wifi chip from turning off. Something like Sync would need that too.

IMO, and based on my experience with pushState, composition is not an important consideration in these APIs. If you want to compose the API, as with the call waiting example, then it's trivial to write a wrapper. There's a cost to asking everyone to carry around these lock objects, and to understand what the heck they are. (Note also that there are a variety of ways you might want to write this reentrant API. You might want to carry around lock objects, or you might want to count the number of calls to lock() and unlock(). It's hard to pick just one.) > If my app successfully calls enableScreenSaver(), I would expect the power-save to be enabled. But > that's not the case; I've only released my window's claim on preventing power-save. I think this is a fair point. The simplest API views the screen lock as a switch. It starts off as unlocked. You can flip the switch to locked, and you can flip it back. How can we design an API which reflects this unambiguously? Maybe we can do (with different names) navigator.requestWakeState("screenOn") navigator.releaseWakeState("screenOn") If we picked good names, it could be clear that you're releasing something you requested; that is, you're flipping a switch.

I think this is not only keeping screen from turning off. It can be keeping wifi, audio, or bluetooth handset from turning off. We should consider scenarios for various peripherals, and try to figure out a generic solution (keep something from turning off API) if possible. For example, as I know, we also need to keep audio from turning off for music players. Maybe, we also need to keep wifi (networking) and audio from turning off for skype or SIP clients. We had better to consider more scenario.

(In reply to Justin Lebar [:jlebar] from bug 708964 comment #7) > > nsIDOMMozWakeLock requestWakeLock(in ACString topic); > > Is this not the function exposed to content? I expected we'd expose this to > content (maybe privileged content only, but at least apps) and then have > some API for our "chrome" to tell whether wake locks are held (see e.g. > https://github.com/andreasgal/gaia/issues/167#issuecomment-3101666). It is only for locking the CPU. I think apps should use a full-fledged pw mgmt implemented in JS. Apps can lock some resources (cpu, screen, peripherals, etc) Power manager could turn off the resource after some period of idle time, if nobody is using them. Something like: #+begin_example PowerManager = function() {} PowerManager.prototype = { screenTimeout: 30, init: function () { idle.addIdleObserver(this, screenTimeout); }, lock: function (aTopic, aWho) { let wl = {}; switch (aTopic) { case "cpu": wl = mozPower.requestWakeLock(aWho); break; default: wl = {isHeld: true, topic: aTopic}; break; } locks.push(wl); return wl; }, observe: function(aSubject, aTopic, aData) { switch (aTopic) { case "idle": for (let i = 0; i < locks.length; i++) { if (locks[i].isHeld) { return; } } mozPower.sleep(); } } } #+end_example Should apps be able to turn on/off the peripherals as well? If yes then they should also check the locks.

Sorry, I don't quite understand. There should be two DOM APIs, right? One is the API which the power manager uses. It has things like "put the phone to sleep", and "reboot the phone" which are not exposed to content. Only the power manager ever calls these functions. The other API is the one which content uses. The only function in this API, as far as I know, is requestWakeLock. Content uses this function to request that the CPU stay on, that the screen stay on, etc. The first API needs to have a function so that the power manager can tell whether any wake locks are currently held for a given topic. I don't think we want each app implementing its own power manager, if that's what you're suggesting here. Otherwise, if there's only one power manager, how does an app get a pointer to it so it can call PowerManager.lock()?

(In reply to Justin Lebar [:jlebar] from comment #23) > Sorry, I don't quite understand. > > There should be two DOM APIs, right? But how do we separate the two APIs? > One is the API which the power manager uses. It has things like "put the > phone to sleep", and "reboot the phone" which are not exposed to content. > Only the power manager ever calls these functions. > > The other API is the one which content uses. The only function in this API, > as far as I know, is requestWakeLock. Content uses this function to request > that the CPU stay on, that the screen stay on, etc. So far, yes. This is what above PowerManager does. > The first API needs to have a function so that the power manager can tell > whether any wake locks are currently held for a given topic. A bit confusing here. When we talked about the power manager, do we mean the DOM object or the one will be in Gaia? > I don't think we want each app implementing its own power manager, if that's > what you're suggesting here. Otherwise, if there's only one power manager, > how does an app get a pointer to it so it can call PowerManager.lock()? There is only one power manager. Please forgive my ignorance, I do not quite understand how Gecko is layered yet. Who will monitor the idle time and manage the power state of each peripherals? I thought it was a object living in Gaia "chrome" and listening to the idle event.

>> There should be two DOM APIs, right? > But how do we separate the two APIs? I'd probably make them two separate interfaces. navigator.mozPower is accessible only from Gaia "chrome", and navigator.requestWakeLock() is callable from content. >> The first API needs to have a function so that the power manager can tell >> whether any wake locks are currently held for a given topic. > > A bit confusing here. When we talked about the power manager, do we mean the DOM object or the one > will be in Gaia? By "power manager", I mean the object in Gaia which uses navigator.mozPower. > Who will monitor the idle time and manage the power state of each peripherals? I thought it was a > object living in Gaia "chrome" and listening to the idle event. Yes, that's right. But how is PowerManager.lock() called in your example? Logically, that function needs to be called from content, right? But content cannot get a reference to the PowerManager object in Gaia. So instead, I was thinking that content calls navigator.requestWakeLock(aTopic) and the PowerManager object has: * a way to tell whether a wake lock is held for a given topic. For example, it could be navigator.mozPower.wakeLockIsHeld(aTopic) * a way to be notified when all wake locks are taken or released for a topic. For example, it could be navigator.mozPower.addWakeLockAcquiredListener(function(aTopic) { alert("Wake lock on " + aTopic + " is acquired."); assert(navigator.mozPower.wakeLockIsHeld(aTopic)); }); navigator.mozPower.addWakeLockReleasedListener(function(aTopic) { assert(!navigator.mozPower.wakeLockIsHeld(aTopic)); });

So.. interface nsIDOMMozPowerManager { // ... void addWakeLockAcquiredListener(in nsIDOMEventListener listener); void addWakeLockReleasedListener(in nsIDOMEventListener listener); boolean wakeLockIsHeld(in ACString aTopic); } interface nsIDOMMozWakeLock { void unlock(); } interface nsIDOMMozNavigatorPower { readonly attribute nsIDOMMozPowerManager mozPower; nsIDOMMozWakeLock requestWakeLock(in ACString aTopic); }

Looks pretty good! Some details for when you translate this to IDL: The |ACString|s should be |DOMString|s. You also need corresponding removeWakeLock{Acquired,Release}Listener functions. The {add,remove}WakeLock{Acquired,Released}Listener functions don't take an nsIDOMEventListener. That is (I think) the interface which DOM objects which can receive events implement. A DOM event (like the onload event) isn't the same thing as a callback; you have callbacks here. You'll need to declare a new interface like this: [function] interface nsIDOMLockListener : nsISupports { void callback(in DOMString aTopic); }; and pass that as the param to {add,remove}WakeLock{Acquired,Released}Listener. We should get someone on the WebAPI team to sign off on this API, but it looks good to me. They may want you to fold addWakeLock{Acquired,Released}Listener() into one addWakeLockListener() function (the callback would take a boolean indicating acquired/released); either way is fine, but I don't like boolean arguments, so I prefer the first way. :) If Jonas doesn't comment in the bug with feedback, you can create an attachment with this pseudocode API and mark "feedback? :sicking".

First off, why put the requestWakeLock and wakeLockIsHeld functions on separate objects? Putting them both on nsIDOMMozPowerManager makes more sense to me. Also, I think we should do something event-based instead. Two options are: Option 1: interface nsIDOMMozPowerManager : EventTarget { // ... boolean wakeLockIsHeld(in DOMString aTopic); void requestWakeLock(in DOMString aTopic); void releaseWakeLock(in DOMString aTopic); attribute Function onwakelockaquired; attribute Function onwakelockreleased; }; And then we'd fire events with the following API: interface WakeLockEvent : Event { readonly attribute DOMString topic; }; We'd fire events with type "wakelockaquired" and "wakelockreleased" targetted at the powermanager. So the code would look something like this: navigator.mozPower.onwakelockreleased = function(event) { if (event.topic == "mytopic") { ... }; } navigator.requestWakeLock("mytopic"); Option 2: interface nsIDOMMozPowerManager : EventTarget { // ... boolean wakeLockIsHeld(in DOMString aTopic); nsIDOMWakeLockRequest requestWakeLock(in DOMString aTopic); void releaseWakeLock(in DOMString aTopic); }; interface nsIDOMWakeLockRequest : EventTarget { readonly attribute DOMString topic; attribute Function onaquired; attribute Function onreleased; }; which would give the code: navigator.mozPower.requestWakeLock("mytopic").onaquired = function(event) { ... } Though I'm not quite understanding the purpose of the topic? Is it just for the website to be able to manage having multiple reasons to keep the screen awake? Or will we actually do something useful with the string-value that they pass in?

Re: comment 26: why put |wakeLockIsHeld(in ACString aTopic)| on navigator.power? I don't understand the use case for that. Putting it on WakeLock might make sense, but I can't really think of a use for that either. Re: comment 29: putting |releaseWakeLock(in DOMString aTopic);| on navigator.power isn't a great idea API-wise, because it eliminates composability of the API. See discussion in comment 7 to comment 15. The proposal in comment 26 is better wrt this concern.

> First off, why put the requestWakeLock and wakeLockIsHeld functions on separate objects? The idea is: requestWakeLock is called by relatively unprivileged content (apps), but wakeLockIsHeld is called only by super-privileged content (gaia). The super-privileged API encompasses wakeLockIsHeld, reboot(), shutDown(), turning the screen on/off, modifying the screen's brightness... Putting the super-privileged API on one object makes the security model clearer.

OK. I thought the intention was for wake lock consumers to see if they still hold a lock. What's the use case for privileged content checking if a lock is being held?

> What's the use case for privileged content checking if a lock is being held? Isn't this the entire point of wake locks? The privileged content which controls the screen's brightness and on/off state checks if a wake lock is held before putting the device to sleep.

In the linux-android kernel, screen wake locks are processed by the kernel. When we have to emulate that, I would expect wake locks to be processed entirely within gecko. What other uses do you have in mind?

Does the kernel handle dimming the screen when the device is inactive? Does the kernel handle automatically turning off wifi? I'd thought there were a number of policy decisions which we'd make in JS.

What policy decisions do you envision "userspace" making, outside of gecko?

For example, exactly how do you manage dimming the screen and putting the device to sleep? There are a number of sub-questions: How long after inactivity do you wait before dimming the screen? How much do you dim the screen? Do you do it gradually, or do you do it stepwise? How many steps, and at what levels? How does this interact with the phone's ambient light sensor? (For example, if an app requests a max-brightness lock but it's really dim inside, do you pin the screen to the true max brightness?) I can imagine wake locks being transparent to at least some of this code, but it's not necessarily simple. For example, the screen.enabled code has to know whether the screen is actually enabled. If we allowed the code to disable the screen and then used a wake lock to keep the screen on, I'm not sure how we'd make this work.

(In reply to Justin Lebar [:jlebar] from comment #37) > How long after inactivity do you wait before dimming the screen? Use a setting. > How much do you dim the screen? Maybe use a setting, but I'm not sure this needs to be configurable. > Do you do it gradually, or do you do it stepwise? How many steps, and at what levels? Maybe setting, maybe hard code. > How does this interact with the phone's > ambient light sensor? (For example, if an app requests a max-brightness > lock but it's really dim inside, do you pin the screen to the true max > brightness?) > Depends on how max-brightness is defined. How is it defined? > I can imagine wake locks being transparent to at least some of this code, > but it's not necessarily simple. For example, the screen.enabled code has > to know whether the screen is actually enabled. If we allowed the code to > disable the screen and then used a wake lock to keep the screen on, I'm not > sure how we'd make this work. That's something the wake lock should specify. Android says, "Makes sure the device is on at the level you asked when you created the wake lock.", which sounds like the request is "clipped" to the power level the device is at when the request is made. There's additionally a "ACQUIRE_CAUSES_WAKEUP : Normally wake locks don't actually wake the device, they just cause it to remain on once it's already on." flag, which makes it sound like clients have to explicitly request upping the power level. We should investigate that.

> Use a setting. > Maybe use a setting, but I'm not sure this needs to be configurable. > Maybe setting, maybe hard code. YAGNI -- no need to add isWakeLockHeld() now if you don't think you're going to need it. But it seems to me that we shouldn't design around specifically *not* allowing Gaia to determine whether a wake-lock is held, especially given our constraint of writing as much code in JS as possible. (Using kernel code that already exists sounds fine to me. I just don't know why we'd opt to write something new in Gecko rather than in JS.)

I agree with cjones. If the use-case isn't for apps to be able to see if they have the lock, I'd prefer to let the requestWakeLock implementation talk directly to the platform which can then handle everything based on user settings. Does this mean that we can remove the callbacks too then? I'd still like to understand the topic-string. Without it it seems like the API becomes as simple as: navigator.wakeLockRequested = true/false; let haveIRequestedWakeLock = navigator.wakeLockRequested; Note that the latter doesn't return true/false depending on if the app has the lock, but simply lets you check if you have requested it.

Chatted with Justin on irc about weather to do screen dimming in gecko or in gaia. I think I've come around to the idea of doing it in Gaia so that we can do things like play animations as we turn off the screen. It also seems like a win to have that be done in JS rather than gecko C++. However I'm thinking we want to separate "gaia specific" APIs more than simply hanging them off of navigator.power. I.e. APIs that don't make sense to have at all on for example android, shouldn't live on navigator. I think stuff on navigator should be things that we'll want to conceivably expose to web pages and/or web apps (though in some cases only if they have special privileges). I don't really care where we expose them though, but having them all in the same place might make sense. So having something like a "platform" object exposed only to gaia where we can hang stuff like this. So gaia could say: platform.onwakelockgrabbed = function(event) { ... }; platform.hasWakeLock("topic");

(In reply to Jonas Sicking (:sicking) from comment #41) > Chatted with Justin on irc about weather to do screen dimming in gecko or in > gaia. I think I've come around to the idea of doing it in Gaia so that we > can do things like play animations as we turn off the screen. It also seems > like a win to have that be done in JS rather than gecko C++. > That's a reasonable feature request. But let me play devil's advocate for a second. To what extent should the content power manager manage screen-off behavior? What happens if another privileged app sets screen.enabled = false? Should the power manager be notified of that? For the power manager to implement "turn off screen when idle", it's going to have to know what the idle timeout is (settings permissions). OK. But it also needs to know when "system activity" happens, so that it can reset its idle timer. How will the power manager do that? As devil's advocate again, I might suggest having gecko manage idle timeouts, screen dimming, and screen-off, and send notifications (maybe intents) asking the content power manager to implement various animations. (I'm not trying to be argumentative, just trying to make sure we're considering all parts of this.) > However I'm thinking we want to separate "gaia specific" APIs more than Quick clarification: there are no "gaia specific" APIs ;). I think "privileged APIs" is what you mean :). (There are other consumers of what we're adding here.)

> What happens if another privileged app sets screen.enabled = false? Aha. We've hit upon the important point here. I think we might as well ask: What happens if another privileged app calls shutdown()? To back up: I envision that there are three rough levels of privilege on the phone: Ring 2) Web privilege. Ring 1) App privilege. Apps likely have more privileges than web pages by default (larger local storage quota, able to raise notifications, etc.). Ring 0) Platform privilege. This is what we've been calling "Gaia", with the understanding that Gaia proper is not the only envisioned consumer. Ring -1) Gecko. There's some granularity in rings 2 and 1 -- maybe this app can access the accelerometers but not GPS, for example. But the main feature which distinguishes rings 2/1 from ring 0 is that ring 0 is as trusted as Gecko. We should be able to recover from a misbehaving ring 2/1 app, but misbehaving ring 0 code can break the phone (just as misbehaving Gecko code can). > What happens if another privileged app sets screen.enabled = false? Should the power > manager be notified of that? Only platform privilege (ring 0) code can set |screen.enabled = false|. Nothing other than the power manager should touch screen.enabled, and we trust that other modules won't step on the power manager's toes. (We do this through code review, just as we would in Gecko.) > For the power manager to implement "turn off screen when idle", it's going to have to > know what the idle timeout is (settings permissions). OK. But it also needs to know > when "system activity" happens, so that it can reset its idle timer. How will the > power manager do that? The power manager will ask Gecko to call it back when there's been X seconds of inactivity. This means exposing an API similar to Gecko's idle service to ring 0. > As devil's advocate again, I might suggest having gecko manage idle timeouts, screen > dimming, and screen-off, and send notifications (maybe intents) asking the content > power manager to implement various animations. This sounds like suggesting that we eliminate ring 0 and push that functionality into gecko. In general, this kind of approach limits extensibility. One can extend the functionality only at the points we define. For example, suppose Gaia wants to display a list of options when you press-and-hold the power button (shut down, restart, turn off screen, enter airplane mode). It sounds like in this model, we'd have to code into gecko a press-and-hold callback which Gaia would listen to. And then we'd need to enumerate, in Gecko, what things the callback is allowed to do -- in general, code can't ask to turn off the screen, so the callback would have to specify its choice through a return value or something, I guess. So you couldn't extend the callback to do something new (say, turn off wifi + bluetooth) without modifying Gecko. Also, suppose you wanted to bring up one menu on short press-and-hold and a different menu on long press-and-hold, or bring up a special menu when both the volume and power buttons are held down. Again, you need to modify Gecko. These use-cases aren't so compelling, but I hope the general idea of ring 0 JS code which handles this kind of thing is.

I don't see the need for inventing a notion of rings. Apps can be entrusted with particular permissions. They're trusted to the extent of the permissions granted. There are some privileges like telephony that we may only grant to apps pre-installed on the device, up to vendors. > Only platform privilege (ring 0) code can set |screen.enabled = false|. Nothing other than the power manager should touch screen.enabled, and we trust that other modules won't step on the power manager's toes. (We do this through code review, just as we would in Gecko.) If I read this as, "only apps with screen-management permission", then I agree. But I don't follow the rest. Are you saying that there should only be one application given screen-management permissions? That could be a pre-install-only permission, but the discussion seems orthogonal. If there's a use case for multiple apps having screen-management perms (maybe there isn't), then we should consider how they're supposed to interact. I can't think of one so I'm happy punting that. > The power manager will ask Gecko to call it back when there's been X seconds of inactivity. This means exposing an API similar to Gecko's idle service to ring 0. > This sounds like suggesting that we eliminate ring 0 and push that functionality into gecko. > In general, this kind of approach limits extensibility. One can extend the functionality only at the points we define. It all boils down to trade-offs. Extensibility means new APIs, which means a lot of design and spec work. Implementing something mostly internal to gecko with small touch points to content is easier. If there are other use cases for apis we want to add, consiliating with "userspace screen manager", then that argues more strongly for adding a new API. An idle notification sounds like a generally useful thing. Has anyone proposed this before? It sounds easy to spec and implement. Now, along with that, if we specify what happens if an app with screen-management permissions tries to set screen.enabled = false when there are active screen-on wakelocks, or tries to change brightness when there's a screen-brightness lock, then you've convinced me we've got all we need to implement this entirely in "userspace".

> I don't see the need for inventing a notion of rings. Apps can be entrusted with particular > permissions. They're trusted to the extent of the permissions granted. There are some privileges > like telephony that we may only grant to apps pre-installed on the device, up to vendors. It seems to me that it's not useful to distinguish between a lot of these mission-critical permissions. As soon as an app has permission to use a ring-zero API, then it is able to make the phone useless and we need to review its code. And if we're reviewing its code, then we don't need to use gecko to restrict what it does. Note that this could be an implementation detail; that is, ring-zero can be an internal gecko designation. We could still have a bunch of separate specs and the notion that you need permission for each one separately, but it could be that gecko always grants permission for these specs as a unit. But anyway, this is an orthogonal discussion which we can have when we finally implement the permissions framework. I don't feel too strongly about this; I just think it'll be easier to have fewer permission bits. > Are you saying that there should only be one application given screen-management permissions? No. I don't want our architecture here to force Gaia/Gaia-alternative developers to implement their front-end in any particular way. > If there's a use case for multiple apps having screen-management perms (maybe there isn't), then > we should consider how they're supposed to interact. What I'm saying is that the screen-management API shouldn't try to be intelligent about what happens when multiple apps call it. If one app sets screen.enabled = false and another app sets screen.enabled = true then last writer wins, just as though if Gecko code called Hal::SetScreenEnabled(false) followed by Hal::SetScreenEnabled(true). The reason I argue that the API doesn't need to do something more clever here (such as, as you suggested, adding another level of indirection so that all these screen.enabled modifications get funneled down to a single power manager app) is that we trust the code which is able to modify screen.enabled. > An idle notification sounds like a generally useful thing. Has anyone proposed this before? It > sounds easy to spec and implement. I talked with people about this at the last work-week, but I don't know if you were there. I'd like to design the API only once we have a consumer, though. So once the front-end people are ready for it, we can design it. (For example, it might make sense to register a callback which says "notify me when the device has been idle for X seconds, or when there are no more wake locks with topic T held, whichever comes last.") > Now, along with that, if we specify what happens if an app with screen-management permissions > tries to set screen.enabled = false when there are active screen-on wakelocks or tries to change > brightness when there's a screen-brightness lock, then you've convinced me we've got all we need > to implement this entirely in "userspace". screen.{brightness,enabled} are low-level functions, and at least the screen/brightness wakelocks are advisory to the screen management code. The screen code is free to ignore the wake locks and enable/disable/dim the screen at will. Otherwise, it can't turn off the screen when the user presses the power button! Even for functionality whose wake-locks interact with the kernel, we're still going to need a way for the power manager to hard-disable it. For example, when you go into airplane mode, we have to turn off the 3G radio, even if a 3G wake lock is held.

> The reason I argue that the API doesn't need to do something more clever here is that we trust the > code which is able to modify screen.enabled. I should say: We trust this code to coordinate between the apps which modify screen.{enabled,brightness}, if necessary. (In practice, I imagine this would consist of reading screen.brightness onpageshow (or something) if I know another app could have modified the brightness while I was hidden.)

(In reply to Justin Lebar [:jlebar] from comment #48) > > Are you saying that there should only be one application given screen-management permissions? > > No. I don't want our architecture here to force Gaia/Gaia-alternative > developers to implement their front-end in any particular way. > Agreed! > > An idle notification sounds like a generally useful thing. Has anyone proposed this before? It > > sounds easy to spec and implement. > > I talked with people about this at the last work-week, but I don't know if > you were there. I'd like to design the API only once we have a consumer, > though. So once the front-end people are ready for it, we can design it. > (For example, it might make sense to register a callback which says "notify > me when the device has been idle for X seconds, or when there are no more > wake locks with topic T held, whichever comes last.") > I think we're ready for it now :). I'll file. > screen.{brightness,enabled} are low-level functions, and at least the > screen/brightness wakelocks are advisory to the screen management code. The > screen code is free to ignore the wake locks and enable/disable/dim the > screen at will. Otherwise, it can't turn off the screen when the user > presses the power button! > That's the point of wake locks, to prevent the screen from being turned off. We need to specify which one "wins". I think it makes more sense for wake locks to "win", since a conformant (privileged) user of screen.enabled wouldn't try to turn off the screen if there are wake locks, anyway. Or at least, wouldn't expect it to turn off. > Even for functionality whose wake-locks interact with the kernel, we're > still going to need a way for the power manager to hard-disable it. For > example, when you go into airplane mode, we have to turn off the 3G radio, > even if a 3G wake lock is held. How does that work in android? If wake locks "win" generally, then what we can do on trying to enter airplane mode is a set a timeout on apps that hold the locks, and kill them if they don't let go soon enough. It might also make sense to build in functionality to wake locks to notify holders when a drop is urgent or pending. Then conforming apps can let go of their locks and stay alive.

> That's the point of wake locks, to prevent the screen from being turned off. I thought the point was to prevent the screen from being turned off *due to idle* -- that is, "wake" refers to keeping the screen from falling asleep. > I think it makes more sense for wake > locks to "win", since a conformant (privileged) user of screen.enabled wouldn't > try to turn off the screen if there are wake locks, anyway. Or at least, > wouldn't expect it to turn off. What if a random page holds a screen-enabled wake-lock and I press the device's power button? We politely ask that app to let go of its wake-lock and then kill it if it doesn't? It seems much simpler just to turn off the screen. Ditto for the device's radio.

Is the "wakelock" API exposed to content on desktop Firefox? It seems very phone/B2G specific at the moment. Strictly from a desktop/content perspective, it would be handy if the API to disable the screensaver was exposed as a simple CSS property, much like the cursor:none CSS property. Then I could have a simple rule such as: video:-moz-full-screen { screen-saver: none; } And then not have to worry about the screen saver kicking in while watching a full-screen video. Or even something more general like: video:not(paused) { screen-saver: none; } It's considerably easier to implement this functionality compared to munging around in C++ (or JS even) to toggle the screen saver at the appropriate times. This also means web developers (and us implementers!) don't need to remember to unlock() or (re)enableScreenSaver() when the screen-saver-disabled document has been gc'd; it happens automatically when the rule no longer applies.

(In reply to Justin Lebar [:jlebar] from comment #51) > > That's the point of wake locks, to prevent the screen from being turned off. > > I thought the point was to prevent the screen from being turned off *due to > idle* -- that is, "wake" refers to keeping the screen from falling asleep. > Actually, I'm not sure how android implements that. Does pressing the power button turn off the screen when a wake lock is held? If so, then I agree with your interpretation. > > I think it makes more sense for wake > > locks to "win", since a conformant (privileged) user of screen.enabled wouldn't > > try to turn off the screen if there are wake locks, anyway. Or at least, > > wouldn't expect it to turn off. > > What if a random page holds a screen-enabled wake-lock and I press the > device's power button? We politely ask that app to let go of its wake-lock > and then kill it if it doesn't? Yes. > It seems much simpler just to turn off the > screen. Ditto for the device's radio. It's not really a question of simplicity, but API semantics. Let's nail that down first.

(In reply to Chris Pearce, Mozilla Corporation (:cpearce) from comment #52) > Is the "wakelock" API exposed to content on desktop Firefox? It seems very > phone/B2G specific at the moment. > Yes, it would be exposed on all platforms. > Strictly from a desktop/content perspective, it would be handy if the API to > disable the screensaver was exposed as a simple CSS property, much like the > cursor:none CSS property. > Interesting idea! > This also means web developers (and us implementers!) don't need to remember > to unlock() or (re)enableScreenSaver() when the screen-saver-disabled > document has been gc'd; it happens automatically when the rule no longer > applies. The problem is if there are conflicting rules. What happens if one embedded iframe has a "screen-saver: none" rule but another has "screen-saver: enabled"? The wakelock primitive makes the semantics of that clear. We could arbitrarily define which wins for a CSS rule, if we go that route.

Could you simply define that the screen saver is enabled by default, and can only be disabled, thus preventing conflicts? So if any document in the focused tab has a screen-saver:none rule active, we disable the screen saver, otherwise we save-screen as per usual. Perhaps that's too simple to handle all use cases?

Yes, we could define that. We want to support locking brightness levels too so we probably want something like "screen: (bright|dim|...|default)" etc. I think we'd still want the JS API, because locking wifi/3g/etc. on doesn't have a clear CSS interpretation to me. But I think having both interfaces is fine.

> This also means web developers (and us implementers!) don't need to remember to > unlock() or (re)enableScreenSaver() when the screen-saver-disabled document has > been gc'd; it happens automatically when the rule no longer applies. When a document is GC'ed (if not before), Gecko will automatically release its wake locks, automatically. A CSS property for this doesn't make much sense to me, because "screen-saver enabled" applies to the whole screen, not an individual element. Maybe others disagree. And then there's the whole question of deciding who gets precedence if one element on the page has screen-saver: enabled and another has screen-saver: disabled. It's only when you remove the element which has screen-saver:enabled from the DOM that you get any benefit from this. But what if your full-screen video is paused? Shouldn't you allow the screen-saver to come on? So I don't know how much effort this CSS property would really save. > It's not really a question of simplicity, but API semantics. Let's nail that down first. What I mean is, it's a much simpler API if pages don't have to listen for "wake lock going away" and explicitly release the wake lock. Simpler APIs are preferable to complex APIs, because there's less room for error, and every mistake which can be made will be made, and by many pages. I think the API should have advisory wake-locks; that is, when I call screen.enabled = false, the screen turns off, period. I can't think of a situation where it makes sense for us to grant a page or app the ability to keep the power manager from turning something off. The power manager will act as the user's agent, and when the user wants the screen or radio turned off, we should turn it off. In the case that we notify the wake lock that it's going away, we're not really giving the page an opportunity to override the screen-turn-off action. It either releases its lock or it dies. Much better would be simply notifying the page that its wake lock has been revoked. I think most of that notification functionality is already there, at least indirectly. A page can already tell when it's no longer visible (either due to tab switching or the screen turning off). And the network API will let pages tell when the network goes up or down, right? I suspect that 99% of pages won't care when their wake lock is revoked, so I'm not sure we need to build revocation directly into the API. (It complicates things -- if my screen wake lock is revoked, and the user turns the screen back on, do I have to re-register my lock, for example? Again, complexity is to be avoided because it leads to mistakes in content.) But we should make sure that pages can at least understand the status of the network/display, for sure.

> I think we'd still want the JS API, because locking wifi/3g/etc. on doesn't have a clear CSS > interpretation to me. This sounds like an excellent argument for implementing in only in JS. The benefit of CSS is tiny compared to the benefit of not having to define what happens when JS and CSS conflict, and when CSS and CSS conflict. And the CSS semantics are already muddled, because "screen saver on" has nothing to do with the style of an element on the page.

Note also that if a page cares whether wifi or 3g is enabled, then that page is going to have to do more than register a wake-lock. The wake-lock might keep us from disabling the radio, but the user might lose the connection for other reasons. And on desktop, the user might just turn off the radio, say with a hardware switch. So taking a wake-lock can't guarantee that the radio will stay on. The page has to listen for network events, if it cares to know when the network is up or down. So again, this suggests that the wake lock should be advisory.

(In reply to Justin Lebar [:jlebar] from comment #58) > > I think we'd still want the JS API, because locking wifi/3g/etc. on doesn't have a clear CSS > > interpretation to me. > > [..] And the > CSS semantics are already muddled, because "screen saver on" has nothing to > do with the style of an element on the page. +1. We shouldn't add new CSS properties that have nothing to do with style sheet even if it makes things cleaner because they are declarative.

(In reply to Justin Lebar [:jlebar] from comment #57) > > It's not really a question of simplicity, but API semantics. Let's nail that down first. > > What I mean is, it's a much simpler API if pages don't have to listen for > "wake lock going away" and explicitly release the wake lock. Simpler APIs > are preferable to complex APIs, because there's less room for error, and > every mistake which can be made will be made, and by many pages. > > I think the API should have advisory wake-locks; that is, when I call > screen.enabled = false, the screen turns off, period. > Not to sound too much like a broken record, but what does android do?

(In reply to Justin Lebar [:jlebar] from comment #58) > > I think we'd still want the JS API, because locking wifi/3g/etc. on doesn't have a clear CSS > > interpretation to me. > > This sounds like an excellent argument for implementing in only in JS. The > benefit of CSS is tiny compared to the benefit of not having to define what > happens when JS and CSS conflict, and when CSS and CSS conflict. And the > CSS semantics are already muddled, because "screen saver on" has nothing to > do with the style of an element on the page. Screen properties are part of a page's presentation, so there's a clear CSS interpretation for screen-on. Relevant previous work is full-screen, which has a JS API to enable, but also provides a CSS pseudo class. That's not what's being proposed here, though. We should continue full-steam on the JS API and think about CSS as a followup. If screen-on isn't a property settable through CSS, then we need to think of use cases for a screen-on pseudo class. I can't think of any off the top of my head.

http://developer.android.com/reference/android/os/PowerManager.html There are four classes of wake locks: PARTIAL_WAKE_LOCK SCREEN_DIM_WAKE_LOCK SCREEN_BRIGHT_WAKE_LOCK FULL_WAKE_LOCK > "If you hold a partial wakelock, the CPU will continue to run, irrespective of any timers and even > after the user presses the power button. In all other wakelocks, the CPU will run, but the user > can still put the device to sleep using the power button." There's also > public void goToSleep (long time) > Force the device to go to sleep. Overrides all the wake locks that are held. I don't know if goToSleep overrides partial wake locks. We haven't really thought about a CPU wake-lock, but at least the screen wake lock acts like I was describing here. It doesn't look like there's any concept of radio wake-locks?

Thanks. OK, that and your interpretation makes sense. Let's go with that. (In reply to Justin Lebar [:jlebar] from comment #63) > We haven't really thought about a CPU wake-lock, but at least the screen > wake lock acts like I was describing here. CPU power-state locks are part of the motivation for the generic requestWakeLock() API. But maybe I misunderstand your meaning. > It doesn't look like there's any > concept of radio wake-locks? There's a wifi lock [1]. I'm not sure if there's an cellular antenna lock but the same concept applies. IMHO having a uniform interface around power-state locks is preferable to than sticking them on particular sub-APIs (navigator.wifi.requestWakeLock, e.g.). [1] http://developer.android.com/reference/android/net/wifi/WifiManager.WifiLock.html

I'm in favor of notifying holders of wake locks when their particular device is going to be forced into a powered-off state.

The question arises then, though, when the power state of a device is overridden, what happens when it's forced back on? Do wake locks magically reacquire as if nothing happened, or do they all get dropped on forced power-off and pages have to reacquire them? The latter makes more sense to me in the abstract, but the former seems less error prone for developers. Maybe the former is the way to go.

> IMHO having a uniform interface around power-state locks is preferable to than sticking them on > particular sub-APIs (navigator.wifi.requestWakeLock, e.g.). Agreed, if for no other reason than I couldn't find the wifi wake lock from the screen wake lock, and neither of us is sure whether there's a radio wake lock! > The question arises then, though, when the power state of a device is overridden, what happens when > it's forced back on? If the wakelock API fires a callback when the lock is force-broken, then we'd probably also want to fire a callback when the lock is re-acquired. This would suggest that the wake lock magically re-acquires as if nothing happened (you can always unregister the wakelock in the lock re-acquired callback). But I'm not sure that the wakelock API needs to notify on force-broken or re-acquired. This is the second half of comment 57. I think most users of the wakelock API won't care when their lock is force-broken or re-acquired. Better to have a simpler wakelock API and expose separate APIs for notifying on "screen enabled/disabled", "network connection enabled/disabled". (Note also that notify on CPU wakelock force-broken doesn't make much sense. We can't notify after the wake-lock was force-broken, because then the CPU isn't running. And we can't notify before, because then we'd have to synchronously notify, at which point the page could spin and keep us from turning off the CPU indefinitely.)

Worth checking prior art here from android.

What I suggested matches the Android API. There's no notification when your wakelock is removed, and you can acquire a screen wakelock while the screen is off, and this doesn't necessarily turn it back on.

Does a force-dropped lock automatically re-acquire when the device comes on?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #70) > Does a force-dropped lock automatically re-acquire when the device comes on? As far as I can tell from the docs. Looks like we'd have to read or write some code to really be sure.

Regardless of what Android does, it doesn't make much sense to me to require you to re-register your wakelock when it's force-unlocked if you don't notify when the wakelock is force-unlocked and when you're eligible to re-acquire the lock.

I agree. I'm tentatively in favor of don't-notify/auto-reacquire.

Hi guys, finally caught up on this bug, and unsurprisingly I have lots of opinions :) First off, let's talk about what we want applications to be able to do: The most basic use-case is for a video-playing app (or webpage) to want to disable the "screen saver" from starting while the user is watching the video. And by "screen saver" I mean the normal screen saver on desktop, and dimming/turning off the screen on mobile. For this use-case, the user pressing the "turn off" button should clearly override the lock. However it shouldn't release the lock (or at least it doesn't need to)! The second use-case would be wanting to keep the CPU going even when the user "sleeps" the device. One situation that I hit very often is when in a camera app. Right after taking a picture I tend to want to hit the sleep button right away and put the camera in my pocket. But the app likely will want to JPEG encode (or hipstamatic reformat) the picture and save it to disk. Another example here is wanting to run a long-running calculation. It's great if the user can start such a task, press the "sleep" button and have the device go to sleep as soon as the task is done. This is something I have wished for many times when compiling firefox and then shutting my laptop. Here too the user can override the lock by force-closing the app or by completely shutting down the device. Possibly in both these instances we'll want to give the app a second or two to finish up whatever it's doing if it's holding the lock though. The third use-case that people has mentioned is wanting to keep different types of network connectivity (wifi/3g/2g) alive. I don't yet fully understand this use-case though, so a few questions: * Can someone provide an example of what type of app needs this. * Would the connection be held open even if the user presses the "sleep" button. (I presume yes?) * Do we need fine-grained control over which connectivity should be kept alive? I.e. does the app need to be able to say "hold 3g alive" or "hold wifi alive". Or is it enough to be able to say "hold data-connectivity alive". The latter seems more convenient for pages. The question is if it supports all use-cases. * Would it be allowed to switch from 3g to wifi when the lock is held? Keeping in mind that this would likely switch IP number and drop all current connections. Or is the idea that as long as the lock is held we try not to switch IP number (which might prevent us from upgrading from 2g to 3g for example, not sure). A different type of use-case that has been discussed is "apps with screen-management permission". While I could see having apps which overrode how/when/how-much we dim the screen, I don't think this is a urgent use-case. I.e. I'd rather not worry about that now. We should certainly allow apps to have access to settings, and have settings for how quickly and how much we dim the screen, but the actual interacting directly with the screen should IMO for now be done just by gaia/gecko. Which brings me to the discussion about rings. The way I look at it gaia and gecko should have the same amount of trust. I.e. I would consider gaia+gecko to make up the "platform". The platform has system level privileges and is trusted to do anything and relied on to do things correctly. Again, long term I think we should allow applications to replace parts of the platform. I.e. it'd be cool with applications which can replace the status bar, the virtual keyboard, the lock-screen, the "desktop" etc. But I don't think we should worry about that for now. iOS doesn't allow any of that, and Android allows little, if any, of it. I absolutely think we should do those types of things, but I think we should keep such APIs separate, and I think we should worry about it once we have more of the "normal" apps pieces in place since normal apps will be the much more common use case and so should be what is the highest priority for the API design. As for using CSS to screen-locking. I agree that while it would let us do elegant solutions for a few use-cases, but it wouldn't cover enough of them. So while we could do: video:-moz-full-screen { screen-saver: none; } we don't have a pseudo-class for paused videos afaik. So what you'd really want to do: video:-moz-full-screen:not(:paused) { screen-saver: none; } doesn't work. And I often watch video not in full-screen, in which case the above wouldn't be enough. I'd rather focus on a JS-API which can solve all our use-cases for now, and add something like CSS-support later if developers ask for it.

(In reply to Jonas Sicking (:sicking) from comment #74) > [...] > The third use-case that people has mentioned is wanting to keep different > types of network connectivity (wifi/3g/2g) alive. I don't yet fully > understand this use-case though, so a few questions: > * Can someone provide an example of what type of app needs this. > * Would the connection be held open even if the user presses the "sleep" > button. (I presume yes?) An IRC client. The IRC client I tried using on android got shut down when I sleep the screen, which was annoying. An SSH or chat client might be another example. > * Do we need fine-grained control over which connectivity should be kept > alive? I.e. does the app need to be able to say "hold 3g alive" or "hold > wifi alive". Or is it enough to be able to say "hold data-connectivity > alive". The latter seems more convenient for pages. The question is if it > supports all use-cases. I think it makes more sense to let the user specify this at a platform level, i.e. have a setting somewhere in the device's config saying "only allow apps to use wifi when screen is off" etc. I'm running Cyanogenmod on my Android phone, and I like how it enables me to specify to download app updates over wifi only, and not 3G. So having the app request to keep data connection, without specifying what type of connection might be enough?

> An IRC client. The IRC client I tried using on android got shut down when I sleep the > screen, which was annoying. An SSH or chat client might be another example. We need to be careful about this. Keeping a network connection alive can very quickly drain the battery, even if you're not sending much data over the connection, aiui. So we need to balance the annoyance of reconnecting to your IRC server with the annoyance of having crummy battery life. As currently spec'ed, websockets don't let you talk on arbitrary ports. Unless we changed this, an IRC client built using websockets would need to proxy its connection through a server. In that case, the proxy can keep the connection to the chat server alive, and the mobile device needs only reconnect to the proxy server. This is what IRCCloud currently does. Anyway, other use-cases for keeping the connection alive include: A music sync app. Maybe it only syncs while your device is on wifi and plugged into power, or something. Or even consider an e-mail app which wants to check your e-mail while the device is asleep and buzz when you get mail.

Justin: Is a music-sync app different from other types of syncing, such as mail-sync or contact sync? It seems to me that syncing is better done by allowing apps to temporarily enable the network, do the syncing and then disable it, rather than keeping the network awake for longer periods of time. Or did you mean that that's what they should do, but if the user puts the device to sleep during such a sync we should let the app finish the current sync before turning off the network connection? In any case it sounds like in all situations when keeping the network "locked" the page would also want to keep the CPU going. It also sounds like we don't need to work any extra hard to avoid switching between wifi/3g/2g just because a network lock is held. I think we as a general principle should try to avoid switching between wifi/(3g+2g) whenever there is active network communication, but probably only for a limited time. In any case that seems like an orthogonal feature that's off-topic for this bug.

> Justin: Is a music-sync app different from other types of syncing, such as mail-sync or contact sync? Only inasmuch as it may use a lot more data. One might feasibly sync e-mail only when the phone is awake, since presumably it'll be fast. But music sync needs to go on even when the phone is "asleep". > In any case it sounds like in all situations when keeping the network "locked" the page would also > want to keep the CPU going. Agreed!

(In reply to Jonas Sicking (:sicking) from comment #74) > Again, long term I think we should allow applications to replace parts of > the platform. I.e. it'd be cool with applications which can replace the > status bar, the virtual keyboard, the lock-screen, the "desktop" etc. But I > don't think we should worry about that for now. iOS doesn't allow any of > that, and Android allows little, if any, of it.f developers ask for it. AFAIK, Android allows apps to override the home screen and the virtual keyboard. (In reply to Justin Lebar [:jlebar] from comment #76) > Anyway, other use-cases for keeping the connection alive include: A music > sync app. Maybe it only syncs while your device is on wifi and plugged into > power, or something. Or even consider an e-mail app which wants to check > your e-mail while the device is asleep and buzz when you get mail. Also a twitter client or any kind of server like sshd running on the phone. (In reply to Chris Pearce, Mozilla Corporation (:cpearce) from comment #75) > (In reply to Jonas Sicking (:sicking) from comment #74) > > [...] > > The third use-case that people has mentioned is wanting to keep different > > types of network connectivity (wifi/3g/2g) alive. I don't yet fully > > understand this use-case though, so a few questions: > > * Can someone provide an example of what type of app needs this. > > * Would the connection be held open even if the user presses the "sleep" > > button. (I presume yes?) > > An IRC client. The IRC client I tried using on android got shut down when I > sleep the screen, which was annoying. An SSH or chat client might be another > example. On Android, network connections can be requested and used even when the screen is shut down. However, all applications are considered as backgrounded and some applications might do some things in that situation or even be killed. > > * Do we need fine-grained control over which connectivity should be kept > > alive? I.e. does the app need to be able to say "hold 3g alive" or "hold > > wifi alive". Or is it enough to be able to say "hold data-connectivity > > alive". The latter seems more convenient for pages. The question is if it > > supports all use-cases. > > I think it makes more sense to let the user specify this at a platform > level, i.e. have a setting somewhere in the device's config saying "only > allow apps to use wifi when screen is off" etc. That might be interesting if Gaia could disable mobile and/or wifi connections in some situations like when the phone is sleeping and not plugged. Then, apps will be able to request a connection when the phone is sleeping and this will happen unless all type of connections have been disabled. > So having the app request to keep data connection, without specifying what > type of connection might be enough? I agree with that: we shouldn't let apps require a specific connection (unless if the correct permission is requested). However, they should be able to warn users that they need wifi to be turned on for example.

>> So having the app request to keep data connection, without specifying what >> type of connection might be enough? > > I agree with that: we shouldn't let apps require a specific connection (unless if the correct > permission is requested). To clarify, what you mean is that an unprivileged app shouldn't be able to say "keep the 3g connection on" but it should be able to say "I'm only going to send data if we're on wifi", right? I think this is right, although it presents complications for apps which want to have a persistent connection.

(In reply to Justin Lebar [:jlebar] from comment #80) > >> So having the app request to keep data connection, without specifying what > >> type of connection might be enough? > > > > I agree with that: we shouldn't let apps require a specific connection (unless if the correct > > permission is requested). > > To clarify, what you mean is that an unprivileged app shouldn't be able to > say "keep the 3g connection on" but it should be able to say "I'm only going > to send data if we're on wifi", right? I think this is right, although it > presents complications for apps which want to have a persistent connection. Yes. So, to be clear -but that might be a bit OT-, I think requesting a connection should require privileges at least because users need to be warn of that a way or another. I think an app should be able to ask for a network connection but shouldn't be able to require the type. The system will create a connection if none exist yet in the type that seems the best. However, an app should be able to check what is the current network type (requires privileges again) and say "seems like you need to use WiFi or Ethernet because I'm not going to send this gig of data trough a mobile network".

(In reply to Jonas Sicking (:sicking) from comment #74) > > A different type of use-case that has been discussed is "apps with > screen-management permission". While I could see having apps which overrode > how/when/how-much we dim the screen, I don't think this is a urgent > use-case. I.e. I'd rather not worry about that now. We should certainly > allow apps to have access to settings, and have settings for how quickly and > how much we dim the screen, but the actual interacting directly with the > screen should IMO for now be done just by gaia/gecko. > > > Which brings me to the discussion about rings. The way I look at it gaia and > gecko should have the same amount of trust. I.e. I would consider gaia+gecko > to make up the "platform". The platform has system level privileges and is > trusted to do anything and relied on to do things correctly. > IMO I would prefer Gaia to stay a normal web app. This is a really useful sandbox to experiment and draw the gap between what the web can do and can't do. I have nothing about exposing some privileged interfaces to the homescreen application until we figured out some correct APIs and find time to add it and this is easy to track how many bridges are exposed from the b2g/ chrome app to Gaia. Also others are doing their own homescreen + set of apps, and that could be helpful to have their opinion/use cases about what is missing. > Again, long term I think we should allow applications to replace parts of > the platform. I.e. it'd be cool with applications which can replace the > status bar, the virtual keyboard, the lock-screen, the "desktop" etc. But I > don't think we should worry about that for now. iOS doesn't allow any of > that, and Android allows little, if any, of it. > The homescreen is just an app for the moment. And it can be replaced by something else. On my Android phone I have 2 homescreens - I have a choice between 'Launcher' and 'B2G' when I hit the 'Home' button.

Jonas, let me know if you'd prefer to review this, or if you're OK doing just the SR. For now, I'm going to assume you're OK with me reviewing.

>+ /** >+ * Request a wakelock for specified resource. >+ * >+ * @param aTopic resource name >+ */ >+ nsIDOMMozWakeLock requestWakeLock(in DOMString aTopic); Please describe what requesting a wakelock does and what the return value is good for. What happens if I request a wake lock on an unrecognized topic? Does the method throw an exception, return null, or return a useless lock object? (I think we probably should throw, but I can see the reasoning behind returning a useless lock.) Jonas, what do you think? Nit: We need to decide how "wakelock" is spelled in code and English. It looks like you're doing "WakeLock" in code, so perhaps it should be "wake lock" in English. >diff --git a/dom/power/nsIDOMPowerManager.idl b/dom/power/nsIDOMPowerManager.idl >--- a/dom/power/nsIDOMPowerManager.idl >+++ b/dom/power/nsIDOMPowerManager.idl >@@ -32,14 +32,19 @@ > * 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 "nsISupports.idl" > >-[scriptable, uuid(6ec16abc-2fe8-4ab3-99b0-0f08405be81b)] >+interface nsIDOMMozWakeLockListener; >+ >+[scriptable, uuid(abf4b2b1-139d-4eff-998d-8f24616910ae)] > interface nsIDOMMozPowerManager : nsISupports > { >- void powerOff(); >- void reboot(); >+ void powerOff(); >+ void reboot(); >+ void addWakeLockListener(in nsIDOMMozWakeLockListener aListener); >+ void removeWakeLockListener(in nsIDOMMozWakeLockListener aListener); >+ boolean wakeLockIsHeld(in DOMString aTopic); > }; This needs a comment explaining that it implements navigator.mozPower. addWakeLockListener needs a comment explaining when the callback is fired. >diff --git a/dom/power/nsIDOMWakeLock.idl b/dom/power/nsIDOMWakeLock.idl >new file mode 100644 >--- /dev/null >+++ b/dom/power/nsIDOMWakeLock.idl >@@ -0,0 +1,14 @@ >+/* -*- Mode: C++; tab-width: 40; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+#include "nsISupports.idl" >+ >+[scriptable, uuid(2e61eed1-5983-4562-8f26-fd361ab4a00d)] >+interface nsIDOMMozWakeLock : nsISupports >+{ >+ readonly attribute DOMString topic; >+ >+ void unlock(); Need to document what happens if I call |unlock| more than once. >diff --git a/dom/power/nsIDOMWakeLockListener.idl b/dom/power/nsIDOMWakeLockListener.idl >new file mode 100644 >--- /dev/null >+++ b/dom/power/nsIDOMWakeLockListener.idl >@@ -0,0 +1,12 @@ >+/* -*- Mode: C++; tab-width: 40; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+#include "nsISupports.idl" >+ >+[scriptable, function, uuid(4e258af8-cffb-47bc-b16d-e8241243426e)] >+interface nsIDOMMozWakeLockListener : nsISupports >+{ >+ void callback(in DOMString aType, in DOMString aTopic); >+}; This needs documentation; in particular, where is this callback registered, and what is aType? >diff --git a/dom/power/nsIPowerManagerService.idl b/dom/power/nsIPowerManagerService.idl >--- a/dom/power/nsIPowerManagerService.idl >+++ b/dom/power/nsIPowerManagerService.idl >-[scriptable, builtinclass, uuid(38919539-4641-4f0b-9f11-6b6294a9386f)] >+interface nsIDOMMozWakeLock; >+interface nsIDOMMozWakeLockListener; >+ >+[scriptable, builtinclass, uuid(235ca1a1-d0c8-41f3-9b4a-dbaa4437d69c)] > interface nsIPowerManagerService : nsISupports This needs a comment explaining how it's different from nsIDOMMozPowerManager. But reading through part 2, I'm thinking we may want to roll this whole interface into hal. Suppose we have a browser app running in a separate process from Gaia. The browser requests a wake lock, and we want Gaia to be able to act upon it. As is, that won't work, because the power manager service does not cross process boundaries. Instead, the power manager service needs to run in the top-level process and be queried by lower-level processes. The infrastructure for doing this IPC is in hal, so calling into hal directly from the relevant Navigator code seems like it may be the way to go. (Btw, I don't mind if you post one big patch rather than three smaller ones; whatever is easier for you.)

Comment on attachment 596639 [details] [diff] [review] Part 1, Add wakelock interfaces Leaving the sr?sicking in place, because the DOM interfaces are not in question.

Comment on attachment 596637 [details] [diff] [review] Part 2, Implement wakelock interfaces One crucial thing which is missing here, leaving aside the IPC discussion above, is how we automatically cancel wake locks. I think all wake locks a document acquires should be void when the document is unloaded (as in, onunload). Some wake locks should probably be canceled earlier than that -- for example, the screen wake lock should probably be canceled if the document which requested the lock becomes invisible (e.g., I switch away from that tab). We can deal with these special cases later, but we should deal with the general case here. A lot of this code is going to change with the changes to part 1, so I'm not doing a full review here. But skimming this patch, one thing to note is that we don't follow the C convention of declaring all our variables at the start of a block. Instead, we declare immediately before use. For example, this + PRUint32 lock; + bool isHeld; + + isHeld = mLockHashtable.Get(aTopic, &lock); becomes + PRUint32 lock; + bool isHeld = mLockHashtable.Get(aTopic, &lock);

Comment on attachment 596641 [details] [diff] [review] Part 3, Add test cases We'll need test cases for things like * Wake locks being canceled when documents are unloaded. * Wake locks being reacquired when documents are re-shown (see comment 73 and earlier). * Correct behavior when double-unlocking a wake-lock. (I'd prefer to throw. Need to also be sure that, even if we throw, we don't decrement our internal lock count.)

> Some wake locks should probably be canceled earlier than that -- for example, the screen wake lock > should probably be canceled if the document which requested the lock becomes invisible (e.g., I > switch away from that tab). Actually, this is a bit more complex than I thought. The trick is, we want all policy to live in Gaia. (Where by "Gaia", I mean Gaia or an alternative.) That should include the meaning of the wake-lock topics. So in order for this to work, isWakeLockHeld() needs to return some information about the documents holding the relevant wake lock. Maybe the info is just [not held, held by at least one visible document, held only by invisible documents]. Chris, do you have any thoughts here?

API looks great. Only question is if we should fire an event on the lock if it's released "early". I guess it greatly depends on when we would do so. If we only release a lock when the user leaves the page then there's obviously no reason to do so. If we release certain locks after a timeout, then that might be more useful. So on second thought, let's worry about that if/when we start adding heuristics for releasing locks early. It'll be a purely additive change so no need to do it prematurely.

> What happens if I request a wake lock on an unrecognized topic? Does the > method throw an exception, return null, or return a useless lock object? I guess we have to return a useless lock object, because gecko doesn't actually know which topics are being monitored and which aren't. So that's simple enough.

> This needs a comment explaining how it's different from > nsIDOMMozPowerManager. The purpose of PowerManagerService is to be used by chrome JS code and C++ code. And since it is a component it can be override by extensions. > But reading through part 2, I'm thinking we may want to roll this whole > interface into hal. Given the reason above, we can move the functional part to hal but still provide the interface.

(In reply to Justin Lebar [:jlebar] from comment #91) > > Some wake locks should probably be canceled earlier than that -- for example, the screen wake lock > > should probably be canceled if the document which requested the lock becomes invisible (e.g., I > > switch away from that tab). > > Actually, this is a bit more complex than I thought. > > The trick is, we want all policy to live in Gaia. (Where by "Gaia", I mean > Gaia or an alternative.) That should include the meaning of the wake-lock > topics. > > So in order for this to work, isWakeLockHeld() needs to return some > information about the documents holding the relevant wake lock. Maybe the > info is just [not held, held by at least one visible document, held only by > invisible documents]. Should we care whether the page is invisible or not? I think the app should do the housekeeping by itself, acquire lock onload and possibly release the lock when it becomes invisible. For example the Music app might want to keep the audio and cpu on when it is running in the background or even when the screen is turned off.

> Given the reason above, we can move the functional part to hal but still provide the interface. Sounds good. > Should we care whether the page is invisible or not? I think the app should do the housekeeping by > itself, acquire lock onload and possibly release the lock when it becomes invisible. Yes, an app *should* do that. But we can't rely on it. > For example the Music app might want to keep the audio and cpu on when it is running in the > background or even when the screen is turned off. Right, and we should allow that. On the other hand, a reader app (or any other app) should not be able to keep the screen on if it's in the background, right?

(In reply to Justin Lebar [:jlebar] from comment #96) > > Given the reason above, we can move the functional part to hal but still provide the interface. > > Sounds good. > > > Should we care whether the page is invisible or not? I think the app should do the housekeeping by > > itself, acquire lock onload and possibly release the lock when it becomes invisible. > > Yes, an app *should* do that. But we can't rely on it. > > > For example the Music app might want to keep the audio and cpu on when it is running in the > > background or even when the screen is turned off. > > Right, and we should allow that. On the other hand, a reader app (or any > other app) should not be able to keep the screen on if it's in the > background, right? They could in theory.. otherwise we have to have different permission group for them.

> They could in theory.. An app could keep the screen turned on while it's in the background only if we design this feature badly, IMO. It will look like *our* bug to a user if the screen is stuck on, even if it's a bug in the app. If we wanted to go down the road of assuming that apps are bug free, we could similarly declare that apps should explicitly release() all wake locks on unload. But we agree that's a bad idea, right? > otherwise we have to have different permission group for them. I'm not sure what you mean by a "permission group". What about, when we ask "is the wakelock for aTopic held?", instead of returning true/false, we returned "not held", "held by at least one foreground app", or "held by only background apps"?

(In reply to Justin Lebar [:jlebar] from comment #98) > > They could in theory.. > > An app could keep the screen turned on while it's in the background only if > we design this feature badly, IMO. It will look like *our* bug to a user if > the screen is stuck on, even if it's a bug in the app. > > If we wanted to go down the road of assuming that apps are bug free, we > could similarly declare that apps should explicitly release() all wake locks > on unload. But we agree that's a bad idea, right? Yeah, I agree that it's very easy to forget to release the lock. We should release all the locks when the page is closed, and we already do. But for background apps, they will want to keep the locks even when the page is invisible. So we can either allow all apps to keep their locks when they are hidden, which I think is bad, or require special permission to do that. > > > otherwise we have to have different permission group for them. > > I'm not sure what you mean by a "permission group". I mean the background app should require special permission to keep the locks. > What about, when we ask "is the wakelock for aTopic held?", instead of > returning true/false, we returned "not held", "held by at least one > foreground app", or "held by only background apps"? That would not work because it's not clear if any "background apps" still needs the resource.

For the code to observe when a tab is no longer visible, see the vibrator code in Navigator.cpp. To observe when a window goes into / out of bfcache (or otherwise just closes) I think you want pagehide / pageshow, but I'm not sure.

>> What about, when we ask "is the wakelock for aTopic held?", instead of >> returning true/false, we returned "not held", "held by at least one >> foreground app", or "held by only background apps"? > > That would not work because it's not clear if any "background apps" still needs the resource. Maybe I'm not explaining myself clearly. Gecko doesn't know or care what any of the wake lock topics do. But Gecko informs the front-end code (either Gaia or Firefox chrome JS) when a wake lock changes between "not held", "held only by background tabs", and "held by at least one foreground tab". Then Gaia or Firefox chrome can decide what to do with this information. For things like the CPU lock, maybe we allow the CPU to stay on if any live page holds a CPU wake lock; we don't require that the page be in the foreground. But for something like the screen wake lock, we enforce that the screen should stay on only if at least one foreground tab holds the screen wake lock. So how this would work is: * An app requests a "screen" wake lock. * In Gaia, our "screen" wake lock listener is poked and informed that the lock is currently held by a visible tab. We therefore disable the screensaver. * Some time later, the tab which requested the screen wake lock is unfocused. * The wake lock listener is poked again; this time, we tell the listener (Gaia code) that the lock is currently held only by invisible tabs. * The Gaia code therefore re-enables the screensaver. * Some time after that, we close the tab. The wake lock listener is poked a third time, informing it that the "screen" wake lock is no longer held by any tabs. We take no action, because the screensaver is already enabled.

FWIW, Justin's design sounds great to me. I think it would be really cool if we could allow the "screen" lock with no security guards to any web page. I think it's something that could be used both by video-playing websites, and by websites with puzzle games. Possibly we'll want to have some sort of UI, but that'll be up to the Firefox front-end people.

> Possibly we'll want to have some sort of UI, but that'll be up to the Firefox front-end people. It strikes me that we could overload the identity popup, which nobody uses atm. When the page starts using something you might want to turn off, we make the identity popup's click target shimmer briefly. When you bring up the popup, it'll say things like "Find your location [*On*] [Off] [Never allow]" and "Keep your screen from turning off [*On*] [Off] [Never]".

One big patch. Maybe I should find :cjones to review hal/ part?

(In reply to Kan-Ru Chen [:kanru] from comment #104) > Created attachment 599555 [details] [diff] [review] > Implement wake lock interface > > One big patch. Maybe I should find :cjones to review hal/ part? Let me see if I feel comfortable with the code.

Try run for 21a53ece95d2 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=21a53ece95d2 Results (out of 282 total builds): exception: 1 success: 228 warnings: 31 failure: 22 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kchen@mozilla.com-21a53ece95d2

ClearOnShutdown should help with the leaks tryserver saw.

Comment on attachment 599555 [details] [diff] [review] Implement wake lock interface > NS_IMETHODIMP > Navigator::GetMozPower(nsIDOMMozPowerManager** aPower) > { > if (!mPowerManager) { >+ nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow); >+ NS_ENSURE_TRUE(win, NS_OK); Hmm...So at the very least, we need to set *aPower = NULL on error. (You can set it to null at the very beginning of the function, for good measure.) It looks like our convention (e.g. for GetGeolocation) is to return null but not throw an error, so returning NS_OK is correct. Sorry to nit on the following comment so much. It's mostly fine, but since there's no spec for this, the comment is our primary documentation, and I'd like to get it right. >+ /** >+ * Request a wake lock for specified resource. s/specified resource/a resource/, or s/specified resource/the specified resource/. Tell me here what it means for a page to hold a wake lock, at a high level. Something like, "A page holds a wake lock to request that a resource not be turned off (or otherwise made unavailable)." >+ * The topic is the name of a resource that might be made unavailable for >+ * various reasons. For example, on mobile device the power manager might >+ * decide to turn off the screen after a period of idle time to save power. >+ * >+ * The resource manager can check the lock state of a topic before shutdown >+ * the resource. So a lock on "screen" could prevent the screensaver from >+ * appearing on Desktop or screen blackout on mobile device. s/on mobile device/on a mobile device/ s/shutdown/shutting down/ I'd restructure this a bit. I'm left searching for the meaning of the topic at the end of the first paragraph. Something like the following: * The resource manager checks the lock state of a topic before turning off * the associated resource. For example, a page could hold a lock on the * "screen" topic to prevent the screensaver from appearing or the screen from * turning off. >+ * The policy is enforced by the resource management code. There is no limit >+ * on the possible topic, but there will be a predefined set of meaningful >+ * topic. * The resource manager defines what each topic means and sets policy. For * example, the resource manager might decide to ignore 'screen' wake locks * held by pages which are not visible. * * One topic can be locked multiple times; it is considered released only when * all locks on the topic have been released. >+ * The returned wake lock object is a token of the lock. One can find the >+ * locked resource through the topic property and release the lock through >+ * the unlock method. The lock is released automatically when the user >+ * navigates away the associated window. >+ * >+ * Same topic can be locked multiple times and is unlocked only after all >+ * wake lock object has been released. s/Same topic/The same topic/ s/wake lock object has/wake lock objects have/ * The returned nsIDOMMozWakeLock object is a token of the lock. You can * unlock the lock via the object's |unlock| method. The lock is released * automatically when its associated window is unloaded. >diff --git a/dom/power/PowerManagerService.cpp b/dom/power/PowerManagerService.cpp >--- a/dom/power/PowerManagerService.cpp >+++ b/dom/power/PowerManagerService.cpp > /* static */ already_AddRefed<nsIPowerManagerService> > PowerManagerService::GetInstance() > { >- nsCOMPtr<nsIPowerManagerService> pmService; >+ nsRefPtr<PowerManagerService> pmService; > > pmService = new PowerManagerService(); >+ pmService->Init(); > > return pmService.forget(); > } Don't you want this to be a singleton? You can make the nsRefPtr a static class member and then call ClearOnShutdown() on it. >+void >+PowerManagerService::Notify(const hal::WakeLockInformation& aWakeLockInfo) >+{ >+ nsAutoString state; >+ ComputeWakeLockState(aWakeLockInfo, state); >+ >+ for (PRUint32 i = mListeners.Length(); i > 0; ) { >+ --i; >+ mListeners[i]->Callback(aWakeLockInfo.topic(), state); >+ } More idiomatic would be for (PRUint32 i = mListeners.Length() - 1; i >= 0; i--) >+NS_IMETHODIMP >+PowerManagerService::AddWakeLockListener(nsIDOMMozWakeLockListener *aListener) >+{ >+ if (mListeners.IndexOf(aListener) != mListeners.NoIndex) >+ return NS_OK; Please use Contains(). >+NS_IMETHODIMP >+PowerManagerService::RemoveWakeLockListener(nsIDOMMozWakeLockListener *aListener) >+{ >+ if (mListeners.IndexOf(aListener) == mListeners.NoIndex) >+ return NS_OK; >+ >+ mListeners.RemoveElement(aListener); >+ return NS_OK; >+} Here you might as well unconditionally call RemoveElement(). >+NS_IMETHODIMP >+PowerManagerService::GetWakeLockState(const nsAString &aTopic, nsAString &aState) >+{ >+ hal::WakeLockInformation info; >+ hal::GetWakeLockInfo(nsString(aTopic), &info); >+ >+ ComputeWakeLockState(info, aState); >+ >+ return NS_OK; >+} >+NS_IMETHODIMP >+PowerManagerService::NewWakeLock(const nsAString &aTopic, >+ nsIDOMWindow *aWindow, >+ nsIDOMMozWakeLock **aWakeLock) >+{ >+ AcquireWakeLock(aTopic, aWindow); >+ >+ nsRefPtr<WakeLock> wakelock = new WakeLock(); >+ nsresult rv = wakelock->Init(aTopic, aWindow); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ NS_ADDREF(*aWakeLock = wakelock); >+ >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+PowerManagerService::AcquireWakeLock(const nsAString &aTopic, >+ nsIDOMWindow *aWindow) >+{ >+ bool hidden = true; >+ >+ if (aWindow) { >+ nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(aWindow); >+ NS_ENSURE_TRUE(win, NS_ERROR_FAILURE); A synonym is NS_ENSURE_STATE(win). I don't much care which you use, but some reviewers prefer ENSURE_STATE, so maybe it's a good habit to be in. >+ nsIDOMDocument* domDoc = win->GetExtantDocument(); >+ NS_ENSURE_TRUE(domDoc, NS_ERROR_FAILURE); >+ domDoc->GetMozHidden(&hidden); >+ } >+ >+ hal::AcquireWakeLock(nsString(aTopic), hidden); hal::AcquireWakeLock and company should take nsAString, not nsString. Then you won't need the nsString() wrapper here or elsewhere. (Apparently IPDL requires nsString's, but let's make that conversion as late as possible.) >+void >+WakeLock::DoLock() >+{ >+ if (!mLocked) { >+ nsCOMPtr<nsIPowerManagerService> pmService = >+ do_GetService(POWERMANAGERSERVICE_CONTRACTID); >+ nsCOMPtr<nsIDOMWindow> window = do_QueryReferent(mWindow); >+ >+ pmService->AcquireWakeLock(mTopic, window); >+ mLocked = true; >+ } >+} >+ >+void >+WakeLock::DoUnlock() >+{ >+ if (mLocked) { >+ nsCOMPtr<nsIPowerManagerService> pmService = >+ do_GetService(POWERMANAGERSERVICE_CONTRACTID); >+ nsCOMPtr<nsIDOMWindow> window = do_QueryReferent(mWindow); >+ >+ pmService->ReleaseWakeLock(mTopic, window); >+ mLocked = false; >+ } I'm concerned about subtle bugs being introduced here. For example, if a window is deleted before it's marked as hidden, then when we DoUnlock, we'll release the wake lock as though the window were hidden, and all our counts will be messed up. We talked in person and agreed that the WakeLock object should remember the last visibilty state it sent to hal. I haven't looked through all of this, but a new version is coming, so I'll wait before looking at the rest.

(In reply to Justin Lebar [:jlebar] from comment #108) > >+void > >+PowerManagerService::Notify(const hal::WakeLockInformation& aWakeLockInfo) > >+{ > >+ nsAutoString state; > >+ ComputeWakeLockState(aWakeLockInfo, state); > >+ > >+ for (PRUint32 i = mListeners.Length(); i > 0; ) { > >+ --i; > >+ mListeners[i]->Callback(aWakeLockInfo.topic(), state); > >+ } > > More idiomatic would be > > for (PRUint32 i = mListeners.Length() - 1; i >= 0; i--) > Not a good idea, unsigned int >= 0 is always true!

Fair! :) Why do we iterate backwards through this list, anyway? We want to call back the most-recently added callbacks before the older ones? I doubt that's important.

Hm...actually, mListeners can be modified while we iterate over it (we're running arbitrary script). I wonder if we handle this elsewhere (e.g. by making a copy).

Kan-Ru, if you need to check this in to b2g (not m-c) Right Now for the demo, that's OK with me, if you've tested that things work as you expect.

(In reply to Justin Lebar [:jlebar] from comment #111) > Hm...actually, mListeners can be modified while we iterate over it (we're > running arbitrary script). I wonder if we handle this elsewhere (e.g. by > making a copy). Someone told me that DOM is single threaded so I didn't bother to check that. But yes, the service might be accessed from other threads :-/

All this code is single-threaded, yes. But running mListeners[i]->Callback() actually runs JS code (afaict). And that code could add or remove a wake lock listener.

Changed wake lock implementation to use hal directly. PowerManagerService now exports only minimum interfaces required to use wake lock. The leaks should be gone now..

Try run for 20e43fdd7711 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=20e43fdd7711 Results (out of 128 total builds): exception: 51 success: 66 warnings: 4 failure: 7 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kchen@mozilla.com-20e43fdd7711

Update test cases

Mounir might want to look at the Hal.cpp changes (I think they're fine).

Try run for 14cc0af010a8 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=14cc0af010a8 Results (out of 279 total builds): exception: 3 success: 225 warnings: 27 failure: 24 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kchen@mozilla.com-14cc0af010a8

Comment on attachment 600050 [details] [diff] [review] Implement wake lock interface v2.1 >@@ -941,24 +946,45 @@ Navigator::GetMozBattery(nsIDOMMozBatter > > return NS_OK; > } > > NS_IMETHODIMP > Navigator::GetMozPower(nsIDOMMozPowerManager** aPower) > { > if (!mPowerManager) { >+ *aPower = nsnull; It's convention to do *aPower = nsnull unconditionally, just so someone doesn't have to scan the code to see that it's correct. >+ * Request a wake lock for a resource. This comment looks great; thanks. >+nsRefPtr<PowerManagerService> PowerManagerService::sSingleton; Please mark this as static. > /* static */ already_AddRefed<nsIPowerManagerService> > PowerManagerService::GetInstance() > { >- nsCOMPtr<nsIPowerManagerService> pmService; >+ if (!sSingleton) { >+ sSingleton = new PowerManagerService(); >+ sSingleton->Init(); >+ ClearOnShutdown(&sSingleton); >+ } > >- pmService = new PowerManagerService(); >+ NS_ADDREF(sSingleton); >+ return sSingleton.get(); This works, but raw addref/release is scary, so we prefer nsCOMPtr<nsIPowerManagerService> service = sSingleton; return service.forget(); >+void >+PowerManagerService::ComputeWakeLockState(const hal::WakeLockInformation& aWakeLockInfo, >+ nsAString &aState) >+{ >+ if (aWakeLockInfo.locks() == 0) { >+ aState.AssignLiteral("notheld"); >+ } else if (aWakeLockInfo.locks() == aWakeLockInfo.hidden()) { >+ aState.AssignLiteral("background"); >+ } else { >+ aState.AssignLiteral("foreground"); >+ } Maybe "unlocked/locked-background/locked-foreground" would be clearer? >+void >+PowerManagerService::Notify(const hal::WakeLockInformation& aWakeLockInfo) >+{ >+ nsAutoString state; >+ ComputeWakeLockState(aWakeLockInfo, state); >+ >+ nsTArray<nsCOMPtr<nsIDOMMozWakeLockListener> > listeners(mListeners); Please add a comment explaining why we're copying this list, and why copying is OK (we expect no more than one listener per window, so it shouldn't be too long). >+NS_IMETHODIMP >+PowerManagerService::NewWakeLock(const nsAString &aTopic, >+ nsIDOMWindow *aWindow, >+ nsIDOMMozWakeLock **aWakeLock) >+{ >+ nsRefPtr<WakeLock> wakelock = new WakeLock(); >+ nsresult rv = wakelock->Init(aTopic, aWindow); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ NS_ADDREF(*aWakeLock = wakelock); Out of general fear of raw addref/release, how about *aWakeLock = wakelock.forget() >diff --git a/dom/power/PowerManagerService.h b/dom/power/PowerManagerService.h >--- a/dom/power/PowerManagerService.h >+++ b/dom/power/PowerManagerService.h > class PowerManagerService > : public nsIPowerManagerService >+ , public WakeLockObserver > { > public: > NS_DECL_ISUPPORTS > NS_DECL_NSIPOWERMANAGERSERVICE > > static already_AddRefed<nsIPowerManagerService> GetInstance(); >+ >+ void Init(); >+ >+ void Notify(const hal::WakeLockInformation& aWakeLockInfo); NotifyWakeLockState, NotifyWakeLockStateChange, or something? >+ void ComputeWakeLockState(const hal::WakeLockInformation& aWakeLockInfo, >+ nsAString &aState); Does this need to be public? >+ >+private: >+ >+ ~PowerManagerService(); >+ static nsRefPtr<PowerManagerService> sSingleton; >+ >+ nsTArray<nsCOMPtr<nsIDOMMozWakeLockListener> > mListeners; mWakeLockListeners? >diff --git a/dom/power/Types.h b/dom/power/Types.h >new file mode 100644 >--- /dev/null >+++ b/dom/power/Types.h >@@ -0,0 +1,21 @@ >+/* -*- Mode: C++; tab-width: 40; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */ >+#ifndef mozilla_dom_power_Types_h >+#define mozilla_dom_power_Types_h >+ >+namespace mozilla { >+namespace hal { >+class WakeLockInformation; >+} // namespace hal >+ >+template <class T> >+class Observer; >+ >+typedef Observer<hal::WakeLockInformation> WakeLockObserver; >+ >+} // namespace mozilla >+ >+#endif // mozilla_dom_power_Types_h Ugh, I hate this stuff, but Mounir likes it, and he owns most of this code. :( >diff --git a/dom/power/WakeLock.cpp b/dom/power/WakeLock.cpp >new file mode 100644 >--- /dev/null >+++ b/dom/power/WakeLock.cpp >@@ -0,0 +1,163 @@ >+nsresult >+WakeLock::Init(const nsAString &aTopic, nsIDOMWindow *aWindow) >+{ >+ mTopic.Assign(aTopic); >+ mWindow = do_GetWeakReference(aWindow); >+ >+ nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow); >+ >+ if (window) { Can you comment here that null windows are explicitly allowed, and considered always invisible? >+ nsCOMPtr<nsIDOMDocument> domDoc = window->GetExtantDocument(); >+ NS_ENSURE_STATE(domDoc); >+ domDoc->GetMozHidden(&mHidden); >+ >+ nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(domDoc); >+ target->AddSystemEventListener(NS_LITERAL_STRING("mozvisibilitychange"), >+ this, true, false); >+ >+ target = do_QueryInterface(window); >+ target->AddSystemEventListener(NS_LITERAL_STRING("pagehide"), >+ this, true, false); >+ target->AddSystemEventListener(NS_LITERAL_STRING("pageshow"), >+ this, true, false); Can you please document the stupid boolean params? e.g. AddSystemEventListener(/* useCapture = */ true, /* wantsUntrusted = */ false); (Full disclosure: I have a personal vendetta against opaque boolean params like these. http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html) >+NS_IMETHODIMP >+WakeLock::Unlock() >+{ >+ if (!mLocked) { >+ return NS_ERROR_FAILURE; >+ } This will cause a JS exception on double-unlock. That's fine (I like APIs which loudly point out errors to users), but I wanted to make sure you were aware. You should add a test for this behavior, if you don't already have one, so we don't regress it. >+NS_IMETHODIMP >+WakeLock::HandleEvent(nsIDOMEvent *aEvent) >+{ >+ nsAutoString type; >+ aEvent->GetType(type); >+ >+ if (type.EqualsLiteral("mozvisibilitychange")) { >+ nsCOMPtr<nsIDOMEventTarget> target; >+ aEvent->GetTarget(getter_AddRefs(target)); >+ nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(target); >+ NS_ENSURE_STATE(domDoc); >+ domDoc->GetMozHidden(&mHidden); >+ >+ if (mHidden) { >+ HideOneWakeLock(mTopic); >+ } else { >+ ShowOneWakeLock(mTopic); >+ } Please explicitly call hal::{Show,Hide}OneWakeLock; it's short and simple enough. >diff --git a/dom/power/WakeLock.h b/dom/power/WakeLock.h >new file mode 100644 >--- /dev/null >+++ b/dom/power/WakeLock.h >+ void DoUnlock(); >+ void DoLock(); Why are these public? >diff --git a/dom/power/nsIDOMPowerManager.idl b/dom/power/nsIDOMPowerManager.idl >--- a/dom/power/nsIDOMPowerManager.idl >+++ b/dom/power/nsIDOMPowerManager.idl >+ /** >+ * The listeners are notified when the first wake lock of a topic >+ * is acquired and the last wake lock of the topic is released. >+ */ >+ void addWakeLockListener(in nsIDOMMozWakeLockListener aListener); >+ void removeWakeLockListener(in nsIDOMMozWakeLockListener aListener); This should probably be reworded now that a lock can be in one of three, not two, states. >+[scriptable, function, uuid(4e258af8-cffb-47bc-b16d-e8241243426e)] >+interface nsIDOMMozWakeLockListener : nsISupports >+{ >+ /** >+ * The callback will be called when: >+ * - a topic is locked first time, >+ * - all wake locks of the topic has been released, s/has/have >+ * - a wake lock changes its visibility Maybe we can simplify this to "a wake lock changes in state", where a wake lock's state is defined as the max of [unlocked, locked but not visible, locked and visible]. At least, I like to think of it mathematically like this... >+ * The visibility of a wake lock is bound to the window that >+ * requested it. When the window is hidden, the wake lock is also >+ * hidden. Possible states are "background", "foreground", >+ * and "notheld". >+ * >+ * - "foreground" means at least one window that holds the wake lock >+ * is visible. >+ * >+ * - "background" means at least one window holds the wake lock, >+ * but all of them are hidden. >+ * >+ * - "notheld" means nobody holds the wake lock. It would be clearer here if you distinguished between the state of an individual wake lock, which is tied to a window, and the lock state of a /topic/, which is the max of the 'unlocked' and the relevant wake locks' states. >+[scriptable, builtinclass, uuid(235ca1a1-d0c8-41f3-9b4a-dbaa4437d69c)] > interface nsIPowerManagerService : nsISupports Could you please add a brief comment explaining what nsIPowerManagerService is for? Like, why not have the windows register with Hal directly? >- void powerOff(); >- void reboot(); >+ void powerOff(); >+ void reboot(); >+ void addWakeLockListener(in nsIDOMMozWakeLockListener aListener); >+ void removeWakeLockListener(in nsIDOMMozWakeLockListener aListener); >+ DOMString getWakeLockState(in DOMString aTopic); >+ >+ /** >+ * Return a wake lock object of aTopic associated with aWindow. >+ * A wake lock without associated window is always considered invisible. >+ */ >+ nsIDOMMozWakeLock newWakeLock(in DOMString aTopic, [optional] in nsIDOMWindow aWindow); I'm not sure what a null window would be good for, but sure. :) (If this is ever useful, I'd imagine we'd want to allow forced-visible locks too. But we'll worry about it later.) >diff --git a/dom/power/test/test_power_basics.html b/dom/power/test/test_power_basics.html >--- a/dom/power/test/test_power_basics.html >+++ b/dom/power/test/test_power_basics.html Not reviewing the test since you just updated it. >diff --git a/hal/Hal.cpp b/hal/Hal.cpp >--- a/hal/Hal.cpp >+++ b/hal/Hal.cpp >@@ -182,52 +182,70 @@ public: > mObservers->RemoveObserver(aObserver); > > if (mObservers->Length() == 0) { > DisableNotifications(); > > delete mObservers; > mObservers = 0; > >- mHasValidCache = false; >+ PostRemoveObserver(); > } > } > >+ void BroadcastInformation(const InfoType& aInfo) { >+ MOZ_ASSERT(mObservers); >+ mObservers->Broadcast(aInfo); >+ } >+ >+protected: >+ virtual void EnableNotifications() = 0; >+ virtual void DisableNotifications() = 0; >+ virtual void PostRemoveObserver() {} This PostRemoveObservers() call was confusing to me. But basically, it's just signaling that DisableNotificaitons() was called, right? Then we should call it OnNotificationsDisabled, or PostNotificationsDisabled or something. >+void >+AcquireWakeLock(const nsAString &aTopic, bool aHidden) >+{ >+ AssertMainThread(); >+ PROXY_IF_SANDBOXED(AcquireWakeLock(aTopic, aHidden)); >+} >+ >+void >+ReleaseWakeLock(const nsAString &aTopic, bool aHidden) >+{ >+ AssertMainThread(); >+ PROXY_IF_SANDBOXED(ReleaseWakeLock(aTopic, aHidden)); >+} Would you mind changing these bools to enum WakeLockState { WAKE_LOCK_STATE_UNLOCKED, WAKE_LOCK_STATE_HIDDEN, WAKE_LOCK_STATE_VISIBLE }; ? >@@ -222,16 +223,79 @@ void NotifyNetworkChange(const hal::Netw > */ > void Reboot(); > > /** > * Power off the device. > */ > void PowerOff(); > >+/** >+ * Enable wake lock notifications from the backend >+ * >+ * This method is only visible from implementation of power manager service. >+ * Rest of the system should not try this. >+ */ >+void EnableWakeLockNotifications(); >+ >+/** >+ * Enable wake lock notifications from the backend >+ * >+ * This method is only visible from implementation of power manager service. >+ * Rest of the system should not try this. >+ */ >+void DisableWakeLockNotifications(); Please end the first sentence with a period. The method is /visible/ outside the power manager service. But it shouldn't be /used/ from elsewhere... (Actually, it shouldn't be used outside the WakeLockObserversManager, not the power manager service, right?) >+ * Increase the wake lock count of aTopic Nit: Period at end of sentence. >+ * Decrease the wake lock count of aTopic >+ * Mark one wake lock of aTopic as hidden >+ * Mark one wake lock of aTopic as visible >+ * Query the wake lock numbers of aTopic Periods here. >diff --git a/hal/Makefile.in b/hal/Makefile.in >--- a/hal/Makefile.in >+++ b/hal/Makefile.in >@@ -70,42 +70,47 @@ CPPSRCS = \ > SandboxHal.cpp \ > WindowIdentifier.cpp \ > $(NULL) > > ifeq (android,$(MOZ_WIDGET_TOOLKIT)) > CPPSRCS += \ > AndroidHal.cpp \ > AndroidSensor.cpp \ >+ FallbackWakeLock.cpp \ > $(NULL) > else ifeq (gonk,$(MOZ_WIDGET_TOOLKIT)) > CPPSRCS += \ > GonkHal.cpp \ > Power.cpp \ > GonkSensor.cpp \ >+ FallbackWakeLock.cpp \ > $(NULL) > else ifeq (Linux,$(OS_TARGET)) > CPPSRCS += \ > LinuxHal.cpp \ > FallbackSensor.cpp \ > Power.cpp \ >+ FallbackWakeLock.cpp \ > $(NULL) > ifdef MOZ_ENABLE_DBUS > CPPSRCS += UPowerClient.cpp > endif > else ifeq (WINNT,$(OS_TARGET)) > CPPSRCS += \ > WindowsHal.cpp \ > WindowsBattery.cpp \ > FallbackSensor.cpp \ >+ FallbackWakeLock.cpp \ > $(NULL) > else > CPPSRCS += \ > FallbackHal.cpp \ > FallbackSensor.cpp \ >+ FallbackWakeLock.cpp \ > $(NULL) > endif Just put FallbackWakeLock.cpp in the unconditional CPPSRCs above. And maybe rename it WakeLock.cpp or HalWakeLock.cpp, since it's fully-functional (i.e., it's not a fallback). >+static int sActiveChildren = 0; >+static nsAutoPtr<nsDataHashtable<nsStringHashKey, PRUint32> > sLockTable; >+static nsAutoPtr<nsDataHashtable<nsStringHashKey, PRUint32> > sHiddenTable; I'd prefer if this was one hash table storing a struct of {lock,hidden}. >+void >+ReleaseWakeLock(const nsAString &aTopic, bool hidden) >+{ >+ if (!sInitialized) { >+ Init(); >+ } >+ >+ PRUint32 hiddenLocks = 0; >+ sHiddenTable->Get(aTopic, &hiddenLocks); >+ if (hidden) { >+ if (hiddenLocks) { >+ hiddenLocks--; >+ sHiddenTable->Put(aTopic, hiddenLocks); >+ } else { >+ sHiddenTable->Remove(aTopic); >+ } Shouldn't we just MOZ_ASSERT(hiddenLocks)? >+ PRUint32 locks = 0; >+ sLockTable->Get(aTopic, &locks); >+ if (locks) { >+ locks--; >+ sLockTable->Put(aTopic, locks); >+ } else { >+ sLockTable->Remove(aTopic); >+ } Similarly here, shouldn't we just assert(locks)? >+ bool lastLock = (locks == 0); >+ bool lastForeground = (!hidden && locks == hiddenLocks); >+ >+ if (sActiveChildren && (lastLock || lastForeground)) { >+ WakeLockInformation info; >+ info.locks() = locks; >+ info.hidden() = hiddenLocks; >+ info.topic() = aTopic; >+ NotifyWakeLockChange(info); Perhaps you should factor this out? >+void >+HideOneWakeLock(const nsAString &aTopic) >+{ >+ PRUint32 hiddenLocks = 0; >+ sHiddenTable->Get(aTopic, &hiddenLocks); >+ hiddenLocks++; >+ sHiddenTable->Put(aTopic, hiddenLocks); >+ >+ PRUint32 locks = 0; >+ sLockTable->Get(aTopic, &locks); We should assert that hiddenLocks <= locks. >+ if (sActiveChildren && (locks == hiddenLocks)) { >+ WakeLockInformation info; >+ info.locks() = locks; >+ info.hidden() = hiddenLocks; >+ info.topic() = aTopic; >+ NotifyWakeLockChange(info); >+ } >+} >+void >+ShowOneWakeLock(const nsAString &aTopic) >+{ >+ PRUint32 hiddenLocks = 0; >+ sHiddenTable->Get(aTopic, &hiddenLocks); >+ hiddenLocks--; >+ sHiddenTable->Put(aTopic, hiddenLocks); Assert that hiddenLocks > 0 before subtracting. >+ PRUint32 locks = 0; >+ sLockTable->Get(aTopic, &locks); >+ if (sActiveChildren && ((locks - hiddenLocks) == 1)) { >+ WakeLockInformation info; >+ info.locks() = locks; >+ info.hidden() = hiddenLocks; >+ info.topic() = aTopic; >+ NotifyWakeLockChange(info); >+ } >+} >+void >+GetWakeLockInfo(const nsAString &aTopic, WakeLockInformation *aWakeLockInfo) >+{ >+ if (!sInitialized) { >+ Init(); >+ } >+ >+ PRUint32 hiddenLocks = 0; >+ sHiddenTable->Get(aTopic, &hiddenLocks); >+ >+ PRUint32 locks = 0; >+ sLockTable->Get(aTopic, &locks); Just to be paranoid, we should probably assert here that hiddenLocks <= locks. >diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl >--- a/hal/sandbox/PHal.ipdl >+++ b/hal/sandbox/PHal.ipdl >@@ -76,24 +76,33 @@ namespace hal { > > namespace hal { > struct NetworkInformation { > double bandwidth; > bool canBeMetered; > }; > } > >+namespace hal { >+ struct WakeLockInformation { >+ uint32_t locks; >+ uint32_t hidden; >+ nsString topic; >+ }; I'd prefer numLocks and numHidden because hidden() looks like it should return a bool. >+ AcquireWakeLock(nsString aTopic, bool hidden); >+ ReleaseWakeLock(nsString aTopic, bool hidden); >+ HideOneWakeLock(nsString aTopic); >+ ShowOneWakeLock(nsString aTopic); If you wanted to be clever, this could be just one function: ModifyWakeLock(nsString topic, int lockAdjust, int hiddenAdjust) where lockAdjust and hiddenLockAdjust are -1, 0, or 1. Actually, that would simplify the FallbackWakeLock code a lot... I'd be OK if the public hal interface were ModifyWakeLock (it's a low-level interface, after all), but I'm also OK with leaving the public interface as-is and translating those calls into ModifyWakeLock(). r- just because I'd like to look at the ModifyWakeLock code, however you decide to do it. I'll go look at the test now.

> Would you mind changing these bools to > > enum WakeLockState { > WAKE_LOCK_STATE_UNLOCKED, > WAKE_LOCK_STATE_HIDDEN, > WAKE_LOCK_STATE_VISIBLE > }; Actually, this is kind of confusing, because UNLOCKED is not a valid state for either function. Maybe we should just go with ModifyWakeLock everywhere. Or you can just have HIDDEN and VISIBLE.

Interdiff v2 --> v2.1 >--- b/browser/installer/package-manifest.in >+++ a/browser/installer/package-manifest.in >@@ -156,17 +156,16 @@ > @BINPATH@/components/dom_events.xpt > @BINPATH@/components/dom_geolocation.xpt > @BINPATH@/components/dom_network.xpt > @BINPATH@/components/dom_notification.xpt > @BINPATH@/components/dom_html.xpt > @BINPATH@/components/dom_indexeddb.xpt > @BINPATH@/components/dom_offline.xpt > @BINPATH@/components/dom_json.xpt >-@BINPATH@/components/dom_power.xpt > @BINPATH@/components/dom_range.xpt > @BINPATH@/components/dom_sidebar.xpt > @BINPATH@/components/dom_sms.xpt > @BINPATH@/components/dom_storage.xpt > @BINPATH@/components/dom_stylesheets.xpt > @BINPATH@/components/dom_traversal.xpt > @BINPATH@/components/dom_xbl.xpt > @BINPATH@/components/dom_xpath.xpt This was checked in, right?

diff --git a/dom/power/test/power_wakelock_child.html b/dom/power/test/power_wakelock_child.html new file mode 100644 --- /dev/null +++ b/dom/power/test/power_wakelock_child.html @@ -0,0 +1,11 @@ +<!DOCTYPE HTML> +<html> +<head> + <title>Test for Power API</title> + <script type="application/javascript"> + lock = navigator.requestWakeLock("test"); + </script> +</head> +<body> +</body> +</html> diff --git a/dom/power/test/test_power_basics.html b/dom/power/test/test_power_basics.html --- a/dom/power/test/test_power_basics.html +++ b/dom/power/test/test_power_basics.html @@ -9,14 +9,102 @@ <p id="display"></p> <div id="content" style="display: none"> </div> <pre id="test"> <script type="application/javascript"> /** Test for Power API **/ +function checkWakeLock() { + var count = 0; + var callback = function wakeLockListener(topic, state) { + is(topic, "test", "WakeLockListener should receive the lock topic"); + switch (state) { + case "foreground": + count += 1; + break; + case "background": + count += 1; + break; + case "notheld": + count -= 1; + break; + } + } + navigator.mozPower.addWakeLockListener(callback); + + var lock1, lock2; + ok(lock1 = navigator.requestWakeLock("test"), "navigator.requestWakeLock should return a wakelock"); + ok(lock2 = navigator.requestWakeLock("test"), "navigator.requestWakeLock should return a wakelock"); + setTimeout(function() { + is(count, 1, "WakeLockListener should only fire once"); + checkProperty(lock1, "topic"); + checkProperty(lock2, "topic"); We don't need these tests; if lock1.topic doesn't exist, then reading it below will throw an exception and break the test. + is(lock1.topic, "test", "wakelock should remember the locked topic"); + is(lock2.topic, "test", "wakelock should remember the locked topic"); + ok(navigator.mozPower.getWakeLockState("test") == "foreground", "mozPower should remember the locked topic"); + lock1.unlock(); + lock2.unlock(); I'd like a test that when you unlock one of two held locks, we don't get a topic unlock notification. + setTimeout(function() { + ok(navigator.mozPower.getWakeLockState("test") == "notheld", "all locks should have been unlocked"); + is(count, 0, "WakeLockListener should only fire once"); + navigator.mozPower.removeWakeLockListener(callback); + ok(lock1 = navigator.requestWakeLock("test"), "navigator.requestWakeLock should return a wakelock"); + setTimeout(function() { + isnot(count, 1, "navigator.mozPower.removeWakeLockListener should have removed the listener"); + lock1.unlock(); + checkWakeLockNavigation(); + }, 1000); + }, 1000); + }, 1000); +} + +function checkWakeLockNavigation() { + childWindow = window.open("power_wakelock_child.html"); + SimpleTest.waitForFocus(function () { Need to wait for the child window's onload event to fire. + ok(navigator.mozPower.getWakeLockState("test") == "foreground", "lock should be held by child window"); + childWindow.location = "about:blank"; + // XXX how to wait page transition? Add an onload listener and you'll hear it. + setTimeout(function() { + ok(navigator.mozPower.getWakeLockState("test") == "notheld", "lock should be canceled when pagehide."); + childWindow.history.back(); + // XXX how to wait page transition? Onload should do it, again. + setTimeout(function () { + ok(navigator.mozPower.getWakeLockState("test") == "foreground", "lock should be reacquired when pageshow."); + childWindow.close(); + ok(navigator.mozPower.getWakeLockState("test") == "notheld", "lock should be canceled when documents are unloaded."); + SimpleTest.finish(); + }, 1000); + }, 1000); + }, childWindow); +} There's no test of background? You may need to open a new tab. I'm not quite sure how to do that from a mochitest, actually. You may need to write a browser-chrome test, which would be unfortunate. +function test() { + SpecialPowers.setCharPref("dom.mozPowerWhitelist", "http://mochi.test:8888"); The mochitest location has changed in the past. Let's use |'http://' + location.host| instead and hope that we never serve mochitests over https. :) + SimpleTest.waitForExplicitFinish(); + checkInterface("PowerManager"); + checkInterface("WakeLock"); + checkInterface("WakeLockListener"); + checkProperty(navigator, "mozPower"); + checkProperty(navigator, "requestWakeLock"); + checkProperty(navigator.mozPower, "getWakeLockState"); + checkProperty(navigator.mozPower, "addWakeLockListener"); + checkProperty(navigator.mozPower, "removeWakeLockListener"); + checkWakeLock(); I'd also like a test of the lock levels: Acquire a foreground and background lock, it should say foreground. Then release the foreground lock, and it should drop to background. Then acquire another foreground lock, and it should switch again. And so on. All of these setTimeout(1000) calls have to go; it's just asking for randomorange. These asynchronous tests get kind of hairy, unfortunately. You can use generators (see runTest() in test_bug500328.html), or you can roll your own with something like: curTestIndex = 0; tests = [ function() { // this is part 1 }, function() { // this is part 2 } ... ]; function onEventReceived () { SimpleTest.executeSoon(function() { tests[curTestIndex]() }); curTestIndex++; } The SimpleTest.executeSoon (equivalent to a setTimeout(0)) is there to force the event loop to spin before running the next test step. You may not need it, but if you get rid of it, you'll have to do function onEventReceived () { curTestIndex++; tests[curTestIndex - 1](); } because, for example, the last line of part 1 might trigger the event which triggers part 2 synchronously. So we'll run test2 before test1 finishes. Thus you have to increment before you run the test. You can probably listen to the wake lock events (and onload on the popup window) to know when you can go on. Feel free to ask for help if you get stuck with this test. It's not easy to get right. Does this need to be checked in tomorrow, or are we no longer waiting on this for the demo?

Try run for b82684542fa7 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=b82684542fa7 Results (out of 12 total builds): success: 11 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kchen@mozilla.com-b82684542fa7

Update tests and fixed some counting bugs. Pushed to try.

Unfortunately interdiff bails on these two patches (presumably because they're not based off the same hg revision?), so I can't tell what changed. Would you mind trying to get an interdiff? What I'd try is make v2.1 and v3 apply to the same base revision of m-c. Then the interdiff command should be able to give meaningful output.

Comment on attachment 602836 [details] [diff] [review] Implement wake lock interface v3 Clearing the review flag until we can figure out the interdiff situation.

Try run for 5eb01c6a9f25 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=5eb01c6a9f25 Results (out of 228 total builds): exception: 2 success: 192 warnings: 31 failure: 3 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kchen@mozilla.com-5eb01c6a9f25 Timed out after 06 hours without completing.

Thanks a lot for the interdiff. >@@ -92,8 +99,13 @@ > { > nsAutoString state; > ComputeWakeLockState(aWakeLockInfo, state); >- >- nsTArray<nsCOMPtr<nsIDOMMozWakeLockListener> > listeners(mListeners); >+ >+ /** >+ * Copy the listeners list before we walk through the callbacks >+ * because the callbacks may install new listeners. We expect no >+ * more than one listener per window, so it shouldn't be too long. >+ */ >+ nsTArray<nsCOMPtr<nsIDOMMozWakeLockListener> > listeners(mWakeLockListeners); Nit: Use nsAutoTArray here. >diff -u b/hal/Hal.cpp b/hal/Hal.cpp >--- b/hal/Hal.cpp >+++ b/hal/Hal.cpp >@@ -184,10 +184,11 @@ > if (mObservers->Length() == 0) { > DisableNotifications(); > >+ // XXX CachingObserversManager invalidate it's cache here, but why? >+ OnNotificationsDisabled(); AIUI caching observers invalidate their cache when notifications are disabled because as soon as they stop listening e.g. to the battery state, their cached battery state is invalid. Does that answer your question here? We should get rid of the XXX before we check in. >- * Increase the wake lock count of aTopic >- * @param aTopic the resource name >- * @param aHidden the visibility of the wake lock >+ * Adjust the internal wake lock counts. >+ * @param aTopic lock topic >+ * @param aLockAdjust to increase or decrease active locks >+ * @param aHiddenAdjust to increase or decrease hidden locks > */ >-void AcquireWakeLock(const nsAString &aTopic, bool aHidden); >+void ModifyWakeLock(const nsAString &aTopic, int aLockAdjust, int aHiddenAdjust); We generally shun raw types (e.g. "int") in our code, since "int" can mean different things on different platforms. Since you're passing WakeLockControl objects, can you just use that type here and elsewhere? (You may need to define the WakeLockControl enum in the ipdl.) If someone needs to call ModifyWakeLock(aTopic, 2), we can revisit this. You can then add the enum value directly to count.numLocks and count.numHidden, bypassing the whole enum to int conversion altogether. >--- /dev/null >+++ b/dom/power/test/browser_bug697132.js >@@ -0,0 +1,197 @@ >+"use strict"; >+ >+waitForExplicitFinish(); >+ >+let pageSource1 = "data:text/html,1"; >+let pageSource2 = "data:text/html,2"; >+ >+let win, win1, win2; >+let tab, tab1, tab2; >+let lock, lock1, lock2; >+let gCurStepIndex = -1; >+let gSteps = [ >+ function basicWakeLock() { >+ tab = gBrowser.addTab(pageSource1); >+ win = gBrowser.getBrowserForTab(tab).contentWindow; >+ let browser = gBrowser.getBrowserForTab(tab); >+ >+ browser.addEventListener("load", function onLoad(e) { >+ browser.removeEventListener("load", onLoad, true); >+ let nav = win.navigator; >+ let power = nav.mozPower; >+ lock = nav.requestWakeLock("test"); >+ >+ ok(lock != null, >+ "navigator.requestWakeLock should return a wake lock"); >+ is(lock.topic, "test", >+ "wake lock should remember the locked topic"); >+ isnot(power.getWakeLockState("test"), "unlocked", >+ "topic is locked"); >+ >+ lock.unlock(); >+ >+ is(lock.topic, "test", >+ "wake lock should remember the locked topic even after unlock"); >+ is(power.getWakeLockState("test"), "unlocked", >+ "topic is unlocked"); >+ >+ try { >+ lock.unlock(); Add ok(false, "Should have thrown an error."); >+function test() { >+ // data url inherits its parent's principal, which is |about:| here. >+ Services.prefs.setCharPref("dom.power.whitelist", "about:"); >+ runNextStep(); >+} Is this necessary? We're chrome here, so the data URLs should have a chrome principal, I'd expect. In any case, please reset the pref to its original value when you're done. This test looks great, but are you confident that you don't need a test where you go back and forward in a background tab? Up to you. >+void >+ModifyWakeLock(const nsAString &aTopic, int aLockAdjust, int aHiddenAdjust) >+{ >+ if (!sInitialized) { >+ Init(); >+ } >+ >+ LockCount count; >+ count.numLocks = 0; >+ count.numHidden = 0; >+ sLockTable->Get(aTopic, &count); >+ MOZ_ASSERT(count.numLocks >= count.numHidden); >+ MOZ_ASSERT(aLockAdjust >= 0 || count.numLocks > 0); >+ MOZ_ASSERT(aHiddenAdjust >= 0 || count.numHidden > 0); >+ >+ WakeLockState oldState = ComputeWakeLockState(count.numLocks, count.numHidden); >+ >+ count.numLocks += aLockAdjust; >+ count.numHidden += aHiddenAdjust; >+ MOZ_ASSERT(count.numLocks >= count.numHidden); >+ >+ sLockTable->Put(aTopic, count); If count.numLocks goes to 0, should we remove the wake lock from the table? >--- /dev/null >+++ b/hal/HalWakeLock.h >@@ -0,0 +1,32 @@ >+/* -*- Mode: C++; tab-width: 40; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+#ifndef __HAL_WAKELOCK_H_ >+#define __HAL_WAKELOCK_H_ >+ >+namespace mozilla { >+namespace hal { >+ >+enum WakeLockState { >+ WAKE_LOCK_STATE_UNLOCKED, >+ WAKE_LOCK_STATE_HIDDEN, >+ WAKE_LOCK_STATE_VISIBLE >+}; >+ >+enum WakeLockControl { >+ WAKE_LOCK_REMOVE_ONE = -1, >+ WAKE_LOCK_NO_CHANGE = 0, >+ WAKE_LOCK_ADD_ONE = 1, >+}; >+ >+/** >+ * >+ */ >+WakeLockState ComputeWakeLockState(int aNumLocks, int aNumHidden); Finish documenting, and, why is this public? There's a function called ComputeWakeLockState which outputs a string, but that's not this one...

Comment on attachment 602836 [details] [diff] [review] Implement wake lock interface v3 lgtm!

(In reply to Justin Lebar [:jlebar] from comment #130) > Thanks a lot for the interdiff. > > >@@ -92,8 +99,13 @@ > > { > > nsAutoString state; > > ComputeWakeLockState(aWakeLockInfo, state); > >- > >- nsTArray<nsCOMPtr<nsIDOMMozWakeLockListener> > listeners(mListeners); > >+ > >+ /** > >+ * Copy the listeners list before we walk through the callbacks > >+ * because the callbacks may install new listeners. We expect no > >+ * more than one listener per window, so it shouldn't be too long. > >+ */ > >+ nsTArray<nsCOMPtr<nsIDOMMozWakeLockListener> > listeners(mWakeLockListeners); > > Nit: Use nsAutoTArray here. How much do you think the nsAutoTArray should be? I think nsAutoTArray<2> is enough. > >diff -u b/hal/Hal.cpp b/hal/Hal.cpp > >--- b/hal/Hal.cpp > >+++ b/hal/Hal.cpp > >@@ -184,10 +184,11 @@ > > if (mObservers->Length() == 0) { > > DisableNotifications(); > > > >+ // XXX CachingObserversManager invalidate it's cache here, but why? > >+ OnNotificationsDisabled(); > > AIUI caching observers invalidate their cache when notifications are > disabled because as soon as they stop listening e.g. to the battery state, > their cached battery state is invalid. I see. The cache is not updated anymore when notifications are disabled. But it only caches the first call to GetCurrentInformation. I suspect the subsequent calls will get old value here. > >+function test() { > >+ // data url inherits its parent's principal, which is |about:| here. > >+ Services.prefs.setCharPref("dom.power.whitelist", "about:"); > >+ runNextStep(); > >+} > > Is this necessary? We're chrome here, so the data URLs should have a chrome > principal, I'd expect. Yes, because I use the API from the tab content so it is necessary. The test script itself does not have associated document. > In any case, please reset the pref to its original value when you're done. > > This test looks great, but are you confident that you don't need a test > where you go back and forward in a background tab? Up to you. Hum.. not even considered. I will add one. > >+void > >+ModifyWakeLock(const nsAString &aTopic, int aLockAdjust, int aHiddenAdjust) > >+{ > >+ if (!sInitialized) { > >+ Init(); > >+ } > >+ > >+ LockCount count; > >+ count.numLocks = 0; > >+ count.numHidden = 0; > >+ sLockTable->Get(aTopic, &count); > >+ MOZ_ASSERT(count.numLocks >= count.numHidden); > >+ MOZ_ASSERT(aLockAdjust >= 0 || count.numLocks > 0); > >+ MOZ_ASSERT(aHiddenAdjust >= 0 || count.numHidden > 0); > >+ > >+ WakeLockState oldState = ComputeWakeLockState(count.numLocks, count.numHidden); > >+ > >+ count.numLocks += aLockAdjust; > >+ count.numHidden += aHiddenAdjust; > >+ MOZ_ASSERT(count.numLocks >= count.numHidden); > >+ > >+ sLockTable->Put(aTopic, count); > > If count.numLocks goes to 0, should we remove the wake lock from the table? Yes.. that make sense. > >--- /dev/null > >+++ b/hal/HalWakeLock.h > >@@ -0,0 +1,32 @@ > >+/* -*- Mode: C++; tab-width: 40; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > >+/* This Source Code Form is subject to the terms of the Mozilla Public > >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, > >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */ > >+ > >+#ifndef __HAL_WAKELOCK_H_ > >+#define __HAL_WAKELOCK_H_ > >+ > >+namespace mozilla { > >+namespace hal { > >+ > >+enum WakeLockState { > >+ WAKE_LOCK_STATE_UNLOCKED, > >+ WAKE_LOCK_STATE_HIDDEN, > >+ WAKE_LOCK_STATE_VISIBLE > >+}; > >+ > >+enum WakeLockControl { > >+ WAKE_LOCK_REMOVE_ONE = -1, > >+ WAKE_LOCK_NO_CHANGE = 0, > >+ WAKE_LOCK_ADD_ONE = 1, > >+}; > >+ > >+/** > >+ * > >+ */ > >+WakeLockState ComputeWakeLockState(int aNumLocks, int aNumHidden); > > Finish documenting, and, why is this public? There's a function called > ComputeWakeLockState which outputs a string, but that's not this one... One is for compute WakeLockState and one is for compute the output string. I think we might want the numbers in WakeLockInformation in the future so I didn't pass the WakeLockState directly to the listeners.

> How much do you think the nsAutoTArray should be? I think nsAutoTArray<2> is enough. Yes, that's fine. > I see. The cache is not updated anymore when notifications are disabled. > > But it only caches the first call to GetCurrentInformation. I suspect the subsequent calls will get > old value here. That's what the CacheInformation() call is for. I might be misunderstanding, but I think CacheInformation is called whenever the underlying value changes, so long as notifications are enabled. But maybe there is a bug, which is that if we call GetCurrentInformation and notifications are disabled, we'll set mHasValidCache to true, which doesn't seem right. If notifications are disabled, the cache is invalid the moment GetCurrentInformationInternal returns. If you think that's right, would you mind filing a bug and cc'ing mounir? > One is for compute WakeLockState and one is for compute the output string. I think we might want > the numbers in WakeLockInformation in the future so I didn't pass the WakeLockState directly to > the listeners. Oh, I see. The numeric overload is used in PowerManagerService. Cool, it just needs that comment filled in. :)

Fixed the addressed issues and add new test case for background tab. Also converted two static_cast to NS_ISUPPORTS_CAST macro.

Comment on attachment 603628 [details] [diff] [review] 602836: Implement wake lock interface v3.1 Review of attachment 603628 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Navigator.cpp @@ +974,4 @@ > } > > + nsCOMPtr<nsIDOMMozPowerManager> power = > + do_QueryInterface(NS_ISUPPORTS_CAST(nsIDOMMozPowerManager*, mPowerManager)); What kind of nonsense is this? nsCOMPtr<nsIDOMMozPowerManager> power = mPowerManager; will do. ::: dom/power/PowerManagerService.cpp @@ +163,5 @@ > + nsresult rv = wakelock->Init(aTopic, aWindow); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsCOMPtr<nsIDOMMozWakeLock> wl = > + do_QueryInterface(NS_ISUPPORTS_CAST(nsIDOMMozWakeLock*, wakelock)); As it would here.

https://hg.mozilla.org/mozilla-central/rev/72bc6f12d1cc Leaving open for unresolved comment 136 nits.

Comment on attachment 604291 [details] [diff] [review] Use nsCOMPtr copy constructor Use assignment, as in nsCOMPtr<nsIDOMMozPowerManager> power = mPowerManager; r=me with that

(In reply to Ms2ger from comment #139) > Comment on attachment 604291 [details] [diff] [review] > Use nsCOMPtr copy constructor > > Use assignment, as in > > nsCOMPtr<nsIDOMMozPowerManager> power = mPowerManager; > > r=me with that OK, I lied. It's not copy constructor but a normal constructor from a raw pointer. I cannot assign mPowerManager to a nsCOMPtr directly because it's a nsRefPtr. Do you want me to use nsCOMPtr<nsIDOMMozPowerManager> power = mPowerManager.get(); instead? It looks uglier then the normal constructor.

I cannot use assignment directly as the assignee is a RefPtr object not a raw pointer nor COMPtr. This version use do_QueryObject to wrap it.

Comment on attachment 604826 [details] [diff] [review] Use do_QueryObject No, that's as inefficient. Just use the constructor, then.

assinged to me for sec action to schedule a meeting

:jlebar any chance we can get a review of this scheduled and done?

Sure, I was confused by comment 146, which I took to mean that I didn't need to do anything here. But at this point, I am not convinced there's actually something to review at the moment. We have an API in this bug which doesn't do anything. Site can request wake locks, but they do nothing. I think it would be more appropriate to do a security review when we expose a way for the user to give permission to sites to use this API. I suspect that any security review we do before then will focus on this hypothetical API anyway.

SecReview is 2012.05.04: More info here https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html?view=month&action=view&invId=110490-110489&pstat=AC&exInvId=110490-171831&useInstance=1&instStartTime=1336150800000&instDuration=3600000

Release Notes on firefox 14.0.1 say there is a api for prevent the display from sleeping could someone please tell me what the api (i was not able to find a documented api) It would be nice to be able to stop a screensaver from coming on when playing video

(In reply to pqwoerituytrueiwoq from comment #150) > Release Notes on firefox 14.0.1 say there is a api for prevent the display > from sleeping could someone please tell me what the api (i was not able to > find a documented api) > It would be nice to be able to stop a screensaver from coming on when > playing video The release notes are wrong. The API exists, but it's not implemented in desktop Firefox.

Ok, thanks Guess I won't be making that Greasemonkey script...

(In reply to Justin Lebar (not reading bugmail) from comment #151) > The release notes are wrong. The API exists, but it's not implemented in > desktop Firefox. And would it be possible to link to the description of this API? Is it documented somewhere on MDN? I am interested in the implementation on FxOS/FxAndroid so lack of implementation on Desktop doesn’t bother me.

(In reply to Matěj Cepl from comment #153) > (In reply to Justin Lebar (not reading bugmail) from comment #151) > > The release notes are wrong. The API exists, but it's not implemented in > > desktop Firefox. > > And would it be possible to link to the description of this API? Is it > documented somewhere on MDN? I am interested in the implementation on > FxOS/FxAndroid so lack of implementation on Desktop doesn’t bother me. https://developer.mozilla.org/en-US/docs/Web/API/Wake_Lock_API