793244 - Convert Screen to WebIDL

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Rewrite mozLockOrientation

As promised :)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Convert Screen

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I think I forgot a SetIsDOMBinding() somewhere...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Did we ever reach agreement on what the IDL for mozLockOrientation should be?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 663483 [details] [diff] [review]
Part a: Rewrite mozLockOrientation

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

Mounir, does this look good to you?

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 663483 [details] [diff] [review]
Part a: Rewrite mozLockOrientation

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

::: dom/base/nsScreen.cpp
@@ +385,3 @@
>        }
>  
> +      JS::RootedString jsString(aCx, JS_ValueToString(aCx, temp));

That means if we have 42 as an integer, we will get "42" string?

@@ +475,5 @@
>        // Now, we need to register a listener so we learn when we leave
>        // full-screen and when we will have to unlock the screen.
>        nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(GetOwner());
>        if (!target) {
> +        return true;

Well, explicitly returning |true| here makes a problem quite obvious: we are fullscreen and need to remove orientation lock when we leave fullscreen but we have no way to do that if we have no target.

Could you add a NS_ERROR() here and maybe return false instead of true?

This could be done in a follow-up.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Mounir Lamouri (:mounir) from comment #5)
> Comment on attachment 663483 [details] [diff] [review]
> Part a: Rewrite mozLockOrientation
> 
> Review of attachment 663483 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsScreen.cpp
> @@ +385,3 @@
> >        }
> >  
> > +      JS::RootedString jsString(aCx, JS_ValueToString(aCx, temp));
> 
> That means if we have 42 as an integer, we will get "42" string?

Yes

> @@ +475,5 @@
> >        // Now, we need to register a listener so we learn when we leave
> >        // full-screen and when we will have to unlock the screen.
> >        nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(GetOwner());
> >        if (!target) {
> > +        return true;
> 
> Well, explicitly returning |true| here makes a problem quite obvious: we are
> fullscreen and need to remove orientation lock when we leave fullscreen but
> we have no way to do that if we have no target.
> 
> Could you add a NS_ERROR() here and maybe return false instead of true?
> 
> This could be done in a follow-up.

Follow-up it is, then :) Will file when out of class.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to :Ms2ger from comment #6)
> (In reply to Mounir Lamouri (:mounir) from comment #5)
> > Comment on attachment 663483 [details] [diff] [review]
> > Part a: Rewrite mozLockOrientation
> > 
> > Review of attachment 663483 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/base/nsScreen.cpp
> > @@ +385,3 @@
> > >        }
> > >  
> > > +      JS::RootedString jsString(aCx, JS_ValueToString(aCx, temp));
> > 
> > That means if we have 42 as an integer, we will get "42" string?
> 
> Yes

Great :)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 663483 [details] [diff] [review]
Part a: Rewrite mozLockOrientation

>+++ b/dom/base/nsScreen.cpp
>+nsScreen::MozLockOrientation(const JS::Value& aOrientation, JSContext* aCx,
>+  NonNull<nsAString> orientation;

You don't need the NonNull thing here.

>+nsScreen::MozLockOrientation(const NonNull<nsAString>& aOrientation, ErrorResult& aRv)

First arg should be "const nsAString&".

r=me with that.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 663485 [details] [diff] [review]
Part b: Convert Screen

>+++ b/dom/base/nsScreen.cpp
>+NS_IMETHODIMP
>+nsScreen::GetPixelDepth(int32_t* aColorDepth)

FORWARD_LONG_GETTER?

> NS_IMETHODIMP
> nsScreen::GetColorDepth(int32_t* aColorDepth)

Likewise?

>+++ b/dom/base/nsScreen.h
>   NS_DECL_ISUPPORTS

_INHERITED?  Not sure why it wasn't already.

>+++ b/dom/bindings/Bindings.conf
>+    'headerFile': 'nsScreen.h',

You shouldn't need that; it should be derived automatically from the nativeType.

>+++ b/dom/bindings/Makefile.in
>+include $(topsrcdir)/ipc/chromium/chromium-config.mk

Hmm.  Why is that needed?
>+++ b/dom/webidl/Screen.webidl
>@@ -0,0 +1,41 @@
>+/* 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/. */

No spec to link to?

>+  [Throws] readonly attribute long availWidth;

Can you put the [Throws] on a separate line here and elsewhere in this webidl?

>+++ b/js/xpconnect/src/dom_quickstubs.qsconf

Maybe leave the quickstubs while this is prefable?

r=me with those nits.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> Comment on attachment 663483 [details] [diff] [review]
> Part a: Rewrite mozLockOrientation
> 
> >+++ b/dom/base/nsScreen.cpp
> >+nsScreen::MozLockOrientation(const JS::Value& aOrientation, JSContext* aCx,
> >+  NonNull<nsAString> orientation;
> 
> You don't need the NonNull thing here.
> 
> >+nsScreen::MozLockOrientation(const NonNull<nsAString>& aOrientation, ErrorResult& aRv)
> 
> First arg should be "const nsAString&".

Oh, that works? Neat.

(In reply to Boris Zbarsky (:bz) from comment #9)
> Comment on attachment 663485 [details] [diff] [review]
> Part b: Convert Screen
> 
> >+++ b/dom/base/nsScreen.cpp
> >+NS_IMETHODIMP
> >+nsScreen::GetPixelDepth(int32_t* aColorDepth)
> 
> FORWARD_LONG_GETTER?
> 
> > NS_IMETHODIMP
> > nsScreen::GetColorDepth(int32_t* aColorDepth)
> 
> Likewise?

Er, yes.

> >+++ b/dom/base/nsScreen.h
> >   NS_DECL_ISUPPORTS
> 
> _INHERITED?  Not sure why it wasn't already.

Can do.

> >+++ b/dom/bindings/Bindings.conf
> >+    'headerFile': 'nsScreen.h',
> 
> You shouldn't need that; it should be derived automatically from the
> nativeType.

OK.

> >+++ b/dom/bindings/Makefile.in
> >+include $(topsrcdir)/ipc/chromium/chromium-config.mk
> 
> Hmm.  Why is that needed?

I believe that was because nsScreen.h includes base/basictypes.h, but I can check if it was actually necessary.

> >+++ b/dom/webidl/Screen.webidl
> >@@ -0,0 +1,41 @@
> >+/* 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/. */
> 
> No spec to link to?

Will add a link.

> >+  [Throws] readonly attribute long availWidth;
> 
> Can you put the [Throws] on a separate line here and elsewhere in this
> webidl?

Yes.

> >+++ b/js/xpconnect/src/dom_quickstubs.qsconf
> 
> Maybe leave the quickstubs while this is prefable?

Will do.

> r=me with those nits.

Thanks!

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Oh, that works? Neat.

Yes, the point of NonNull is to look like an object reference to the callee.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> Comment on attachment 663485 [details] [diff] [review]
> Part b: Convert Screen
> >+++ b/dom/bindings/Makefile.in
> >+include $(topsrcdir)/ipc/chromium/chromium-config.mk
> 
> Hmm.  Why is that needed?

For posterity:

In file included from ../../dist/include/mozilla/dom/ScreenOrientation.h:8:0,
                 from /m-c/dom/base/nsScreen.h:10,
                 from /obj/dom/bindings/ScreenBinding.cpp:14:
../../dist/include/ipc/IPCMessageUtils.h:10:45: fatal error: chrome/common/ipc_message_utils.h: No such file or directory
compilation terminated.

Comment 13

[Mounir Lamouri (:mounir)]

Arf, I forgot it was using hal and hal's using ipc. Please, beat me :)

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/5468a80570b2
https://hg.mozilla.org/mozilla-central/rev/b740202ee0b8