I want to remove all uses of JS_NewObject like "JS_NewObject(cx, nullptr, JS::NullPtr(), JS::NullPtr());" and replace them with JS_NewPlainObject. The end goal here is to always have a proto available when creating a new object.

Comment on attachment 8553988 [details] [diff] [review] v1 - Use NewPlainObject in JS Review of attachment 8553988 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsobj.cpp @@ +2706,5 @@ > > bool > js::FindClassObject(ExclusiveContext *cx, MutableHandleObject protop, const Class *clasp) > { > + MOZ_CRASH("should not be invoked"); Ignore this :)

So, I think this patch doesn't have random other stuff in it.

Comment on attachment 8554021 [details] [diff] [review] v2 - Use NewPlainObject in JS Why NewObjectWithClassProto instead of just: return js::NewObjectWithGivenProto(cx, &PlainObject::class_, cx->global()->getOrCreateObjectPrototype(), cx->global()); seems like that should do less work than NewObjectWithClassProto has to do (all that icky FindClassPrototype junk).

Comment on attachment 8554050 [details] [diff] [review] v1 - Use NewPlainObject in the browser >+ JS::Rooted<JSObject*> components(cx, JS_NewPlainObject(cx)); // XXX Pretty sure this ok? cx and global have the same compartment == same global? Looks like cx and global do indeed end up same-compartment here, based on code inspection. Please add a MOZ_ASSERT to that effect? This looks awesome, r=me (Though, really, if you wanted to add a few void***** arguments to JS_NewPlainObject so people kept having to pass in all those nullptr, we could keep the old ergonomics, right? ;) )

Comment on attachment 8554021 [details] [diff] [review] v2 - Use NewPlainObject in JS Review of attachment 8554021 [details] [diff] [review]: ----------------------------------------------------------------- Great stuff. We should add more of these APIs that abstract away all the legacy garbage in our current APIs, and will over time gradually supplant them.

(In reply to Boris Zbarsky [:bz] from comment #5) > Why NewObjectWithClassProto instead of just: > > return js::NewObjectWithGivenProto(cx, &PlainObject::class_, > > cx->global()->getOrCreateObjectPrototype(), > cx->global()); > > seems like that should do less work than NewObjectWithClassProto has to do > (all that icky FindClassPrototype junk). Fair points, I was just looking for equivalence when reviewing just now. Do make sure to null-check the getOrCreateObjectPrototype call, tho, and store it into a Rooted through the final object's creation.

https://hg.mozilla.org/integration/mozilla-inbound/rev/09d0ebd5f0f4 https://hg.mozilla.org/integration/mozilla-inbound/rev/ff86ca36785b

Unfortunately, NewBuiltinClassInstance ends up calling NewObjectWithClassProto, no? So it's taking the slow path...

https://hg.mozilla.org/mozilla-central/rev/09d0ebd5f0f4 https://hg.mozilla.org/mozilla-central/rev/ff86ca36785b

I am not sure why you think this is the "slow" path. We will just lookup the cached prototype for JSProto_Object. So instead of duplicating this everywhere we let the function basically the same thing. After bug 1125567 this is going to look a lot simpler, which makes this more obvious and has less jumping around in unnecessary functions.

It's the slow path because it involves a bunch of extra OOL function calls. If bug 1125567 helps, great, but I'd like us to compare the two codepaths once that's done...

Tom, can you tech review https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_NewPlainObject and mark this as dev-doc-complete?

Looks good, thanks Florian!