223536 - Exposing XPConnect jsval <-> nsIVariant to conversion functionality

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Stef Walter]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030728 Mozilla Firebird/0.6.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6a) Gecko/20031016 Firebird/0.7+

While adding scripting to a mozilla based application I need to be able to
convert between nsIVariant's and jsvals. The code for this exists in XPConnect
but is not exposed. 

Instead of reinventing the wheel in app, I'd like to suggest exposing this
functionality. I'd imagine it would be beneficial to other developers as well.
Included is a patch which exposes the conversions as a service.


Reproducible: Always

Steps to Reproduce:

Comment 1

[Stef Walter]

Attachment: Patch to add feature

Comment 2

[Stef Walter]

Attachment: The right patch

Attached the wrong patch. Currently on a trip, so my brain's a bit scattered :)

Comment 3

[David Bradley]

Do we really need a new interface for this? I'm thinking exposing it on one of
the existing xpconnect interfaces might be better. Was thinking of nsIXPConnect.
It would seem in line with the other js to native and native to js operations.

Comment 4

[Stef Walter]

True. Good point. The service added by the patch doesn't contain any state
information, so adding it to another service would be trivial. OTOH,
nsIXPConnect is already a massive interface. 

Regardless, I'll prepare a patch incorporating this change when I get a chance

Comment 5

[Stef Walter]

Attachment: Patch to add feature (on nsIXPConnect)

As suggested by Bradley this patch adds the converter functionality to the
nsIXPConnect interface rather than creating a whole new interface/service.

Comment 6

[Stef Walter]

Brad, any chance of getting this reviewed?

Comment 7

[David Bradley]

Comment on attachment 134093 [details] [diff] [review]
The right patch

Cleanup, comments on other patch to follow

Comment 8

[David Bradley]

Comment on attachment 135045 [details] [diff] [review]
Patch to add feature (on nsIXPConnect)

r=dbradley with the following addressed

I don't believe you need the NS_ADDREF in VariantFromJS. XPCVariant::newVariant
has already incremented the ref count.

I'd include the nsIVariant interface idl file in nsXPConnect.idl rather than
forward declaring it.

nits, your comments stated variantToJs and variantFromJs but the S should be
uppercase

You have several "return" statements after if's that are indented only by two,
should be four

Comment 9

[Stef Walter]

Attachment: Updated patch

Thanks. I've updated the patch. 
Do I need to get superreview on it from someone else?

Comment 10

[David Bradley]

Comment on attachment 143433 [details] [diff] [review]
Updated patch

Yes, seeing if jst has time.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 143433 [details] [diff] [review]
Updated patch

+    JSVal variantToJS(in JSContextPtr ctx, in JSObjectPtr scope, in nsIVariant
value);
+    nsIVariant variantFromJS(in JSContextPtr ctx, in JSVal value);

I would make that variantToJS() and JStoVariant, but I can deal either way.

sr=jst

Comment 12

[Stef Walter]

Attachment: Alternative patch with jst's suggestion

Bradley, do you have time to check this in?

Comment 13

[David Bradley]

Sure I can get it checked in. There's no need to get this in before 1.7 final is
there?

Comment 14

[Stef Walter]

Nope, later is fine. Thanks.

Comment 15

[David Bradley]

I'll get this in within the next few days. I was hoping to get to it tonight,
but my tree was in a strange state and I had to create a new one.

Comment 16

[[e-mail]]

Comment on attachment 145027 [details] [diff] [review]
Alternative patch with jst's suggestion

>+    JSVal variantToJS(in JSContextPtr ctx, in JSObjectPtr scope, in nsIVariant
value);
>+    nsIVariant JSToVariant(in JSContextPtr ctx, in JSVal value);
Shouldn't these be marked [noscript]?

Comment 17

[Stef Walter]

(In reply to comment #16)
> Shouldn't these be marked [noscript]?

The entire nsIXPConnect interface is not marked scriptable, so I don't think
that's necessary. 

Comment 18

[[e-mail]]

(In reply to comment #17)
>(In reply to comment #16)
>> Shouldn't these be marked [noscript]?
>The entire nsIXPConnect interface is not marked scriptable, so I don't think
>that's necessary. 
D'oh! :-[

Comment 19

[David Bradley]

Patch checked in