Closed
Bug 793244
Opened 12 years ago
Closed 12 years ago
Convert Screen to WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: Ms2ger, Assigned: Ms2ger)
References
Details
Attachments
(2 files)
11.98 KB,
patch
|
bzbarsky
:
review+
mounir
:
feedback+
|
Details | Diff | Splinter Review |
15.83 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
As promised :)
Attachment #663483 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #663485 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•12 years ago
|
||
I think I forgot a SetIsDOMBinding() somewhere...
Comment 3•12 years ago
|
||
Did we ever reach agreement on what the IDL for mozLockOrientation should be?
Assignee | ||
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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+
Assignee | ||
Comment 6•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
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 9•12 years ago
|
||
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+
Assignee | ||
Comment 10•12 years ago
|
||
(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•12 years ago
|
||
> Oh, that works? Neat.
Yes, the point of NonNull is to look like an object reference to the callee.
Assignee | ||
Comment 12•12 years ago
|
||
(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•12 years ago
|
||
Arf, I forgot it was using hal and hal's using ipc. Please, beat me :)
Assignee | ||
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5468a80570b2 https://hg.mozilla.org/mozilla-central/rev/b740202ee0b8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•