Closed Bug 952079 Opened 6 years ago Closed 6 years ago

Porting nsIDOMWakeLock to WebIDL

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: baku, Assigned: baku)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 4 obsolete files)

No description provided.
Attached patch wakelock.patch (obsolete) — Splinter Review
Attachment #8350008 - Flags: review?(Ms2ger)
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?
Attachment #8350008 - Flags: review?(Ms2ger) → feedback+
Attached patch wakelock.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=bf3edd95bb49
Attachment #8350008 - Attachment is obsolete: true
Attachment #8350137 - Flags: review?(Ms2ger)
> > +  nsCOMPtr<nsIDOMEventListener> eventListener =
> > +    static_cast<nsIDOMEventListener*>(wakelock.get());
> 
> Why the cast?

Because nsISupports ambiguity... nsIDOMEventListener or nsIObserver.
(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.
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.
Attached patch wakelock.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=49a5c38b8eaa
Attachment #8350137 - Attachment is obsolete: true
Attachment #8350137 - Flags: review?(Ms2ger)
Attachment #8350498 - Flags: review?(Ms2ger)
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
Attachment #8350498 - Flags: review?(Ms2ger) → feedback+
Attached patch wakelock.patch (obsolete) — Splinter Review
Attachment #8350498 - Attachment is obsolete: true
Attachment #8351147 - Flags: review?(Ms2ger)
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?
Attachment #8351147 - Flags: review?(Ms2ger)
Comment on attachment 8351147 [details] [diff] [review]
wakelock.patch

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

Bz, can you review this patch?
Attachment #8351147 - Flags: review?(bzbarsky)
Comment on attachment 8351147 [details] [diff] [review]
wakelock.patch

Olli, can you do this one?
Attachment #8351147 - Flags: review?(bzbarsky) → review?(bugs)
Yes, within next 3 days.
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.
Attachment #8351147 - Flags: review?(bugs) → review-
Attached patch wakelock.patchSplinter Review
I'll fix the fullscreen issue in a follow up.

https://tbpl.mozilla.org/?tree=Try&rev=52e98c6b72b2
Attachment #8351147 - Attachment is obsolete: true
Attachment #8356013 - Flags: review?(bugs)
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.)
Attachment #8356013 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/9fc62be40607
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 957899
Whiteboard: [qa-]
See Also: → 1184292
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.