Consider removing all or most of nsIDOMXULElement

RESOLVED FIXED in Firefox 54

Status

()

Core
XUL
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
mozilla54
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(7 attachments)

8.00 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.87 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.21 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.17 KB, patch
smaug
: review+
Details | Diff | Splinter Review
19.37 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.12 KB, patch
smaug
: review+
Details | Diff | Splinter Review
18.01 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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?
Depends on: 824589
Created attachment 8829055 [details] [diff] [review]
part 1.  Switch nsIDOMXULElement::GetBuilder consumers to nsXULElement instead
Attachment #8829055 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8829056 [details] [diff] [review]
part 2.  Switch nsIDOMXULElement::GetDatabase consumers to nsXULElement instead
Attachment #8829056 - Flags: review?(peterv)
Created attachment 8829057 [details] [diff] [review]
part 3.  Switch nsIDOMXULElement::DoCommand consumers to nsXULElement instead
Attachment #8829057 - Flags: review?(peterv)
Created attachment 8829058 [details] [diff] [review]
part 4.  Change nsXULElement::ClickWithInputSource to return void and switch consumers of nsIDOMXULElement::Click to nsXULElement
Attachment #8829058 - Flags: review?(peterv)
Created attachment 8829059 [details] [diff] [review]
part 5.  Change nsIDOMXULElement::GetBoxObject consumers to nsXULElement
Attachment #8829059 - Flags: review?(peterv)
Created attachment 8829060 [details] [diff] [review]
part 6.  Change nsIDOMXULElement::GetControllers consumers to nsXULElement
Attachment #8829060 - Flags: review?(peterv)
Created attachment 8829061 [details] [diff] [review]
part 7.  Remove all the remaining bits of nsIDOMXULElement
Attachment #8829061 - Flags: review?(peterv)
Blocks: 1332812
Happy to review here if peterv is busy/away for awhile.
Put the patches to my queue.
Attachment #8829055 - Flags: review?(peterv) → review?(bugs)
Attachment #8829056 - Flags: review?(peterv) → review?(bugs)
Attachment #8829057 - Flags: review?(peterv) → review?(bugs)
Attachment #8829058 - Flags: review?(peterv) → review?(bugs)
Attachment #8829059 - Flags: review?(peterv) → review?(bugs)
Attachment #8829060 - Flags: review?(peterv) → review?(bugs)
Attachment #8829061 - Flags: review?(peterv) → review?(bugs)
As discussed, all auto usage should become RefPtr<XULElement>
Comment on attachment 8829055 [details] [diff] [review]
part 1.  Switch nsIDOMXULElement::GetBuilder consumers to nsXULElement instead

replace all 'auto' with RefPtr<nsXULElement>
Attachment #8829055 - Flags: review?(bugs) → review+
Comment on attachment 8829056 [details] [diff] [review]
part 2.  Switch nsIDOMXULElement::GetDatabase consumers to nsXULElement instead

replace all 'auto' with RefPtr<nsXULElement>
Attachment #8829056 - Flags: review?(bugs) → review+
Comment on attachment 8829057 [details] [diff] [review]
part 3.  Switch nsIDOMXULElement::DoCommand consumers to nsXULElement instead

replace all 'auto' with RefPtr<nsXULElement>
Attachment #8829057 - Flags: review?(bugs) → review+
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
Attachment #8829058 - Flags: review?(bugs) → review+
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)
Attachment #8829059 - Flags: review?(bugs) → review+
Attachment #8829060 - Flags: review?(bugs) → review+
Attachment #8829061 - Flags: review?(bugs) → review+
> 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.
Flags: needinfo?(bugs)
ok, fine then with click()
Flags: needinfo?(bugs)

Comment 17

10 months ago
Pushed by bzbarsky@mozilla.com:
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

10 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

10 months ago
Depends on: 1336768
You need to log in before you can comment on or make changes to this bug.