Closed Bug 824589 Opened 7 years ago Closed 7 years ago

Convert XULElement to WebIDL

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 4 obsolete files)

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.
Actually, we don't even need to do this to get rid of nsIDOMNodeSelector.  We can just nix it.
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.
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?
Flags: needinfo?(peterv)
Per discussion with Peter on irc, I'll try the "implements" approach.
Flags: needinfo?(peterv)
Depends on: 829072
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.  :(
Attached patch More WIP. (obsolete) — Splinter Review
This is almost green on try except for a CC crash trying to read freed memory.  Not sure why yet.
Attachment #695585 - Attachment is obsolete: true
Blocks: 829867
Depends on: CVE-2013-0765
Attachment #701407 - Attachment is obsolete: true
Whiteboard: [need review]
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);
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.  :(
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.
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.
Attachment #702399 - Flags: review?(peterv)
Attachment #702130 - Attachment is obsolete: true
Attachment #702130 - Flags: review?(peterv)
Attachment #702656 - Flags: review?(peterv)
Attachment #702399 - Attachment is obsolete: true
Attachment #702399 - Flags: review?(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?
Attachment #702656 - Flags: review?(peterv) → review+
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.
Blocks: 835787
https://hg.mozilla.org/integration/mozilla-inbound/rev/22695cac3896
Blocks: 835981
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla21
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/22695cac3896
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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
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.
https://hg.mozilla.org/mozilla-central/rev/134c19beec11
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.