835981 - Consider removing all or most of nsIDOMXULElement

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Now that XULElement is on WebIDL bindings, nsIDOMXULElement is only relevant for C++ consumers and instanceof checks.

Can we convert the former to using nsXULElement directly and work on making nsIDOMXULElement an empty interface?

Are there extension compat worries here?  Can binary extensions use nsXULElement if needed?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Switch nsIDOMXULElement::GetBuilder consumers to nsXULElement instead

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Switch nsIDOMXULElement::GetDatabase consumers to nsXULElement instead

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Switch nsIDOMXULElement::DoCommand consumers to nsXULElement instead

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Change nsXULElement::ClickWithInputSource to return void and switch consumers of nsIDOMXULElement::Click to nsXULElement

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Change nsIDOMXULElement::GetBoxObject consumers to nsXULElement

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Change nsIDOMXULElement::GetControllers consumers to nsXULElement

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Remove all the remaining bits of nsIDOMXULElement

Comment 8

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

Happy to review here if peterv is busy/away for awhile.
Put the patches to my queue.

Comment 9

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

As discussed, all auto usage should become RefPtr<XULElement>

Comment 10

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

Comment on attachment 8829055 [details] [diff] [review]
part 1.  Switch nsIDOMXULElement::GetBuilder consumers to nsXULElement instead

replace all 'auto' with RefPtr<nsXULElement>

Comment 11

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

Comment on attachment 8829056 [details] [diff] [review]
part 2.  Switch nsIDOMXULElement::GetDatabase consumers to nsXULElement instead

replace all 'auto' with RefPtr<nsXULElement>

Comment 12

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

Comment on attachment 8829057 [details] [diff] [review]
part 3.  Switch nsIDOMXULElement::DoCommand consumers to nsXULElement instead

replace all 'auto' with RefPtr<nsXULElement>

Comment 13

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

Comment on attachment 8829058 [details] [diff] [review]
part 4.  Change nsXULElement::ClickWithInputSource to return void and switch consumers of nsIDOMXULElement::Click to nsXULElement

Do we still use XULElements for <video> or <audio>? Do we possibly use click() there? If so, dispatching non-trusted from native anonymous content would be wrong.

Please don't change the behavior in this patch, so leave NeedsCallerType out and always dispatch trusted events, as is done currently.

and RefPtr, not auto

Comment 14

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

Comment on attachment 8829059 [details] [diff] [review]
part 5.  Change nsIDOMXULElement::GetBoxObject consumers to nsXULElement

> nsCoreUtils::GetTreeBodyBoxObject(nsITreeBoxObject *aTreeBoxObj)
> {
>   nsCOMPtr<nsIDOMElement> tcElm;
>   aTreeBoxObj->GetTreeBody(getter_AddRefs(tcElm));
>-  nsCOMPtr<nsIDOMXULElement> tcXULElm(do_QueryInterface(tcElm));
>-  if (!tcXULElm)
>+  nsCOMPtr<nsIContent> tcContent(do_QueryInterface(tcElm));
>+  if (!tcContent)
>     return nullptr;
>+  auto tcXULElm = nsXULElement::FromContent(tcContent);
a11y folks tend to like new line after 'if'

>     multiSelect->GetCurrentIndex(&currentIndex);
>     if (currentIndex >= 0) {
>-      nsCOMPtr<nsIDOMXULElement> xulElement(do_QueryInterface(aCurrentEl));
>+      auto xulElement = nsXULElement::FromContent(focusedContent);
RefPtr

>   if (IsXULListBox(aContainer) &&
>       aChild->IsXULElement(nsGkAtoms::listitem)) {
>-    nsCOMPtr<nsIDOMXULElement> xulElement = do_QueryInterface(aContainer);
>-    nsCOMPtr<nsIBoxObject> boxObject;
>-    xulElement->GetBoxObject(getter_AddRefs(boxObject));
>+    auto xulElement = nsXULElement::FromContent(aContainer);
RefPtr

>   int32_t junk;
>   aTreeBox->GetCoordsForCellItem(aRow, aCol, EmptyCString(), aX, aY, &junk, &junk);
>-  nsCOMPtr<nsIDOMXULElement> xulEl(do_QueryInterface(aSourceNode));
>-  nsCOMPtr<nsIBoxObject> bx;
>-  xulEl->GetBoxObject(getter_AddRefs(bx));
>+  auto xulEl = nsXULElement::FromContent(aSourceNode);
RefPtr

>   nsIContent* parent = mContent->GetParent();
>   if (parent) {
>     nsIContent* grandParent = parent->GetParent();
>-    nsCOMPtr<nsIDOMXULElement> treeElement = do_QueryInterface(grandParent);
>+    auto treeElement = nsXULElement::FromContentOrNull(grandParent);
RefPtr

(RefPtr since I'm not sure if GetBoxObject may trigger some JS to run, like flush layout or something)

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Do we still use XULElements for <video> or <audio>?

Yes.  See toolkit/content/widgets/videocontrols.xml.

> Do we possibly use click() there?

We don't seem to.  But more to the point, I'm not changing behavior.  We used to have two Click() methods:

1)  The XPCOM method.  This was called from precisely one spot in our codebase, XULTabAccessible::DoAction, and always dispatched a trusted event.

2)  The WebIDL method.  This dispatched a trusted event if and only if nsContentUtils::IsCallerChrome().

In the new setup we only have the WebIDL method, it dispatches a trusted event if and only if aCallerType == CallerType::System (which is equivalent for scripted callers), and XULTabAccessible::DoAction calls it with CallerType::System, thus preserving the existing behavior at that callsite.

> nsCoreUtils::GetTreeBodyBoxObject

OK, will do.

Will fix all the RefPtr bits.

Comment 16

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

ok, fine then with click()

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dba36028e64
part 1.  Switch nsIDOMXULElement::GetBuilder consumers to nsXULElement instead.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/c32788f892f3
part 2.  Switch nsIDOMXULElement::GetDatabase consumers to nsXULElement instead.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb91c8b6aac2
part 3.  Switch nsIDOMXULElement::DoCommand consumers to nsXULElement instead.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec6cfcc02aa5
part 4.  Change nsXULElement::ClickWithInputSource to return void and switch consumers of nsIDOMXULElement::Click to nsXULElement.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4cafa51586a
part 5.  Change nsIDOMXULElement::GetBoxObject consumers to nsXULElement.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/befd0f485232
part 6.  Change nsIDOMXULElement::GetControllers consumers to nsXULElement.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/59a111dcc25c
part 7.  Remove all the remaining bits of nsIDOMXULElement.  r=peterv

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3dba36028e64
https://hg.mozilla.org/mozilla-central/rev/c32788f892f3
https://hg.mozilla.org/mozilla-central/rev/cb91c8b6aac2
https://hg.mozilla.org/mozilla-central/rev/ec6cfcc02aa5
https://hg.mozilla.org/mozilla-central/rev/d4cafa51586a
https://hg.mozilla.org/mozilla-central/rev/befd0f485232
https://hg.mozilla.org/mozilla-central/rev/59a111dcc25c