Closed Bug 939294 Opened 6 years ago Closed 6 years ago

Change xpidl jsval to handles

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

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
Attached patch change-idl (obsolete) — Splinter Review
I am not finished yet and stopped for today.
Attached patch change code, unfinished (obsolete) — Splinter Review
Good work!  I don't envy you changing all the code.
Depends on: 939302
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?
Also: 95 files changed, 476 insertions(+), 522 deletions(-)
Handle<Value> and Value& are both Value* on the binary level, yes.
Attachment #833171 - Attachment is obsolete: true
Sadly this doesn't compile in unified mode.
Assignee: nobody → evilpies
Attachment #833174 - Attachment is obsolete: true
Status: NEW → ASSIGNED
This makes the patches compile.
Attachment #8350718 - Attachment is obsolete: true
Attached patch handle-xpconnectSplinter Review
This is probably the biggest consumer.
Attachment #8350761 - Flags: review?(bobbyholley+bmo)
Attached patch handle-domSplinter Review
Attached patch handle-contentSplinter Review
Attached patch toolkitSplinter Review
Attached patch restSplinter Review
29 very simple changes to all kinds of files in different directories.
Attachment #8350762 - Attachment is patch: true
Attachment #8350762 - Flags: review?(khuey)
Attachment #8350764 - Flags: review?(bugs)
Attachment #8350765 - Flags: review?(bugs)
Attachment #8350767 - Flags: review?(terrence)
I am terrible bad at selecting reviewers for these things. If somebody wants to volunteer just reassign?
Attachment #8350737 - Flags: review?(benjamin)
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 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 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 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 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-
(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.
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.
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?
inout jsval has certainly been used, and addons may use it.
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+
Changed this part to "inout" also generates JS::MutableHandleValue.
Attachment #8350737 - Attachment is obsolete: true
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 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+
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.
Blocks: 958119
There is still a bunch of stuff for b2g here that I need to do.
Attached patch make b2g compileSplinter Review
Attachment #8359896 - Flags: review?(bzbarsky)
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+
https://hg.mozilla.org/mozilla-central/rev/f017ae03bb6c
https://hg.mozilla.org/mozilla-central/rev/46b6115b38f3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
jcranmer pushed a bustage fix for comm-central: https://hg.mozilla.org/comm-central/rev/385ea939531d
Depends on: 961488
You need to log in before you can comment on or make changes to this bug.