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