454660 - Create a system information service

Status: NEW

(Core :: Widget, defect, P3)

Description

[Shawn Wilsher :sdwilsh]

We should be able to find out when we are on battery and not - much like the idle service.

OS X can do it with http://developer.apple.com/documentation/Darwin/Reference/IOKit/IOPowerSources/index.html.  Works on 10.3 and later, so we are good.

Windows can do it with http://msdn.microsoft.com/en-us/library/aa372693(VS.85).aspx to get the current value, and http://msdn.microsoft.com/en-us/library/aa372715(VS.85).aspx to get notifications of changes.  Works with windows 2000 and later, so we're good.

campd says he knows how to do it on Linux as well.

Comment 1

[Shawn Wilsher :sdwilsh]

I can drive this because it's important for places work.  Won't happen until after beta 1 though.

Comment 2

[Wayne Mery (:wsmwk)]

is this this still going to make 1.9.1?

Also blocked bug with target milestone of FF3.2a1

Comment 3

[Shawn Wilsher :sdwilsh]

No, we are feature frozen.

Comment 4

[Rob Arnold [:robarnold]]

It would be nice if we could obtain the user's intentions for power usage.

Under Vista and above, we can call PowerGetActiveScheme and check the resulting GUID against a few predefined values for "max power savings," "balanced" and "min power savings." I don't know exactly what values we want to define, but it is useful information and we should use it. Someone will probably want a pref to disable it.

Unfortunately, OSX and XP/2k can only tell us if the computer is on battery or AC (or a UPS if that's somehow interesting) so we will need to infer the user's intentions from that.

On Linux/other Unixes, I am not sure what library would provide this information and be available to use.

Comment 5

[Richard Hughes]

(In reply to comment #4)
> On Linux/other Unixes, I am not sure what library would provide this
> information and be available to use.

Sounds like you're looking for DeviceKit-power: http://people.freedesktop.org/~hughsient/DeviceKit-power/gtk-doc/Power.html

Using HAL isn't a good plan at all, as you have to work out whether a power device is supplying or being supplied by the computer. DeviceKit-power makes this very easy to get.

Richard.

Comment 6

[Shawn Wilsher :sdwilsh]

I actually wasn't looking at HAL, but rather http://people.freedesktop.org/~hughsient/temp/dbus-interface.html

It has a slightly better interface than the one you linked to because it has a GetPowerSaveStatus method.

Comment 7

[Richard Hughes]

Right, we're deprecating the session based interface, and trying to move as much of the logic and processing down into the system layer as possible. I think GNOME was the only DE that implements org.freedesktop.Powermanagement in the session, as KPowersave is now obsolete.

Also, my argument is that we shouldn't define whether we are running on low power mode, or high power mode, but that we should always run in low power mode; there is no point in doing things that don't need doing. If you want to control the power mode using a QOS latency request, the QoS interface[1] works on Linux using PMQOS.

I do think that firefox should respect the battery setting, and the low battery setting, as you don't want to have big caches of unsaved data when on low power, and probably want to do as little on battery power as possible.

Richard.

[1] http://people.freedesktop.org/~hughsient/DeviceKit-power/gtk-doc/QoS.html

Comment 8

[Boris Zbarsky [:bzbarsky]]

> there is no point in doing things that don't need doing.

"Need" is an absolute...  It might make sense to drop the framerate of paintingfrom 50fps to 25fps in low-power mode, for example...

Comment 9

[Shawn Wilsher :sdwilsh]

There are also things that we need to do, but will try to avoid doing them when on battery.

Comment 10

[Richard Hughes]

(In reply to comment #8)
> "Need" is an absolute...  It might make sense to drop the framerate of
> paintingfrom 50fps to 25fps in low-power mode, for example...

No, you should use the lowest framerate that the most demanding widget requires. There's no point refreshing at 1000Hz on AC power when the widget is not moving, or only changing at 5Hz.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yes, of course; that's assumed.  If nothing changed, we don't paint.  I'm talking about dropping the frame rate to 25Hz even if the page is moving stuff around every 10ms (which is what web pages tend to do).  On AC power, we might want to paint at 50Hz or 100Hz (opinion is divided on that, for what it's worth) in that situation, but in low-power situations it might makes sense to throttle the painting even further.

Comment 12

[Richard Hughes]

(In reply to comment #11)
> Yes, of course; that's assumed.  If nothing changed, we don't paint.  I'm
> talking about dropping the frame rate to 25Hz even if the page is moving stuff
> around every 10ms (which is what web pages tend to do).

I'm not sure that's going to be liked very much by users. Maybe you could justify that when the battery is very low, but not on a general battery case.

> On AC power, we might
> want to paint at 50Hz or 100Hz (opinion is divided on that, for what it's
> worth) in that situation, but in low-power situations it might makes sense to
> throttle the painting even further.

Is there much point in painting faster than the monitor refresh? On battery we can reduce the refresh rate of lcd panels in Linux (on some display drivers) and you don't want to be refreshing faster than the display.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I'm not sure that's going to be liked very much by users.

Honestly, it might not matter in a lot of cases in terms of what the user sees.  And yes, I was talking about low-power situations, not just "might become low power".

> Is there much point in painting faster than the monitor refresh?

Absolutely not.  But monitor refresh is something like 60Hz in a lot of cases, and typical web animations run at 100Hz (because setTimeout is clamped to a minimum of 10ms), so painting at 60Hz could lead to weird effects with those animations.  Like I said, that's a totally separate discussion.

Comment 14

[Shawn Wilsher :sdwilsh]

Hey guys - I appreciate the excitement about this, but this discussion isn't terribly related to this bug, so could you maybe take it to the newsgroups?  Thanks!

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: generic API v1.0

This contains the changes to add the basic folder structure as well as the idl file defining this interface.

Each platform will have to have its own implementation.

Comment 16

[Shawn Wilsher :sdwilsh]

Attachment: mac implementation v1.0

This is the mac implementation.  I'm still working on tests (which may turn out to be not wise to do).  Trying to mock the system calls in them.

Comment 17

[Josh Aas]

I'm still not totally clear on why we want this. Can you write out a detailed explanation of exactly how we plan to use this?

Comment 18

[Rob Arnold [:robarnold]]

I would like to change the way we calculate the date on Windows to be more reliable. This requires changing the tick frequency of the kernel which reduces battery life. The solution, which Chrome uses, involves switching to the higher frequency only when on AC.

I know Shawn has plans to delay other computation and disk IO until we're on AC.

Comment 19

[Josh Aas]

Comment on attachment 382216 [details] [diff] [review]
mac implementation v1.0

>+#import <Carbon/Carbon.h>

Do you actually need Carbon here? Try to just use <CoreFoundation/CoreFoundation.h>. This is included in Carbon but Carbon also has a bunch of other stuff we probably don't want.

>+/**
>+ * Used to make sure we always release CFTypeRef.
>+ */
>+template < >
>+class nsAutoRefTraits<CFTypeRef>
>+{
>+public:
>+  typedef CFTypeRef RawRef;
>+  static void Release(CFTypeRef ref) { ::CFRelease(ref); }
>+  static CFTypeRef Void() { return NULL; }
>+};
>+typedef nsAutoRef<CFTypeRef> CFTypeAutoRef;
>+
>+/**
>+ * Used to make sure we always release CFArrayRef.
>+ */
>+template < >
>+class nsAutoRefTraits<CFArrayRef>
>+{
>+public:
>+  typedef CFArrayRef RawRef;
>+  static void Release(CFArrayRef ref) { ::CFRelease(ref); }
>+  static CFArrayRef Void() { return NULL; }
>+};
>+typedef nsAutoRef<CFArrayRef> CFArrayAutoRef;

Isn't it possible to just use the CFTypeRef version for CFArrayRef as well and avoid two of these classes? IIRC CFArray is a CFType.

>+nsresult
>+PowerNotificationService::initialize()

Shouldn't this be capitalized, "Initialize"?

>+void
>+PowerNotificationService::handleChange(void *aContext)

Shouldn't this also be capitalized, "Handle..."?

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> Do you actually need Carbon here? Try to just use
> <CoreFoundation/CoreFoundation.h>. This is included in Carbon but Carbon also
> has a bunch of other stuff we probably don't want.
just importing CoreFoundation works

> Isn't it possible to just use the CFTypeRef version for CFArrayRef as well and
> avoid two of these classes? IIRC CFArray is a CFType.
I would have to cast it to a CFArray at each call site which is icky.

> Shouldn't this be capitalized, "Initialize"?
> Shouldn't this also be capitalized, "Handle..."?
We don't actually specify this in our generic style guidelines.  I was following the storage style guidelines for this (http://mxr.mozilla.org/mozilla-central/source/storage/style.txt)

Comment 21

[Shawn Wilsher :sdwilsh]

So, as it turns out, mocking the system calls is really really hard.  Might be doable on linux, but windows and os x would require mocking the entire dll or framework, respectively.  Not worth doing, so this will require a litmus test.

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 382215 [details] [diff] [review]
generic API v1.0

>+/**
>+ * The power notification service lets consumers get information about the power
>+ * status of the device running this software.
>+ *
>+ * Consumers can register with the observer service for topics when each of
>+ * these attributes change.  The subject of the observe calls will be this
>+ * service.
>+ *   "power-notification-battery-changed" is dispatched when onBattery changes.
>+ *   "power-notification-power-conservation-changed" is dispatched when
>+ *     shouldConservePower changes.
>+ */

I don't think we need to pass the service, I'd probably just pass the topic plus the state we changed to.

i.e. notifyObservers(nsnull, "power-notification-battery-changed", "onBattery" / "pluggedIn") seems a lot better, and consumers don't have to go back through xpconnect to get the value...

r+sr=me with these changes addressed.  that probably means a new patch rev for the Mac patch, since it inverts one of the booleans and changes all the notifyObservers calls... :)

Comment 23

[Mike Connor [:mconnor]]

comment got eaten, the other change is to replace "shouldConservePower" with "canUseFullPower" to invert the way people think about these things (by default be conservative, be aggressive when we can, etc)

Comment 24

[Shawn Wilsher :sdwilsh]

per discussion with mconnor in person, morphing this bug.

Comment 25

[Shawn Wilsher :sdwilsh]

Attachment: generic API v2.0

Per conversation.  I'll have an updated mac version of the patch tomorrow.

Comment 26

[Mike Connor [:mconnor]]

Comment on attachment 382613 [details] [diff] [review]
generic API v2.0

sweet.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 382613 [details] [diff] [review]
generic API v2.0

>diff --git a/toolkit/components/system-information/public/nsISystemInformationService.idl b/toolkit/components/system-information/public/nsISystemInformationService.idl

>+ * The power notification service lets consumers get information about the power
>+ * status of the device running this software.

Forgot to change this?

>+interface nsISystemInformationService : nsISupports
>+{
>+  /**
>+   * Indicates if the device we are running on is using battery power or not.

"Indicates whether the system is using battery power or not." ? "the device" doesn't seem quite right - this is the "System" information service after all.

>+  /**
>+   * Indicates that power-intensive but not immediately essential routines can
>+   * be used.

This comment is kind of odd - you're describing the implication rather than the actual meaning. How about something like:

"Returns false when the system is resource-constrained. For example, this may return false when the system is low on battery power, or when the CPU is throttled to preserve energy."

Comment 28

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #27)
> "Indicates whether the system is using battery power or not."

s/or not//, even.

Comment 29

[Chris Lawson (gone)]

(In reply to comment #27)

> return false when the system is low on battery power, or when the CPU is
> throttled to preserve energy."

s/preserve/conserve

Energy can be neither created nor destroyed; we just want to minimise the rate at which it's converted from one form to another ;)

Comment 30

[Shawn Wilsher :sdwilsh]

Attachment: generic API v2.1

Addresses drive-by review comments.

Comment 31

[Shawn Wilsher :sdwilsh]

Attachment: mac implementation v2.0

Here is the updated mac implementation.  I think we should spin off the windows and linux implementation in different bugs.

Comment 32

[Josh Aas]

Comment on attachment 382782 [details] [diff] [review]
mac implementation v2.0

+#import <Carbon/Carbon.h>
...
+   * Handles the notifications given to use by Carbon when a power device is

Still more mentions of Carbon to remove.

+  CFTypeAutoRef data(::IOPSCopyPowerSourcesInfo());
+  NS_ENSURE_TRUE(data, NS_ERROR_FAILURE);
+
+  CFArrayAutoRef list(::IOPSCopyPowerSourcesList(data));
+  NS_ENSURE_TRUE(list, NS_ERROR_FAILURE);

Both of these leak. "Caller must CFRelease() the returned CFArrayRef." This is usually the case for any CoreFoundation functions containing the word "Copy".

Comment 33

[Shawn Wilsher :sdwilsh]

(In reply to comment #32)
> +  CFTypeAutoRef data(::IOPSCopyPowerSourcesInfo());
> +  NS_ENSURE_TRUE(data, NS_ERROR_FAILURE);
> +
> +  CFArrayAutoRef list(::IOPSCopyPowerSourcesList(data));
> +  NS_ENSURE_TRUE(list, NS_ERROR_FAILURE);
> Both of these leak. "Caller must CFRelease() the returned CFArrayRef." This is
> usually the case for any CoreFoundation functions containing the word "Copy".
That's why it's CF[Type|Array]AutoRef, which is our typedef for nsAutoRef<CF[Type|Array]Ref>, which has the traits to release when it falls out of scope.  We do call CFRelease every time the variable falls out of scope.

Comment 34

[Josh Aas]

Oh duh, I forgot that was added, I should have read more carefully :)

Comment 35

[Josh Aas]

Comment on attachment 382782 [details] [diff] [review]
mac implementation v2.0

remove the carbon stuff, r+

Comment 36

[Shawn Wilsher :sdwilsh]

Attachment: mac implementation v2.1

Addresses review comments.

Comment 37

[Shawn Wilsher :sdwilsh]

Two things I noticed:
1) CFRunLoopRemoveSource needs to be used in the desctructor, not CFRunLoopAddSource
2) I have a comment that says "Batter Power" and not "Battery Power"

Comment 38

[Mike Connor [:mconnor]]

Comment on attachment 383518 [details] [diff] [review]
mac implementation v2.1


>+SystemInformationService::handleChange(void *aContext)

>+  data = onBattery ? FALSE_STRING : FALSE_STRING;

I don't even know who you are anymore.

This looks fine otherwise, though I think bsmedberg wants to kill /components (see snav) so maybe this should be in toolkit toplevel?  Please ask him as module owner?

Comment 39

[Benjamin Smedberg]

Please make the interface toolkit/system/nsIPowerStatus.idl (nsIPowerStatus) and put the impl in toolkit/system.

Comment 40

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 41

[Antoine Turmel [:GeekShadow]]

Could be related to bug 678694