Closed Bug 793244 Opened 7 years ago Closed 7 years ago

Convert Screen to WebIDL

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(2 files)

As promised :)
Attachment #663483 - Flags: review?(bzbarsky)
Attachment #663485 - Flags: review?(bzbarsky)
Depends on: 793263
I think I forgot a SetIsDOMBinding() somewhere...
Did we ever reach agreement on what the IDL for mozLockOrientation should be?
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?
Attachment #663483 - Flags: feedback?(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.
Attachment #663483 - Flags: feedback?(mounir) → feedback+
(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.
(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 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.
Attachment #663483 - Flags: review?(bzbarsky) → review+
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.
Attachment #663485 - Flags: review?(bzbarsky) → review+
(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!
> Oh, that works? Neat.

Yes, the point of NonNull is to look like an object reference to the callee.
(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.
Arf, I forgot it was using hal and hal's using ipc. Please, beat me :)
Blocks: 792957
https://hg.mozilla.org/mozilla-central/rev/5468a80570b2
https://hg.mozilla.org/mozilla-central/rev/b740202ee0b8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Duplicate of this bug: 798857
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.