linux-traipu team mailing list archive
-
linux-traipu team
-
Mailing list archive
-
Message #05736
[Bug 604635] Re: Have default settings be energy star 5.0 compliant
Launchpad has imported 150 comments from the remote bug at
https://bugzilla.mozilla.org/show_bug.cgi?id=697132.
If you reply to an imported comment from within Launchpad, your comment
will be sent to the remote bug automatically. Read more about
Launchpad's inter-bugtracker facilities at
https://help.launchpad.net/InterBugTracking.
------------------------------------------------------------------------
On 2011-10-25T15:41:46+00:00 Justin-lebar+bug wrote:
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
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/28
------------------------------------------------------------------------
On 2011-10-25T16:40:25+00:00 Jonas-sicking wrote:
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 :)
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/29
------------------------------------------------------------------------
On 2011-10-25T16:42:03+00:00 Justin-lebar+bug wrote:
> 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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/30
------------------------------------------------------------------------
On 2011-10-26T00:01:14+00:00 Justin-lebar+bug wrote:
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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/31
------------------------------------------------------------------------
On 2011-10-26T00:32:46+00:00 Chris Jones wrote:
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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/32
------------------------------------------------------------------------
On 2011-10-26T00:40:18+00:00 Justin-lebar+bug wrote:
> I assume you want to do this in bug 681009?
Yes. :)
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/33
------------------------------------------------------------------------
On 2011-10-27T01:48:13+00:00 Jonas-sicking wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/34
------------------------------------------------------------------------
On 2011-10-27T01:52:20+00:00 Chris Jones wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/35
------------------------------------------------------------------------
On 2011-10-27T02:06:37+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/36
------------------------------------------------------------------------
On 2011-10-27T02:20:46+00:00 Chris Jones wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/37
------------------------------------------------------------------------
On 2011-10-27T02:36:02+00:00 Justin-lebar+bug wrote:
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.)
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/38
------------------------------------------------------------------------
On 2011-10-27T04:06:00+00:00 Jonas-sicking wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/39
------------------------------------------------------------------------
On 2011-10-27T04:11:11+00:00 Chris Jones wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/40
------------------------------------------------------------------------
On 2011-10-27T04:15:58+00:00 Chris Jones wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/41
------------------------------------------------------------------------
On 2011-10-27T04:29:49+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/42
------------------------------------------------------------------------
On 2011-10-27T05:58:40+00:00 Chris Jones wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/43
------------------------------------------------------------------------
On 2011-10-27T05:59:39+00:00 Chris Jones wrote:
Would this proposed API interact with full-screen at all? I haven't
followed that spec.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/44
------------------------------------------------------------------------
On 2011-10-27T06:17:27+00:00 Jonas-sicking wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/45
------------------------------------------------------------------------
On 2011-10-27T06:28:40+00:00 Chris Jones wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/46
------------------------------------------------------------------------
On 2011-10-27T06:30:49+00:00 Chris Jones wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/47
------------------------------------------------------------------------
On 2011-10-27T14:03:41+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/48
------------------------------------------------------------------------
On 2011-11-08T09:00:10+00:00 Tlee wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/49
------------------------------------------------------------------------
On 2011-12-14T10:19:47+00:00 Kchen-d wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/50
------------------------------------------------------------------------
On 2011-12-14T13:02:52+00:00 Justin-lebar+bug wrote:
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()?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/51
------------------------------------------------------------------------
On 2011-12-14T16:01:09+00:00 Kchen-d wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/52
------------------------------------------------------------------------
On 2011-12-14T16:10:36+00:00 Justin-lebar+bug wrote:
>> 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));
});
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/53
------------------------------------------------------------------------
On 2011-12-15T09:54:08+00:00 Kchen-d wrote:
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);
}
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/54
------------------------------------------------------------------------
On 2011-12-15T14:30:45+00:00 Justin-lebar+bug wrote:
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".
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/55
------------------------------------------------------------------------
On 2011-12-16T10:00:10+00:00 Kchen-d wrote:
Created attachment 582210
Proposed interfaces
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/56
------------------------------------------------------------------------
On 2011-12-23T00:44:21+00:00 Jonas-sicking wrote:
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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/57
------------------------------------------------------------------------
On 2011-12-23T02:32:27+00:00 Chris Jones wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/58
------------------------------------------------------------------------
On 2011-12-23T22:26:02+00:00 Justin-lebar+bug wrote:
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/59
------------------------------------------------------------------------
On 2011-12-23T22:47:34+00:00 Chris Jones wrote:
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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/60
------------------------------------------------------------------------
On 2011-12-23T22:58:12+00:00 Justin-lebar+bug wrote:
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/61
------------------------------------------------------------------------
On 2011-12-23T23:01:17+00:00 Chris Jones wrote:
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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/62
------------------------------------------------------------------------
On 2011-12-23T23:05:07+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/63
------------------------------------------------------------------------
On 2011-12-23T23:08:16+00:00 Chris Jones wrote:
What policy decisions do you envision "userspace" making, outside of
gecko?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/64
------------------------------------------------------------------------
On 2011-12-23T23:24:32+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/65
------------------------------------------------------------------------
On 2011-12-23T23:36:55+00:00 Chris Jones wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/66
------------------------------------------------------------------------
On 2011-12-24T05:20:09+00:00 Justin-lebar+bug wrote:
> 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.)
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/67
------------------------------------------------------------------------
On 2011-12-24T05:31:05+00:00 Jonas-sicking wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/68
------------------------------------------------------------------------
On 2011-12-24T06:02:58+00:00 Jonas-sicking wrote:
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");
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/69
------------------------------------------------------------------------
On 2011-12-25T16:28:29+00:00 Chris Jones wrote:
(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.)
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/70
------------------------------------------------------------------------
On 2011-12-25T19:31:07+00:00 Justin-lebar+bug wrote:
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/71
------------------------------------------------------------------------
On 2011-12-29T17:21:00+00:00 Chris Jones wrote:
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".
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/72
------------------------------------------------------------------------
On 2012-01-03T09:25:50+00:00 Kchen-d wrote:
Created attachment 585362
Part 1, Add wakelock interfaces
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/73
------------------------------------------------------------------------
On 2012-01-03T09:25:57+00:00 Kchen-d wrote:
Created attachment 585363
Part 2, Implement wakelock interfaces
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/74
------------------------------------------------------------------------
On 2012-01-03T09:26:02+00:00 Kchen-d wrote:
Created attachment 585364
Part 3, Add initial PowerManager code to b2g
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/75
------------------------------------------------------------------------
On 2012-01-03T14:36:15+00:00 Justin-lebar+bug wrote:
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/76
------------------------------------------------------------------------
On 2012-01-03T14:45:32+00:00 Justin-lebar+bug wrote:
> 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.)
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/77
------------------------------------------------------------------------
On 2012-01-03T22:10:29+00:00 Chris Jones wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/78
------------------------------------------------------------------------
On 2012-01-03T22:28:19+00:00 Justin-lebar+bug wrote:
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/79
------------------------------------------------------------------------
On 2012-01-03T22:32:57+00:00 Cpearce-t wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/80
------------------------------------------------------------------------
On 2012-01-03T22:35:45+00:00 Chris Jones wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/81
------------------------------------------------------------------------
On 2012-01-03T22:38:59+00:00 Chris Jones wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/82
------------------------------------------------------------------------
On 2012-01-03T22:44:32+00:00 Cpearce-t wrote:
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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/83
------------------------------------------------------------------------
On 2012-01-03T22:48:25+00:00 Chris Jones wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/84
------------------------------------------------------------------------
On 2012-01-03T22:52:11+00:00 Justin-lebar+bug wrote:
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/85
------------------------------------------------------------------------
On 2012-01-03T22:54:35+00:00 Justin-lebar+bug wrote:
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/86
------------------------------------------------------------------------
On 2012-01-03T23:05:19+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/87
------------------------------------------------------------------------
On 2012-01-03T23:23:23+00:00 Mounir wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/88
------------------------------------------------------------------------
On 2012-01-04T00:09:32+00:00 Chris Jones wrote:
(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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/89
------------------------------------------------------------------------
On 2012-01-04T00:14:50+00:00 Chris Jones wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/90
------------------------------------------------------------------------
On 2012-01-04T00:19:34+00:00 Justin-lebar+bug wrote:
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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/91
------------------------------------------------------------------------
On 2012-01-04T00:31:33+00:00 Chris Jones wrote:
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
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/92
------------------------------------------------------------------------
On 2012-01-04T00:33:46+00:00 Chris Jones wrote:
I'm in favor of notifying holders of wake locks when their particular
device is going to be forced into a powered-off state.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/93
------------------------------------------------------------------------
On 2012-01-04T00:35:15+00:00 Chris Jones wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/94
------------------------------------------------------------------------
On 2012-01-04T16:11:26+00:00 Justin-lebar+bug wrote:
> 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.)
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/95
------------------------------------------------------------------------
On 2012-01-04T22:23:16+00:00 Chris Jones wrote:
Worth checking prior art here from android.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/96
------------------------------------------------------------------------
On 2012-01-04T22:33:38+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/97
------------------------------------------------------------------------
On 2012-01-04T23:22:20+00:00 Chris Jones wrote:
Does a force-dropped lock automatically re-acquire when the device comes
on?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/98
------------------------------------------------------------------------
On 2012-01-04T23:33:30+00:00 Justin-lebar+bug wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/99
------------------------------------------------------------------------
On 2012-01-04T23:41:31+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/100
------------------------------------------------------------------------
On 2012-01-04T23:45:42+00:00 Chris Jones wrote:
I agree.
I'm tentatively in favor of don't-notify/auto-reacquire.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/101
------------------------------------------------------------------------
On 2012-01-07T01:00:20+00:00 Jonas-sicking wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/102
------------------------------------------------------------------------
On 2012-01-08T21:45:07+00:00 Cpearce-t wrote:
(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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/103
------------------------------------------------------------------------
On 2012-01-08T23:11:06+00:00 Justin-lebar+bug wrote:
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/104
------------------------------------------------------------------------
On 2012-01-09T07:00:36+00:00 Jonas-sicking wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/105
------------------------------------------------------------------------
On 2012-01-09T13:34:12+00:00 Justin-lebar+bug wrote:
> 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!
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/106
------------------------------------------------------------------------
On 2012-01-10T14:38:17+00:00 Mounir wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/107
------------------------------------------------------------------------
On 2012-01-10T14:45:34+00:00 Justin-lebar+bug wrote:
>> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/108
------------------------------------------------------------------------
On 2012-01-10T14:55:02+00:00 Mounir wrote:
(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".
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/109
------------------------------------------------------------------------
On 2012-01-10T15:26:50+00:00 21-vingtetun wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/110
------------------------------------------------------------------------
On 2012-02-13T11:19:52+00:00 Kchen-d wrote:
Created attachment 596637
Part 2, Implement wakelock interfaces
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/111
------------------------------------------------------------------------
On 2012-02-13T11:24:23+00:00 Kchen-d wrote:
Created attachment 596639
Part 1, Add wakelock interfaces
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/112
------------------------------------------------------------------------
On 2012-02-13T11:26:47+00:00 Kchen-d wrote:
Created attachment 596641
Part 3, Add test cases
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/113
------------------------------------------------------------------------
On 2012-02-13T21:32:39+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/114
------------------------------------------------------------------------
On 2012-02-13T22:36:27+00:00 Justin-lebar+bug wrote:
>+ /**
>+ * 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.)
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/115
------------------------------------------------------------------------
On 2012-02-13T22:37:25+00:00 Justin-lebar+bug wrote:
Comment on attachment 596639
Part 1, Add wakelock interfaces
Leaving the sr?sicking in place, because the DOM interfaces are not in
question.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/116
------------------------------------------------------------------------
On 2012-02-13T22:45:00+00:00 Justin-lebar+bug wrote:
Comment on attachment 596637
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);
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/117
------------------------------------------------------------------------
On 2012-02-13T22:48:38+00:00 Justin-lebar+bug wrote:
Comment on attachment 596641
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.)
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/118
------------------------------------------------------------------------
On 2012-02-13T22:58:46+00:00 Justin-lebar+bug wrote:
> 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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/119
------------------------------------------------------------------------
On 2012-02-14T05:30:14+00:00 Jonas-sicking wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/120
------------------------------------------------------------------------
On 2012-02-14T05:33:43+00:00 Justin-lebar+bug wrote:
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/121
------------------------------------------------------------------------
On 2012-02-14T09:28:06+00:00 Kchen-d wrote:
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/122
------------------------------------------------------------------------
On 2012-02-14T09:37:46+00:00 Kchen-d wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/123
------------------------------------------------------------------------
On 2012-02-14T14:44:05+00:00 Justin-lebar+bug wrote:
> 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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/124
------------------------------------------------------------------------
On 2012-02-15T01:03:27+00:00 Kchen-d wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/125
------------------------------------------------------------------------
On 2012-02-15T06:19:38+00:00 Justin-lebar+bug wrote:
> 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"?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/126
------------------------------------------------------------------------
On 2012-02-15T06:50:44+00:00 Kchen-d wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/127
------------------------------------------------------------------------
On 2012-02-15T06:54:41+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/128
------------------------------------------------------------------------
On 2012-02-15T07:05:39+00:00 Justin-lebar+bug wrote:
>> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/129
------------------------------------------------------------------------
On 2012-02-15T07:45:06+00:00 Jonas-sicking wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/130
------------------------------------------------------------------------
On 2012-02-15T16:22:40+00:00 Justin-lebar+bug wrote:
> 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]".
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/131
------------------------------------------------------------------------
On 2012-02-22T12:03:05+00:00 Kchen-d wrote:
Created attachment 599555
Implement wake lock interface
One big patch. Maybe I should find :cjones to review hal/ part?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/132
------------------------------------------------------------------------
On 2012-02-22T15:05:38+00:00 Justin-lebar+bug wrote:
(In reply to Kan-Ru Chen [:kanru] from comment #104)
> Created attachment 599555
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/133
------------------------------------------------------------------------
On 2012-02-22T16:16:23+00:00 Release-a wrote:
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@xxxxxxxxxxx-21a53ece95d2
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/134
------------------------------------------------------------------------
On 2012-02-22T16:25:03+00:00 Justin-lebar+bug wrote:
ClearOnShutdown should help with the leaks tryserver saw.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/135
------------------------------------------------------------------------
On 2012-02-22T17:28:20+00:00 Justin-lebar+bug wrote:
Comment on attachment 599555
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/136
------------------------------------------------------------------------
On 2012-02-22T20:51:33+00:00 Reuben-bmo wrote:
(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!
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/137
------------------------------------------------------------------------
On 2012-02-23T09:35:48+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/138
------------------------------------------------------------------------
On 2012-02-23T09:36:58+00:00 Justin-lebar+bug wrote:
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).
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/139
------------------------------------------------------------------------
On 2012-02-23T09:41:37+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/140
------------------------------------------------------------------------
On 2012-02-23T09:46:02+00:00 Kchen-d wrote:
(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 :-/
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/141
------------------------------------------------------------------------
On 2012-02-23T09:53:55+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/142
------------------------------------------------------------------------
On 2012-02-23T14:00:55+00:00 Kchen-d wrote:
Created attachment 599965
Implement wake lock interface v2
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..
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/143
------------------------------------------------------------------------
On 2012-02-23T16:02:40+00:00 Release-a wrote:
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@xxxxxxxxxxx-20e43fdd7711
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/144
------------------------------------------------------------------------
On 2012-02-23T17:19:18+00:00 Kchen-d wrote:
Created attachment 600050
Implement wake lock interface v2.1
Update test cases
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/145
------------------------------------------------------------------------
On 2012-02-23T17:34:49+00:00 Justin-lebar+bug wrote:
Mounir might want to look at the Hal.cpp changes (I think they're fine).
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/146
------------------------------------------------------------------------
On 2012-02-23T19:16:36+00:00 Release-a wrote:
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@xxxxxxxxxxx-14cc0af010a8
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/147
------------------------------------------------------------------------
On 2012-02-23T19:55:22+00:00 Justin-lebar+bug wrote:
Comment on attachment 600050
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/148
------------------------------------------------------------------------
On 2012-02-23T20:07:28+00:00 Justin-lebar+bug wrote:
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/149
------------------------------------------------------------------------
On 2012-02-23T20:24:10+00:00 Justin-lebar+bug wrote:
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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/150
------------------------------------------------------------------------
On 2012-02-23T20:48:18+00:00 Justin-lebar+bug wrote:
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?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/151
------------------------------------------------------------------------
On 2012-02-23T21:46:04+00:00 Release-a wrote:
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@xxxxxxxxxxx-b82684542fa7
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/152
------------------------------------------------------------------------
On 2012-03-05T10:40:23+00:00 Kchen-d wrote:
Created attachment 602836
Implement wake lock interface v3
Update tests and fixed some counting bugs. Pushed to try.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/153
------------------------------------------------------------------------
On 2012-03-05T15:52:58+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/154
------------------------------------------------------------------------
On 2012-03-05T15:54:00+00:00 Justin-lebar+bug wrote:
Comment on attachment 602836
Implement wake lock interface v3
Clearing the review flag until we can figure out the interdiff
situation.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/155
------------------------------------------------------------------------
On 2012-03-05T17:31:59+00:00 Release-a wrote:
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@xxxxxxxxxxx-5eb01c6a9f25
Timed out after 06 hours without completing.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/156
------------------------------------------------------------------------
On 2012-03-06T02:36:01+00:00 Kchen-d wrote:
Created attachment 603136
v2.1 and v3 interdiff
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/157
------------------------------------------------------------------------
On 2012-03-06T19:20:56+00:00 Justin-lebar+bug wrote:
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...
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/158
------------------------------------------------------------------------
On 2012-03-06T19:27:10+00:00 Justin-lebar+bug wrote:
Comment on attachment 602836
Implement wake lock interface v3
lgtm!
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/159
------------------------------------------------------------------------
On 2012-03-07T04:43:24+00:00 Kchen-d wrote:
(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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/160
------------------------------------------------------------------------
On 2012-03-07T05:31:10+00:00 Justin-lebar+bug wrote:
> 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. :)
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/161
------------------------------------------------------------------------
On 2012-03-07T07:50:23+00:00 Kchen-d wrote:
Created attachment 603628
602836: Implement wake lock interface v3.1
Fixed the addressed issues and add new test case for background tab.
Also converted two static_cast to NS_ISUPPORTS_CAST macro.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/162
------------------------------------------------------------------------
On 2012-03-07T11:08:57+00:00 Dao wrote:
http://hg.mozilla.org/integration/mozilla-inbound/rev/72bc6f12d1cc
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/163
------------------------------------------------------------------------
On 2012-03-08T17:56:29+00:00 Ms2ger wrote:
Comment on attachment 603628
602836: Implement wake lock interface v3.1
Review of attachment 603628:
-----------------------------------------------------------------
::: 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/164
------------------------------------------------------------------------
On 2012-03-08T22:10:04+00:00 Bmo-edmorley wrote:
https://hg.mozilla.org/mozilla-central/rev/72bc6f12d1cc
Leaving open for unresolved comment 136 nits.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/165
------------------------------------------------------------------------
On 2012-03-09T03:53:39+00:00 Kchen-d wrote:
Created attachment 604291
Use nsCOMPtr copy constructor
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/166
------------------------------------------------------------------------
On 2012-03-09T07:17:20+00:00 Ms2ger wrote:
Comment on attachment 604291
Use nsCOMPtr copy constructor
Use assignment, as in
nsCOMPtr<nsIDOMMozPowerManager> power = mPowerManager;
r=me with that
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/167
------------------------------------------------------------------------
On 2012-03-09T07:39:35+00:00 Kchen-d wrote:
(In reply to Ms2ger from comment #139)
> Comment on attachment 604291
> 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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/168
------------------------------------------------------------------------
On 2012-03-12T03:30:52+00:00 Kchen-d wrote:
Created attachment 604826
Use do_QueryObject
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/169
------------------------------------------------------------------------
On 2012-03-12T09:05:25+00:00 Ms2ger wrote:
Comment on attachment 604826
Use do_QueryObject
No, that's as inefficient. Just use the constructor, then.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/170
------------------------------------------------------------------------
On 2012-03-12T10:05:41+00:00 Kchen-d wrote:
Created attachment 604868
Use nsCOMPtr constructor.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/171
------------------------------------------------------------------------
On 2012-03-14T22:01:59+00:00 Ryanvm wrote:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25af1ffbabce
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/172
------------------------------------------------------------------------
On 2012-03-15T15:17:57+00:00 Mak77 wrote:
https://hg.mozilla.org/mozilla-central/rev/25af1ffbabce
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/173
------------------------------------------------------------------------
On 2012-03-28T21:24:53+00:00 Curtisk wrote:
assinged to me for sec action to schedule a meeting
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/174
------------------------------------------------------------------------
On 2012-04-11T18:52:17+00:00 Curtisk wrote:
:jlebar any chance we can get a review of this scheduled and done?
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/175
------------------------------------------------------------------------
On 2012-04-11T19:10:43+00:00 Justin-lebar+bug wrote:
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.
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/176
------------------------------------------------------------------------
On 2012-05-04T15:50:10+00:00 Curtisk wrote:
SecReview is 2012.05.04: More info here
https://mail.mozilla.com/home/ckoenig@xxxxxxxxxxx/Security%20Review.html?view=month&action=view&invId=110490-110489&pstat=AC&exInvId=110490-171831&useInstance=1&instStartTime=1336150800000&instDuration=3600000
Reply at: https://bugs.launchpad.net/chromium-
browser/+bug/604635/comments/177
** Changed in: firefox
Status: Unknown => Fix Released
** Changed in: firefox
Importance: Unknown => Medium
--
You received this bug notification because you are a member of UBUNTU -
AL - BR, which is subscribed to Chromium Browser.
https://bugs.launchpad.net/bugs/604635
Title:
Have default settings be energy star 5.0 compliant
Status in Chromium Browser:
Unknown
Status in The Mozilla Firefox Browser:
Fix Released
Status in Gnome Powermanager:
Won't Fix
Status in OEM Priority Project:
Fix Released
Status in “chromium-browser” package in Ubuntu:
Triaged
Status in “firefox” package in Ubuntu:
Triaged
Status in “gnome-power-manager” package in Ubuntu:
Fix Released
Status in “rhythmbox” package in Ubuntu:
Fix Released
Bug description:
Binary package hint: gnome-power-manager
Energy star is a standard backed by the US government to set standards for energy use of devices.
The specification for Operating System settings can be found here:
http://www.energystar.gov/index.cfm?c=revisions.computer_spec
Spec direct link (page 13):
http://www.energystar.gov/ia/partners/prod_development/revisions/downloads/computer/Version5.0_Computer_Spec.pdf
Also to better help understand Energy Star. The Open Suse team actually heavily looked into this, and have a great presentation here:
http://files.opensuse.org/opensuse/en/3/34/EnergyStar.pdf
The reason you want to be Energy star compliant:
- Many retailers will not sell machines that are not Energy Star compliant
- Most Government agencies will not buy hardware solutions that are not Energy Star compliant
- OEMs are now pushing heavily to have all hardware be Energy Star compliant
- The OS software default settings are apart of the Energy Star compliance of machines
There are only two default settings preventing Ubuntu from being energy star compliant:
* Put display to Sleep within 15 minutes of user inactivity when on A/C
- Currently this is set to 30 min. So setting it to 15 or below would fix this.
* Activate computer's Sleep mode within 30 minutes of user inactivity when on A/C.
- Currently this is set to never
- It is also under stood that this could potentially have user experience impact
If both settings cannot be done by deafult. It has been requested to
have an Energy Star button in Gnome Power Management interface that
would set settings to be energy star compliant.
To manage notifications about this bug go to:
https://bugs.launchpad.net/chromium-browser/+bug/604635/+subscriptions
