Andrea Marchesini [:baku] Assignee
	Description • 11 years ago

      No description provided.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 2 • 11 years ago

Comment on attachment 8350008 [details] [diff] [review]
wakelock.patch

Review of attachment 8350008 [details] [diff] [review]:
-----------------------------------------------------------------

Would be better with a patch below it to make the class mozilla::dom::WakeLock.

::: content/html/content/src/HTMLVideoElement.cpp
@@ +304,5 @@
>    if (mScreenWakeLock && (mPaused || hidden)) {
> +    ErrorResult rv;
> +    mScreenWakeLock->Unlock(rv);
> +    if (rv.Failed()) {
> +      NS_WARNING("Failed to unlock the wakelock.");

NS_WARN_IF_FALSE, maybe

@@ +318,5 @@
>      NS_ENSURE_TRUE_VOID(pmService);
>  
> +    ErrorResult rv;
> +    mScreenWakeLock = pmService->NewWakeLock(NS_LITERAL_STRING("screen"),
> +                                              OwnerDoc()->GetWindow(),

Indent

::: dom/power/PowerManagerService.cpp
@@ +210,5 @@
> +                                 nsIDOMWindow* aWindow,
> +                                 ErrorResult& aRv)
> +{
> +  nsRefPtr<WakeLock> wakelock = new WakeLock();
> +  nsresult rv = wakelock->Init(aTopic, aWindow);

aRv = ...
if (aRv.Failed()) {
  return ...

@@ +233,3 @@
>  
> +  nsCOMPtr<nsIDOMEventListener> eventListener =
> +    static_cast<nsIDOMEventListener*>(wakelock.get());

Why the cast?

::: dom/power/WakeLock.cpp
@@ +22,5 @@
>  namespace mozilla {
>  namespace dom {
>  namespace power {
>  
> +NS_IMPL_QUERY_INTERFACE_INHERITED3(WakeLock, nsDOMEventTargetHelper,

Cycle collection?

::: dom/power/WakeLock.h
@@ +52,5 @@
>  
> +  // WebIDL methods
> +  nsISupports* GetParentObject() const
> +  {
> +    return nullptr;

Oh?

::: dom/system/gonk/nsVolumeMountLock.h
@@ +6,5 @@
>  #define mozilla_system_nsvolumemountlock_h__
>  
>  #include "nsIVolumeMountLock.h"
>  
> +#include "mozilla/dom/MozWakeLock.h"

Uh, does this build?

	Andrea Marchesini [:baku] Assignee
	Comment 3 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=bf3edd95bb49

	Andrea Marchesini [:baku] Assignee
	Comment 4 • 11 years ago

> > +  nsCOMPtr<nsIDOMEventListener> eventListener =
> > +    static_cast<nsIDOMEventListener*>(wakelock.get());
> 
> Why the cast?

Because nsISupports ambiguity... nsIDOMEventListener or nsIObserver.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 5 • 11 years ago

(In reply to Andrea Marchesini (:baku) from comment #4)
> > > +  nsCOMPtr<nsIDOMEventListener> eventListener =
> > > +    static_cast<nsIDOMEventListener*>(wakelock.get());
> > 
> > Why the cast?
> 
> Because nsISupports ambiguity... nsIDOMEventListener or nsIObserver.

You're assigning to an nsCOMPtr<nsIDOMEventListener>, so there shouldn't be an issue.

	Masatoshi Kimura [:emk]
	Comment 6 • 11 years ago

Comment on attachment 8350137 [details] [diff] [review]
wakelock.patch

Review of attachment 8350137 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/MozWakeLock.webidl
@@ +3,5 @@
> +/* 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/. */
> +
> +interface MozWakeLock

Func="Navigator::HasWakeLockSupport"], please.

	Andrea Marchesini [:baku] Assignee
	Comment 7 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=49a5c38b8eaa

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 8 • 11 years ago

Comment on attachment 8350498 [details] [diff] [review]
wakelock.patch

Review of attachment 8350498 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Navigator.cpp
@@ +1142,4 @@
>    // Maybe it went away for some reason... Or maybe we're just called
>    // from our XPCOM method.
> +  nsRefPtr<power::PowerManagerService> pmService =
> +    power::PowerManagerService::GetInstance();

Keep this where it was

::: dom/power/WakeLock.cpp
@@ +3,5 @@
>   * 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 "WakeLock.h"
> +#include "mozilla/dom/MozWakeLockBinding.h"

M goes after C

@@ +8,4 @@
>  #include "mozilla/dom/ContentParent.h"
>  #include "mozilla/Hal.h"
>  #include "mozilla/HalWakeLock.h"
>  #include "nsDOMClassInfoID.h"

Can probably drop this

::: dom/system/gonk/nsVolumeMountLock.h
@@ +42,5 @@
>    nsresult Init();
>  
> +  nsString                 mVolumeName;
> +  int32_t                  mVolumeGeneration;
> +  nsRefPtr<dom::WakeLock>  mWakeLock;

Nit: feel free to move the nsRefPtr first, to save a word of memory on x64

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 10 • 11 years ago

Comment on attachment 8351147 [details] [diff] [review]
wakelock.patch

Review of attachment 8351147 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, I think, but I don't have time for a full review right now.

::: dom/power/WakeLock.cpp
@@ +23,2 @@
>  
> +NS_IMPL_CYCLE_COLLECTION_CLASS(WakeLock)

Do you actually need a cc class?

	Andrea Marchesini [:baku] Assignee
	Comment 11 • 11 years ago

Comment on attachment 8351147 [details] [diff] [review]
wakelock.patch

Review of attachment 8351147 [details] [diff] [review]:
-----------------------------------------------------------------

Bz, can you review this patch?

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 11 years ago

Comment on attachment 8351147 [details] [diff] [review]
wakelock.patch

Olli, can you do this one?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 11 years ago

Yes, within next 3 days.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 14 • 11 years ago

Comment on attachment 8351147 [details] [diff] [review]
wakelock.patch

> HTMLMediaElement::WakeLockCreate()
> {
...
>+    ErrorResult rv;
>+    mWakeLock = pmService->NewWakeLock(NS_LITERAL_STRING("cpu"),
>+                                       OwnerDoc()->GetWindow(),
>+                                       rv);
Not your code but OwnerDoc()->GetWindow() looks odd. I'd expect use of inner window here.

> HTMLVideoElement::WakeLockUpdate()
> {
...
>+    ErrorResult rv;
>+    mScreenWakeLock = pmService->NewWakeLock(NS_LITERAL_STRING("screen"),
>+                                             OwnerDoc()->GetWindow(),
>+                                             rv);
ditto


>@@ -5690,23 +5690,31 @@ nsGlobalWindow::SetFullScreenInternal(bo
...
>-    pmService->NewWakeLock(NS_LITERAL_STRING("DOM_Fullscreen"), this, getter_AddRefs(mWakeLock));
>+    ErrorResult rv;
>+    mWakeLock = pmService->NewWakeLock(NS_LITERAL_STRING("DOM_Fullscreen"),
>+                                       this, rv);
ditto. 
Looks like we add event listener to the window, and that means the event listener is added to the inner window, yet
nsGlobalWindow checks the existence of mWakeLock in the outerwindow. So if the page is navigated, this setup just doesn't work.
Could you fix this? Use inner window, and nsGlobalWindow::mWakeLock should be stored in the inner window.
Or file a followup for this stuff.






>+NS_IMPL_CYCLE_COLLECTION_CLASS(WakeLock)
>+
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(WakeLock, nsDOMEventTargetHelper)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(WakeLock, nsDOMEventTargetHelper)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>+
>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(WakeLock)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMEventListener)
>   NS_INTERFACE_MAP_ENTRY(nsIObserver)
>   NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
>-  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(MozWakeLock)
>-NS_INTERFACE_MAP_END
>+NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
> 
>-NS_IMPL_ADDREF(WakeLock)
>-NS_IMPL_RELEASE(WakeLock)
>+NS_IMPL_ADDREF_INHERITED(WakeLock, nsDOMEventTargetHelper)
>+NS_IMPL_RELEASE_INHERITED(WakeLock, nsDOMEventTargetHelper)
I don't understand this stuff. How/why do we end up making WakeLock EventTarget?

>-class WakeLock
>-  : public nsIDOMMozWakeLock
>+class WakeLock MOZ_FINAL
>+  : public nsDOMEventTargetHelper
>   , public nsIDOMEventListener
>   , public nsIObserver
>   , public nsSupportsWeakReference
...here. Why we start to inherit nsDOMEventTargetHelper?
Don't you just want nsWrapperCache or something.

	Andrea Marchesini [:baku] Assignee
	Comment 15 • 11 years ago

I'll fix the fullscreen issue in a follow up.

https://tbpl.mozilla.org/?tree=Try&rev=52e98c6b72b2

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 16 • 11 years ago

Comment on attachment 8356013 [details] [diff] [review]
wakelock.patch

>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(WakeLock)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMEventListener)
>   NS_INTERFACE_MAP_ENTRY(nsIObserver)
>   NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
>-  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(MozWakeLock)
>+  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> NS_INTERFACE_MAP_END
Please move NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY to the first in the list, before
NS_INTERFACE_MAP_ENTRY(nsIDOMEventListener)

>+class WakeLock MOZ_FINAL
>+  : public nsWrapperCache
>   , public nsIDOMEventListener
>   , public nsIObserver
>   , public nsSupportsWeakReference
Move nsWrapperCache to be after nsIObserver or something.
That way nsIDOMEventListener stays as the canonical pointer.


(And please file the followup for the fullscreen stuff.)

	Andrea Marchesini [:baku] Assignee
	Comment 17 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc62be40607

	Ed Morley [:emorley]
	Comment 18 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/9fc62be40607