824589 - Convert XULElement to WebIDL

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This will, I think, allow us to completely nuke nsIDOMNodeSelector and eliminate the nsIDOMElementCSSInlineStyle quickstubs.

This is going to depend on the patch in bug 824481.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Actually, we don't even need to do this to get rid of nsIDOMNodeSelector.  We can just nix it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Er, no.  Rats.  We need it for Xrays (bug 816343), so we have to convert _all_ elements.

But we can certainly nuke the nsIDOMElementCSSInlineStyle quickstubs.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So one open question is how to handle it currently implementing nsIInlineEventHandlers and nsITouchEventReceiver.

Peter, would you prefer I copy/paste those or do them as separate interfaces that XULElement, HTMLElement, and Document all implement via "implements", with [NoInterfaceObject] and skipgen to make sure they don't end up showing up anywhere?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Work in progress; still needs all the event gunk

Comment 5

[Boris Zbarsky [:bzbarsky]]

Per discussion with Peter on irc, I'll try the "implements" approach.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So an issue I ran into.  menu.xml has this bit:

43       <property name="parentContainer" readonly="true">
44         <getter>
45           for (var parent = this.parentNode; parent; parent = parent.parentNode) {
46             if (parent instanceof Components.interfaces.nsIDOMXULContainerElement)
47               return parent;
48           }
49           return null;
50         </getter>
51       </property>

This is always returning null now, since there are no elements with nsIDOMXULContainerElement in their classinfo and we restrict instanceof to classinfo for WebIDL bindings.  Note that that nsIDOMXULContainerElement is XBL-implemented, so not really something we can easily add to classinfo.  :(

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: More WIP.

This is almost green on try except for a CC crash trying to read freed memory.  Not sure why yet.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Still waiting on final try green, but I think this is ready for review

Comment 9

[[e-mail]]

Comment on attachment 702130 [details] [diff] [review]
Still waiting on final try green, but I think this is ready for review

>     nsCOMPtr<nsIDOMXULElement> XULElement(do_QueryInterface(docShellElement));
>-    if (XULElement)
>-      XULElement->GetId(windowElementId);
>+    if (XULElement) {
>+      nsCOMPtr<mozilla::dom::Element> el(do_QueryInterface(docShellElement));
>+      el->GetId(windowElementId);
>+    }
Could save a QI perhaps by using
nsCOMPtr<mozilla::dom::Element> XULElement(do_QueryInterface(docShellElement));
if (XULElement && XULElement->IsXUL())
  XULElement->GetId(windowElementId);

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah, could do that.

So this still has one failure on try:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_tab_dragdrop.js | uncaught exception - TypeError: content is null at chrome://browser/content/browser.js:8534

I don't have any idea why, yet.  :(

Comment 11

[Boris Zbarsky [:bzbarsky]]

So the reason for that is that swapFrameLoaders is no longer an XPConnect method, so when it triggers a pageshow event whose handler throws because it's buggy, NS_ScriptErrorReporter does not see an XPCCallContext on the stack and actually reports the error, which makes the test fail.

I'll add a null-check to the handler so it's not buggy.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Convert XULElement to WebIDL. assertions in nsINode and nsWrapperCache are to eagerly catch

failures to override those methods.

The classinfo change for XULTreeBuilder is needed because one of those
is returned via an nsIXULTemplateBuilder attribute on XULElement.
Alternately, I could mark it notflattened in Bindings.conf, but Enn
said he prefers this anyway.

The change to the QI impl in BindingUtils is needed because when
XPConnect converts an IID from C++ to JS it makes is an nsJSID, not an
nsJSIID.  We've run into this before, sadly.

I removed "id" from nsIDOMXULElement because it's already on Element.
I suppose I could have left it there, but this seems cleaner.

The nsJSIID::HasInstance changes are needed to support XBL-implemented
interfaces.  Sadly, this does mean that if the underlying object QIs
to something but we didn't put those props on the WebIDL we'll end up
testing true for instanceof but not exposing the props.  I don't see
an obviously better way.  We should work on killing off uses of
"instanceof someinterface".

The browser.js change is needed to avoid throwing exceptions during
browser-chrome tests that are now getting reported because our
swapFrameLoaders is no longer an XPConnect method.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to bug 828815

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 702656 [details] [diff] [review]
Merged to bug 828815

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

Do we want to s/nsXULElement/mozilla::dom::XULElement/ too?

Comment 15

[Boris Zbarsky [:bzbarsky]]

I'm not sure.  It's not an actual spec DOM interface, but maybe we should do the rename just for consistency...  I'll file a followup.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Filed bug 835787.

Comment 17

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22695cac3896

Comment 18

[Boris Zbarsky [:bzbarsky]]

And backed out on suspicion of Ts regression: http://hg.mozilla.org/integration/mozilla-inbound/rev/b0ee4b6eb114

I'll try to do some try runs to figure out what's going on here, I guess.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22695cac3896

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(In reply to Boris Zbarsky (:bz) from comment #18)
> And backed out on suspicion of Ts regression:
> http://hg.mozilla.org/integration/mozilla-inbound/rev/b0ee4b6eb114
> 
> I'll try to do some try runs to figure out what's going on here, I guess.

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

Looks like the Ts issue is only Mac 10.8, and that test is _very_ noisy.  Current best guess is that we shifted the timing of some GC a slight bit and now it's being measured...

Talked to Ehsan and going to reland this.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Relanded as https://hg.mozilla.org/integration/mozilla-inbound/rev/134c19beec11

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/134c19beec11