1275315 - Make the JS_Get*Prototype signatures consistent

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now we have:

 JS_GetFunctionPrototype(JSContext* cx, JS::HandleObject forObj);
 JS_GetObjectPrototype(JSContext* cx, JS::HandleObject forObj);
 JS_GetArrayPrototype(JSContext* cx, JS::HandleObject forObj);
 JS_GetErrorPrototype(JSContext* cx);
 JS_GetIteratorPrototype(JSContext* cx);

The last two get it from cx->global().  The first three assert cx and forObj same-compartment then use forObj->global().

Oh, and they're inconsistent in terms of global().getOrCreateFunctionPrototype() vs GlobalObject::getOrCreateArrayPrototype(cx, global)...  Might be nice to align the impls too, I guess.

Or is there a compat reason we want to leave the current setup?

Comment 1

[Jeff Walden [:Waldo]]

There's certainly no reason to leave things as they are.  The question is what's best.

Definitely GO::goC... is best for implementation.  Static functions that take handles are always preferable to this-functions that aren't obviously GC-safe.

As to whether to expose "give me this realm's Error.prototype" or "give me the Error.prototype for the realm of this object": seems like, if we make entering a realm/entering a compartment a clear enough operation, then "this realm" semantics seem adequate for both cases.  And at a skim, it looks like most users in Gecko are asking for "this realm" semantics.  So maybe we should centralize on current-realm semantics.

Let's add js/public/Realm.h, initially containing

  namespace JS {

  extern JS_PUBLIC_API(bool)
  GetRealmObjectPrototype(JSContext* cx);
  ...and all the rest...
  
  } // namespace JS

and then define these functions, cleanly, in js/src/vm/Realm.cpp.  There's other stuff we could move into realm-land, like global-object access, but this seems like an easy start and mostly on-topic for this bug.

(I could also go for a JS::realm namespace.  Not sure if it's worth it enough or not.  I'm not horribly happy about poor-man's namespacing everything with "Realm" in the function names.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a Realm.h that defines getters for some standard prototype objects

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Use the new Realm getters in binding code

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8761910 [details] [diff] [review]
part 1.  Add a Realm.h that defines getters for some standard prototype objects

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

::: js/public/Realm.h
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* Ways to get various per-Realm objects. All the getters declared in this
> +   header operate on the Realm corresponding to the current compartment on the
> +   JSContext. */

/*
 * Ways to get...
 * ...
 */

::: js/src/vm/Realm.cpp
@@ +15,5 @@
> +JS_PUBLIC_API(JSObject*)
> +JS::GetRealmObjectPrototype(JSContext* cx)
> +{
> +  CHECK_REQUEST(cx);
> +  return GlobalObject::getOrCreateObjectPrototype(cx, cx->global());

Four-space indent for all these.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Done, both of those.

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 8761911 [details] [diff] [review]
part 2.  Use the new Realm getters in binding code

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

::: dom/bindings/DOMJSClass.h
@@ +359,2 @@
>  /**
> + * Returns a handle to the relevent WebIDL prototype object for current

...*the* current?

While you're here, it think it's "relevant".

::: dom/promise/Promise.cpp
@@ +1668,5 @@
>  
>    // Step 3.  We want to use aCalleeGlobal here because it will do the
>    // right thing for us via Xrays (where we won't find @@species on
>    // our promise constructor for now).
>    JS::Rooted<JSObject*> calleeGlobal(aCx, aCalleeGlobal);

We might be able to remove calleeGlobal now?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> ...*the* current?
> While you're here, it think it's "relevant".

Yes on both counts.

> We might be able to remove calleeGlobal now?

Indeed, good catch.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/408eb09d21b4
part 1.  Add a Realm.h that defines getters for some standard prototype objects.  r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/617d7849ad73
part 2.  Use the new Realm getters in binding code.  r=peterv

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cedef6f7c497
followup.  Actually include all the headers we need, to fix non-unified builds. r=bustage

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/408eb09d21b4
https://hg.mozilla.org/mozilla-central/rev/617d7849ad73
https://hg.mozilla.org/mozilla-central/rev/cedef6f7c497