830660 - Implement trusted screen lock

Assignee

Description

•

11 years ago

We need to implement some kind of trusted lock to inhibit screensaver, see bug 811261.

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 1

•

11 years ago

Justin, can you please provide some hints how to implement the trusted lock API? As we discussed at Bug 811261. Thanks!

Justin Lebar (not reading bugmail)

Comment 2

•

11 years ago

If I were doing this, I'd do something like

* Add a NewTrustedWakeLock() method to nsIPowerManagerService.
* Add a "trusted" property to the wake lock objects.
* Track this bit in the wake lock back-end.
* Report this bit in the wake lock status object.

Is that too high-level?

Tyler Downer [He/Him]

Updated

•

11 years ago

Whiteboard: [Advo]

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 3

•

11 years ago

Ahh, looks like I missed your reply. Thanks for the list, I'll try to come with something.

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 4

•

11 years ago

What do you mean with "Report this bit in the wake lock status object"? Do you mean to add it to the WakeLockInformation structure and report it as substring of aState returned by PowerManagerService::ComputeWakeLockState()?

Justin Lebar (not reading bugmail)

Comment 5

•

11 years ago

> Do you mean to add it to the WakeLockInformation structure and report it as substring of aState 
> returned by PowerManagerService::ComputeWakeLockState()?

Yes, although using substrings like that doesn't really scale now that we want to return three bits of information ("locked/unlocked", "background/foreground", "trusted/untrusted").

Instead of returning a string we should probably be returning some sort of object to the webpage.

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 6

•

11 years ago

Okay, I guess I have the API implemented, but how to ensure that the NewTrustedWakeLock() is called from Gecko only? I suppose the NewTrustedWakeLock() should do some check that it's called from gecko code, right?

Justin Lebar (not reading bugmail)

Comment 7

•

11 years ago

(In reply to Martin Stránský from comment #6)
> Okay, I guess I have the API implemented, but how to ensure that the
> NewTrustedWakeLock() is called from Gecko only? I suppose the
> NewTrustedWakeLock() should do some check that it's called from gecko code,
> right?

If you don't expose it to the web, then the web can't call it.  So I think perhaps we don't mean the same thing by "gecko code".

Feel free to attach a patch, but I'm swamped with critical b2g work for the next few days at least.

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 8

•

11 years ago

Thanks. 

Another question is what shall we do if there are two hal locks with the same topic but different trusted flags. I guess we should hash/search the hal lock hash table (sLockTable) for both, topic and trusted, right?

Justin Lebar (not reading bugmail)

Comment 9

•

11 years ago

> Another question is what shall we do if there are two hal locks with the same topic but different 
> trusted flags.

Sorry, I don't have a good enough model of your implementation in my mind to give decent advice here.

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 10

•

11 years ago

Attached patch patch (obsolete) — Details — Splinter Review

Implementation of the trusted wake locks. The status string is replaced by nsDOMMozWakeLockState, the hash table key is composed from topic+trusted status so trusted and untrusted locks are not mixed.

Attachment #745110 - Flags: review?(justin.lebar+bug)

Justin Lebar (not reading bugmail)

Comment 11

•

11 years ago

I've got some B2G blockers on my plate, which means I probably won't be able to give this the attention it deserves for a few days.  I won't forget about you, though.

Justin Lebar (not reading bugmail)

Comment 12

•

11 years ago

Comment on attachment 745110 [details] [diff] [review]
patch

This is a very good start!

>diff --git a/dom/power/nsIDOMPowerManager.idl b/dom/power/nsIDOMPowerManager.idl
>--- a/dom/power/nsIDOMPowerManager.idl
>+++ b/dom/power/nsIDOMPowerManager.idl

>@@ -24,29 +25,20 @@ interface nsIDOMMozPowerManager : nsISup
>      *  - locked and visible
>      */
>     void    addWakeLockListener(in nsIDOMMozWakeLockListener aListener);
>     void    removeWakeLockListener(in nsIDOMMozWakeLockListener aListener);
> 
>     /**
>      * Query the wake lock state of the topic.
>      *
>-     * Possible states are:
>-     *
>-     *  - "unlocked" - nobody holds the wake lock.
>-     *
>-     *  - "locked-foreground" - at least one window holds the wake lock,
>-     *    and it is visible.
>-     *
>-     *  - "locked-background" - at least one window holds the wake lock,
>-     *    but all of them are hidden.
>-     *
>      * @param aTopic The resource name related to the wake lock.
>+     * @param aState The wake lock state.
>      */
>-    DOMString getWakeLockState(in DOMString aTopic);
>+    void getWakeLockState(in DOMString aTopic, in nsIDOMMozWakeLockState aState);

Is there some reason we can't return nsIDOMMozWakeLockState?  Web interfaces
don't usually have outparams.  (And I now realize that this is both an outparam
and an inparam, or something, because we read aState.trusted?  That's not
idiomatic.)

We'll have to modify Gaia so it doesn't break when we make this change.

>diff --git a/dom/power/nsIDOMWakeLock.idl b/dom/power/nsIDOMWakeLock.idl
>--- a/dom/power/nsIDOMWakeLock.idl
>+++ b/dom/power/nsIDOMWakeLock.idl

>@@ -1,19 +1,20 @@
> /* -*- 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/. */
> 
> #include "nsISupports.idl"
> 
>-[scriptable, uuid(2e61eed1-5983-4562-8f26-fd361ab4a00d)]
>+[scriptable, uuid(5ac7ae2a-2cac-4df1-b975-5ba5a2b73c45)]
> interface nsIDOMMozWakeLock : nsISupports
> {
>     readonly attribute DOMString topic;
>+    readonly attribute boolean   trusted;

I think we should match nsIDOMEvent and call this isTrusted.

>diff --git a/dom/power/nsIDOMWakeLockListener.idl b/dom/power/nsIDOMWakeLockListener.idl
>--- a/dom/power/nsIDOMWakeLockListener.idl
>+++ b/dom/power/nsIDOMWakeLockListener.idl

>@@ -1,29 +1,36 @@
> /* -*- 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/. */
> 
> #include "nsISupports.idl"
> 
>-[scriptable, function, uuid(4e258af8-cffb-47bc-b16d-e8241243426e)]
>+/**
>+* Possible states are:
>+*
>+*  locked   - set true is somebody holds the wake lock.
>+*
>+*  visible  - true if at least one window holds the wake lock,
>+*             and it is visible.
>+*  trusted  - the lock is held by Gecko

Nit: Newline above "trusted".

Also, please indicate in a sentence or two what this interface is for.

>+*/
>+[scriptable, uuid(d07c8a71-1503-4eb9-8b6d-79c672daeb08)]
>+interface nsIDOMMozWakeLockState : nsISupports
>+{
>+  attribute boolean locked;
>+  attribute boolean hidden;
>+  attribute boolean trusted;
>+};

Could you please move this into a separate file?  I know it's annoying to have
so many files, but merging interfaces makes it hard to know which file the
interface is in, which is also annoying.

These attributes should also all be readonly.

>diff --git a/dom/power/nsIPowerManagerService.idl b/dom/power/nsIPowerManagerService.idl
>--- a/dom/power/nsIPowerManagerService.idl
>+++ b/dom/power/nsIPowerManagerService.idl

>@@ -32,17 +33,23 @@ interface nsIPowerManagerService : nsISu
> 
>   /**
>    * This API will restart the Gecko processes without powering off the machine.
>    */
>   void              restart();
> 
>   void              addWakeLockListener(in nsIDOMMozWakeLockListener aListener);
>   void              removeWakeLockListener(in nsIDOMMozWakeLockListener aListener);
>-  DOMString         getWakeLockState(in DOMString aTopic);
>+  void              getWakeLockState(in DOMString aTopic, in nsIDOMMozWakeLockState aState);

Again, can we return nsIDOMMozWakeLockState?

>   /**
>    * Return a wake lock object of aTopic associated with aWindow.
>    * A wake lock without associated window, e.g. used in chrome, is
>    * always considered invisible.
>    */
>   nsIDOMMozWakeLock newWakeLock(in DOMString aTopic, [optional] in nsIDOMWindow aWindow);
>+
>+  /**
>+   * The same as newWakeLock() but the result is a trusted wake lock, 
>+   * which is requested by some resources, e.g. a screensaver.
>+   */
>+  [noscript] nsIDOMMozWakeLock newTrustedWakeLock(in DOMString aTopic, [optional] in nsIDOMWindow aWindow);

This doesn't have to be noscript; this whole interface is Gecko-only (to a
first approximation: the name doesn't start with "nsIDOM").

Also, please wrap lines at 80 chars.  (I know we don't follow that rule in this
file, but still.)

>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl
>--- a/hal/sandbox/PHal.ipdl
>+++ b/hal/sandbox/PHal.ipdl

>-    sync GetWakeLockInfo(nsString aTopic)
>+    sync GetWakeLockInfo(nsString aTopic, bool aTrusted)
>       returns (WakeLockInformation aWakeLockInfo);

It's not clear to me why we pass aTrusted here, since the WakeLockInformation
will tell me whether the lock is trusted.  Maybe I'll understand later on.

If we have to send the aTrusted bit, I'd prefer that we did it with two methods:

  GetTrustedWakeLockInfo()
  GetUntrustedWakeLockInfo()

See my personal vendetta:
http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html

>diff --git a/dom/ipc/ProcessPriorityManager.cpp b/dom/ipc/ProcessPriorityManager.cpp
>--- a/dom/ipc/ProcessPriorityManager.cpp
>+++ b/dom/ipc/ProcessPriorityManager.cpp

>@@ -465,20 +465,20 @@ ParticularProcessPriorityManager::Init()
>     os->AddObserver(this, "remote-browser-frame-shown", /* ownsWeak */ false);
>     os->AddObserver(this, "ipc:browser-destroyed", /* ownsWeak */ false);
>     os->AddObserver(this, "frameloader-visible-changed", /* ownsWeak */ false);
>   }
> 
>   // This process may already hold the CPU lock; for example, our parent may
>   // have acquired it on our behalf.
>   WakeLockInformation info1, info2;
>-  GetWakeLockInfo(NS_LITERAL_STRING("cpu"), &info1);
>+  GetWakeLockInfo(NS_LITERAL_STRING("cpu"), false, &info1);
>   mHoldsCPUWakeLock = info1.lockingProcesses().Contains(ChildID());
> 
>-  GetWakeLockInfo(NS_LITERAL_STRING("high-priority"), &info2);
>+  GetWakeLockInfo(NS_LITERAL_STRING("high-priority"), false, &info2);

If we really /must/ have a boolean param, it needs to be commented as

    GetWakeLockInfo(..., /* aTrusted */ false, ...);

But I think this is moot.

>diff --git a/dom/power/PowerManager.h b/dom/power/PowerManager.h
>--- a/dom/power/PowerManager.h
>+++ b/dom/power/PowerManager.h

>@@ -9,16 +9,32 @@
> #include "nsTArray.h"
> #include "nsIDOMPowerManager.h"
> #include "nsIDOMWakeLockListener.h"
> #include "nsIDOMWindow.h"
> #include "nsWeakReference.h"
> 
> class nsPIDOMWindow;
> 
>+/* Header file */
>+class nsDOMMozWakeLockState : public nsIDOMMozWakeLockState

Please remove this comment; it's not adding anything.

>diff --git a/dom/power/PowerManager.cpp b/dom/power/PowerManager.cpp
>--- a/dom/power/PowerManager.cpp
>+++ b/dom/power/PowerManager.cpp

>@@ -17,16 +17,60 @@
> #include "nsError.h"
> 
> DOMCI_DATA(MozPowerManager, mozilla::dom::power::PowerManager)
> 
> namespace mozilla {
> namespace dom {
> namespace power {
> 
>+NS_IMPL_ISUPPORTS1(nsDOMMozWakeLockState, nsIDOMMozWakeLockState)
>+nsDOMMozWakeLockState::nsDOMMozWakeLockState(void)

We do foo(), not foo(void).

But anyway, I think we shouldn't have a void constructor; if possible, we
should set the state when we construct this.

>diff --git a/dom/power/PowerManagerService.h b/dom/power/PowerManagerService.h
>--- a/dom/power/PowerManagerService.h
>+++ b/dom/power/PowerManagerService.h

>@@ -47,22 +47,26 @@ public:
>    *  - The wake lock unlocks itself if the /given/ process dies, and
>    *  - The /given/ process shows up in WakeLockInfo::lockingProcesses.
>    *
>    */
>   already_AddRefed<nsIDOMMozWakeLock>
>   NewWakeLockOnBehalfOfProcess(const nsAString& aTopic,
>                                ContentParent* aContentParent);
> 
>+  already_AddRefed<nsIDOMMozWakeLock>
>+  NewTrustedWakeLockOnBehalfOfProcess(const nsAString& aTopic,
>+                                      ContentParent* aContentParent);
>+
> private:
> 
>   ~PowerManagerService();
> 
>   void ComputeWakeLockState(const hal::WakeLockInformation& aWakeLockInfo,
>-                            nsAString &aState);
>+                            nsIDOMMozWakeLockState *aState);

Please use an outparam (i.e., nsIDOMMozWakeLockState **aState).  Probably
better would be to return already_AddRefed<nsIDOMMozWakeLockState>.

>diff --git a/dom/power/PowerManagerService.cpp b/dom/power/PowerManagerService.cpp
>--- a/dom/power/PowerManagerService.cpp
>+++ b/dom/power/PowerManagerService.cpp

>@@ -59,48 +60,41 @@ PowerManagerService::Init()
> void
> PowerManagerService::ComputeWakeLockState(const WakeLockInformation& aWakeLockInfo,
>-                                          nsAString &aState)
>+                                          nsIDOMMozWakeLockState *aState)
> {
>-  WakeLockState state = hal::ComputeWakeLockState(aWakeLockInfo.numLocks(),
>+  WakeLockState state = hal::ComputeWakeLockState(aWakeLockInfo.trusted(),
>+                                                  aWakeLockInfo.numLocks(),
>                                                   aWakeLockInfo.numHidden());
>-  switch (state) {
>-  case WAKE_LOCK_STATE_UNLOCKED:
>-    aState.AssignLiteral("unlocked");
>-    break;
>-  case WAKE_LOCK_STATE_HIDDEN:
>-    aState.AssignLiteral("locked-background");
>-    break;
>-  case WAKE_LOCK_STATE_VISIBLE:
>-    aState.AssignLiteral("locked-foreground");
>-    break;
>-  }
>+  aState->SetLocked(state&WAKE_LOCK_STATE_LOCKED);
>+  aState->SetHidden(state&WAKE_LOCK_STATE_HIDDEN);
>+  aState->SetTrusted(state&WAKE_LOCK_STATE_TRUSTED);

Spaces around operators, please.

Also, using an int for WakeLockInformation isn't good.  It robs us of type
safety and pushes complexity onto consumers.

If you want to use flags, we could write a flags class, like

  class WakeLockState
  {
    bool IsLocked()
    {
      return mLocked;
    }

    bool IsHidden();
    bool IsTrusted();

  private:
    bool mLocked : 1;
    bool mHidden : 1;
    bool mTrusted : 1; 
  };

But it seems simpler to do

  nsRefPtr<nsDOMMozWakeLockState> state = new nsDOMMozWakeLockState(aWakeLockInfo);

and have nsDOMMozWakeLockState's constructor check the properties of
aWakeLockInfo to set its members.  Then we could stop exposing
ComputeWakeLockState outside of hal, which would be great.

> void
> PowerManagerService::Notify(const WakeLockInformation& aWakeLockInfo)
> {
>-  nsAutoString state;
>-  ComputeWakeLockState(aWakeLockInfo, state);
>+  nsDOMMozWakeLockState state;

It's illegal (and extremely dangerous) to declare an XPCOM object on the stack
like this.

The reason is, suppose ComputeWakeLockState (or any of the methods which
ComputeWakeLockState calls) does

  nsCOMPtr<nsIDOMMozWakeLockState> state = aState;

When this runs, the refcount of aState is increased from 0 to 1.  When |state|
goes out of scope, the refcount of aState will go down to 0.  This will cause
us to run |delete this| on *aState.  If aState is stack-allocated, that will be
a disaster.

>+  ComputeWakeLockState(aWakeLockInfo, &state);
> 
>   /**
>    * 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.
>    */
>   nsAutoTArray<nsCOMPtr<nsIDOMMozWakeLockListener>, 2> listeners(mWakeLockListeners);
> 
>   for (uint32_t i = 0; i < listeners.Length(); ++i) {
>-    listeners[i]->Callback(aWakeLockInfo.topic(), state);
>+    listeners[i]->Callback(aWakeLockInfo.topic(), &state);

And indeed, Callback() is arbitrary code, which may well addref and release state.

>@@ -170,20 +164,23 @@ PowerManagerService::AddWakeLockListener
> NS_IMETHODIMP
>-PowerManagerService::GetWakeLockState(const nsAString &aTopic, nsAString &aState)
>+PowerManagerService::GetWakeLockState(const nsAString &aTopic, nsIDOMMozWakeLockState *aState)
> {
>   WakeLockInformation info;
>-  GetWakeLockInfo(aTopic, &info);
>+  bool trusted;
>+
>+  aState->GetTrusted(&trusted);
>+  GetWakeLockInfo(aTopic, trusted, &info);

It is unidiomatic to use aState as both an in-param and an out-param; we should
not do that.  But I still don't understand why we have to pass |trusted| to
GetWakeLockInfo in the first place.

>@@ -205,11 +202,36 @@ PowerManagerService::NewWakeLockOnBehalf

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

    Init(aTopic, aWindow, /* trusted */ true);

or

    InitTrusted(aTopic, aWindow);

(And rename Init to InitUntrusted.)

>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIDOMMozWakeLock> wl(wakelock);
>+  wl.forget(aWakeLock);

You /may/ be able to do

    wakelock.forget(aWakeLock);

If that doesn't work, the nsCOMPtr thing is fine.  But please be consistent
with whether we use |nsRefPtr<Foo> foo(aFoo)| versus |nsRefPtr<Foo> foo =
aFoo|.  I prefer the second one.

>diff --git a/dom/power/WakeLock.h b/dom/power/WakeLock.h
>--- a/dom/power/WakeLock.h
>+++ b/dom/power/WakeLock.h

>@@ -35,31 +35,34 @@ public:
>   NS_DECL_NSIOBSERVER
> 
>   WakeLock();
>   virtual ~WakeLock();
> 
>   // Initialize this wake lock on behalf of the given window.  Null windows are
>   // allowed; a lock without an associated window is always considered
>   // invisible.
>-  nsresult Init(const nsAString &aTopic, nsIDOMWindow* aWindow);
>+  nsresult Init(const nsAString &aTopic, nsIDOMWindow* aWindow, 
>+                bool aTrusted = false);
> 
>   // Initialize this wake lock on behalf of the given process.  If the process
>   // dies, the lock is released.  A wake lock initialized via this method is
>   // always considered visible.
>-  nsresult Init(const nsAString &aTopic, ContentParent* aContentParent);
>+  nsresult Init(const nsAString &aTopic, ContentParent* aContentParent, 
>+                bool aTrusted = false);

Nit: Explicit is better than implicit, so don't make aTrusted have a default.

(We can apply a bit of Python's zen to our ugly C++.  :)
http://www.python.org/dev/peps/pep-0020/

>diff --git a/dom/power/WakeLock.cpp b/dom/power/WakeLock.cpp
>--- a/dom/power/WakeLock.cpp
>+++ b/dom/power/WakeLock.cpp

> nsresult
>-WakeLock::Init(const nsAString &aTopic, nsIDOMWindow *aWindow)
>+WakeLock::Init(const nsAString &aTopic, nsIDOMWindow *aWindow, bool aTrusted)

If we were going to leave the default param for aTrusted, we'd want to say
|bool aTrusted /* = false */| here, to be explicit about it.

>diff --git a/hal/HalWakeLock.h b/hal/HalWakeLock.h
>--- a/hal/HalWakeLock.h
>+++ b/hal/HalWakeLock.h

>@@ -4,23 +4,24 @@
>  * 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
>-};
>+#define WAKE_LOCK_STATE_TRUSTED (1<<0)
>+#define WAKE_LOCK_STATE_LOCKED  (1<<1)
>+#define WAKE_LOCK_STATE_HIDDEN  (1<<2)
>+
>+typedef int WakeLockState;

We avoid |int| wherever possible, because it has different widths on different
platforms.  (I have no idea why ComputeWakeLockState was using |int| before;
that's probably my mistake for not catching it in review.)  But I'd prefer not
to use flags like this anyway.

> /**
>  * Return the wake lock state according to the numbers.
>  */
>-WakeLockState ComputeWakeLockState(int aNumLocks, int aNumHidden);
>+WakeLockState ComputeWakeLockState(bool aTrusted, int aNumLocks, int aNumHidden);

Maybe we can simply get rid of this function; I think that might be best.  See
below.

> } // hal
> } // mozilla
> 
> #endif /* __HAL_WAKELOCK_H_ */

>+void
>+ComposeHashKey(const nsAString& aTopics, bool aTrusted, nsAString& aKey)
>+{
>+  aKey = aTrusted ? NS_LITERAL_STRING("T") : 
>+                    NS_LITERAL_STRING("U");
>+  aKey.Append(aTopics);
>+}
>+
>+bool
>+IsHashKeyTrusted(const nsAString& aKey)
>+{
>+  return (aKey.First() == 'T');
> }

These two methods should not be in the mozilla::hal namespace; they're static
to this file.

> } // hal
> } // mozilla
 
>@@ -176,95 +194,108 @@ EnableWakeLockNotifications()
>-  ProcessLockTable* table = sLockTable->Get(aTopic);
>+  nsString key;
>+  ComposeHashKey(aTopic, aTrusted, key);

It would be cleaner (although more code) to create a custom hash key, instead
of stuffing everything into a string.  But I don't think we're going to
structure the hashtable this way, anyway.

>-  WakeLockState newState = ComputeWakeLockState(totalCount.numLocks, totalCount.numHidden);
>+  WakeLockState newState = ComputeWakeLockState(aTrusted,
>+                                                totalCount.numLocks,
>+                                                totalCount.numHidden);

It's not clear to me that we need ComputeWakeLockState anymore; maybe we can
just make what we're doing here be a method on LockCount.

>   if (sActiveListeners && oldState != newState) {
>     WakeLockInformation info;
>-    GetWakeLockInfo(aTopic, &info);
>+    GetWakeLockInfo(aTopic, aTrusted, &info);
>     NotifyWakeLockChange(info);
>   }
> }
> 
> void
>-GetWakeLockInfo(const nsAString& aTopic, WakeLockInformation* aWakeLockInfo)
>+GetWakeLockInfo(const nsAString& aTopic, bool aTrusted, WakeLockInformation* aWakeLockInfo)
> {
>   if (sIsShuttingDown) {
>     NS_WARNING("You don't want to get wake lock information during xpcom-shutdown!");
>     return;
>   }
>   if (!sInitialized) {
>     Init();
>   }
> 
>-  ProcessLockTable* table = sLockTable->Get(aTopic);
>+  nsString key;
>+  ComposeHashKey(aTopic, aTrusted, key);

Aha, I finally understand the mysterious aTrusted param.  :)

It would be better to treat aTrusted like we treat visible/invisible, I think.
That is, you ask for the info of a topic, and you get back an object describing
all of the wake locks on that topic.

But then this brings up a problem.  Right now we return

  {
    locked: true/false,
    hidden: true/false,
    trusted: true/false
  }

But it's possible that we could have a trusted but hidden lock and an untrusted
and unhidden lock.  What we want to do under these circumstances depends on
whether we're interested in trusted locks or not.  It doesn't seem like this
struct is powerful enough to represent this information!  (Perhaps this is why
you chose to implement this function this way!)

It sounds like we need a more powerful interface.

Can we return something like

  {
    trusted: { locked, hidden }
    untrusted: { locked, hidden }
    all: { locked, hidden }
  }

?  An alternative would be to do something like return a list:

  [ { locked, hidden, trusted } ]

This would be nice because then we could e.g. include an origin in the list.
But we'd still want methods for summarizing the locked state, so maybe we
should go with something like the first idea.

Also, sorry, but I think I've bitrotted you here a bit.  You should update your
tree.

>diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp
>--- a/hal/sandbox/SandboxHal.cpp
>+++ b/hal/sandbox/SandboxHal.cpp

>   MOZ_ASSERT(aProcessID != CONTENT_PROCESS_ID_UNKNOWN);
>-  Hal()->SendModifyWakeLock(nsString(aTopic), aLockAdjust, aHiddenAdjust, aProcessID);
>+  Hal()->SendModifyWakeLock(nsString(aTopic), aTrusted, aLockAdjust, aHiddenAdjust, aProcessID);
> }

> void
>-GetWakeLockInfo(const nsAString &aTopic, WakeLockInformation *aWakeLockInfo)
>+GetWakeLockInfo(const nsAString &aTopic, bool aTrusted, WakeLockInformation *aWakeLockInfo)
> {
>-  Hal()->SendGetWakeLockInfo(nsString(aTopic), aWakeLockInfo);
>+  Hal()->SendGetWakeLockInfo(nsString(aTopic), aTrusted, aWakeLockInfo);
> }

>     // We allow arbitrary content to use wake locks.
>-    hal::ModifyWakeLock(aTopic, aLockAdjust, aHiddenAdjust, aProcessID);
>+    hal::ModifyWakeLock(aTopic, aTrusted, aLockAdjust, aHiddenAdjust, aProcessID);

>   virtual bool
>-  RecvGetWakeLockInfo(const nsString &aTopic, WakeLockInformation *aWakeLockInfo) MOZ_OVERRIDE
>+  RecvGetWakeLockInfo(const nsString &aTopic, const bool& aTrusted, WakeLockInformation *aWakeLockInfo) MOZ_OVERRIDE

Nit: Please wrap lines to 80 chars (even though we don't do it already; sorry).

Attachment #745110 - Flags: review?(justin.lebar+bug) → feedback+

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 13

•

11 years ago

Attached patch patch v.2 (obsolete) — Details — Splinter Review

Thanks!

This one should address all your remarks except the:

>>+  NS_ENSURE_SUCCESS(rv, rv);
>>+
>>+  nsCOMPtr<nsIDOMMozWakeLock> wl(wakelock);
>>+  wl.forget(aWakeLock);
> You /may/ be able to do
>
>    wakelock.forget(aWakeLock);
>
>If that doesn't work, the nsCOMPtr thing is fine.  But please be consistent
>with whether we use |nsRefPtr<Foo> foo(aFoo)| versus |nsRefPtr<Foo> foo =
>aFoo|.  I prefer the second one.

NewTrustedWakeLock() a 1:1 copy of NewWakeLock() so I preffer to use the same code to aviod confusion, because those functions are identical except the trusted parameter.

Attachment #745110 - Attachment is obsolete: true

Attachment #753207 - Flags: review?(justin.lebar+bug)

Martin Stránský [:stransky] (ni? me)

Assignee

Updated

•

11 years ago

Attachment #745110 - Attachment is obsolete: false

Justin Lebar (not reading bugmail)

Updated

•

11 years ago

Attachment #745110 - Attachment is obsolete: true

Justin Lebar (not reading bugmail)

Comment 14

•

11 years ago

btw we'll have to change all our Gaia consumers before we can land this.  I'm
not worried about changing the API after b2g 1.0 because it's highly-privileged
and not available to the vast majority of apps.

We also need to convert our existing wake locks to trusted wake locks as
appropriate.  We can do this in a separate patch, but it should probably be the
same bug, because we don't want to land this without also marking existing wake
locks as trusted.

>diff --git a/dom/power/nsIDOMWakeLockState.idl b/dom/power/nsIDOMWakeLockState.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/power/nsIDOMWakeLockState.idl

>+[scriptable, uuid(d07c8a71-1503-4eb9-8b6d-79c672daeb08)]
>+interface nsIDOMMozWakeLockState : nsISupports
>+{
>+  /* The states mean:
>+   *
>+   *  - WAKE_LOCK_STATE_UNLOCKED - nobody holds the wake lock.
>+   *
>+   *  - WAKE_LOCK_STATE_VISIBLE - at least one window holds the wake lock,
>+   *    and it is visible.
>+   *
>+   *  - WAKE_LOCK_STATE_HIDDEN - at least one window holds the wake lock,
>+   *    but all of them are hidden.
>+   */
>+  const unsigned long WAKE_LOCK_STATE_UNLOCKED = 0;
>+  const unsigned long WAKE_LOCK_STATE_HIDDEN   = 1;
>+  const unsigned long WAKE_LOCK_STATE_VISIBLE  = 2;
>+
>+  readonly attribute long stateTrusted;
>+  readonly attribute long stateUntrusted;
>+  readonly attribute long stateAll;

That this is a DOM API, for use by webpages.  I don't think we want to use
magic constants there, like we do in e.g. XHR's readyState.  I'd prefer for us
to use strings.

stateTrusted &co need comments explaining what they are.

How would you feel if we called these |state|, |trustedState|, and
|untrustedState|?

I'm also kind of tempted to say that we should remove |untrustedState|.  I
can't think of a case when you'd want to read that, and if you really care
about it, you can compute it from |state| and |trustedState|.  What do you
think?

>diff --git a/dom/power/nsIPowerManagerService.idl b/dom/power/nsIPowerManagerService.idl
>--- a/dom/power/nsIPowerManagerService.idl
>+++ b/dom/power/nsIPowerManagerService.idl

>+  /**
>+   * The same as newWakeLock() but the result is a trusted wake lock,
>+   * which is requested by some resources, e.g. a screensaver.
>+   */

I don't understand the second line of this comment.

Maybe we can simply say "which affects the trustedState field of
nsIDOMMozWakeLockState."

>+  nsIDOMMozWakeLock newTrustedWakeLock(in DOMString aTopic, 
>+                                       [optional] in nsIDOMWindow aWindow);
> };

>diff --git a/dom/power/PowerManager.h b/dom/power/PowerManager.h
>--- a/dom/power/PowerManager.h
>+++ b/dom/power/PowerManager.h

>@@ -4,25 +4,46 @@
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> #ifndef mozilla_dom_power_PowerManager_h
> #define mozilla_dom_power_PowerManager_h
> 
> #include "nsCOMPtr.h"
> #include "nsTArray.h"
> #include "nsIDOMPowerManager.h"
> #include "nsIDOMWakeLockListener.h"
>+#include "nsIDOMWakeLockState.h"
> #include "nsIDOMWindow.h"
> #include "nsWeakReference.h"
>+#include "Types.h"
> 
> class nsPIDOMWindow;
> 
> namespace mozilla {
> namespace dom {
> namespace power {
> 
>+class nsDOMMozWakeLockState : public nsIDOMMozWakeLockState

I'd prefer if this had its own header and cpp files (for the same reason as the
interface should be in its own file.)

>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIDOMMOZWAKELOCKSTATE
>+
>+  nsDOMMozWakeLockState(const hal::WakeLockInformation& aWakeLockInfo);
>+  virtual ~nsDOMMozWakeLockState() {};
>+
>+private:
>+  // Overall locks count (untrusted + trusted)
>+  uint32_t mLocksCount;
>+  uint32_t mHiddenCount;
>+
>+  // Trusted locks count
>+  uint32_t mTrustedCount;
>+  uint32_t mTrustedHiddenCount;

s/locks count/lock count/

>diff --git a/dom/power/PowerManager.cpp b/dom/power/PowerManager.cpp
>--- a/dom/power/PowerManager.cpp
>+++ b/dom/power/PowerManager.cpp

>@@ -17,16 +17,71 @@
> #include "nsError.h"
> 
> DOMCI_DATA(MozPowerManager, mozilla::dom::power::PowerManager)
> 
> namespace mozilla {
> namespace dom {
> namespace power {
> 
>+NS_IMPL_ISUPPORTS1(nsDOMMozWakeLockState, nsIDOMMozWakeLockState)
>+nsDOMMozWakeLockState::nsDOMMozWakeLockState(const hal::WakeLockInformation& aWakeLockInfo)

Newline between these two lines, please.

>+{
>+  mLocksCount = aWakeLockInfo.numLocks();
>+  mHiddenCount = aWakeLockInfo.numHidden();
>+  mTrustedCount = aWakeLockInfo.numTrusted();
>+  mTrustedHiddenCount = aWakeLockInfo.numTrustedHidden();
>+}

Please use an initialization list.  Then we should be able to make these
members const.

>+/* readonly attribute long stateTrusted; */
>+NS_IMETHODIMP nsDOMMozWakeLockState::GetStateTrusted(int32_t *aStateTrusted)

Preferred style these days is int32_t* aFoo.

>+{
>+  if (mTrustedCount == 0) {
>+    *aStateTrusted = WAKE_LOCK_STATE_UNLOCKED;
>+  } else if (mTrustedCount == mTrustedHiddenCount) {
>+    *aStateTrusted = WAKE_LOCK_STATE_HIDDEN;
>+  } else {
>+    *aStateTrusted = WAKE_LOCK_STATE_VISIBLE;
>+  }
>+  return NS_OK;
>+}
>+
>+/* readonly attribute long stateUntrusted; */
>+NS_IMETHODIMP nsDOMMozWakeLockState::GetStateUntrusted(int32_t *aStateUntrusted)
>+{
>+  int32_t untrustedCount = (mLocksCount - mHiddenCount);

Shouldn't this be |mLocksCount - mTrustedCount|?

>+  if (untrustedCount == 0) {
>+    *aStateUntrusted = WAKE_LOCK_STATE_UNLOCKED;
>+  } 
>+  else {

Preferred style these days is } else {.

We also prefer early returns to |else|'s where feasible, so you can do that
here.

>diff --git a/dom/power/PowerManagerService.cpp b/dom/power/PowerManagerService.cpp
>--- a/dom/power/PowerManagerService.cpp
>+++ b/dom/power/PowerManagerService.cpp

>+already_AddRefed<nsIDOMMozWakeLockState>
>+PowerManagerService::ComputeWakeLockState(const WakeLockInformation& aWakeLockInfo)

Do we still need this method?  Can we instead just call |new
nsDOMMozWakeLockState| in the two places where we call this method currently?

>diff --git a/hal/Hal.h b/hal/Hal.h
>--- a/hal/Hal.h
>+++ b/hal/Hal.h

> /**
>- * Query the wake lock numbers of aTopic.
>+ * Query the wake lock numbers of aTopic, look up untrusted locks only.
>  * @param aTopic        lock topic
>  * @param aWakeLockInfo wake lock numbers
>  */

GetWakeLockInfo only gets the untrusted info?  Why is that?

>-void GetWakeLockInfo(const nsAString &aTopic, hal::WakeLockInformation *aWakeLockInfo);
>+void GetWakeLockInfo(const nsAString &aTopic, 
>+                     hal::WakeLockInformation *aWakeLockInfo);

>diff --git a/hal/HalWakeLock.cpp b/hal/HalWakeLock.cpp
>--- a/hal/HalWakeLock.cpp
>+++ b/hal/HalWakeLock.cpp

> void
>-ModifyWakeLock(const nsAString& aTopic,
>+ModifyWakeLock(const nsAString& aTopic,               
>+               bool aTrusted,
>                hal::WakeLockControl aLockAdjust,
>                hal::WakeLockControl aHiddenAdjust,
>                uint64_t aProcessID)
> {
>   MOZ_ASSERT(NS_IsMainThread());
>   MOZ_ASSERT(aProcessID != CONTENT_PROCESS_ID_UNKNOWN);
> 
>   if (sIsShuttingDown) {

>@@ -221,44 +215,59 @@ ModifyWakeLock(const nsAString& aTopic,
>     table->Init();
>     sLockTable->Put(aTopic, table);
>   } else {
>     table->Get(aProcessID, &processCount);
>     table->EnumerateRead(CountWakeLocks, &totalCount);
>   }
> 
>   MOZ_ASSERT(processCount.numLocks >= processCount.numHidden);
>+  MOZ_ASSERT(processCount.numLocks >= processCount.numTrusted);
>   MOZ_ASSERT(aLockAdjust >= 0 || processCount.numLocks > 0);
>-  MOZ_ASSERT(aHiddenAdjust >= 0 || processCount.numHidden > 0);
>+  MOZ_ASSERT(aLockAdjust >= 0 || processCount.numTrusted > 0);
>+  MOZ_ASSERT(aHiddenAdjust >= 0 || (aTrusted) ?
>+                                   (processCount.numTrustedHidden > 0):
>+                                   (processCount.numHidden > 0));
>   MOZ_ASSERT(totalCount.numLocks >= totalCount.numHidden);
>+  MOZ_ASSERT(totalCount.numLocks >= totalCount.numTrusted);
>   MOZ_ASSERT(aLockAdjust >= 0 || totalCount.numLocks > 0);
>-  MOZ_ASSERT(aHiddenAdjust >= 0 || totalCount.numHidden > 0);
>+  MOZ_ASSERT(aLockAdjust >= 0 || totalCount.numTrusted > 0);
>+  MOZ_ASSERT(aHiddenAdjust >= 0 || (aTrusted) ?
>+                                   (totalCount.numTrustedHidden > 0):
>+                                   (totalCount.numHidden > 0));

It's probably clearer to use MOZ_ASSERT_IF for the complex aTrusted assertions.
For example,

  MOZ_ASSERT_IF(aTrusted && aLockAdjust < 0, totalCount.numTrusted > 0);

>-  WakeLockState oldState = ComputeWakeLockState(totalCount.numLocks, totalCount.numHidden);
>+  LockCount oldCount = totalCount;
>+
>   bool processWasLocked = processCount.numLocks > 0;
> 
>   processCount.numLocks += aLockAdjust;
>   processCount.numHidden += aHiddenAdjust;
>-
>   totalCount.numLocks += aLockAdjust;
>   totalCount.numHidden += aHiddenAdjust;
> 
>+  if (aTrusted) {
>+    processCount.numTrusted += aLockAdjust;
>+    processCount.numTrustedHidden += aHiddenAdjust;
>+    totalCount.numTrusted += aLockAdjust;
>+    totalCount.numTrustedHidden += aHiddenAdjust;
>+  }
>+
>   if (processCount.numLocks) {
>     table->Put(aProcessID, processCount);
>   } else {
>     table->Remove(aProcessID);
>   }
>   if (!totalCount.numLocks) {
>     sLockTable->Remove(aTopic);
>   }
> 
>   if (sActiveListeners &&
>-      (oldState != ComputeWakeLockState(totalCount.numLocks,
>-                                        totalCount.numHidden) ||
>-       processWasLocked != (processCount.numLocks > 0))) {
>+      ((oldCount.numLocks != totalCount.numLocks ||
>+        oldCount.numHidden != totalCount.numHidden) ||
>+        processWasLocked != (processCount.numLocks > 0))) {

Boolean-or is associative, so no parens are needed in (a || b) || c.

But also this isn't the right logic, I think.

This fires a notification whenever the number of locks changes, which should be
almost every time this method is run (unless aLockAdjust == aHiddenAdjust == 0).

But we want to fire a notification whenever the nsIDOMMozWakeLockState for this
wake lock changes.

This patch is good, but I'd like to take another look.

Justin Lebar (not reading bugmail)

Updated

•

11 years ago

Attachment #753207 - Flags: review?(justin.lebar+bug) → review-

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 15

•

11 years ago

Attached patch patch v.3 (obsolete) — Details — Splinter Review

Thanks, I hope this one address that. Some notes:

> I'm also kind of tempted to say that we should remove |untrustedState|.  I
> can't think of a case when you'd want to read that, and if you really care
> about it, you can compute it from |state| and |trustedState|.  What do you
> think?

It's not so easy to extract the untrustedState but I can't imagine why we should need that after all. So |state| and |trustedState| are perfectly fine for me.

>diff --git a/hal/Hal.h b/hal/Hal.h
>--- a/hal/Hal.h
>+++ b/hal/Hal.h
> /**
>- * Query the wake lock numbers of aTopic.
>+ * Query the wake lock numbers of aTopic, look up untrusted locks only.
>  * @param aTopic        lock topic
>  * @param aWakeLockInfo wake lock numbers
>  */
> GetWakeLockInfo only gets the untrusted info?  Why is that?

Juts a leftover, it returns all wake locks.

> This fires a notification whenever the number of locks changes, which should be
> almost every time this method is run (unless aLockAdjust == aHiddenAdjust == 0).
> But we want to fire a notification whenever the nsIDOMMozWakeLockState for this
> wake lock changes.

Ahh, okay. I implemented a local WakeLockState enum for that but if you find it confusing we can change the name.

Attachment #753207 - Attachment is obsolete: true

Attachment #764080 - Flags: review?(justin.lebar+bug)

Justin Lebar (not reading bugmail)

Comment 16

•

11 years ago

Have you started implementing the thing that you want to use the trusted screen lock?  It might be prudent to start.  I'd prefer not to land this until we also have a consumer ready to land.

You'll also need to prepare patches changing Gaia (and any other consumers of the wake lock notification API).

We also need tests here.

Justin Lebar (not reading bugmail)

Comment 17

•

11 years ago

Comment on attachment 764080 [details] [diff] [review]
patch v.3

It looks like the definition of nsDOMMozWakeLockState is missing from this patch.  Unless that landed somewhere else in the meantime?

Attachment #764080 - Flags: review?(justin.lebar+bug) → review-

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 18

•

11 years ago

Attached patch patch, v4 — Details — Splinter Review

Ahh, sorry, I didn't add those files to hg so the patch is incomplete. This one contains them.

> Have you started implementing the thing that you want to use the trusted
> screen lock?  It might be prudent to start.  I'd prefer not to land this
> until we also have a consumer ready to land.
> 
> You'll also need to prepare patches changing Gaia (and any other consumers
> of the wake lock notification API).
> 
> We also need tests here.

I'm going to work on that, but let's finish one thing (this patch) and then we can base on it. I think the implementation would be easy, but we need a finished API for that.

Thanks!

Attachment #764080 - Attachment is obsolete: true

Attachment #765854 - Flags: review?(justin.lebar+bug)

Justin Lebar (not reading bugmail)

Comment 19

•

11 years ago

I'm sorry for taking so long here; I've been traveling for work, but I've also been tasked with some drop-everything-else B2G work.  I'll get to this review as soon as I can; sorry again for making you wait.

Justin Lebar (not reading bugmail)

Comment 20

•

11 years ago

B2G has finally calmed down, so I'm coming back to take a look at this.

First things first, there are some whitespace errors.  :)

$ git apply <(xclip -o)
/dev/fd/63:438: trailing whitespace.
  nsresult Init(const nsAString &aTopic, nsIDOMWindow* aWindow, 
/dev/fd/63:445: trailing whitespace.
  nsresult Init(const nsAString &aTopic, ContentParent* aContentParent, 
/dev/fd/63:794: trailing whitespace.
  nsIDOMMozWakeLock newTrustedWakeLock(in DOMString aTopic, 
/dev/fd/63:867: trailing whitespace.
void GetWakeLockInfo(const nsAString &aTopic, 
/dev/fd/63:950: trailing whitespace.
    return WAKE_LOCK_STATE_UNLOCKED;

Justin Lebar (not reading bugmail)

Comment 21

•

11 years ago

Comment on attachment 765854 [details] [diff] [review]
patch, v4

This looks very good to me; I'm sorry again for keeping you waiting for so
long.

The good news is that your patch doesn't seem to have rotted significantly in
the past few months; the only conflicts I got were in Makefiles.

>diff --git a/dom/power/nsIDOMWakeLockState.idl b/dom/power/nsIDOMWakeLockState.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/power/nsIDOMWakeLockState.idl

>@@ -0,0 +1,24 @@
>+/* -*- 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/. */
>+
>+#include "nsISupports.idl"
>+
>+[scriptable, uuid(d07c8a71-1503-4eb9-8b6d-79c672daeb08)]
>+interface nsIDOMMozWakeLockState : nsISupports
>+{
>+  /* Possible states are:
>+   *
>+   *  - "unlocked" - nobody holds the wake lock.
>+   *
>+   *  - "locked-foreground" - at least one window holds the wake lock,
>+   *    and it is visible.
>+   *
>+   *  - "locked-background" - at least one window holds the wake lock,
>+   *    but all of them are hidden.
>+   *
>+   */
>+  readonly attribute DOMString state;
>+  readonly attribute DOMString stateTrusted;
>+};

Please add some more documentation here; it's not clear how |state| and
|stateTrusted| behave.

>diff --git a/dom/power/nsIPowerManagerService.idl b/dom/power/nsIPowerManagerService.idl
>--- a/dom/power/nsIPowerManagerService.idl
>+++ b/dom/power/nsIPowerManagerService.idl

>@@ -32,17 +33,24 @@ interface nsIPowerManagerService : nsISu
> 
>   /**
>    * This API will restart the Gecko processes without powering off the machine.
>    */
>   void              restart();
> 
>   void              addWakeLockListener(in nsIDOMMozWakeLockListener aListener);
>   void              removeWakeLockListener(in nsIDOMMozWakeLockListener aListener);
>-  DOMString         getWakeLockState(in DOMString aTopic);
>+  nsIDOMMozWakeLockState getWakeLockState(in DOMString aTopic);

/me grumbles about this silly indentation, which always gets messed up in this
way...

>   /**
>    * Return a wake lock object of aTopic associated with aWindow.
>    * A wake lock without associated window, e.g. used in chrome, is
>    * always considered invisible.
>    */
>   nsIDOMMozWakeLock newWakeLock(in DOMString aTopic, [optional] in nsIDOMWindow aWindow);
>+
>+  /**
>+   * The same as newWakeLock() but the result is a trusted wake lock,
>+   * which affects the trustedState field of nsIDOMMozWakeLockState.
>+   */

It's called "stateTrusted" now.

>+  nsIDOMMozWakeLock newTrustedWakeLock(in DOMString aTopic, 
>+                                       [optional] in nsIDOMWindow aWindow);
> };

>diff --git a/dom/power/PowerManager.h b/dom/power/PowerManager.h
>--- a/dom/power/PowerManager.h
>+++ b/dom/power/PowerManager.h

>@@ -4,18 +4,20 @@
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> #ifndef mozilla_dom_power_PowerManager_h
> #define mozilla_dom_power_PowerManager_h
> 
> #include "nsCOMPtr.h"
> #include "nsTArray.h"
> #include "nsIDOMPowerManager.h"
> #include "nsIDOMWakeLockListener.h"
>+#include "nsIDOMWakeLockState.h"
> #include "nsIDOMWindow.h"
> #include "nsWeakReference.h"
>+#include "Types.h"
> 
> class nsPIDOMWindow;
> 
> namespace mozilla {
> namespace dom {
> namespace power {
> 
> class PowerManager

It's odd to me that you have to add includes to this header; I'd expect the
forward-declarations in the IDL file to be sufficient.  But whatever.

>diff --git a/dom/power/PowerManagerService.h b/dom/power/PowerManagerService.h
>--- a/dom/power/PowerManagerService.h
>+++ b/dom/power/PowerManagerService.h

>@@ -8,16 +8,17 @@
> #include "nsCOMPtr.h"
> #include "nsDataHashtable.h"
> #include "nsHashKeys.h"
> #include "nsTArray.h"
> #include "nsIPowerManagerService.h"
> #include "mozilla/Observer.h"
> #include "Types.h"
> #include "mozilla/StaticPtr.h"
>+#include "PowerManager.h"
> 
> namespace mozilla {
> namespace dom {
> 
> class ContentParent;
> 
> namespace power {

Similarly I don't get why you need to include PowerManager.h here.

(Sorry if this seems picky; we've started an effort to prune unnecessary
includes from our headers, and I don't want to run afoul of the people doing
that stuff.)

>diff --git a/dom/power/PowerManagerService.cpp b/dom/power/PowerManagerService.cpp
>--- a/dom/power/PowerManagerService.cpp
>+++ b/dom/power/PowerManagerService.cpp

>-void
> PowerManagerService::Notify(const WakeLockInformation& aWakeLockInfo)
> {
>-  nsAutoString state;
>-  ComputeWakeLockState(aWakeLockInfo, state);
>+  nsCOMPtr<nsIDOMMozWakeLockState> state = new nsDOMMozWakeLockState(aWakeLockInfo);

Nit: Please wrap lines at 80 chars (even if the rest of this file doesn't do
that, for some bizarre reason).

> NS_IMETHODIMP
> PowerManagerService::NewWakeLock(const nsAString &aTopic,
>                                  nsIDOMWindow *aWindow,
>                                  nsIDOMMozWakeLock **aWakeLock)
> {
>   nsRefPtr<WakeLock> wakelock = new WakeLock();
>-  nsresult rv = wakelock->Init(aTopic, aWindow);
>+  nsresult rv = wakelock->Init(aTopic, aWindow, /* trusted */ false);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIDOMMozWakeLock> wl(wakelock);
>+  wl.forget(aWakeLock);
>+
>+  return NS_OK;
>+}

This is technically correct and follows XPCOM's rules, but it would be safer if
you did *aWakeLock = nullptr before the NS_ENSURE_SUCCESS(rv, rv).  Otherwise,
if someone doesn't check rv (in violation of XPCOM's rules, but it happens all
the time), they can end up dereferencing uninitialized memory.

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

Same here.

>   nsCOMPtr<nsIDOMMozWakeLock> wl(wakelock);
>   wl.forget(aWakeLock);
> 
>   return NS_OK;
> }

>diff --git a/dom/power/WakeLockState.h b/dom/power/WakeLockState.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/power/WakeLockState.h

>+class nsDOMMozWakeLockState : public nsIDOMMozWakeLockState

>+private:
>+  // Overall locks count (untrusted + trusted)
>+  const uint32_t mLocksCount;
>+  const uint32_t mHiddenCount;
>+
>+  // Trusted locks count
>+  const uint32_t mTrustedCount;

Nit: Maybe "mTrustedLocksCount" would be a better name?  (It'd be more parallel
with "mTrustedHiddenCount".)

>+  const uint32_t mTrustedHiddenCount;
>+};

>diff --git a/dom/power/WakeLockState.cpp b/dom/power/WakeLockState.cpp
>+++ b/dom/power/WakeLockState.cpp

>+/* readonly attribute long stateAll; */
>+NS_IMETHODIMP nsDOMMozWakeLockState::GetState(nsAString& aState)

You don't need this comment, unless you like it.

>+/* readonly attribute long stateTrusted; */
>+NS_IMETHODIMP nsDOMMozWakeLockState::GetStateTrusted(nsAString& aStateTrusted)

Same here.

>@@ -221,43 +245,67 @@ ModifyWakeLock(const nsAString& aTopic,
>     table->Init();
>     sLockTable->Put(aTopic, table);
>   } else {
>     table->Get(aProcessID, &processCount);
>     table->EnumerateRead(CountWakeLocks, &totalCount);
>   }
> 
>   MOZ_ASSERT(processCount.numLocks >= processCount.numHidden);
>-  MOZ_ASSERT(aLockAdjust >= 0 || processCount.numLocks > 0);
>-  MOZ_ASSERT(aHiddenAdjust >= 0 || processCount.numHidden > 0);
>+  MOZ_ASSERT(processCount.numLocks >= processCount.numTrusted);
>+  MOZ_ASSERT_IF(aLockAdjust < 0, processCount.numLocks > 0);
>+  MOZ_ASSERT_IF(aLockAdjust < 0, processCount.numTrusted > 0);

This assertion doesn't seem right; isn't it valid to have numTrusted == 0 if
aLockAdjust == -1 && !aTrusted?

>+  if (aTrusted) {
>+    MOZ_ASSERT_IF(aHiddenAdjust < 0, processCount.numTrustedHidden > 0);
>+  } else {
>+    MOZ_ASSERT_IF(aHiddenAdjust < 0, processCount.numHidden > 0);
>+  }

If aTrusted, shouldn't we check that numTrustedHidden > 0 /and/ numHidden > 0?

>   MOZ_ASSERT(totalCount.numLocks >= totalCount.numHidden);
>-  MOZ_ASSERT(aLockAdjust >= 0 || totalCount.numLocks > 0);
>-  MOZ_ASSERT(aHiddenAdjust >= 0 || totalCount.numHidden > 0);
>+  MOZ_ASSERT(totalCount.numLocks >= totalCount.numTrusted);
>+  MOZ_ASSERT_IF(aLockAdjust < 0, totalCount.numLocks > 0);
>+  MOZ_ASSERT_IF(aLockAdjust < 0, totalCount.numTrusted > 0);

Similarly.

>+  if (aTrusted) {
>+    MOZ_ASSERT_IF(aHiddenAdjust < 0, totalCount.numTrustedHidden > 0);
>+  } else {
>+    MOZ_ASSERT_IF(aHiddenAdjust < 0, totalCount.numHidden > 0);
>+  }

Similarly.

>-  WakeLockState oldState = ComputeWakeLockState(totalCount.numLocks, totalCount.numHidden);
>+  WakeLockState oldState = ComputeWakeLockState(totalCount);
>+  WakeLockState oldTrustedState = ComputeWakeLockState(totalCount);
>+
>   bool processWasLocked = processCount.numLocks > 0;
>
>   processCount.numLocks += aLockAdjust;
>   processCount.numHidden += aHiddenAdjust;
>-
>   totalCount.numLocks += aLockAdjust;
>   totalCount.numHidden += aHiddenAdjust;
> 
>+  if (aTrusted) {
>+    processCount.numTrusted += aLockAdjust;
>+    processCount.numTrustedHidden += aHiddenAdjust;
>+    totalCount.numTrusted += aLockAdjust;
>+    totalCount.numTrustedHidden += aHiddenAdjust;
>+  }

Maybe it would make sense to move the relevant assertions to right before we do
relevant the +=, so there's less question about whether we got them right.

I'm repeating myself, but so I remember: We need a test for this, Gaia needs to
be updated.  I suspect existing tests will also need to be updated.

Attachment #765854 - Flags: review?(justin.lebar+bug) → review+

Martin Stránský [:stransky] (ni? me)

Assignee

Updated

•

11 years ago

Keywords: checkin-needed

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 22

•

11 years ago

Ahh, sorry, wrong window.

Keywords: checkin-needed

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 23

•

11 years ago

Attached patch patch, v5 — Details — Splinter Review

An updated patch.

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 24

•

11 years ago

Justin, what do you think it needs to be tested here? And is it enough to build b2g desktop with this patch to cover the changes in Gaia? I expect a try build shows the broken tests.

Justin Lebar (not reading bugmail)

Comment 25

•

11 years ago

> Justin, what do you think it needs to be tested here?

We should test that acquiring/releasing a trusted WL causes the appropriate events, and that the data associated with the event is appropriate.  That sort of thing.  See also dom/power/test/browser_wakelocks.js.

> And is it enough to build b2g desktop with this patch to cover the changes in Gaia?

That should be fine.  You'll of course want to use grep (as opposed to manual testing) to find the sites which need to be changed.

> I expect a try build shows the broken tests.

Yes.

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 26

•

11 years ago

Attached patch patch v6+tests — Details — Splinter Review

This patch fixes WakeLockState export to js, fixes B2G/Android code and updates existing tests. Try looks promising: https://tbpl.mozilla.org/?tree=Try&rev=b795d249b104

I'm going to update the browser tests to check the new trusted lock API.

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 27

•

11 years ago

Justin, do we want to convert any of existing locks to the trusted ones? And I guess the trusted lock code has to be written in C++, because trusted locks are not exported to js now, right?

Flags: needinfo?(justin.lebar+bug)

Justin Lebar (not reading bugmail)

Comment 28

•

11 years ago

> Justin, do we want to convert any of existing locks to the trusted ones?

Yes; most of them, I think.  Offhand, the only ones we definitely /don't/ want to convert are the ones which come directly from navigator.requestWakeLock.

> And I guess the 
> trusted lock code has to be written in C++, because trusted locks are not exported to js 
> now, right?

Yes, at least as-is.

FYI my last day at Mozilla is on Friday.  Mounir or khuey should be able to help you out here.

Flags: needinfo?(justin.lebar+bug)

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 29

•

11 years ago

Okay. The recent patch - v6 - implements the new trusted wake locks, but does not use them. I've found those locks so far:

"cpu":
dom/ipc/ProcessPriorityManager.cpp
dom/ipc/ContentParent.cpp
dom/messages/SystemMessageInternal.js
dom/system/gonk/RadioInterfaceLayer.js
content/html/content/src/HTMLMediaElement.cpp

"high-priority":
dom/ipc/ProcessPriorityManager.cpp

"screen":
content/html/content/src/HTMLVideoElement.cpp

"DOM_Fullscreen":
dom/base/nsGlobalWindow.cpp

mount locks:
dom/system/gonk/nsVolumeMountLock.cpp

All recent locks ("cpu", "DOM_Fullscreen", "screen", and mount locks) remain untrusted ones in the v6 patch. Unfortunately only "DOM_Fullscreen"/"screen" which is used for screen saver inhibition and maybe mount locks can be converted to trused one. The "cpu" locks is used from both C and js code.

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 30

•

11 years ago

Another question is how to implement a Callback in WakeLockListener in nsAppShell.cpp for Android. It does not send trusted lock changes to AndroidBridge::Bridge().

Karl Tomlinson (:karlt)

Comment 31

•

10 years ago

IIUC bug 963366 makes this unnecessary, for now at least.

Depends on: 963366

Martin Stránský [:stransky] (ni? me)

Assignee

Comment 32

•

10 years ago

Yes, let's close it for now.

Status: NEW → RESOLVED

Closed: 10 years ago

Resolution: --- → WONTFIX

patch 11 years ago Martin Stránský [:stransky] (ni? me) 35.56 KB, patch	justin.lebar+bug : feedback+	Details \| Diff \| Splinter Review
patch v.2 11 years ago Martin Stránský [:stransky] (ni? me) 37.60 KB, patch	justin.lebar+bug : review-	Details \| Diff \| Splinter Review
patch v.3 11 years ago Martin Stránský [:stransky] (ni? me) 34.30 KB, patch	justin.lebar+bug : review-	Details \| Diff \| Splinter Review
patch, v4 11 years ago Martin Stránský [:stransky] (ni? me) 38.19 KB, patch	justin.lebar+bug : review+	Details \| Diff \| Splinter Review
patch, v5 11 years ago Martin Stránský [:stransky] (ni? me) 37.56 KB, patch		Details \| Diff \| Splinter Review
patch v6+tests 11 years ago Martin Stránský [:stransky] (ni? me) 57.35 KB, patch		Details \| Diff \| Splinter Review