Comment 139 for bug 604635

Comment on attachment 599555
Implement wake lock interface

> NS_IMETHODIMP
> Navigator::GetMozPower(nsIDOMMozPowerManager** aPower)
> {
> if (!mPowerManager) {
>+ nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
>+ NS_ENSURE_TRUE(win, NS_OK);

Hmm...So at the very least, we need to set *aPower = NULL on error. (You can set it to null at the very beginning of the function, for good measure.)

It looks like our convention (e.g. for GetGeolocation) is to return null but not throw an error, so returning NS_OK is correct.

Sorry to nit on the following comment so much. It's mostly fine, but since there's no spec for this, the comment is our primary documentation, and I'd like to get it right.

>+ /**
>+ * Request a wake lock for specified resource.

s/specified resource/a resource/, or s/specified resource/the specified resource/.

Tell me here what it means for a page to hold a wake lock, at a high level. Something like, "A page holds a wake lock to request that a resource not be turned off (or otherwise made unavailable)."

>+ * The topic is the name of a resource that might be made unavailable for
>+ * various reasons. For example, on mobile device the power manager might
>+ * decide to turn off the screen after a period of idle time to save power.
>+ *
>+ * The resource manager can check the lock state of a topic before shutdown
>+ * the resource. So a lock on "screen" could prevent the screensaver from
>+ * appearing on Desktop or screen blackout on mobile device.

s/on mobile device/on a mobile device/
s/shutdown/shutting down/

I'd restructure this a bit. I'm left searching for the meaning of the topic at the end of the first paragraph. Something like the following:

  * The resource manager checks the lock state of a topic before turning off
  * the associated resource. For example, a page could hold a lock on the
  * "screen" topic to prevent the screensaver from appearing or the screen from
  * turning off.

>+ * The policy is enforced by the resource management code. There is no limit
>+ * on the possible topic, but there will be a predefined set of meaningful
>+ * topic.

  * The resource manager defines what each topic means and sets policy. For
  * example, the resource manager might decide to ignore 'screen' wake locks
  * held by pages which are not visible.
  *
  * One topic can be locked multiple times; it is considered released only when
  * all locks on the topic have been released.

>+ * The returned wake lock object is a token of the lock. One can find the
>+ * locked resource through the topic property and release the lock through
>+ * the unlock method. The lock is released automatically when the user
>+ * navigates away the associated window.
>+ *
>+ * Same topic can be locked multiple times and is unlocked only after all
>+ * wake lock object has been released.

s/Same topic/The same topic/
s/wake lock object has/wake lock objects have/

  * The returned nsIDOMMozWakeLock object is a token of the lock. You can
  * unlock the lock via the object's |unlock| method. The lock is released
  * automatically when its associated window is unloaded.

>diff --git a/dom/power/PowerManagerService.cpp b/dom/power/PowerManagerService.cpp
>--- a/dom/power/PowerManagerService.cpp
>+++ b/dom/power/PowerManagerService.cpp
> /* static */ already_AddRefed<nsIPowerManagerService>
> PowerManagerService::GetInstance()
> {
>- nsCOMPtr<nsIPowerManagerService> pmService;
>+ nsRefPtr<PowerManagerService> pmService;
>
> pmService = new PowerManagerService();
>+ pmService->Init();
>
> return pmService.forget();
> }

Don't you want this to be a singleton? You can make the nsRefPtr a static class member and then call ClearOnShutdown() on it.

>+void
>+PowerManagerService::Notify(const hal::WakeLockInformation& aWakeLockInfo)
>+{
>+ nsAutoString state;
>+ ComputeWakeLockState(aWakeLockInfo, state);
>+
>+ for (PRUint32 i = mListeners.Length(); i > 0; ) {
>+ --i;
>+ mListeners[i]->Callback(aWakeLockInfo.topic(), state);
>+ }

More idiomatic would be

  for (PRUint32 i = mListeners.Length() - 1; i >= 0; i--)

>+NS_IMETHODIMP
>+PowerManagerService::AddWakeLockListener(nsIDOMMozWakeLockListener *aListener)
>+{
>+ if (mListeners.IndexOf(aListener) != mListeners.NoIndex)
>+ return NS_OK;

Please use Contains().

>+NS_IMETHODIMP
>+PowerManagerService::RemoveWakeLockListener(nsIDOMMozWakeLockListener *aListener)
>+{
>+ if (mListeners.IndexOf(aListener) == mListeners.NoIndex)
>+ return NS_OK;
>+
>+ mListeners.RemoveElement(aListener);
>+ return NS_OK;
>+}

Here you might as well unconditionally call RemoveElement().

>+NS_IMETHODIMP
>+PowerManagerService::GetWakeLockState(const nsAString &aTopic, nsAString &aState)
>+{
>+ hal::WakeLockInformation info;
>+ hal::GetWakeLockInfo(nsString(aTopic), &info);
>+
>+ ComputeWakeLockState(info, aState);
>+
>+ return NS_OK;
>+}

>+NS_IMETHODIMP
>+PowerManagerService::NewWakeLock(const nsAString &aTopic,
>+ nsIDOMWindow *aWindow,
>+ nsIDOMMozWakeLock **aWakeLock)
>+{
>+ AcquireWakeLock(aTopic, aWindow);
>+
>+ nsRefPtr<WakeLock> wakelock = new WakeLock();
>+ nsresult rv = wakelock->Init(aTopic, aWindow);
>+ NS_ENSURE_SUCCESS(rv, rv);
>+
>+ NS_ADDREF(*aWakeLock = wakelock);
>+
>+ return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+PowerManagerService::AcquireWakeLock(const nsAString &aTopic,
>+ nsIDOMWindow *aWindow)
>+{
>+ bool hidden = true;
>+
>+ if (aWindow) {
>+ nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(aWindow);
>+ NS_ENSURE_TRUE(win, NS_ERROR_FAILURE);

A synonym is NS_ENSURE_STATE(win). I don't much care which you use, but some reviewers prefer ENSURE_STATE, so maybe it's a good habit to be in.

>+ nsIDOMDocument* domDoc = win->GetExtantDocument();
>+ NS_ENSURE_TRUE(domDoc, NS_ERROR_FAILURE);
>+ domDoc->GetMozHidden(&hidden);
>+ }
>+
>+ hal::AcquireWakeLock(nsString(aTopic), hidden);

hal::AcquireWakeLock and company should take nsAString, not nsString. Then you
won't need the nsString() wrapper here or elsewhere.

(Apparently IPDL requires nsString's, but let's make that conversion as late as
possible.)

>+void
>+WakeLock::DoLock()
>+{
>+ if (!mLocked) {
>+ nsCOMPtr<nsIPowerManagerService> pmService =
>+ do_GetService(POWERMANAGERSERVICE_CONTRACTID);
>+ nsCOMPtr<nsIDOMWindow> window = do_QueryReferent(mWindow);
>+
>+ pmService->AcquireWakeLock(mTopic, window);
>+ mLocked = true;
>+ }
>+}
>+
>+void
>+WakeLock::DoUnlock()
>+{
>+ if (mLocked) {
>+ nsCOMPtr<nsIPowerManagerService> pmService =
>+ do_GetService(POWERMANAGERSERVICE_CONTRACTID);
>+ nsCOMPtr<nsIDOMWindow> window = do_QueryReferent(mWindow);
>+
>+ pmService->ReleaseWakeLock(mTopic, window);
>+ mLocked = false;
>+ }

I'm concerned about subtle bugs being introduced here. For example, if a window is deleted before it's marked as hidden, then when we DoUnlock, we'll release the wake lock as though the window were hidden, and all our counts will be messed up.

We talked in person and agreed that the WakeLock object should remember the last visibilty state it sent to hal.

I haven't looked through all of this, but a new version is coming, so I'll wait before looking at the rest.