1461742 - Provide a way to use nsIDOMXUL* interfaces with DOM nodes

Status: RESOLVED FIXED

(Core :: XUL, task, P5)

Description

[Brian Grinstead [:bgrins]]

Spinning this out from a discussion in #content (https://mozilla.logbot.info/content/20180515#c14758524).

The problem is that we can't directly use the [implements] machinery for Custom Elements as per https://bugzilla.mozilla.org/show_bug.cgi?id=1404420#c15. There's some special-cased XBL code to handle this, and we don't want to replicate that for Custom Elements.

We discussed an alternative to using QI that looks sort of like:

- We should have a way of asking IsFoo() (e.g. IsButton()) on XULElement.
- We should have a way of doing AsButton() on XULElement, which presumably either asserts IsButton() or returns null if not IsButton().
- The return value of AsButton() is a thing that lets you call button functionality implemented in JS on the node.

We have 2 options for allowing the return value to call into JS:
1) Keep having an XPCOM interface like now, and basically have AsButton() do the WrapJSAggregatedToNative thing with the right IID and return an already_AddRefed<nsIWhateverWeCallIt>. We could even keep the existing nsIDOMXUL* interfaces, like nsIDOMXULButtonElement.
2) Define a WebIDL callback interface and create an instance of that around the JSObject, and return already_AddRefed<OurWebidlThing>

There was a general consensus that option 1 would make migrating from XBL easier. And making migration easy seems like the right thing to optimize for, since a number of these consumers may ultimately be removable (or at least able to be simplified). For example, if the only consumer is accessibility we may really only need to check `IsFoo()` and then read some info from the DOM rather than calling into JS.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I seem to recall Enn having some thoughts about this.

Comment 2

[ExE Boss]

Well, there’s the future reserved `implements` keyword¹, which could work for this if it will be used in the way I think it is (to allow ES6 classes to implement interfaces in the same way as Java classes can implement Java interfaces, which would make sense given that JavaScript has a Java/C‑esque syntax), but I don’t know how far along that particular proposal is, or if it’s even being worked on at all.

¹ https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#Future_reserved_keywords

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to ExE Boss from comment #2)

Thanks for your comment; this bug is about having native code (the rest of Firefox that is not in JavaScript) access to the JS-implemented parts. I don't recall any JS feature being developed could be related in this regard. I would invite you to join us on IRC and other places so you could have better context. But then again, yes, we strive to retain more structured information like problem statement/alternative proposals/etc on Bugzilla for future reference.

Comment 4

[Neil Deakin]

Attachment: Prototype of how this might work

Comment 5

[Neil Deakin]

Attachment: Prototype of how this might work

Comment 6

[Neil Deakin]

Attachment: Sample change for nsIDOMXULControlElement with sample test

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Neil Deakin from comment #6)
> Created attachment 8977055 [details] [diff] [review]
> Sample change for nsIDOMXULControlElement with sample test

That looks pretty straightforward from the JS side, and would also give flexibility to change the interface applied to an element without C++ changes. My main concern is that if looks like it would require us to migrate the entire subtree of XBL inheritance all at one time. That is, once we change the C++ consumers to queryinterface via nsContentUtils::CallGetHelper, that code will no longer work for still-implemented-in-xbl controls that do [implements=nsIDOMXULControlElement], right?

The reasoning around the proposal to move this behavior more into C++ (in this case we'd have an IsControlElement() and AsControlElement()) was that we could rip out XBL [implements] on bindings before converting them to CE. Which would allow us to convert an individual control element (say, colorpicker or listcell) to CE without breaking others that inherit from basecontrol.

If your prototype could be extended so that existing XBL bindings could also be migrated away from [implements] and to getHelperCallback / QI I think that would achieve the same goal.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Please don't add the getHelper thing unconditionally.  At the very least it should be [ChromeOnly].  Otherwise it's web-observable and a spec violation (and will in fact cause web platform test failures).

For the rest, I really do think the right API here is an Element or XULElement method per interface we want to return.  They can be implemented under the hood via some sort of helper mechanism, but having consumers need to nsContentUtils::CallGetHelper is pretty annoying.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Triage: code cleanup

Comment 10

[Neil Deakin]

Attachment: Part 1 - Updated prototype

Comment 11

[Neil Deakin]

Attachment: Part 2 - function to apply a helper to a class

Comment 12

[Neil Deakin]

Attachment: Part 3 - sample of how getHelper would be used

Comment 13

[Neil Deakin]

Two things to still work out, suggestions welcome:

1. The name 'getHelper' isn't very good.
2. In part 2, a function is added so that the helper can be added to any custom element class. This uses a proxy object to allow the interface methods to apply directly to the class. Unfortunately, you can't seem to use the methods on a proxy for an element directly. So methods have to use a 'thisElement' property to get the element in two different ways depending on the caller. Alternately, separate wrapper methods could be created.

Comment 14

[Brian Grinstead [:bgrins]]

AFAICT this approach would still require that we migrate the whole subtree of bindings to Custom Elements at once, right? IOW, will calling code from C++ that changes from do_QueryInterface to GetExtendedInterface no longer work for XBL bindings that  implement that interface?

Comment 15

[Neil Deakin]

If a getHelper method doesn't exist, then it falls back to QueryInterface. In the sample in part 3, a <fakebutton> implements nsIDOMXULControlElement but other elements that use implements="nsIDOMXULControlElement" will still work.

Naturally, we wouldn't actually be able to remove base-control while derived bindings that use it still exist.

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Neil Deakin from comment #15)
> If a getHelper method doesn't exist, then it falls back to QueryInterface.

OK that's good, that was my main concern.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Hey - are you the right person to give feedback to Neil to help him answer his questions in comment 13? If not, do you know who would that be?

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8985069 [details] [diff] [review]
Part 3 - sample of how getHelper would be used

Discussed this with Mossop earlier - forwarding the request (also see part 2 which implements the helper function used here).

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 8985068 [details] [diff] [review]
Part 2 - function to apply a helper to a class

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

::: toolkit/content/customElements.js
@@ +72,5 @@
> +   * @param names
> +   *        Array of string interface names
> +   */
> +  static implementHelper(cls, names) {
> +    let ifaces = names.map(name => Ci[names]);

Might be nice to throw an error if Ci[names] is not defined.

@@ +75,5 @@
> +  static implementHelper(cls, names) {
> +    let ifaces = names.map(name => Ci[names]);
> +    cls.prototype.QueryInterface = ChromeUtils.generateQI(ifaces);
> +    cls.prototype.getHelperCallback = function getHelperCallback(name) {
> +      if (names.includes(name) != -1) {

names.includes returns a boolean.

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 8985069 [details] [diff] [review]
Part 3 - sample of how getHelper would be used

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

::: dom/base/nsGlobalWindowInner.cpp
@@ +7518,5 @@
>  {
>  #ifdef MOZ_XUL
>    // Don't snap to a disabled button.
> +  nsCOMPtr<nsIDOMXULControlElement> xulControl;
> +  (&aDefaultButton)->GetExtendedInterface(NS_LITERAL_STRING("nsIDOMXULControlElement"), xulControl);

I guess the one comment I'd make here is that I'm surprised this doesn't follow the pattern of our other code and use getter_AddRefs.

Comment 21

[Dave Townsend [:mossop]]

(In reply to Neil Deakin from comment #13)
> Two things to still work out, suggestions welcome:
> 
> 1. The name 'getHelper' isn't very good.

You mean getHelperCallback? Really this is doing something similar to nsIInterfaceRequestor.getInterface. Maybe it should just be called getInterface? Though I know that isn't exactly right because we're passing a string rather than an iid.

> 2. In part 2, a function is added so that the helper can be added to any
> custom element class. This uses a proxy object to allow the interface
> methods to apply directly to the class. Unfortunately, you can't seem to use
> the methods on a proxy for an element directly. So methods have to use a
> 'thisElement' property to get the element in two different ways depending on
> the caller. Alternately, separate wrapper methods could be created.

As I recall you can, but "this" isn't defined correctly. I can think of two things you can do, one is to return a whole new object and at creation time check the methods defined on the original class an make forwarding methods on the new object. The other is to use the proxy approach but register the `get` trap and if the property is a function then call the original method with func.apply(originalThis, args).

Does that make sense?

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8985069 [details] [diff] [review]
Part 3 - sample of how getHelper would be used

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

::: toolkit/content/widgets/general.js
@@ +76,2 @@
>  }
> +MozXULElement.implementHelper(Control, ["nsIDOMXULControlElement"]);

How would we handle inherited helpers? For example the basecontrol binding implements nsIDOMXULControlElement and basetext (which extends basecontrol) explicitly implements nsIDOMXULLabeledControlElement, but ends up implementing both nsIDOMXULControlElement and nsIDOMXULLabeledControlElement due to XBL inheritance.

Would JS class inheritance along with this helper provide the same thing, or would the subclass have to explicitly implement both?

Comment 23

[Neil Deakin]

Attachment: Part 1 - custom element support for custom interfaces

Comment 24

[Neil Deakin]

Attachment: Part 2 - helper method to add interface implementations to custom elements

Comment 25

[Neil Deakin]

Attachment: Sample showing test

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8991951 [details] [diff] [review]
Part 1 - custom element support for custom interfaces

>+++ b/dom/base/CustomElementRegistry.cpp
>+already_AddRefed<nsISupports>
>+CustomElementRegistry::CallGetCustomInterface(JSContext* aCx,

I would prefer we not pass in a JSContext here.  The reason for that is that we don't care which Realm the JSContext is in (because nsXPConnect::WrapJS doesn't care), but callers have no way to know that.  So it's very non-obvious at callsites that just passing in a default-initialized AutoJSAPI is ok.

>+  if (!aElement) {

How would aElement be null here?  The only caller passes "this".

>+  if (nsContentUtils::IsChromeDoc(aElement->OwnerDoc())) {
>+    CustomElementDefinition* definition = aElement->GetCustomElementDefinition();
>+    if (definition && definition->mCallbacks &&
>+        definition->mLocalName == aElement->NodeInfo()->NameAtom()) {
>+      if (definition->mCallbacks->mGetCustomInterfaceCallback.WasPassed()) {

Why not just include the WasPassed() test in our existing if?

>+        CallbackFunction* func = definition->mCallbacks->mGetCustomInterfaceCallback.Value();

How about:

  auto* func = definition->mCallbacks->mGetCustomInterfaceCallback.Value();

since .Value() here returns a LifecycleGetCustomInterfaceCallback*.  Then you won't need the static_cast below.

>+        if (func) {

Why do we want to allow null here?  I'd remove the '?' in the idl and then you know "func" is not null.

>+          JS::Rooted<JSObject*> getCustomInterface(aCx);

If we're not passing a JSContext, then this can use RootingCx().

>+          static_cast<LifecycleGetCustomInterfaceCallback *>(func)->Call(aElement, aName, &getCustomInterface);

"getCustomInterface" is not the getter.  It's the actual "interface implementation", right?  It should probably be named "customInterface" or something.

>+            nsCOMPtr<nsISupports> wrapper;
>+            nsXPConnect* xpc = nsXPConnect::XPConnect();
>+            xpc->WrapJS(aCx, getCustomInterface, NS_GET_IID(nsISupports), getter_AddRefs(wrapper));
>+            return wrapper.forget();

Do we want to WrapJS, or explicitly call nsXPCWrappedJS::GetNewOrUsed?  Mostly this affects what happens if the callback returns a reflector for a C++ thing.  Do we want to unwrap, or double-wrap?

Either way, going back to the original element via QI will not work, right?  That sees ok, as long as we carefully audit code for attempts to do that.

>+++ b/dom/base/CustomElementRegistry.h

Please watch the 80-char boundary in the comments and code here.

>+   * This returns null if aTarget is not from a chrome document.

s/aTarget/aElement/?

>+++ b/dom/base/Element.cpp
>+Element::GetCustomInterface(JSContext* aCx, nsIID aType,

Again, should not take JSContext.

>diff --git a/dom/base/Element.h b/dom/base/Element.h
>+  /**
>+   * GetCustomInterface is somewhat like a QueryInterface, but it is expected
>+   * that the implementation is provided by a custom element or via the 
>+   * the XBL implements keyword.

It's really more like GetInterface, not QI, since you can't go back to the element from the result.

I'm still not convinced the right public API on elements is an unbounded set of strings.  I would much prefer it if we had a set of "element types" we care about, exposed public API for those (again, GetAsButton() or whatnot) and mapped it to strings as needed under the hood when calling a private Element method.

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8991953 [details] [diff] [review]
Part 2 - helper method to add interface implementations to custom elements

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

Please see the question in Comment 22 as well

::: toolkit/content/customElements.js
@@ +73,5 @@
> +   * @param names
> +   *        Array of string interface names
> +   */
> +  static implementCustomInterface(cls, names) {
> +    let ifaces = names.map(name => {

Rather than passing strings and mapping them into Ci[name] how about we pass an array of Ci[name]? So for the test case in the next patch, instead of:

MozXULElement.implementCustomInterface(Control, ["nsIDOMXULControlElement"]);

it would be:

MozXULElement.implementCustomInterface(Control, [Ci.nsIDOMXULControlElement]);

This would be more similar to other Ci accesses.

If we still want to receive `name` into getCustomInterfaceCallback we could build up a set of names via something like:


```
static implementCustomInterface(cls, ifaces) {
  let names = new Set(ifaces.map(iface=>iface.name)));
  ...
```

And then update the call from `names.includes(name)` to `names.has(name)`

@@ +83,5 @@
> +
> +    cls.prototype.QueryInterface = ChromeUtils.generateQI(ifaces);
> +    cls.prototype.getCustomInterfaceCallback = function getCustomInterfaceCallback(name) {
> +      if (names.includes(name)) {
> +        if (!this._EIhelper) {

I'm not sure what _EIhelper means. Is there another name that would represent what this is doing - maybe `customInterfaceProxy`?

@@ +84,5 @@
> +    cls.prototype.QueryInterface = ChromeUtils.generateQI(ifaces);
> +    cls.prototype.getCustomInterfaceCallback = function getCustomInterfaceCallback(name) {
> +      if (names.includes(name)) {
> +        if (!this._EIhelper) {
> +          this._EIhelper = new Proxy(this, {

it looks like the Proxy doesn't rely on anything specific passed into getCustomInterfaceCallback or implementCustomInterface. If that's the case, can you break this out into a function below like:

```
function getCustomInterfaceProxy(obj) {
  return new Proxy(obj, {
    ...
  });
}
```

Then change the call to:

```
if (!this._EIhelper) {
  this._EIhelper = getCustomInterfaceProxy(this);
}
```

That'll make it easier to read and provide a bit of documentation in the form of the function name about what this is doing.

@@ +88,5 @@
> +          this._EIhelper = new Proxy(this, {
> +            get(target, prop, receiver) {
> +              let propOrMethod = target[prop];
> +              if (typeof propOrMethod == "function") {
> +                /* eslint-disable no-undef */

MozQueryInterface should really be added to the eslint environment. Or at the least we should put this at the top of the file after the license block instead of disabling no-undef here

/* globals MozQueryInterface */

@@ +92,5 @@
> +                /* eslint-disable no-undef */
> +                if (propOrMethod instanceof MozQueryInterface) {
> +                  return Reflect.get(target, prop, receiver);
> +                }
> +                /* eslint-enable no-undef */

AFAICT this eslint comment is unneeded - please remove it

Comment 28

[Neil Deakin]

> Would JS class inheritance along with this helper provide the same thing, or
> would the subclass have to explicitly implement both?

Ideally, it would handle inheritance, but I'm not sure if there is a way to retrieve this. Otherwise, one could just add each interface to the list.

Comment 29

[Neil Deakin]

Attachment: Part 1 - custom element support for custom interfaces

I also changed the methods to use IIDs instead of string interface names.

Comment 30

[Neil Deakin]

Attachment: Part 2 - helper method to add interface implementations to custom elements

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8992798 [details] [diff] [review]
Part 1 - custom element support for custom interfaces

>+++ b/dom/base/CustomElementRegistry.h
>+   * 'getCustomInterfaceCallback'. This method takes an IID -- and returns an

s/-- //

>+   * object which implements an XPCOM interface. If this doesn't exist,

"If there is no getCustomInterfaceCallback or the callback doesn't return an object"?

r=me.  Thank you!

Comment 32

[Brian Grinstead [:bgrins]]

Comment on attachment 8992799 [details] [diff] [review]
Part 2 - helper method to add interface implementations to custom elements

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

Code change looks good. I'd like to have a check in test_custom_element_base.xul for this behavior. I know it'll be covered implicitly as we migrate bindings, but I'd prefer we have something direct as well that confirms from JS that calling getCustomInterfaceCallback on the element with a matching interface returns the proxy and otherwise returns null.

Comment 33

[Neil Deakin]

Attachment: Test added to test_custom_element_base.xul

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30bd77de7bf5
add a mechanism so that custom elements can implement interfaces akin to XBL implements. This is accomplished by an additional chrome-only callback getCustomInterface that can be implemented by custom elements, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca5415dbc51a
shared method to indicate that a custom element implements one or more interfaces, r=bgrins

Comment 35

[Natalia Csoregi [:nataliaCs]]

Backed out for build bustage on CustomElementRegistry.cpp:97

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/17dd12ac3d4e9c1d5a13da773cc88a9c143955ec

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7eb107c36789ef57de4aeb4549b6433d71286319

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=188911820&repo=mozilla-inbound&lineNumber=18196

[task 2018-07-19T10:31:57.104Z] 10:31:57     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/dom/base'
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -B /builds/worker/workspace/build/src/gcc/bin -o Unified_cpp_dom_base0.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DHAVE_SIDEBAR -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/obj-firefox/dom/base -I/builds/worker/workspace/build/src/dom/battery -I/builds/worker/workspace/build/src/dom/events -I/builds/worker/workspace/build/src/dom/media -I/builds/worker/workspace/build/src/dom/network -I/builds/worker/workspace/build/src/caps -I/builds/worker/workspace/build/src/docshell/base -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/dom/file -I/builds/worker/workspace/build/src/dom/geolocation -I/builds/worker/workspace/build/src/dom/html -I/builds/worker/workspace/build/src/dom/ipc -I/builds/worker/workspace/build/src/dom/storage -I/builds/worker/workspace/build/src/dom/svg -I/builds/worker/workspace/build/src/dom/u2f -I/builds/worker/workspace/build/src/dom/xbl -I/builds/worker/workspace/build/src/dom/xml -I/builds/worker/workspace/build/src/dom/xslt/xpath -I/builds/worker/workspace/build/src/dom/xul -I/builds/worker/workspace/build/src/gfx/2d -I/builds/worker/workspace/build/src/image -I/builds/worker/workspace/build/src/js/xpconnect/loader -I/builds/worker/workspace/build/src/js/xpconnect/src -I/builds/worker/workspace/build/src/js/xpconnect/wrappers -I/builds/worker/workspace/build/src/layout/base -I/builds/worker/workspace/build/src/layout/forms -I/builds/worker/workspace/build/src/layout/generic -I/builds/worker/workspace/build/src/layout/style -I/builds/worker/workspace/build/src/layout/svg -I/builds/worker/workspace/build/src/layout/xul -I/builds/worker/workspace/build/src/netwerk/base -I/builds/worker/workspace/build/src/security/manager/ssl -I/builds/worker/workspace/build/src/widget -I/builds/worker/workspace/build/src/xpcom/ds -I/builds/worker/workspace/build/src/netwerk/sctp/datachannel -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -U_FORTIFY_SOURCE -fno-common -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wloop-analysis -Wc++1z-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fsanitize=address -U_FORTIFY_SOURCE -fno-common -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -O1 -fno-omit-frame-pointer -Werror -I/builds/worker/workspace/build/src/widget/gtk/compat-gtk3 -pthread -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/freetype2 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/libpng12 -I/usr/include/gtk-3.0/unix-print -Wno-error=shadow  -MD -MP -MF .deps/Unified_cpp_dom_base0.o.pp   /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base0.cpp
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base0.cpp:119:
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -  /builds/worker/workspace/build/src/dom/base/CustomElementRegistry.cpp:97:7: error: use of undeclared identifier 'NS_NOTREACHED'
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -        NS_NOTREACHED("Don't call GetCustomInterface through callback");
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -        ^
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -  /builds/worker/workspace/build/src/dom/base/CustomElementRegistry.cpp:484:7: error: use of undeclared identifier 'NS_NOTREACHED'
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -        NS_NOTREACHED("Don't call GetCustomInterface through callback");
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -        ^
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -  2 errors generated.
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1052: recipe for target 'Unified_cpp_dom_base0.o' failed
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -  make[4]: *** [Unified_cpp_dom_base0.o] Error 1
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/base'
[task 2018-07-19T10:31:57.114Z] 10:31:57     INFO -  make[4]: *** Waiting for unfinished jobs....

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68da215bdf87
add a mechanism so that custom elements can implement interfaces akin to XBL implements. This is accomplished by an additional chrome-only callback getCustomInterface that can be implemented by custom elements, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/19cb7b9936ee
shared method to indicate that a custom element implements one or more interfaces, r=bgrins

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca12371f08ac
test for getCustomInterfaceCallback, r=bgrins

Comment 38

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/68da215bdf87
https://hg.mozilla.org/mozilla-central/rev/19cb7b9936ee

Comment 39

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/ca12371f08ac

Comment 40

[Brian Grinstead [:bgrins]]

Neil, when we convert individual bindings that use [implements] will we need to change C++ code that uses QueryInterface to use CallGetCustomInterface? If so, what do you think about going ahead and mass-replace calls from C++ that reference [implements] that XBL bindings use? That would simplify individual migrations and also allow us to work on frontend-only migrations in artifact builds.

I think this is basically everything highlighted at https://bgrins.github.io/xbl-analysis/tree/#implements. Except maybe nsIObserver which I assume we can typically stop requiring when migrating by listening for notifications from a subproperty on the element.

Comment 41

[Neil Deakin]

I am already working on that and determining whether that is the best approach.