Closed Bug 835981 Opened 7 years ago Closed 3 years ago

Consider removing all or most of nsIDOMXULElement

Categories

(Core :: XUL, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(7 files)

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?
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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)
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
Depends on: 1336768
You need to log in before you can comment on or make changes to this bug.