Have default settings be energy star 5.0 compliant

Bug #604635 reported by Jerone Young on 2010-07-12
52
This bug affects 4 people
Affects Status Importance Assigned to Milestone
Chromium Browser
Unknown
Unknown
Mozilla Firefox
Fix Released
Medium
OEM Priority Project
Medium
Unassigned
gnome-power
Won't Fix
Low
chromium-browser (Ubuntu)
Low
Unassigned
firefox (Ubuntu)
Low
Chris Coulson
gnome-power-manager (Ubuntu)
Low
Martin Pitt
rhythmbox (Ubuntu)
Low
Martin Pitt

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.

Jerone Young (jerone) on 2010-07-12
Changed in oem-priority:
importance: Undecided → Medium
Jerone Young (jerone) on 2010-07-12
description: updated
Jerone Young (jerone) on 2010-07-12
Changed in oem-priority:
status: New → Confirmed
Brent Fox (brent-s-fox) wrote :

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

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

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>

Thanks Mario. Taking this then.

Changed in gnome-power-manager (Ubuntu):
assignee: nobody → Martin Pitt (pitti)
importance: Undecided → Low
status: New → In Progress
Changed in gnome-power:
status: Unknown → New
Martin Pitt (pitti) wrote :

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

Changed in gnome-power-manager (Ubuntu):
status: In Progress → Fix Committed
Launchpad Janitor (janitor) wrote :

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

Changed in gnome-power-manager (Ubuntu):
status: Fix Committed → Fix Released
Martin Pitt (pitti) wrote :

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?

Changed in oem-priority:
status: Confirmed → Incomplete
Jerone Young (jerone) wrote :

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

Changed in oem-priority:
status: Incomplete → In Progress
Jerone Young (jerone) wrote :

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

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

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

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

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?

Changed in firefox (Ubuntu):
importance: Undecided → Low
status: New → Triaged
Changed in chromium-browser (Ubuntu):
importance: Undecided → Low
status: New → Triaged
Martin Pitt (pitti) wrote :

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

Changed in rhythmbox (Ubuntu):
assignee: nobody → Martin Pitt (pitti)
importance: Undecided → Low
status: New → In Progress
Martin Pitt (pitti) wrote :

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

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

Changed in rhythmbox (Ubuntu):
status: In Progress → Fix Released

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

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

Jerone Young (jerone) on 2010-07-19
summary: - [Maverick] Have default settings be energy star 5.0 compliant
+ Have default settings be energy star 5.0 compliant

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

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

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

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

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

Changed in firefox (Ubuntu):
assignee: nobody → Chris Coulson (chrisccoulson)
Jonathan Matthew (jmatthew) wrote :

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

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

Changed in gnome-power:
importance: Unknown → Low
Changed in oem-priority:
status: In Progress → Fix Released
Changed in gnome-power:
status: New → Won't Fix
3 comments hidden view all 183 comments

Content ought to be able to keep the device's screen from turning off.

I don't think we should extend this privilege to all pages. Users coming from other platforms expect that the phone will manage the display, within most applications.

OTOH, it's kind of a bummer that a movie-watching app would need to explicitly ask for permission to keep the display on. If we made this facility available to all pages, it wouldn't need to ask.

We have [1]; I'm not sure what the status of this API is. But I also don't like this API. It should just be lock() and unlock() with no args.

We'll need a separate, more privileged API for Gaia itself to manage the current brightness.

[1] https://mxr.mozilla.org/mozilla-central/source/widget/public/nsIScreen.idl

I definitely agree we need this API. On phones it would prevent the phone from turning off, and on desktop it would prevent the screensaver from appearing.

For installed webapps I definitely think we could enable the API without any warnings etc.

Likewise I think it would be ok to do for pages that had gone into fullscreen mode.

For plain (non-fullscreen'ed) webpages I feel like we could almost do the same. We could do it if we at the same time enabled a mechanism which allowed the user turn off the disable-screensaver mode. Or we could simply show a permission doorhanger, that might be fine too :)

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

Could you rephrase this?

Oh, I think I understand. We can let normal pages disable screensaver without a prompt, so long as we give the user a way to re-enable the screensaver without leaving the page. How would we do that without being...annoying?

Also, Chris, can you give me an idea of this bug's priority relative to other current B2G work?

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

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

Yes. :)

As far as API goes here. I think something like this would work:

interface Navigator {

  Request disableScreenSaver(Boolean);

}

interface Request : EventTarget {
  Function onsuccess;
}

This makes for a clean API for most use cases where the page doesn't really care if the request succeeded or not. All you'd do is:

navigator.disableScreenSaver(true);
or
navigator.disableScreenSaver(false);

For pages which do want to display a warning to users where it's not able to disable the screensaver it can use the success event handler.

Note that for our standard doorhanger UI we can't tell when the user denies the request (since simply ignoring the doorhanger or clicking outside it isn't equivalent to "no", but rather "I'll decide later").

I don't know for sure that we'll ever end up using a doorhanger UI here. But with the above API we keep our options open.

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

Can we do

  disableScreenSaver,
  enableScreenSaver

? Functions which take magic booleans are a pet peeve of mine. (I guess this boolean isn't so magic, but I don't like disableScreenSaver(false) is still unfortunate.)

I'm not sure we need a reentrant solution like the Android wake lock -- an author can easily code that up around the API if necessary, right?

I'm also not sure pages need to be able to warn the user if they can't disable the screensaver; do you have a use-case, Jonas? I'm afraid of "you must disable your screensaver in order to use this app" kind of BS.

(In reply to Justin Lebar [:jlebar] from comment #8)
> I'm not sure we need a reentrant solution like the Android wake lock -- an
> author can easily code that up around the API if necessary, right?
>

Not with nested iframes, etc, or if something like the status bar and a game are trying to change power-save state. Note that the "lock" API (or something like it) is also extensible, which is nice to have and reduces API surface area.

103 comments hidden view all 183 comments

(In reply to Justin Lebar [:jlebar] from comment #111)
> Hm...actually, mListeners can be modified while we iterate over it (we're
> running arbitrary script). I wonder if we handle this elsewhere (e.g. by
> making a copy).

Someone told me that DOM is single threaded so I didn't bother to check that. But yes, the service might be accessed from other threads :-/

All this code is single-threaded, yes.

But running mListeners[i]->Callback() actually runs JS code (afaict). And that code could add or remove a wake lock listener.

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

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>

Created attachment 600050
Implement wake lock interface v2.1

Update test cases

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

Try run for 14cc0af010a8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=14cc0af010a8
Results (out of 279 total builds):
    exception: 3
    success: 225
    warnings: 27
    failure: 24
Builds (or logs if builds failed) available at:
http://<email address hidden>

Download full text (17.1 KiB)

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

> Would you mind changing these bools to
>
> enum WakeLockState {
> WAKE_LOCK_STATE_UNLOCKED,
> WAKE_LOCK_STATE_HIDDEN,
> WAKE_LOCK_STATE_VISIBLE
> };

Actually, this is kind of confusing, because UNLOCKED is not a valid state for either function. Maybe we should just go with ModifyWakeLock everywhere. Or you can just have HIDDEN and VISIBLE.

Interdiff v2 --> v2.1

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

This was checked in, right?

Download full text (6.1 KiB)

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

Read more...

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>

Created attachment 602836
Implement wake lock interface v3

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

Unfortunately interdiff bails on these two patches (presumably because they're not based off the same hg revision?), so I can't tell what changed.

Would you mind trying to get an interdiff? What I'd try is make v2.1 and v3 apply to the same base revision of m-c. Then the interdiff command should be able to give meaningful output.

Comment on attachment 602836
Implement wake lock interface v3

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

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

Created attachment 603136
v2.1 and v3 interdiff

Download full text (5.4 KiB)

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

Read more...

Comment on attachment 602836
Implement wake lock interface v3

lgtm!

Download full text (4.0 KiB)

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

Read more...

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

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.

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.

Created attachment 604291
Use nsCOMPtr copy constructor

Comment on attachment 604291
Use nsCOMPtr copy constructor

Use assignment, as in

nsCOMPtr<nsIDOMMozPowerManager> power = mPowerManager;

r=me with that

(In reply to Ms2ger from comment #139)
> Comment on attachment 604291
> 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.

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.

Comment on attachment 604826
Use do_QueryObject

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

Created attachment 604868
Use nsCOMPtr constructor.

assinged to me for sec action to schedule a meeting

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

Sure, I was confused by comment 146, which I took to mean that I didn't need to do anything here.

But at this point, I am not convinced there's actually something to review at the moment. We have an API in this bug which doesn't do anything. Site can request wake locks, but they do nothing.

I think it would be more appropriate to do a security review when we expose a way for the user to give permission to sites to use this API. I suspect that any security review we do before then will focus on this hypothetical API anyway.

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

149 comments hidden view all 183 comments
shawnlandden (shawnlandden) wrote :

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

Changed in firefox:
importance: Unknown → Medium
status: Unknown → Fix Released
150 comments hidden view all 183 comments

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

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

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

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

Displaying first 40 and last 40 comments. View all 183 comments or add a comment.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.