Have default settings be energy star 5.0 compliant

[Jerone Young]

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.

[Brent Fox (brent-s-fox)]

So, if I could restate the request a bit:

Goal: To have Ubuntu be Energy Star Compliant.

Why?: Some customer tenders are beginning to list Energy Star Compliance as a requirement, therefore the OEMs must have an Energy Star compliant OS in order to respond to these tenders.

Implementation Options:
1) Make Ubuntu's default settings comply with Energy Star requirements:
* Put display to Sleep within 15 minutes of user inactivity when on A/C
        - Currently this is set to 30 min.
* Activate computer's Sleep mode within 30 minutes of user inactivity when on A/C.
        - Currently this is set to "never"

Cons: These settings may not be a desirable default for the majority of users

2) Add a new option to Gnome Power Management to toggle both settings at once with a checkbox: "Enable Energy Star Power Mode" or somesuch. I'm guessing the best place for that toggle would be under the "General" tab instead of "On AC Power" or "On DC Power".

Cons: Requires UI changes that would likely need to be pushed upsteam. Also introduces new strings which would need to be translated.

[Chris Coulson (chrisccoulson)]

The second option is basically adding support for profiles in g-p-m, which I think has already been rejected once before upstream (although I'm not completely certain about this).

The first option would be really bad for machines such as my desktop, which are quite happy to suspend - but they always fail to resume. People with machines like this would be pretty annoyed to do a fresh install and then lose a bunch of work because the machine automatically suspended over their lunch break or something

Couldn't you just ship a custom set of defaults for OEM machines?

[Martin Pitt (pitti)]

As a close approximation of this, could we always have the DPMS mode after 15 minutes (instead of 30), and suspend after 30 only if we are on battery? Or only if we are a laptop, i. e. have a lid? The latter would be closer to your request, but require a small code patch (but that shouldn't stop us, upstream is very open to such kind of things)

[Mario Limonciello (superm1)]

Checking that we are a laptop for the suspend after 30 would cover a
majority of OEM enabled machines, so I think would definitely be a step in
the right direction. For desktops that need the requirement then, it would
only be a matter of setting a single gconf key.

On Wed, Jul 14, 2010 at 00:04, Martin Pitt <email address hidden> wrote:

> As a close approximation of this, could we always have the DPMS mode
> after 15 minutes (instead of 30), and suspend after 30 only if we are on
> battery? Or only if we are a laptop, i. e. have a lid? The latter would
> be closer to your request, but require a small code patch (but that
> shouldn't stop us, upstream is very open to such kind of things)
>
> --
> [Maverick] Have default settings be energy star 5.0 compliant
> https://bugs.launchpad.net/bugs/604635
> You received this bug notification because you are a member of The Dell
> Team, which is a direct subscriber.
>

--
Mario Limonciello
<email address hidden>

[Martin Pitt (pitti)]

Thanks Mario. Taking this then.

[Martin Pitt (pitti)]

I pushed this change to the Ubuntu packaging bzr for now. I proposed it upstream, and will discuss with Richard about possible refinements.

[Launchpad Janitor (janitor)]

This bug was fixed in the package gnome-power-manager - 2.30.1-1ubuntu2

---------------
gnome-power-manager (2.30.1-1ubuntu2) maverick; urgency=low

  * Add 00git-port-to-libupower.patch: Port from libdevkit-power to
    libupower-glib, the former will go away soon.
  * debian/control.in: Replace libdevkit-power-gobject-dev with
    libupower-glib-dev build dependency.
  * 12-add-appindicators.patch: Update to upower port.
  * Add 13-energy-star.patch: Change default timings to be Energy Star 5.0
    compliant, except for automatic suspend on AC (since that might be both
    inconvenient, and suspend might not work at all). (LP: #604635)
 -- Martin Pitt <email address hidden> Wed, 14 Jul 2010 16:19:03 +0200

[Martin Pitt (pitti)]

Jerone, that's about as far as we can push it from the distro side. At least the code patch is in, so a possible customization from OEM would merely involve to add one extra line to debian/gconf-defaults on tested hardware. Is that acceptable?

[Jerone Young (jerone)]

@Martin Pitt
               Thanks Martin! I would agree. That does lesson the work needed. As I mentioned Activating sleep after 30 minutes is potentially has potential impact on machines that do not suspend well, as well as potential user experience issues.

                With what Mario mentioned I would say that if a Desktop is having issues with suspend, the preferred method would be to have the defaults be energy star compliant (suspend after 30min inactivity), and we would then do something to basically change the suspend setting so for that machine we wouldn't suspend. Essentially breaking the energy star compliance.

                Though we can wait to see what the response upstream is toward energy start compliance. Would like to follow more given upstream & even perhaps UX team & community input.

[Jerone Young (jerone)]

To add some more comments.

Thinking about the 30 minute suspsend when idle more. Off the top of my head I can think of some use cases that can be impacted by this:

* A long download via firefox or chromium
* watching a movie or listening to music

Though a solution seems to this already exist. I know that totem has the ability to tell gnome-power-manager that the it is busy so don't do anything automatically. This would also be something that firefox or chromium could have as well when they are downloading content.

I was also told that Transimmision Bittorrent Client has the same logic to prevent gnome-power-manager from automatically like suspend when it is running.

Not sure if they are reporting the system not being idle or something else. But I believe it's all done over dbus.

[Chris Coulson (chrisccoulson)]

transmission has the option to inhibit suspend when downloading, but i think that preference is off by default.

Totem creates an idle inhibitor when watching movies. Rhythmbox can do this too when listening to music, but i think that plugin is disabled by default

[Mario Limonciello (superm1)]

Chris:

Would it be worthwhile then maybe to set up some tasks for those projects to make sure that inhibitors get setup by default, so as to see this change that was made for gnome-power-manager doesn't cause a negative experience on those applications?

[Jerone Young (jerone)]

@Martin
              I would like to mark the gnome-power-management task in progress for the 30 min suspend. Along with this would be to open bugs or tasks on this bug for projects that would get effected by this (as Mario suggests in comment #11). This may not be something for 10.10 .. but 11.04 seems to be where everything can come together. What do you think ?

[Martin Pitt (pitti)]

Chris, with our recent move to almost-pure-upstream builds I figure adding a screensaver/session inhibitor is something that we should get upstream to do. Can you please forward those two (firefox/chromium) to upstream bugs and discuss it there? I guess it's just adding a D-Bus call on starting the first download and ending the last?

[Martin Pitt (pitti)]

I can look into enabling Ryhthmbox's power management plugin by default and test it.

[Martin Pitt (pitti)]

I tested this on current lucid with totem and Rhythmbox. It works as it should with totem, i. e. when playing a movie, the idle and suspend timeouts are inhibited, but closing the lid always works.

The Rhythmbox plugin works fine as well. One potentially debatable issue is that inhibits the idle timeout, instead of the suspend one, so while playing music the screensaver and DPMS standby never get active. I suppose this is meant for "party mode" where you want to see the playlist, or the goom visualizer, etc. I propose to leave it like it is for now.

[Launchpad Janitor (janitor)]

This bug was fixed in the package rhythmbox - 0.13.0git20100715-0ubuntu3

---------------
rhythmbox (0.13.0git20100715-0ubuntu3) maverick; urgency=low

  * debian/rhythmbox.gconf-defaults: Enable power-manager plugin by default.
    Drop the power-manager/hidden override, since it's the upstream default
    anyway. (LP: #604635)
 -- Martin Pitt <email address hidden> Mon, 19 Jul 2010 18:08:58 +0200

[Evan Martin (Chromium) (evan-chromium)]

Most movies in browsers is via Flash, which browsers don't get a lot of insight into. I think we can tell that Flash is iterating its main loop but not whether it's painting or just running a timer; full-screened Flash creates its own window out of our control. I think this means we can't control power management for this primary use case. (HTML5 video could definitely be fixed, and I'd be happy to review a patch to do it, but realistically HTML5 video isn't very common yet.)

A coworker proposed a hack that, after reflection, I think is actually pretty good: the compositing manager has ultimate knowledge of what's getting dirtied on your screen. A heuristic like "if something in a region at least X by Y is repainting regularly don't sleep" would probably get you pretty far without needing to change every app.

[Jerone Young (jerone)]

Thinking about his more after Martin's comment on rhythmbox. I think another level of idleness has to be created ?

If you have something downloading in Chromium or Firefox .. you do want the screen lock or screen blank to happen. But what you do not want is for the machine to go to sleep or hibernate automatically.

Almost like something that says that I'm idle .. but don't go to sleep.

@Martin
              You have thoughts on that?

[Phill Whiteside (phillw)]

Hi, I do recall from my dark murky past with Windows, that there is an
energy-star system for monitoring data

http://www.energystar.gov/index.cfm?c=power_mgt.pr_power_mgt_wol

<http://www.energystar.gov/index.cfm?c=power_mgt.pr_power_mgt_wol>I'm not
sure if it is of any help, but provide it for you.

Regards,

Phill.

On Mon, Jul 19, 2010 at 6:04 PM, Jerone Young <email address hidden>wrote:

> Thinking about his more after Martin's comment on rhythmbox. I think
> another level of idleness has to be created ?
>
> If you have something downloading in Chromium or Firefox .. you do want
> the screen lock or screen blank to happen. But what you do not want is
> for the machine to go to sleep or hibernate automatically.
>
> Almost like something that says that I'm idle .. but don't go to sleep.
>
> @Martin
> You have thoughts on that?
>
> --
> [Maverick] Have default settings be energy star 5.0 compliant
> https://bugs.launchpad.net/bugs/604635
> You received this bug notification because you are a member of Lubuntu
> Packages Team, which is subscribed to chromium-browser in ubuntu.
>
> Status in Gnome Powermanager: New
> Status in OEM Priority Project: In Progress
> 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.
>
>
>
>
>

[Chris Coulson (chrisccoulson)]

On Mon, 2010-07-19 at 17:04 +0000, Jerone Young wrote:
> Thinking about his more after Martin's comment on rhythmbox. I think
> another level of idleness has to be created ?
>
> If you have something downloading in Chromium or Firefox .. you do want
> the screen lock or screen blank to happen. But what you do not want is
> for the machine to go to sleep or hibernate automatically.
>
> Almost like something that says that I'm idle .. but don't go to sleep.
>
> @Martin
> You have thoughts on that?
>

This already exists, and is what transmission uses

Rhythmbox could be more intelligent. It could create a suspend inhibit
when listening to music with no visuals, and an idle inhibit if you have
visuals on.

I'll look at implementing this functionality for downloads in Firefox as
an extension. I think I can do it with nsIDownloadManager and
nsIDownloadProgressListener

[Mario Limonciello (superm1)]

Martin:
It was raised to me that the GPM patch isn't actually checking for this being a laptop (and filling AC settings accordingly). You said previously that you should be able to check if it's a laptop (eg has a lid). Is that going to be in an upcoming patch?

[Jerone Young (jerone)]

@Mario
       I believe at this point we are looking at energy star settings for all machines. Regardless of if they are laptops or desktops.

       The main issue at this point is what work needs to happen to allow for suspend after 30min idle to be possible, and also allow a good user experience. This is where the application scenerios come in.

        So far we seem to be on the right track. Though this all may not make it for 10.10.

[Martin Pitt (pitti)]

Hello Mario,

Mario Limonciello [2010-07-20 4:57 -0000]:
> It was raised to me that the GPM patch isn't actually checking for
> this being a laptop (and filling AC settings accordingly). You said
> previously that you should be able to check if it's a laptop (eg has
> a lid). Is that going to be in an upcoming patch?

That'd require some more intrusive code changes, and also be a bit
contradictory to the current configuration structure. Before I do
those I'd like to get an answer from Richard what he thinks about the
change in general.

--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)

[Chris Coulson (chrisccoulson)]

I wrote an extension for Firefox at the sprint last week, which inhibits suspend when downloading files. It seems to work pretty well now, and you can download it from http://edge.launchpad.net/moz-gnome-pm/trunk/0.1/+download/moz-gnome-pm-0.1.xpi

[Jonathan Matthew (jmatthew)]

The Rhythmbox plugin works like it currently does because the two relevant inhibit flags that gnome-session provides are 'suspend' and 'idle'. 'Suspend' (as far as I know) inhibits all suspend operations, including user-initiated ones such as closing the laptop lid. WIth the older version of the plugin, using the gnome-power-manager dbus interface, we had reports of users almost frying their laptops because they assumed closing the lid would cause it to suspend, but the Rhythmbox plugin inhibited it, so placing it in an enclosed space caused it to overheat.

'Idle' only inhibits events that occur due to the machine being idle, which unfortunately for us includes screensaver and DPMS standby as well as suspending the machine. I figured this was the safer choice.

If there was an 'idle suspend' inhibit flag I'd certainly switch to it, but I don't seem to have done anything about asking for one yet..

[Jonathan Matthew (jmatthew)]

Maybe I should have tested that before I commented. The suspend flag does what we want now.

[Justin-lebar+bug (justin-lebar+bug)]

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

[Jonas-sicking (jonas-sicking)]

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 :)

[Justin-lebar+bug (justin-lebar+bug)]

> 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?

[Justin-lebar+bug (justin-lebar+bug)]

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?

[Chris Jones (jones-chris-g)]

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?

[Justin-lebar+bug (justin-lebar+bug)]

> I assume you want to do this in bug 681009?

Yes. :)

[Jonas-sicking (jonas-sicking)]

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.

[Chris Jones (jones-chris-g)]

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.

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Chris Jones (jones-chris-g)]

(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.

[Justin-lebar+bug (justin-lebar+bug)]

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.)

[Jonas-sicking (jonas-sicking)]

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.

[Chris Jones (jones-chris-g)]

(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.

[Chris Jones (jones-chris-g)]

(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.

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Chris Jones (jones-chris-g)]

(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 i...

[Chris Jones (jones-chris-g)]

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

[Jonas-sicking (jonas-sicking)]

(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.

[Chris Jones (jones-chris-g)]

(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.

[Chris Jones (jones-chris-g)]

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

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Tlee (tlee)]

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.

[Kchen-d (kchen-d)]

(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.

[Justin-lebar+bug (justin-lebar+bug)]

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()?

[Kchen-d (kchen-d)]

(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.

[Justin-lebar+bug (justin-lebar+bug)]

>> 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));
  });

[Kchen-d (kchen-d)]

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);
}

[Justin-lebar+bug (justin-lebar+bug)]

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".

[Kchen-d (kchen-d)]

Created attachment 582210
Proposed interfaces

[Jonas-sicking (jonas-sicking)]

First off, why put the requestWakeLock and wakeLockIsHeld functions on separate objects? Putting them both on nsIDOMMozPowerManager makes more sense to me. Also, I think we should do something event-based instead. Two options are:

Option 1:
interface nsIDOMMozPowerManager : EventTarget
{
  // ...
  boolean wakeLockIsHeld(in DOMString aTopic);
  void requestWakeLock(in DOMString aTopic);
  void releaseWakeLock(in DOMString aTopic);

  attribute Function onwakelockaquired;
  attribute Function onwakelockreleased;
};

And then we'd fire events with the following API:

interface WakeLockEvent : Event {
  readonly attribute DOMString topic;
};

We'd fire events with type "wakelockaquired" and "wakelockreleased" targetted at the powermanager.

So the code would look something like this:

navigator.mozPower.onwakelockreleased = function(event) {
  if (event.topic == "mytopic") { ... };
}
navigator.requestWakeLock("mytopic");

Option 2:
interface nsIDOMMozPowerManager : EventTarget
{
  // ...
  boolean wakeLockIsHeld(in DOMString aTopic);
  nsIDOMWakeLockRequest requestWakeLock(in DOMString aTopic);
  void releaseWakeLock(in DOMString aTopic);
};

interface nsIDOMWakeLockRequest : EventTarget
{
  readonly attribute DOMString topic;
  attribute Function onaquired;
  attribute Function onreleased;
};

which would give the code:

navigator.mozPower.requestWakeLock("mytopic").onaquired = function(event) {
  ...
}

Though I'm not quite understanding the purpose of the topic? Is it just for the website to be able to manage having multiple reasons to keep the screen awake? Or will we actually do something useful with the string-value that they pass in?

[Chris Jones (jones-chris-g)]

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.

[Justin-lebar+bug (justin-lebar+bug)]

> 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.

[Chris Jones (jones-chris-g)]

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?

[Justin-lebar+bug (justin-lebar+bug)]

> 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.

[Chris Jones (jones-chris-g)]

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?

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Chris Jones (jones-chris-g)]

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

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Chris Jones (jones-chris-g)]

(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.

[Justin-lebar+bug (justin-lebar+bug)]

> 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.)

[Jonas-sicking (jonas-sicking)]

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.

[Jonas-sicking (jonas-sicking)]

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");

[Chris Jones (jones-chris-g)]

(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.)

[Justin-lebar+bug (justin-lebar+bug)]

> 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 genera...

[Chris Jones (jones-chris-g)]

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".

[Kchen-d (kchen-d)]

Created attachment 585362
Part 1, Add wakelock interfaces

[Kchen-d (kchen-d)]

Created attachment 585363
Part 2, Implement wakelock interfaces

[Kchen-d (kchen-d)]

Created attachment 585364
Part 3, Add initial PowerManager code to b2g

[Justin-lebar+bug (justin-lebar+bug)]

> 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 ...

[Justin-lebar+bug (justin-lebar+bug)]

> 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.)

[Chris Jones (jones-chris-g)]

(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.

[Justin-lebar+bug (justin-lebar+bug)]

> 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.

[Cpearce-t (cpearce-t)]

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.

[Chris Jones (jones-chris-g)]

(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.

[Chris Jones (jones-chris-g)]

(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.

[Cpearce-t (cpearce-t)]

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?

[Chris Jones (jones-chris-g)]

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.

[Justin-lebar+bug (justin-lebar+bug)]

> 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.

[Justin-lebar+bug (justin-lebar+bug)]

> 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.

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Mounir (mounir)]

(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.

[Chris Jones (jones-chris-g)]

(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?

[Chris Jones (jones-chris-g)]

(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.

[Justin-lebar+bug (justin-lebar+bug)]

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?

[Chris Jones (jones-chris-g)]

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

[Chris Jones (jones-chris-g)]

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

[Chris Jones (jones-chris-g)]

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.

[Justin-lebar+bug (justin-lebar+bug)]

> 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.)

[Chris Jones (jones-chris-g)]

Worth checking prior art here from android.

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Chris Jones (jones-chris-g)]

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

[Justin-lebar+bug (justin-lebar+bug)]

(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.

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Chris Jones (jones-chris-g)]

I agree.

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

[Jonas-sicking (jonas-sicking)]

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 s...

[Cpearce-t (cpearce-t)]

(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?

[Justin-lebar+bug (justin-lebar+bug)]

> 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.

[Jonas-sicking (jonas-sicking)]

Justin: Is a music-sync app different from other types of syncing, such as mail-sync or contact sync?

It seems to me that syncing is better done by allowing apps to temporarily enable the network, do the syncing and then disable it, rather than keeping the network awake for longer periods of time. Or did you mean that that's what they should do, but if the user puts the device to sleep during such a sync we should let the app finish the current sync before turning off the network connection?

In any case it sounds like in all situations when keeping the network "locked" the page would also want to keep the CPU going. It also sounds like we don't need to work any extra hard to avoid switching between wifi/3g/2g just because a network lock is held. I think we as a general principle should try to avoid switching between wifi/(3g+2g) whenever there is active network communication, but probably only for a limited time. In any case that seems like an orthogonal feature that's off-topic for this bug.

[Justin-lebar+bug (justin-lebar+bug)]

> Justin: Is a music-sync app different from other types of syncing, such as mail-sync or contact sync?

Only inasmuch as it may use a lot more data. One might feasibly sync e-mail only when the phone is awake, since presumably it'll be fast. But music sync needs to go on even when the phone is "asleep".

> In any case it sounds like in all situations when keeping the network "locked" the page would also
> want to keep the CPU going.

Agreed!

[Mounir (mounir)]

(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.

[Justin-lebar+bug (justin-lebar+bug)]

>> 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.

[Mounir (mounir)]

(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".

[21-vingtetun (21-vingtetun)]

(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.

[Kchen-d (kchen-d)]

Created attachment 596637
Part 2, Implement wakelock interfaces

[Kchen-d (kchen-d)]

Created attachment 596639
Part 1, Add wakelock interfaces

[Kchen-d (kchen-d)]

Created attachment 596641
Part 3, Add test cases

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Justin-lebar+bug (justin-lebar+bug)]

>+ /**
>+ * 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 :...

[Justin-lebar+bug (justin-lebar+bug)]

Comment on attachment 596639
Part 1, Add wakelock interfaces

Leaving the sr?sicking in place, because the DOM interfaces are not in question.

[Justin-lebar+bug (justin-lebar+bug)]

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);

[Justin-lebar+bug (justin-lebar+bug)]

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.)

[Justin-lebar+bug (justin-lebar+bug)]

> 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?

[Jonas-sicking (jonas-sicking)]

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.

[Justin-lebar+bug (justin-lebar+bug)]

> 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.

[Kchen-d (kchen-d)]

> 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.

[Kchen-d (kchen-d)]

(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.

[Justin-lebar+bug (justin-lebar+bug)]

> 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?

[Kchen-d (kchen-d)]

(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.

[Justin-lebar+bug (justin-lebar+bug)]

> 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"?

[Kchen-d (kchen-d)]

(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.

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Justin-lebar+bug (justin-lebar+bug)]

>> 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.

[Jonas-sicking (jonas-sicking)]

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.

[Justin-lebar+bug (justin-lebar+bug)]

> 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]".

[Kchen-d (kchen-d)]

Created attachment 599555
Implement wake lock interface

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

[Justin-lebar+bug (justin-lebar+bug)]

(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.

[Release-a (release-a)]

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://<email address hidden>

[Justin-lebar+bug (justin-lebar+bug)]

ClearOnShutdown should help with the leaks tryserver saw.

[Justin-lebar+bug (justin-lebar+bug)]

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/...

[Reuben-bmo (reuben-bmo)]

(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!

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Justin-lebar+bug (justin-lebar+bug)]

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).

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Kchen-d (kchen-d)]

(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 :-/

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Kchen-d (kchen-d)]

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..

[Release-a (release-a)]

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://<email address hidden>

[Kchen-d (kchen-d)]

Created attachment 600050
Implement wake lock interface v2.1

Update test cases

[Justin-lebar+bug (justin-lebar+bug)]

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

[Release-a (release-a)]

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://<email address hidden>

[Justin-lebar+bug (justin-lebar+bug)]

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:
>+
>+ ~PowerM...

[Justin-lebar+bug (justin-lebar+bug)]

> 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.

[Justin-lebar+bug (justin-lebar+bug)]

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?

[Justin-lebar+bug (justin-lebar+bug)]

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(fun...

[Release-a (release-a)]

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://<email address hidden>

[Kchen-d (kchen-d)]

Created attachment 602836
Implement wake lock interface v3

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

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Justin-lebar+bug (justin-lebar+bug)]

Comment on attachment 602836
Implement wake lock interface v3

Clearing the review flag until we can figure out the interdiff situation.

[Release-a (release-a)]

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://<email address hidden>
 Timed out after 06 hours without completing.

[Kchen-d (kchen-d)]

Created attachment 603136
v2.1 and v3 interdiff

[Justin-lebar+bug (justin-lebar+bug)]

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 l...

[Justin-lebar+bug (justin-lebar+bug)]

Comment on attachment 602836
Implement wake lock interface v3

lgtm!

[Kchen-d (kchen-d)]

(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...

[Justin-lebar+bug (justin-lebar+bug)]

> 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. :)

[Kchen-d (kchen-d)]

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.

[Dao (dao)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/72bc6f12d1cc

[Ms2ger (ms2ger)]

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.

[Bmo-edmorley (bmo-edmorley)]

https://hg.mozilla.org/mozilla-central/rev/72bc6f12d1cc

Leaving open for unresolved comment 136 nits.

[Kchen-d (kchen-d)]

Created attachment 604291
Use nsCOMPtr copy constructor

[Ms2ger (ms2ger)]

Comment on attachment 604291
Use nsCOMPtr copy constructor

Use assignment, as in

nsCOMPtr<nsIDOMMozPowerManager> power = mPowerManager;

r=me with that

[Kchen-d (kchen-d)]

(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.

[Kchen-d (kchen-d)]

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.

[Ms2ger (ms2ger)]

Comment on attachment 604826
Use do_QueryObject

No, that's as inefficient. Just use the constructor, then.

[Kchen-d (kchen-d)]

Created attachment 604868
Use nsCOMPtr constructor.

[Ryanvm (ryanvm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25af1ffbabce

[Mak77 (mak77)]

https://hg.mozilla.org/mozilla-central/rev/25af1ffbabce

[Curtisk (curtisk)]

assinged to me for sec action to schedule a meeting

[Curtisk (curtisk)]

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

[Justin-lebar+bug (justin-lebar+bug)]

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.

[Curtisk (curtisk)]

SecReview is 2012.05.04: More info here
https://<email address hidden>/Security%20Review.html?view=month&action=view&invId=110490-110489&pstat=AC&exInvId=110490-171831&useInstance=1&instStartTime=1336150800000&instDuration=3600000

[shawnlandden (shawnlandden)]

Firefox had added power-inhibit logic so this can be more sensible, while still supporting watching movies without pissing off users.

https://bugzilla.mozilla.org/show_bug.cgi?id=697132
https://bugzilla.mozilla.org/show_bug.cgi?id=517870

Here is a chromium bug on this:

https://code.google.com/p/chromium/issues/detail?id=111043

[pqwoerituytrueiwoq (pqwoerituytrueiwoq)]

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

[Justin-lebar+bug (justin-lebar+bug)]

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

The release notes are wrong. The API exists, but it's not implemented in desktop Firefox.

[pqwoerituytrueiwoq (pqwoerituytrueiwoq)]

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

[Matěj Cepl (mcepl)]

(In reply to Justin Lebar (not reading bugmail) from comment #151)
> The release notes are wrong. The API exists, but it's not implemented in
> desktop Firefox.

And would it be possible to link to the description of this API? Is it documented somewhere on MDN? I am interested in the implementation on FxOS/FxAndroid so lack of implementation on Desktop doesn’t bother me.

[Kanru-d (kanru-d)]

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

https://developer.mozilla.org/en-US/docs/Web/API/Wake_Lock_API