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

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

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?

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

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()?

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

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?

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

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?

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

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.

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.

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

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.

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.

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.

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.

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?

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!

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.

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;

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.

Ahh, sorry, wrong window.

An updated patch.

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

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.

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?

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

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.

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

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

Yes, let's close it for now.