Comment on attachment 600050
Implement wake lock interface v2.1

>@@ -941,24 +946,45 @@ Navigator::GetMozBattery(nsIDOMMozBatter
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> Navigator::GetMozPower(nsIDOMMozPowerManager** aPower)
> {
>   if (!mPowerManager) {
>+    *aPower = nsnull;

It's convention to do *aPower = nsnull unconditionally, just so someone doesn't
have to scan the code to see that it's correct.

>+   * Request a wake lock for a resource.

This comment looks great; thanks.

>+nsRefPtr<PowerManagerService> PowerManagerService::sSingleton;

Please mark this as static.

> /* static */ already_AddRefed<nsIPowerManagerService>
> PowerManagerService::GetInstance()
> {
>-  nsCOMPtr<nsIPowerManagerService> pmService;
>+  if (!sSingleton) {
>+    sSingleton = new PowerManagerService();
>+    sSingleton->Init();
>+    ClearOnShutdown(&sSingleton);
>+  }
> 
>-  pmService = new PowerManagerService();
>+  NS_ADDREF(sSingleton);
>+  return sSingleton.get();

This works, but raw addref/release is scary, so we prefer

  nsCOMPtr<nsIPowerManagerService> service = sSingleton;
  return service.forget();

>+void
>+PowerManagerService::ComputeWakeLockState(const hal::WakeLockInformation& aWakeLockInfo,
>+                                          nsAString &aState)
>+{
>+  if (aWakeLockInfo.locks() == 0) {
>+    aState.AssignLiteral("notheld");
>+  } else if (aWakeLockInfo.locks() == aWakeLockInfo.hidden()) {
>+    aState.AssignLiteral("background");
>+  } else {
>+    aState.AssignLiteral("foreground");
>+  }

Maybe "unlocked/locked-background/locked-foreground" would be clearer?

>+void
>+PowerManagerService::Notify(const hal::WakeLockInformation& aWakeLockInfo)
>+{
>+  nsAutoString state;
>+  ComputeWakeLockState(aWakeLockInfo, state);
>+  
>+  nsTArray<nsCOMPtr<nsIDOMMozWakeLockListener> > listeners(mListeners);

Please add a comment explaining why we're copying this list, and why copying is OK (we expect no more than one listener per window, so it shouldn't be too long).

>+NS_IMETHODIMP
>+PowerManagerService::NewWakeLock(const nsAString &aTopic,
>+                                 nsIDOMWindow *aWindow,
>+                                 nsIDOMMozWakeLock **aWakeLock)
>+{
>+  nsRefPtr<WakeLock> wakelock = new WakeLock();
>+  nsresult rv = wakelock->Init(aTopic, aWindow);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  NS_ADDREF(*aWakeLock = wakelock);

Out of general fear of raw addref/release, how about

  *aWakeLock = wakelock.forget()

>diff --git a/dom/power/PowerManagerService.h b/dom/power/PowerManagerService.h
>--- a/dom/power/PowerManagerService.h
>+++ b/dom/power/PowerManagerService.h
> class PowerManagerService
>   : public nsIPowerManagerService
>+  , public WakeLockObserver
> {
> public:
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSIPOWERMANAGERSERVICE
> 
>   static already_AddRefed<nsIPowerManagerService> GetInstance();
>+
>+  void Init();
>+
>+  void Notify(const hal::WakeLockInformation& aWakeLockInfo);

NotifyWakeLockState, NotifyWakeLockStateChange, or something?

>+  void ComputeWakeLockState(const hal::WakeLockInformation& aWakeLockInfo,
>+                            nsAString &aState);

Does this need to be public?

>+
>+private:
>+
>+  ~PowerManagerService();
>+  static nsRefPtr<PowerManagerService> sSingleton;
>+
>+  nsTArray<nsCOMPtr<nsIDOMMozWakeLockListener> > mListeners;

mWakeLockListeners?

>diff --git a/dom/power/Types.h b/dom/power/Types.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/power/Types.h
>@@ -0,0 +1,21 @@
>+/* -*- 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 mozilla_dom_power_Types_h
>+#define mozilla_dom_power_Types_h
>+
>+namespace mozilla {
>+namespace hal {
>+class WakeLockInformation;
>+} // namespace hal
>+
>+template <class T>
>+class Observer;
>+
>+typedef Observer<hal::WakeLockInformation> WakeLockObserver;
>+
>+} // namespace mozilla
>+
>+#endif // mozilla_dom_power_Types_h

Ugh, I hate this stuff, but Mounir likes it, and he owns most of this code.  :(

>diff --git a/dom/power/WakeLock.cpp b/dom/power/WakeLock.cpp
>new file mode 100644
>--- /dev/null
>+++ b/dom/power/WakeLock.cpp
>@@ -0,0 +1,163 @@

>+nsresult
>+WakeLock::Init(const nsAString &aTopic, nsIDOMWindow *aWindow)
>+{
>+  mTopic.Assign(aTopic);
>+  mWindow = do_GetWeakReference(aWindow);
>+
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
>+
>+ if (window) {

Can you comment here that null windows are explicitly allowed, and considered
always invisible?

>+   nsCOMPtr<nsIDOMDocument> domDoc = window->GetExtantDocument();
>+   NS_ENSURE_STATE(domDoc);
>+   domDoc->GetMozHidden(&mHidden);
>+
>+   nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(domDoc);
>+   target->AddSystemEventListener(NS_LITERAL_STRING("mozvisibilitychange"),
>+                                  this, true, false);
>+
>+   target = do_QueryInterface(window);
>+   target->AddSystemEventListener(NS_LITERAL_STRING("pagehide"),
>+                                  this, true, false);
>+   target->AddSystemEventListener(NS_LITERAL_STRING("pageshow"),
>+                                  this, true, false);

Can you please document the stupid boolean params?  e.g.

  AddSystemEventListener(/* useCapture = */ true, /* wantsUntrusted = */ false);

(Full disclosure: I have a personal vendetta against opaque boolean params like these.
http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html)

>+NS_IMETHODIMP
>+WakeLock::Unlock()
>+{
>+  if (!mLocked) {
>+    return NS_ERROR_FAILURE;
>+  }

This will cause a JS exception on double-unlock.  That's fine (I like APIs
which loudly point out errors to users), but I wanted to make sure you were
aware.  You should add a test for this behavior, if you don't already have
one, so we don't regress it.

>+NS_IMETHODIMP
>+WakeLock::HandleEvent(nsIDOMEvent *aEvent)
>+{
>+  nsAutoString type;
>+  aEvent->GetType(type);
>+
>+  if (type.EqualsLiteral("mozvisibilitychange")) {
>+    nsCOMPtr<nsIDOMEventTarget> target;
>+    aEvent->GetTarget(getter_AddRefs(target));
>+    nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(target);
>+    NS_ENSURE_STATE(domDoc);
>+    domDoc->GetMozHidden(&mHidden);
>+
>+    if (mHidden) {
>+      HideOneWakeLock(mTopic);
>+    } else {
>+      ShowOneWakeLock(mTopic);
>+    }

Please explicitly call hal::{Show,Hide}OneWakeLock; it's short and simple
enough.

>diff --git a/dom/power/WakeLock.h b/dom/power/WakeLock.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/power/WakeLock.h
>+  void     DoUnlock();
>+  void     DoLock();

Why are these public?

>diff --git a/dom/power/nsIDOMPowerManager.idl b/dom/power/nsIDOMPowerManager.idl
>--- a/dom/power/nsIDOMPowerManager.idl
>+++ b/dom/power/nsIDOMPowerManager.idl
>+    /**
>+     * The listeners are notified when the first wake lock of a topic
>+     * is acquired and the last wake lock of the topic is released.
>+     */
>+    void    addWakeLockListener(in nsIDOMMozWakeLockListener aListener);
>+    void    removeWakeLockListener(in nsIDOMMozWakeLockListener aListener);

This should probably be reworded now that a lock can be in one of three, not two, states.

>+[scriptable, function, uuid(4e258af8-cffb-47bc-b16d-e8241243426e)]
>+interface nsIDOMMozWakeLockListener : nsISupports
>+{
>+  /**
>+   * The callback will be called when:
>+   *  - a topic is locked first time,
>+   *  - all wake locks of the topic has been released,

s/has/have

>+   *  - a wake lock changes its visibility

Maybe we can simplify this to "a wake lock changes in state", where a wake lock's state is defined as the max of [unlocked, locked but not visible, locked and visible].

At least, I like to think of it mathematically like this...

>+   * The visibility of a wake lock is bound to the window that
>+   * requested it. When the window is hidden, the wake lock is also
>+   * hidden. Possible states are "background", "foreground",
>+   * and "notheld".
>+   *
>+   *  - "foreground" means at least one window that holds the wake lock
>+   *    is visible.
>+   *
>+   *  - "background" means at least one window holds the wake lock,
>+   *    but all of them are hidden.
>+   *
>+   *  - "notheld" means nobody holds the wake lock.

It would be clearer here if you distinguished between the
state of an individual wake lock, which is tied to a window, and the lock state of a /topic/, which is the max of the 'unlocked' and the relevant wake locks' states.

>+[scriptable, builtinclass, uuid(235ca1a1-d0c8-41f3-9b4a-dbaa4437d69c)]
> interface nsIPowerManagerService : nsISupports

Could you please add a brief comment explaining what nsIPowerManagerService is
for?  Like, why not have the windows register with Hal directly?

>-    void powerOff();
>-    void reboot();
>+  void              powerOff();
>+  void              reboot();
>+  void              addWakeLockListener(in nsIDOMMozWakeLockListener aListener);
>+  void              removeWakeLockListener(in nsIDOMMozWakeLockListener aListener);
>+  DOMString         getWakeLockState(in DOMString aTopic);
>+
>+  /**
>+   * Return a wake lock object of aTopic associated with aWindow.
>+   * A wake lock without associated window is always considered invisible.
>+   */
>+  nsIDOMMozWakeLock newWakeLock(in DOMString aTopic, [optional] in nsIDOMWindow aWindow);

I'm not sure what a null window would be good for, but sure.  :)  (If this is
ever useful, I'd imagine we'd want to allow forced-visible locks too.  But
we'll worry about it later.)

>diff --git a/dom/power/test/test_power_basics.html b/dom/power/test/test_power_basics.html
>--- a/dom/power/test/test_power_basics.html
>+++ b/dom/power/test/test_power_basics.html

Not reviewing the test since you just updated it.

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp
>--- a/hal/Hal.cpp
>+++ b/hal/Hal.cpp
>@@ -182,52 +182,70 @@ public:
>     mObservers->RemoveObserver(aObserver);
> 
>     if (mObservers->Length() == 0) {
>       DisableNotifications();
> 
>       delete mObservers;
>       mObservers = 0;
> 
>-      mHasValidCache = false;
>+      PostRemoveObserver();
>     }
>   }
> 
>+  void BroadcastInformation(const InfoType& aInfo) {
>+    MOZ_ASSERT(mObservers);
>+    mObservers->Broadcast(aInfo);
>+  }
>+
>+protected:
>+  virtual void EnableNotifications() = 0;
>+  virtual void DisableNotifications() = 0;
>+  virtual void PostRemoveObserver() {}

This PostRemoveObservers() call was confusing to me.  But basically, it's just signaling that DisableNotificaitons() was called, right?  Then we should call it OnNotificationsDisabled, or PostNotificationsDisabled or something.

>+void
>+AcquireWakeLock(const nsAString &aTopic, bool aHidden)
>+{
>+  AssertMainThread();
>+  PROXY_IF_SANDBOXED(AcquireWakeLock(aTopic, aHidden));
>+}
>+
>+void
>+ReleaseWakeLock(const nsAString &aTopic, bool aHidden)
>+{
>+  AssertMainThread();
>+  PROXY_IF_SANDBOXED(ReleaseWakeLock(aTopic, aHidden));
>+}

Would you mind changing these bools to 

enum WakeLockState {
  WAKE_LOCK_STATE_UNLOCKED,
  WAKE_LOCK_STATE_HIDDEN,
  WAKE_LOCK_STATE_VISIBLE
};

?

>@@ -222,16 +223,79 @@ void NotifyNetworkChange(const hal::Netw
>  */
> void Reboot();
> 
> /**
>  * Power off the device.
>  */
> void PowerOff();
> 
>+/**
>+ * Enable wake lock notifications from the backend
>+ *
>+ * This method is only visible from implementation of power manager service.
>+ * Rest of the system should not try this.
>+ */
>+void EnableWakeLockNotifications();
>+
>+/**
>+ * Enable wake lock notifications from the backend
>+ *
>+ * This method is only visible from implementation of power manager service.
>+ * Rest of the system should not try this.
>+ */
>+void DisableWakeLockNotifications();

Please end the first sentence with a period.

The method is /visible/ outside the power manager service.  But it shouldn't be /used/ from elsewhere...

(Actually, it shouldn't be used outside the WakeLockObserversManager, not the power manager service, right?)

>+ * Increase the wake lock count of aTopic

Nit: Period at end of sentence.

>+ * Decrease the wake lock count of aTopic
>+ * Mark one wake lock of aTopic as hidden
>+ * Mark one wake lock of aTopic as visible
>+ * Query the wake lock numbers of aTopic

Periods here.

>diff --git a/hal/Makefile.in b/hal/Makefile.in
>--- a/hal/Makefile.in
>+++ b/hal/Makefile.in
>@@ -70,42 +70,47 @@ CPPSRCS = \
>   SandboxHal.cpp \
>   WindowIdentifier.cpp \
>   $(NULL)
> 
> ifeq (android,$(MOZ_WIDGET_TOOLKIT))
> CPPSRCS += \
>   AndroidHal.cpp \
>   AndroidSensor.cpp \
>+  FallbackWakeLock.cpp \
>   $(NULL)
> else ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
> CPPSRCS += \
>   GonkHal.cpp \
>   Power.cpp \
>   GonkSensor.cpp \
>+  FallbackWakeLock.cpp \
>   $(NULL)
> else ifeq (Linux,$(OS_TARGET))
> CPPSRCS += \
>   LinuxHal.cpp \
>   FallbackSensor.cpp \
>   Power.cpp \
>+  FallbackWakeLock.cpp \
>   $(NULL)
> ifdef MOZ_ENABLE_DBUS
> CPPSRCS += UPowerClient.cpp
> endif
> else ifeq (WINNT,$(OS_TARGET))
> CPPSRCS += \
>   WindowsHal.cpp \
>   WindowsBattery.cpp \
>   FallbackSensor.cpp \
>+  FallbackWakeLock.cpp \
>   $(NULL)
> else
> CPPSRCS += \
>   FallbackHal.cpp \
>   FallbackSensor.cpp \
>+  FallbackWakeLock.cpp \
>   $(NULL)
> endif

Just put FallbackWakeLock.cpp in the unconditional CPPSRCs above.  And maybe rename it WakeLock.cpp or HalWakeLock.cpp, since it's fully-functional (i.e., it's not a fallback).

>+static int sActiveChildren = 0;
>+static nsAutoPtr<nsDataHashtable<nsStringHashKey, PRUint32> > sLockTable;
>+static nsAutoPtr<nsDataHashtable<nsStringHashKey, PRUint32> > sHiddenTable;

I'd prefer if this was one hash table storing a struct of {lock,hidden}.

>+void
>+ReleaseWakeLock(const nsAString &aTopic, bool hidden)
>+{
>+  if (!sInitialized) {
>+    Init();
>+  }
>+
>+  PRUint32 hiddenLocks = 0;
>+  sHiddenTable->Get(aTopic, &hiddenLocks);
>+  if (hidden) {
>+    if (hiddenLocks) {
>+      hiddenLocks--;
>+      sHiddenTable->Put(aTopic, hiddenLocks);
>+    } else {
>+      sHiddenTable->Remove(aTopic);
>+    }

Shouldn't we just MOZ_ASSERT(hiddenLocks)?

>+  PRUint32 locks = 0;
>+  sLockTable->Get(aTopic, &locks);
>+  if (locks) {
>+    locks--;
>+    sLockTable->Put(aTopic, locks);
>+  } else {
>+    sLockTable->Remove(aTopic);
>+  }

Similarly here, shouldn't we just assert(locks)?

>+  bool lastLock = (locks == 0);
>+  bool lastForeground = (!hidden && locks == hiddenLocks);
>+
>+  if (sActiveChildren && (lastLock || lastForeground)) {
>+    WakeLockInformation info;
>+    info.locks() = locks;
>+    info.hidden() = hiddenLocks;
>+    info.topic() = aTopic;
>+    NotifyWakeLockChange(info);

Perhaps you should factor this out?

>+void
>+HideOneWakeLock(const nsAString &aTopic)
>+{
>+  PRUint32 hiddenLocks = 0;
>+  sHiddenTable->Get(aTopic, &hiddenLocks);
>+  hiddenLocks++;
>+  sHiddenTable->Put(aTopic, hiddenLocks);
>+
>+  PRUint32 locks = 0;
>+  sLockTable->Get(aTopic, &locks);

We should assert that hiddenLocks <= locks.

>+  if (sActiveChildren && (locks == hiddenLocks)) {
>+    WakeLockInformation info;
>+    info.locks() = locks;
>+    info.hidden() = hiddenLocks;
>+    info.topic() = aTopic;
>+    NotifyWakeLockChange(info);
>+  }
>+}

>+void
>+ShowOneWakeLock(const nsAString &aTopic)
>+{
>+  PRUint32 hiddenLocks = 0;
>+  sHiddenTable->Get(aTopic, &hiddenLocks);
>+  hiddenLocks--;
>+  sHiddenTable->Put(aTopic, hiddenLocks);

Assert that hiddenLocks > 0 before subtracting.

>+  PRUint32 locks = 0;
>+  sLockTable->Get(aTopic, &locks);
>+  if (sActiveChildren && ((locks - hiddenLocks) == 1)) {
>+    WakeLockInformation info;
>+    info.locks() = locks;
>+    info.hidden() = hiddenLocks;
>+    info.topic() = aTopic;
>+    NotifyWakeLockChange(info);
>+  }
>+}

>+void
>+GetWakeLockInfo(const nsAString &aTopic, WakeLockInformation *aWakeLockInfo)
>+{
>+  if (!sInitialized) {
>+    Init();
>+  }
>+
>+  PRUint32 hiddenLocks = 0;
>+  sHiddenTable->Get(aTopic, &hiddenLocks);
>+
>+  PRUint32 locks = 0;
>+  sLockTable->Get(aTopic, &locks);

Just to be paranoid, we should probably assert here that hiddenLocks <= locks.

>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl
>--- a/hal/sandbox/PHal.ipdl
>+++ b/hal/sandbox/PHal.ipdl
>@@ -76,24 +76,33 @@ namespace hal {
> 
> namespace hal {
>   struct NetworkInformation {
>     double bandwidth;
>     bool   canBeMetered;
>   };
> }
> 
>+namespace hal {
>+  struct WakeLockInformation {
>+    uint32_t locks;
>+    uint32_t hidden;
>+    nsString topic;
>+  };

I'd prefer numLocks and numHidden because hidden() looks like it should return a bool.

>+    AcquireWakeLock(nsString aTopic, bool hidden);
>+    ReleaseWakeLock(nsString aTopic, bool hidden);
>+    HideOneWakeLock(nsString aTopic);
>+    ShowOneWakeLock(nsString aTopic);

If you wanted to be clever, this could be just one function:

  ModifyWakeLock(nsString topic, int lockAdjust, int hiddenAdjust)

where lockAdjust and hiddenLockAdjust are -1, 0, or 1.

Actually, that would simplify the FallbackWakeLock code a lot...

I'd be OK if the public hal interface were ModifyWakeLock (it's a low-level interface, after all), but I'm also OK with leaving the public interface as-is and translating those calls into ModifyWakeLock().

r- just because I'd like to look at the ModifyWakeLock code, however you decide to do it.

I'll go look at the test now.