884410 - GC: Handlify the JSAPI

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: TransplantObject*; v0

I've been doing these piecemeal while building other things and will be attaching pieces as I create them. So far they have been trivial.

Anyone that wants to pitch in and handlify a few functions would be more than welcome.

Comment 1

[Terrence Cole [:terrence]]

Attachment: ResolveStandardClass; v0

Comment 2

[Terrence Cole [:terrence]]

Attachment: EnumerateStandardClasses; v0

Comment 3

[Terrence Cole [:terrence]]

Attachment: REMOVAL EnumerateResolvedStandardClasses; v0

The browser does not use this and I don't think we want to expose such functionality anyway, even in the friend api.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 764240 [details] [diff] [review]
TransplantObject*; v0

Review of attachment 764240 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +2395,5 @@
>      } else {
>        JS::Rooted<JSObject*> global(cx,
>          xpc_UnmarkGrayObject(newInnerWindow->mJSObject));
> +      JS::Rooted<JSObject*> outerObject(cx,
> +        NewOuterWindowProxy(cx, global, thisChrome));

You'll want a dom peer as usual

::: js/src/jsapi.h
@@ +2012,4 @@
>  
>  extern JS_FRIEND_API(JSObject *)
>  js_TransplantObjectWithWrapper(JSContext *cx,
> +                               JS::HandleObject origobj,

I thought we just decided over in bug 884371 that we'd use JS::Handle<JSObject*> in jsapi.h?

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 764242 [details] [diff] [review]
ResolveStandardClass; v0

Review of attachment 764242 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/xbl/src/nsXBLDocumentInfo.cpp
@@ +162,5 @@
>    NS_RELEASE(nativeThis);
>  }
>  
>  static JSBool
> +nsXBLDocGlobalObject_resolve(JSContext *cx, JS::HandleObject obj, JS::HandleId id)

Conflicts with bug 884371, and doesn't seem necessary?

Comment 6

[Terrence Cole [:terrence]]

(In reply to :Ms2ger from comment #5)
> ::: content/xbl/src/nsXBLDocumentInfo.cpp
> >  static JSBool
> > +nsXBLDocGlobalObject_resolve(JSContext *cx, JS::HandleObject obj, JS::HandleId id)
> 
> Conflicts with bug 884371, and doesn't seem necessary?

That bug seemed to only be about MutableHandleValue. Besides, they will be trivial to merge and I'd rather over-fix than miss some.

(In reply to :Ms2ger from comment #4)
> ::: dom/base/nsGlobalWindow.cpp
> @@ +2395,5 @@
> >      } else {
> >        JS::Rooted<JSObject*> global(cx,
> >          xpc_UnmarkGrayObject(newInnerWindow->mJSObject));
> > +      JS::Rooted<JSObject*> outerObject(cx,
> > +        NewOuterWindowProxy(cx, global, thisChrome));
> 
> You'll want a dom peer as usual

For something this trivial?
 
> ::: js/src/jsapi.h
> @@ +2012,4 @@
> >  
> >  extern JS_FRIEND_API(JSObject *)
> >  js_TransplantObjectWithWrapper(JSContext *cx,
> > +                               JS::HandleObject origobj,
> 
> I thought we just decided over in bug 884371 that we'd use
> JS::Handle<JSObject*> in jsapi.h?

Well, JSAPI is technically in JS. There is a fair argument to be made that the API should look how we expect users of the API to look, but we haven't really had that discussion yet.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 764246 [details] [diff] [review]
REMOVAL EnumerateResolvedStandardClasses; v0

Review of attachment 764246 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 764243 [details] [diff] [review]
EnumerateStandardClasses; v0

Review of attachment 764243 [details] [diff] [review]:
-----------------------------------------------------------------

All looks good.  We should decide on a style for jsapi.h though.

Comment 9

[Terrence Cole [:terrence]]

Try looks good:
https://tbpl.mozilla.org/?tree=Try&rev=bb17ccf250fd

Pushed:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c55d7332c83a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b07c97f12e34
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b1636ba097f9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/95952d257a56

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c55d7332c83a
https://hg.mozilla.org/mozilla-central/rev/b07c97f12e34
https://hg.mozilla.org/mozilla-central/rev/b1636ba097f9
https://hg.mozilla.org/mozilla-central/rev/95952d257a56

Comment 11

[Tom S [:evilpie]]

Attachment: JS_GetPrototype v0

Not sure if this needs addional reviewers.

Comment 12

[Till Schneidereit [:till]]

This is going to see quite some more activity

Comment 13

[Tom S [:evilpie]]

Attachment: JS_SetPrototype v0

All our rooting work really shows.

Comment 14

[Terrence Cole [:terrence]]

Comment on attachment 765256 [details] [diff] [review]
JS_GetPrototype v0

Review of attachment 765256 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ Thanks! r=me

::: js/src/jsapi.cpp
@@ +3329,5 @@
>      return obj->getPrivate();
>  }
>  
>  JS_PUBLIC_API(JSBool)
> +JS_GetPrototype(JSContext *cx, JS::Handle<JSObject*> obj, JS::MutableHandle<JSObject*> protop)

As below.

::: js/src/jsapi.h
@@ +3099,5 @@
>  JS_GetInstancePrivate(JSContext *cx, JSObject *obj, JSClass *clasp,
>                        jsval *argv);
>  
>  extern JS_PUBLIC_API(JSBool)
> +JS_GetPrototype(JSContext *cx, JS::Handle<JSObject*> obj, JS::MutableHandle<JSObject*> protop);

Lets stick with the typedefs for now. I'm not sure how this is going to shake out eventually, but lets at least remain consistent until we get a clear concensus.

Comment 15

[Terrence Cole [:terrence]]

Comment on attachment 765299 [details] [diff] [review]
JS_SetPrototype v0

Review of attachment 765299 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: js/src/jsapi.h
@@ +3102,5 @@
>  extern JS_PUBLIC_API(JSBool)
>  JS_GetPrototype(JSContext *cx, JS::Handle<JSObject*> obj, JS::MutableHandle<JSObject*> protop);
>  
>  extern JS_PUBLIC_API(JSBool)
> +JS_SetPrototype(JSContext *cx, JS::Handle<JSObject*> obj, JS::Handle<JSObject*> proto);

As per the prior patch, lets stick with typedefs for now.

Comment 16

[Tom S [:evilpie]]

Oh damn, I forgot about these patches.

Comment 17

[Terrence Cole [:terrence]]

It seems we're going with external style in jsapi.h, so ignore my review comments.

Comment 18

[Tom S [:evilpie]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f67fd341dd2e
http://hg.mozilla.org/integration/mozilla-inbound/rev/a2f13bbf1457

I had to make some minor changes to the first patch.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f67fd341dd2e
https://hg.mozilla.org/mozilla-central/rev/a2f13bbf1457

Comment 20

[Tom S [:evilpie]]

Attachment: Remove JS_ValueToECMAInt32

Removes JS_ValueToECMAInt32 and replaces it by ToInt32, which already used handles. I had a patch for the uint32 version, but I screwed something up and can't find the changes anymore.

Comment 21

[Terrence Cole [:terrence]]

Comment on attachment 788955 [details] [diff] [review]
Remove JS_ValueToECMAInt32

Review of attachment 788955 [details] [diff] [review]:
-----------------------------------------------------------------

The handlification changes look fine to me.

I do, however, vaguely recall there being some extensive discussion about the naming of the method you are removing: a difference between machine int32 and JS's "Int32" that we wanted to distinguis in the API. Please ask around (guessing Waldo, but I don't remember exactly) or make a search to ensure that this renaming is actually okay.

Comment 22

[Tom S [:evilpie]]

This function was just a wrapper around the ToInt32. ToInt32 also appears in the ES spec. If we still expose some non spec-conform version we should give that one an ugly name.

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 788955 [details] [diff] [review]
Remove JS_ValueToECMAInt32

Review of attachment 788955 [details] [diff] [review]:
-----------------------------------------------------------------

There used to be JS_ValueToInt32 and JS_ValueToECMAInt32 both.  The former did weird clamping stuff, I don't remember.  The latter implemented, exactly, the spec's ToInt32 method.  Since JS::ToInt32 does exactly the spec's method, this looks fine on all those points.  I'm fairly sure that's the distinction being mooted in comment 21.

Comment 24

[Terrence Cole [:terrence]]

Comment on attachment 788955 [details] [diff] [review]
Remove JS_ValueToECMAInt32

Review of attachment 788955 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent! In that case, r=me.

Comment 25

[(no longer active)]

Backed out because of build bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/911fcdcadf4d
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3d9664b78ca7

Comment 26

[Tom S [:evilpie]]

Made some small fix and landed again today:
http://hg.mozilla.org/integration/mozilla-inbound/rev/339041b86edc
Try run: https://tbpl.mozilla.org/?tree=Try&rev=1bb8f3e147e8

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/339041b86edc

Comment 28

[Tom S [:evilpie]]

Attachment: Remove JS_ValueToECMAUint32

Not try test yet, but compiles. Test some of the dumpHeap functionality.

Comment 29

[Terrence Cole [:terrence]]

Comment on attachment 804206 [details] [diff] [review]
Remove JS_ValueToECMAUint32

Review of attachment 804206 [details] [diff] [review]:
-----------------------------------------------------------------

In future, please try to split stuff like this up to make reviewing less context-switchy. It is a righteous cleanup, however, so I won't complain too much. :-)

r=me, but I would like Andrew to take a look at the DumpHeap changes too: he is DumpHeap's owner after all.

::: ipc/testshell/XPCShellEnvironment.cpp
@@ +327,3 @@
>      if (!fileName) {
>          dumpFile = stdout;
>      } else {

ditto

::: js/src/shell/js.cpp
@@ +2205,5 @@
> +                          startThing.isUndefined() ? nullptr : startThing.toGCThing(),
> +                          startThing.isUndefined() ? JSTRACE_OBJECT : startThing.get().gcKind(),
> +                          thingToFind.isUndefined() ? nullptr : thingToFind.toGCThing(),
> +                          maxDepth,
> +                          thingToIgnore.isUndefined() ? nullptr : thingToIgnore.toGCThing());

Has the shell finally gotten the right C++0x flags to compile everywhere with nullptr? I thought there were still some platforms that require an include to get the shim?

::: js/xpconnect/shell/xpcshell.cpp
@@ +534,1 @@
>      } else {

FILE *dumpFile = stdout;
if (fileName) {
    dumpFile = fopen(...);
    ....
}

Comment 30

[Andrew McCreight [:mccr8]]

I can get to it in a day or two, or Bill can probably take a look.

Comment 31

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I would suggest deleting these two implementations of DumpHeap. Would that be okay with you, Andrew?

Comment 32

[Andrew McCreight [:mccr8]]

Oh, that's DumpHeap, not DumpHeapComplete, I don't know much about that.  I think the idea is that you can use it from the gdb and get somewhat useful results from it.  But yeah, I don't know if anybody uses it from xpcshell or the shell like that, so it is probably okay to get rid of.

Comment 33

[Tom S [:evilpie]]

Attachment: JS_WrapObject

Comment 34

[Terrence Cole [:terrence]]

Comment on attachment 816704 [details] [diff] [review]
JS_WrapObject

Review of attachment 816704 [details] [diff] [review]:
-----------------------------------------------------------------

Awesomesauce, deep fried and dipped in more awesomesauce. r=me

Comment 35

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5126b48adf91

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Just for the record, this landed with an incorrect bug number (8844195).
https://hg.mozilla.org/mozilla-central/rev/5126b48adf91

Comment 37

[Tom S [:evilpie]]

How in gods name did I end up with that number? :( Sorry for that.

Comment 38

[Tom S [:evilpie]]

Attachment: Remove JS_ValueToNumber

Not try approved, but I just think this okay.

Comment 39

[Terrence Cole [:terrence]]

Comment on attachment 817851 [details] [diff] [review]
Remove JS_ValueToNumber

Review of attachment 817851 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me with one dev-doc-needed. I'd definitely try-server this before landing though.

::: js/src/jsapi.h
@@ -1041,5 @@
>  extern JS_PUBLIC_API(JSString *)
>  JS_ValueToSource(JSContext *cx, jsval v);
>  
> -extern JS_PUBLIC_API(bool)
> -JS_ValueToNumber(JSContext *cx, jsval v, double *dp);

It looks like JS::ToNumber isn't documented in MDN. Could you add that (maybe just copy the JS_ValueToNumber article), deprecate JS_ValueToNumber and link it to JS::ToNumber.

Comment 40

[Tom S [:evilpie]]

I added some amount of documentation now.
http://hg.mozilla.org/integration/mozilla-inbound/rev/313eee20c52f

Comment 41

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/313eee20c52f

We should probably close this bug.

Comment 42

[Tom S [:evilpie]]

Okay let's close this one.