Closed
Bug 939294
Opened 11 years ago
Closed 10 years ago
Change xpidl jsval to handles
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(9 files, 4 obsolete files)
204.89 KB,
patch
|
Details | Diff | Splinter Review | |
57.44 KB,
patch
|
gkrizsanits
:
review-
|
Details | Diff | Splinter Review |
74.27 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
34.11 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
19.12 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
15.45 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
4.34 KB,
patch
|
Details | Diff | Splinter Review | |
57.22 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
29.07 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
I am not finished yet and stopped for today.
Assignee | ||
Comment 1•11 years ago
|
||
Comment 2•11 years ago
|
||
Good work! I don't envy you changing all the code.
Assignee | ||
Comment 3•11 years ago
|
||
I finished this up. I will wait with posting the patch until 939302 is resolved. I hadd to work around some build issues like this: 0:10.02 In file included from /home/tom/mozilla-inbound/toolkit/components/places/nsNavHistory.cpp:17: 0:10.02 In file included from /home/tom/mozilla-inbound/toolkit/components/places/History.h:17: 0:10.02 In file included from ../../../dist/include/mozilla/dom/Link.h:17: 0:10.02 In file included from ../../../dist/include/nsIContent.h:10: 0:10.02 In file included from ../../../dist/include/nsINode.h:19: 0:10.02 In file included from ../../../dist/include/mozilla/ErrorResult.h:16: 0:10.02 ../../../dist/include/js/Value.h:1676:7: error: explicit specialization of 'js::MutableHandleBase<JS::Value>' after instantiation 0:10.02 class MutableHandleBase<JS::Value> : public MutableValueOperations<JS::MutableHandle<JS::Value> > 0:10.02 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0:10.02 ../../../dist/include/js/RootingAPI.h:500:46: note: implicit instantiation first required here 0:10.02 class MOZ_STACK_CLASS MutableHandle : public js::MutableHandleBase<T> 0:10.02 ^ 0:10.02 1 error generated. I am not sure why I didn't need to change any of the XPConnect dispatch code (I actually didn't even look at it), maybe Handle and const jsval& is compatible?
Assignee | ||
Comment 4•11 years ago
|
||
Also: 95 files changed, 476 insertions(+), 522 deletions(-)
Assignee | ||
Updated•11 years ago
|
Blocks: ExactRootingBrowser
Comment 5•11 years ago
|
||
Handle<Value> and Value& are both Value* on the binary level, yes.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #833171 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Sadly this doesn't compile in unified mode.
Assignee | ||
Comment 8•10 years ago
|
||
This makes the patches compile.
Attachment #8350718 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
This is probably the biggest consumer.
Attachment #8350761 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 years ago
|
||
29 very simple changes to all kinds of files in different directories.
Assignee | ||
Updated•10 years ago
|
Attachment #8350762 -
Attachment is patch: true
Assignee | ||
Updated•10 years ago
|
Attachment #8350762 -
Flags: review?(khuey)
Assignee | ||
Updated•10 years ago
|
Attachment #8350764 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8350765 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8350767 -
Flags: review?(terrence)
Assignee | ||
Comment 14•10 years ago
|
||
I am terrible bad at selecting reviewers for these things. If somebody wants to volunteer just reassign?
Assignee | ||
Updated•10 years ago
|
Attachment #8350737 -
Flags: review?(benjamin)
Comment 15•10 years ago
|
||
Comment on attachment 8350767 [details] [diff] [review] rest Review of attachment 8350767 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: xpcom/ds/nsVariant.cpp @@ +1859,5 @@ > return nsVariant::ConvertToISupports(mData, _retval); > } > > /* jsval getAsJSVal() */ > +NS_IMETHODIMP nsVariant::GetAsJSVal(JS::MutableHandleValue) JS::MutableHandle<JS::Value>
Attachment #8350767 -
Flags: review?(terrence) → review+
Comment 16•10 years ago
|
||
Comment on attachment 8350764 [details] [diff] [review] handle-content >+nsDOMFileReader::GetResult(JSContext* aCx, JS::MutableHandle<JS::Value> aResult) > { > JS::Rooted<JS::Value> result(aCx); > if (mDataFormat == FILE_AS_ARRAYBUFFER) { > if (mReadyState == nsIDOMFileReader::DONE && mResultArrayBuffer) { >- result.setObject(*mResultArrayBuffer); >+ aResult.setObject(*mResultArrayBuffer); > } else { >- result.setNull(); >+ aResult.setNull(); > } >- if (!JS_WrapValue(aCx, &result)) { >+ if (!JS_WrapValue(aCx, aResult)) { This is a bit error prone. If JS_WrapValue fails, aResult might continue pointing to mResultArrayBuffer. And you leave result variable unused. So, please use result and assign to aResult when returning success. >+ JS::RootedValue params(aCx, aParams.WasPassed() JS::Rooted<JS::Value>
Attachment #8350764 -
Flags: review?(bugs) → review+
Comment 17•10 years ago
|
||
Comment on attachment 8350765 [details] [diff] [review] toolkit >+++ b/toolkit/components/places/History.cpp >@@ -31,16 +31,17 @@ > #include "nsIXPConnect.h" > #include "mozilla/unused.h" > #include "nsContentUtils.h" // for nsAutoScriptBlocker > #include "mozilla/ipc/URIUtils.h" > #include "nsPrintfCString.h" > #include "nsTHashtable.h" > #include "jsapi.h" > >+ extra newline
Attachment #8350765 -
Flags: review?(bugs) → review+
Comment 18•10 years ago
|
||
Comment on attachment 8350761 [details] [diff] [review] handle-xpconnect From skimming: XPConnect cpp files should do |using namespace JS|, and drop the JS:: prefixing. Additionally, we should use HandleValue and friends instead of Handle<Value>. Please fix those things. Delegating the line-by-line review to gabor.
Attachment #8350761 -
Flags: review?(bobbyholley+bmo) → review?(gkrizsanits)
Comment 19•10 years ago
|
||
Comment on attachment 8350761 [details] [diff] [review] handle-xpconnect Review of attachment 8350761 [details] [diff] [review]: ----------------------------------------------------------------- Addition to Bobby's requests a few more things: many of the function signatures you are changing do not follow XPConnect convention, that is: JSContext *cx instead of JSContext* cx and JSContext * cx... Could you fix these as well while you're at it? JSVAL_IS_PRIMITIVE made sense in the old times, now I think !val.isObject() is simply more descriptive than val.isPrimitive() (and is in align with the comments...) could you use that please? - jsval testJsval(in jsval a, inout jsval b); + jsval testJsval(in jsval a, in jsval b); I don't get this change, you seem to be eliminating the inout option for jsval params. This might be OK but I don't know why, is it not used anywhere? Didn't MutableHandleValue work out for inout's? I r- it because of this part but if you can shred some light on this for me and fix the nits, I promise you a real quick review. Also, these patch are awesome, can't wait for JSNative to take Handles too...
Attachment #8350761 -
Flags: review?(gkrizsanits) → review-
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19) > Comment on attachment 8350761 [details] [diff] [review] > handle-xpconnect > > Review of attachment 8350761 [details] [diff] [review]: > ----------------------------------------------------------------- > > Addition to Bobby's requests a few more things: many of the function > signatures you are changing do not follow XPConnect convention, that is: > JSContext *cx instead of JSContext* cx and JSContext * cx... Could you fix > these as well while you're at it? I mostly didn't want to change to much to these old headers, but I can do that and "using namespace JS;" as well. > > JSVAL_IS_PRIMITIVE made sense in the old times, now I think !val.isObject() > is simply more descriptive than val.isPrimitive() (and is in align with the > comments...) could you use that please? Super idea. > > - jsval testJsval(in jsval a, inout jsval b); > + jsval testJsval(in jsval a, in jsval b); > > I don't get this change, you seem to be eliminating the inout option for > jsval params. This might be OK but I don't know why, is it not used > anywhere? Didn't MutableHandleValue work out for inout's? I r- it because of > this part but if you can shred some light on this for me and fix the nits, I > promise you a real quick review. I don't really see how inout jsval parameters are supposed to work. From my understand the JS engine doesn't actually support changing arguments in such a way. > Also, these patch are awesome, can't wait > for JSNative to take Handles too... JSNatives are probably going to get a CallArgs pointer at some time. You should already be using CallArgsFromVp anyway, which gives you handles to arguments/rval.
Assignee | ||
Comment 21•10 years ago
|
||
Oh I think I might have miss-understood the whole inout jsval business. Seems like you're suppose to pass an object like {value: "test"}. That would of course work, I need to look at how this is actually special cased.
Assignee | ||
Comment 22•10 years ago
|
||
Sorry for the spam: I keep thinking if nobody needs it we shouldn't allow it. jsvals are already kind of special in xpidl and disallowing unused features seems like a good idea to me. Thoughts?
Comment 23•10 years ago
|
||
inout jsval has certainly been used, and addons may use it.
Updated•10 years ago
|
Attachment #8350737 -
Flags: review?(benjamin) → review+
Comment on attachment 8350762 [details] [diff] [review] handle-dom Review of attachment 8350762 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDOMWindowUtils.cpp @@ +2127,5 @@ > return widget->DispatchEvent(&event, status); > } > > NS_IMETHODIMP > +nsDOMWindowUtils::GetClassName(JS::Handle<JS::Value> aObject, JSContext* aCx, char** aName) Break this line @@ +2235,3 @@ > } > + > + aParent.setObject(*parent); Move this outside of the if, I think. We should explicitly set aParent to null, right? ::: dom/base/nsJSEnvironment.cpp @@ +1257,5 @@ > nsCOMPtr<nsIVariant> variant(do_QueryInterface(arg)); > if (variant != nullptr) { > + JS::Rooted<JS::Value> temp(cx); > + rv = xpc->VariantToJS(cx, aScope, variant, &temp); > + *thisval = temp.get(); What is the bug for getting rid of these temporaries? ::: dom/camera/DOMCameraCapabilities.cpp @@ +160,5 @@ > return JS_FreezeObject(aCx, aArray) ? NS_OK : NS_ERROR_FAILURE; > } > > nsresult > +DOMCameraCapabilities::StringListToNewObject(JSContext* aCx, JS::MutableHandle<JS::Value> aArray, uint32_t aKey) Break this line. @@ +172,5 @@ > return NS_OK; > } > > nsresult > +DOMCameraCapabilities::DimensionListToNewObject(JSContext* aCx, JS::MutableHandle<JS::Value> aArray, uint32_t aKey) and here @@ +185,5 @@ > } > > /* readonly attribute jsval previewSizes; */ > NS_IMETHODIMP > +DOMCameraCapabilities::GetPreviewSizes(JSContext* cx, JS::MutableHandle<JS::Value> aPreviewSizes) and eveyrwhere ::: dom/camera/DOMCameraCapabilities.h @@ +32,5 @@ > uint32_t aKey, > ParseItemAndAddFunc aParseItemAndAdd > ); > + nsresult StringListToNewObject(JSContext* aCx, JS::MutableHandle<JS::Value> aArray, uint32_t aKey); > + nsresult DimensionListToNewObject(JSContext* aCx, JS::MutableHandle<JS::Value> aArray, uint32_t aKey); Break these lines. ::: dom/ipc/Blob.cpp @@ +1052,5 @@ > return static_cast<typename ActorType::ProtocolType*>(mActor); > } > > NS_IMETHOD > + GetLastModifiedDate(JSContext* cx, JS::MutableHandle<JS::Value> aLastModifiedDate) MOZ_OVERRIDE Break this line please. ::: dom/src/json/nsJSON.cpp @@ +358,5 @@ > return rv; > } > > NS_IMETHODIMP > +nsJSON::Decode(const nsAString& json, JSContext* cx, JS::MutableHandle<JS::Value> aRetval) Break this line. @@ +384,4 @@ > } > > NS_IMETHODIMP > +nsJSON::DecodeToJSVal(const nsAString &str, JSContext *cx, JS::MutableHandle<JS::Value> result) And this one.
Attachment #8350762 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 25•10 years ago
|
||
Changed this part to "inout" also generates JS::MutableHandleValue.
Attachment #8350737 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
Everything just seems to work with the previous patch and a minor change. So inout is back. https://tbpl.mozilla.org/?tree=Try&rev=efe5122cfc7d
Attachment #8357433 -
Flags: review?(gkrizsanits)
Comment 27•10 years ago
|
||
Comment on attachment 8357433 [details] [diff] [review] handle-xpconnect v2 Review of attachment 8357433 [details] [diff] [review]: ----------------------------------------------------------------- Great now it looks better. But you still have not addressed any of my nits (isPrimitive -> !isObject, fixing up the styling in all the function signatures you modify). r+ with having those fixed.
Attachment #8357433 -
Flags: review?(gkrizsanits) → review+
Comment 28•10 years ago
|
||
As discussed on IRC with Waldo, I'm totally fine with addressing the nits as a follow-up patch given the non trivial task of landing a patch like these and it's importance.
Assignee | ||
Comment 29•10 years ago
|
||
Thanks Gabor, I created the followup bug 958119. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c97e58ebc5f4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/89e9d3fa16fc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fed59539afc1 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c9ade6c871c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/37098c13e59e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c3aa9973da0
Comment 30•10 years ago
|
||
Backed out for B2G bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/61692f603cc4 https://tbpl.mozilla.org/php/getParsedLog.php?id=32769967&tree=Mozilla-Inbound
Assignee | ||
Comment 31•10 years ago
|
||
There is still a bunch of stuff for b2g here that I need to do.
Assignee | ||
Comment 32•10 years ago
|
||
Attachment #8359896 -
Flags: review?(bzbarsky)
Comment 33•10 years ago
|
||
Comment on attachment 8359896 [details] [diff] [review] make b2g compile I really hope the BluetoothHfpManager code is guaranteed to be running on main thread. And I think you need to enter a request there? I hope AutoMounterSetting::AutoMounterSetting is on main thread too (though AutoSafeJSContext means no need to enter a request). >+++ b/dom/system/gonk/TimeZoneSettingObserver.cpp >+ NS_IMETHOD Handle(const nsAString &aName, JS::Handle<JS::Value> aResult) { Extra space there. The rest looks great. r=me
Attachment #8359896 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 34•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f017ae03bb6c
Assignee | ||
Comment 35•10 years ago
|
||
Hopefully fix the b2g failure: https://hg.mozilla.org/integration/mozilla-inbound/rev/46b6115b38f3
Comment 36•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f017ae03bb6c https://hg.mozilla.org/mozilla-central/rev/46b6115b38f3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 37•10 years ago
|
||
jcranmer pushed a bustage fix for comm-central: https://hg.mozilla.org/comm-central/rev/385ea939531d
You need to log in
before you can comment on or make changes to this bug.
Description
•