Comment 161 for bug 604635

Thanks a lot for the interdiff.

>@@ -92,8 +99,13 @@
> {
> nsAutoString state;
> ComputeWakeLockState(aWakeLockInfo, state);
>-
>- nsTArray<nsCOMPtr<nsIDOMMozWakeLockListener> > listeners(mListeners);
>+
>+ /**
>+ * Copy the listeners list before we walk through the callbacks
>+ * because the callbacks may install new listeners. We expect no
>+ * more than one listener per window, so it shouldn't be too long.
>+ */
>+ nsTArray<nsCOMPtr<nsIDOMMozWakeLockListener> > listeners(mWakeLockListeners);

Nit: Use nsAutoTArray here.

>diff -u b/hal/Hal.cpp b/hal/Hal.cpp
>--- b/hal/Hal.cpp
>+++ b/hal/Hal.cpp
>@@ -184,10 +184,11 @@
> if (mObservers->Length() == 0) {
> DisableNotifications();
>
>+ // XXX CachingObserversManager invalidate it's cache here, but why?
>+ OnNotificationsDisabled();

AIUI caching observers invalidate their cache when notifications are disabled because as soon as they stop listening e.g. to the battery state, their cached battery state is invalid.

Does that answer your question here? We should get rid of the XXX before we check in.

>- * Increase the wake lock count of aTopic
>- * @param aTopic the resource name
>- * @param aHidden the visibility of the wake lock
>+ * Adjust the internal wake lock counts.
>+ * @param aTopic lock topic
>+ * @param aLockAdjust to increase or decrease active locks
>+ * @param aHiddenAdjust to increase or decrease hidden locks
> */
>-void AcquireWakeLock(const nsAString &aTopic, bool aHidden);
>+void ModifyWakeLock(const nsAString &aTopic, int aLockAdjust, int aHiddenAdjust);

We generally shun raw types (e.g. "int") in our code, since "int" can mean different things on different platforms.

Since you're passing WakeLockControl objects, can you just use that type here and elsewhere? (You may need to define the WakeLockControl enum in the ipdl.) If someone needs to call ModifyWakeLock(aTopic, 2), we can revisit this. You can then add the enum value directly to count.numLocks and count.numHidden, bypassing the whole enum to int conversion altogether.

>--- /dev/null
>+++ b/dom/power/test/browser_bug697132.js
>@@ -0,0 +1,197 @@
>+"use strict";
>+
>+waitForExplicitFinish();
>+
>+let pageSource1 = "data:text/html,1";
>+let pageSource2 = "data:text/html,2";
>+
>+let win, win1, win2;
>+let tab, tab1, tab2;
>+let lock, lock1, lock2;
>+let gCurStepIndex = -1;
>+let gSteps = [
>+ function basicWakeLock() {
>+ tab = gBrowser.addTab(pageSource1);
>+ win = gBrowser.getBrowserForTab(tab).contentWindow;
>+ let browser = gBrowser.getBrowserForTab(tab);
>+
>+ browser.addEventListener("load", function onLoad(e) {
>+ browser.removeEventListener("load", onLoad, true);
>+ let nav = win.navigator;
>+ let power = nav.mozPower;
>+ lock = nav.requestWakeLock("test");
>+
>+ ok(lock != null,
>+ "navigator.requestWakeLock should return a wake lock");
>+ is(lock.topic, "test",
>+ "wake lock should remember the locked topic");
>+ isnot(power.getWakeLockState("test"), "unlocked",
>+ "topic is locked");
>+
>+ lock.unlock();
>+
>+ is(lock.topic, "test",
>+ "wake lock should remember the locked topic even after unlock");
>+ is(power.getWakeLockState("test"), "unlocked",
>+ "topic is unlocked");
>+
>+ try {
>+ lock.unlock();

Add ok(false, "Should have thrown an error.");

>+function test() {
>+ // data url inherits its parent's principal, which is |about:| here.
>+ Services.prefs.setCharPref("dom.power.whitelist", "about:");
>+ runNextStep();
>+}

Is this necessary? We're chrome here, so the data URLs should have a chrome principal, I'd expect.

In any case, please reset the pref to its original value when you're done.

This test looks great, but are you confident that you don't need a test where you go back and forward in a background tab? Up to you.

>+void
>+ModifyWakeLock(const nsAString &aTopic, int aLockAdjust, int aHiddenAdjust)
>+{
>+ if (!sInitialized) {
>+ Init();
>+ }
>+
>+ LockCount count;
>+ count.numLocks = 0;
>+ count.numHidden = 0;
>+ sLockTable->Get(aTopic, &count);
>+ MOZ_ASSERT(count.numLocks >= count.numHidden);
>+ MOZ_ASSERT(aLockAdjust >= 0 || count.numLocks > 0);
>+ MOZ_ASSERT(aHiddenAdjust >= 0 || count.numHidden > 0);
>+
>+ WakeLockState oldState = ComputeWakeLockState(count.numLocks, count.numHidden);
>+
>+ count.numLocks += aLockAdjust;
>+ count.numHidden += aHiddenAdjust;
>+ MOZ_ASSERT(count.numLocks >= count.numHidden);
>+
>+ sLockTable->Put(aTopic, count);

If count.numLocks goes to 0, should we remove the wake lock from the table?

>--- /dev/null
>+++ b/hal/HalWakeLock.h
>@@ -0,0 +1,32 @@
>+/* -*- Mode: C++; tab-width: 40; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef __HAL_WAKELOCK_H_
>+#define __HAL_WAKELOCK_H_
>+
>+namespace mozilla {
>+namespace hal {
>+
>+enum WakeLockState {
>+ WAKE_LOCK_STATE_UNLOCKED,
>+ WAKE_LOCK_STATE_HIDDEN,
>+ WAKE_LOCK_STATE_VISIBLE
>+};
>+
>+enum WakeLockControl {
>+ WAKE_LOCK_REMOVE_ONE = -1,
>+ WAKE_LOCK_NO_CHANGE = 0,
>+ WAKE_LOCK_ADD_ONE = 1,
>+};
>+
>+/**
>+ *
>+ */
>+WakeLockState ComputeWakeLockState(int aNumLocks, int aNumHidden);

Finish documenting, and, why is this public? There's a function called
ComputeWakeLockState which outputs a string, but that's not this one...