Comment 163 for bug 604635

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