895495 - Add faster way to create JS-implemented WebIDL objects from chrome when the chrome-side object is already known

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The current setup for creating a JS-implemented WebIDL object from chrome is something like:

  var x = new contentWindow.Foo();

What this does is call the Foo constructor, which uses a contractid to createInstance a JS-implemented XPCOM component, then pulls out the JSObject from that component and uses it to create the content-side WebIDL object.

This is slow.  In particular, we create an XPCWrappedJS that we then throw away, we end up with createInstance overhead, scripted QI calls, etc, etc.  Unfortunately, we don't really have a good way right now of creating the chrome-side object in any other way in the general case.

But in many cases the chrome-side code can easily create the right chrome-side object by just calling new FooImpl or some such.

So the proposal is to add a way to create a content-side WebIDL object that uses an explicitly provided chrome-side JSObject.

Possible syntax options:

1) new contentWindow.Foo(chromeobj)
2) new Foo(contentWindow, chromeobj)
3) contentWindow.Foo._createFromExisting(chromeobj)
4) Foo._createFromExisting(contentWindow, chromeobj)

Any other ideas?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Implement option 4, with ._create

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Same thing as diff -w so the whitespace fixes don't get in the way

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Hmm, all the syntax options look quite odd. What if you need to pass some data to the ctor?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

...but, I don't quite understand this. What is chromeobj exactly?

Comment 5

[Boris Zbarsky [:bzbarsky]]

The context where this came up is in implementing the contacts manager.  In that situation, we're in ContactManager.jsm.  We want to create a new Contact object.  Right now we're doing that like so:

  var contact = new contentWindow.Contact();
  contact.init(stuff);

where "init" is a [ChromeOnly] method.  But under the hood this goes through the rigmarole in comment 0 to create a ContactImpl object from that same JSM.  So with this patch, this code will instead look like this:

  var contact = Contact._create(contentWindow, new ContactImpl());
  contact.init(stuff);

If you need to pass data to the constructor, then you can use the |new contentWindow.Contact| thing, which still works, but slower.  Or if the data is not part of the WebIDL constructor exposed to web pages, then you need an init() method like above anyway.

Comment 6

[Andrew McCreight [:mccr8]]

I'll hopefully get to this by early next week.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 777957 [details] [diff] [review]
Implement option 4, with ._create

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

Thanks for the -w diff.

::: dom/bindings/Codegen.py
@@ +8436,5 @@
>          requiresContentUtils = any(d.interface.hasInterfaceObject() for d in descriptors)
>          def descriptorHasChromeOnly(desc):
>              return (any(isChromeOnly(a) for a in desc.interface.members) or
> +                    desc.interface.getExtendedAttribute("ChromeOnly") is not None or
> +                    desc.interface.isJSImplemented())

Does the new condition you added also need to check |descr.interface.hasInterfaceObject()|?

@@ +9456,5 @@
> +            "// GlobalObject will go through wrappers as needed for us, and\n"
> +            "// is simpler than the right UnwrapArg incantation.\n"
> +            "GlobalObject global(cx, &args[0].toObject());\n"
> +            "if (global.Failed()) {\n"
> +            "  return false;\n"

Does the GlobalObject constructor handle throwing the error message?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Does the new condition you added also need to check
> |descr.interface.hasInterfaceObject()|?

It doesn't have to, but it's a good idea.  Fixed.

> Does the GlobalObject constructor handle throwing the error message?

Yes.

Comment 9

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c811f1e3d12

Comment 10

[Reuben Morais [:reuben]]

(In reply to Boris Zbarsky (:bz) from comment #5)
> So with this patch, this code will instead look like this:
> 
>   var contact = Contact._create(contentWindow, new ContactImpl());

Except |Contact| is in contentWindow, so this ends up being:

  contentWindow.Contact._create(contentWindow, new ContactImpl());

Am I missing something here?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Except |Contact| is in contentWindow

Hrm...  Oh, because your global scope is not a Window?

I guess the options are to either do what you have above or to add ContactBinding to the "it's set up for all globals" bit in nsXPConnect::InitClassesWithNewWrappedGlobal.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c811f1e3d12

Comment 13

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

Comment on attachment 777957 [details] [diff] [review]
Implement option 4, with ._create

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

::: dom/bindings/Codegen.py
@@ +9460,5 @@
> +            "  return false;\n"
> +            "}\n"
> +            "nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(global.Get());\n"
> +            "if (!window) {\n"
> +            '  return ThrowErrorMessage(cx, MSG_DOES_NOT_IMPLEMENT_INTERFACE, "Argument 1 of ${ifaceName}._create");\n'

This looks like it needs two strings.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Good catch.  https://hg.mozilla.org/integration/mozilla-inbound/rev/980551965c2a with some claimed review from you.  ;)

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/980551965c2a

Comment 16

[Boris Zbarsky [:bzbarsky]]

Documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Creating_JS-implemented_WebIDL_objects