1446961 - Replace popup boxobject properties with element methods

Status: RESOLVED FIXED

(Core :: XUL, task, P5)

Description

[Neil Deakin]

This is an investigation into removing menupopup/panel boxobjects and replacing them with a subclass or some other mechanism. A subclass of XULElement for XULPopupElement is added. It mostly removes the need for the popup-base binding.

The patches work but some tests fail and there are a couple of remaining issues.

Comment 1

[Neil Deakin]

Attachment: Part 1 - replace obsolete usage of popup methods

Comment 2

[Neil Deakin]

Attachment: Part 2 - add a means to subclass nsXULElement

Some of this code may be removed as I had hoped to put the tag check in the xul prototype node instead, yet that isn't available later when a real element is created. This would need to be rafactored or just allow the extra tagchecks per element.

Comment 3

[Neil Deakin]

Attachment: Part 3 - remove popupboxobject, implement nsXULPopupElement and remove most of popup-base binding

Comment 4

[Neil Deakin]

Attachment: Part 2/3B Alternate using wrapper

For reference, here is an alternate version of using a different object returned from WrapNode instead of subclassing nsXULElement.

Comment 5

[Neil Deakin]

Some simple performance testing shows no different with these patches, or a variation with a really big if block checking other tags.

Comment 6

[Neil Deakin]

Attachment: Part 1 - replace obsolete usage of popup methods

Comment 7

[Neil Deakin]

Attachment: Part 2 - add a means to subclass nsXULElement

Comment 8

[Neil Deakin]

Attachment: Part 3 - remove popupboxobject, implement nsXULPopupElement and remove most of popup-base binding

This version should pass all tests.

Comment 9

[:Paolo Amadini]

Comment on attachment 8965014 [details] [diff] [review]
Part 3 - remove popupboxobject, implement nsXULPopupElement and remove most of popup-base binding

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

::: toolkit/content/widgets/popup.xml
@@ -120,5 @@
> -          try {
> -            popupBox = this.popupBoxObject;
> -          } catch (e) {}
> -          try {
> -            menuBox = this.parentNode.boxObject;

This is checking the parent node for a MenuBoxObject, and using the openMenu method instead if this is found. I don't see this implemented after the conversion. Is this something we don't need anymore?

Comment 10

[Neil Deakin]

No, the menuboxobject ends up calling HidePopup with the same arguments, except now the cancel argument isn't ignored. Compare:

https://searchfox.org/mozilla-central/source/layout/xul/MenuBoxObject.cpp#50
https://searchfox.org/mozilla-central/source/layout/xul/PopupBoxObject.cpp#58

Comment 11

[:Paolo Amadini]

I may be misreading the current code, as it's quite convoluted, but it seems to me that, for the opening case, if the parent element has a MenuBoxObject then the opening process would end up in showMenu instead of showPopup, and the former method does some more things:

https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/layout/xul/nsXULPopupManager.cpp#708

Does this ever happen in practice? Things like buttons of type="menu" currently have a MenuBoxObject.

I remember various instances where moving menus or panels away from a triggering element would cause subtle changes in behavior, so if we could simplify things that would definitely be welcome.

Comment 12

[:Paolo Amadini]

Comment on attachment 8965007 [details] [diff] [review]
Part 1 - replace obsolete usage of popup methods

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

::: layout/base/crashtests/372576.xul
@@ +8,5 @@
>  </script>
>  <toolbox>
>  		<toolbar>
>  			<toolbarbutton type="menu" class="toolbarbutton-1 firefly_files" label="Crash" tooltiptext="Crash">
> +        <menupopup ignorekeys="true">

This is probably an example where the parent has a MenuBoxObject. Can you clarify why this test case had to be changed? Is it unrelated to comment 11?

Comment 13

[:Paolo Amadini]

(I see that enableKeyboardNavigator has been removed, wondering if it's just this.)

Comment 14

[:Paolo Amadini]

Comment on attachment 8965007 [details] [diff] [review]
Part 1 - replace obsolete usage of popup methods

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

This looks good to me, I'll do a final pass tomorrow and I have just a few more minor questions in the meantime.

::: toolkit/content/widgets/browser.xml
@@ -1250,5 @@
>              let popupY = Math.max(minY, Math.min(maxY, screenY));
> -            this._autoScrollPopup.showPopup(document.documentElement,
> -                                            popupX,
> -                                            popupY,
> -                                            "popup", null, null);

Is dropping document.documentElement inconsequential here?

::: toolkit/content/widgets/tree.xml
@@ +1555,5 @@
>          <![CDATA[
>            if (event.originalTarget == this) {
>              var popup = document.getAnonymousElementByAttribute(this, "anonid", "popup");
>              this.buildPopup(popup);
> +            popup.openPopup(this, "after_end");

Is this changing any alignment?

Comment 15

[Neil Deakin]

> Does this ever happen in practice? Things like buttons of type="menu"
> currently have a MenuBoxObject.

I'll add a special case in the part 3 patch for openPopup that calls into showMenu as needed.


> This is probably an example where the parent has a MenuBoxObject. Can you
> clarify why this test case had to be changed? Is it unrelated to comment 11?

I removed enableKeyboardNavigator which just sets the attribute.


> ::: toolkit/content/widgets/browser.xml
> @@ -1250,5 @@
> >              let popupY = Math.max(minY, Math.min(maxY, screenY));
> > -            this._autoScrollPopup.showPopup(document.documentElement,
> > -                                            popupX,
> > -                                            popupY,
> > -                                            "popup", null, null);
> 
> Is dropping document.documentElement inconsequential here?

Shouldn't be. A null anchor means align to the the root nsIFrame.


> ::: toolkit/content/widgets/tree.xml
> @@ +1555,5 @@
> >          <![CDATA[
> >            if (event.originalTarget == this) {
> >              var popup = document.getAnonymousElementByAttribute(this, "anonid", "popup");
> >              this.buildPopup(popup);
> > +            popup.openPopup(this, "after_end");
> 
> Is this changing any alignment?

No, after_end corresponds to an anchor of "bottomright" and an alignment of "topright"

Comment 16

[:Paolo Amadini]

Comment on attachment 8965007 [details] [diff] [review]
Part 1 - replace obsolete usage of popup methods

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

Thanks!

Comment 17

[Neil Deakin]

Gijs, given the bug 1437638 that you filed, do you think that the approach here (creating a subclass of nsXULElement) is sufficient and would be worth moving forward on?

Comment 18

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #17)
> Gijs, given the bug 1437638 that you filed, do you think that the approach
> here (creating a subclass of nsXULElement) is sufficient and would be worth
> moving forward on?

Yep, this seems fine to me.

Comment 19

[Neil Deakin]

Attachment: Part 2 - add a means to subclass nsXULElement

Comment 20

[Neil Deakin]

Attachment: Part 3 - remove deprecated showPopup method

Bug 1445912 removed some of the other unneeded methods.

Comment 21

[Neil Deakin]

Attachment: Part 4 - remove now unused methods now that showPopup has been removed

Comment 22

[Neil Deakin]

Attachment: Part 5 - add support for dictionary argument to openPopup as popup.xml#openPopup does

Comment 23

[Neil Deakin]

Attachment: Part 6 - support two special cases used in popup.xml

Comment 24

[Neil Deakin]

Attachment: Part 7 - remove PopupBoxObject and replace with a nsXULElement subclass

Note that I'm not sure if the label and position attributes are necessary but left them in to be consistent with the existing popup binding API.

Comment 25

[Neil Deakin]

Attachment: Part 8 - test changes

Changes needed for some tests.

Comment 26

[Neil Deakin]

Comment on attachment 8968868 [details] [diff] [review]
Part 3 - remove deprecated showPopup method

dom/webidl changes

Comment 27

[Neil Deakin]

Comment on attachment 8968871 [details] [diff] [review]
Part 6 - support two special cases used in popup.xml

Paolo, not sure if you would be able to review these

Comment 28

[Neil Deakin]

Comment on attachment 8968874 [details] [diff] [review]
Part 7 - remove PopupBoxObject and replace with a nsXULElement subclass

Perhaps I should add the new interface to chrome-webidl instead?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8968867 [details] [diff] [review]
Part 2 - add a means to subclass nsXULElement

Why ConstructElement instead of just Construct?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8968868 [details] [diff] [review]
Part 3 - remove deprecated showPopup method

s/method's/methods'/ in the commit message.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8968870 [details] [diff] [review]
Part 5 - add support for dictionary argument to openPopup as popup.xml#openPopup does

>+  RefPtr<mozilla::dom::Event> triggerEvent = aTriggerEvent;

Do you need the "mozilla::dom::" bit?

>+  void openPopup(optional Element? anchorElement = null,
>+                 optional StringOrOpenPopupOptions options,
>                  long x,

I don't believe this will let you call openPopup with 1 or 2 args...  Have you checked?

>+      UNWRAP_OBJECT(Event, options.mTriggerEvent, triggerEvent);

Why isn't the triggerEvent in the dictionary just an "Event? triggerEvent = null;"?

Comment 32

[:Paolo Amadini]

Comment on attachment 8968869 [details] [diff] [review]
Part 4 - remove now unused methods now that showPopup has been removed

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

These look good to me. The code is in the XPToolkit module so you may want a rubberstamp from bz as well.

Comment 33

[:Paolo Amadini]

Comment on attachment 8968871 [details] [diff] [review]
Part 6 - support two special cases used in popup.xml

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

::: layout/xul/PopupBoxObject.cpp
@@ +81,5 @@
> +    // As a special case for popups that are menus when no anchor or position are
> +    // specified, open the popup with ShowMenu instead of ShowPopup so that the
> +    // popup is aligned with the menu.
> +    if (!aAnchorElement && position.IsEmpty() && mContent && mContent->GetPrimaryFrame()) {
> +      nsMenuFrame* menu = do_QueryFrame(mContent->GetPrimaryFrame()->GetParent());

So this is slightly different from the check in comment 11. Would this work when the parent is a button with type="menu"? If this is the case or it actually doesn't matter, then this looks good to me. However, bz should probably take a look as well.

@@ +92,2 @@
>      nsCOMPtr<nsIContent> anchorContent(do_QueryInterface(aAnchorElement));
>      pm->ShowPopup(mContent, anchorContent, position, aXPos, aYPos,

So mContent can be null here now?

Comment 34

[:Paolo Amadini]

Comment on attachment 8968874 [details] [diff] [review]
Part 7 - remove PopupBoxObject and replace with a nsXULElement subclass

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

r+ on the Toolkit code removal, but I have a comment on the class name checks.

::: toolkit/modules/WindowDraggingUtils.jsm
@@ +51,5 @@
>      }
>      return true;
>    },
>    isPanel() {
> +    return ChromeUtils.getClassName(this._elem) == "XULPopupElement" &&

Can we maybe check the element namespace instead?

There are other places that look for "XULElement", for example in devtools:

https://dxr.mozilla.org/mozilla-central/rev/3cc613bf13443acc2fea4804872fb3ca56757181/devtools/server/actors/inspector/node.js#411

Are we breaking these in some way for XUL panel elements now? This is interesting to know especially for when we start subclassing more elements.

Comment 35

[:Paolo Amadini]

Comment on attachment 8968875 [details] [diff] [review]
Part 8 - test changes

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

Some test changes may apply to code I've not reviewed. I'd actually move each test to the patch that requires it to be changed.

Comment 36

[Boris Zbarsky [:bzbarsky]]

> Are we breaking these in some way for XUL panel elements now? 

Yes.  You should switch them to checking namespace or using XULElement.isInstance().

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8968874 [details] [diff] [review]
Part 7 - remove PopupBoxObject and replace with a nsXULElement subclass

>+++ b/dom/bindings/Bindings.conf
>+    'nativeType': 'nsXULPopupElement',

For new things, please just give them the right name: mozilla::dom::XULPopupElement, defined in XULPopupElement.h, exported to mozilla/dom.  Then you wouldn't need to sprinkle "mozilla::dom::" all over the header, too.

>+    'resultNotAddRefed': ['triggerNode', 'anchorNode'],

This is useless noise.  Looks like it landed in bug 979835 just a few days after bug 849567 made it useless noise...  Please just drop it.

While you're here, want to remove the same annotation from BoxObject too?

>+++ b/dom/webidl/XULPopupElement.webidl

Ah, now the args all become optional...  That should be in the previous patch.

>+++ b/dom/xul/nsXULPopupElement.cpp

This should probably be called XULPopupElement.cpp given the suggested renaming of the header and class above.

>+nsXULElement* NS_NewXULPopupElement(already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo)

Linebreak after return type, please.

>+  nsCOMPtr<nsIContent> hold = this; // keep a reference

Please file a followup to annotate this MOZ_CAN_RUN_SCRIPT so we can remove this bit (because we'll know "this" is alive).  Also, this variable should be named kungFuDeathGrip, probably.

>+  nsCOMPtr<nsIContent> anchorContent(do_QueryInterface(aAnchorElement));

I know you just outdented existing code, but this QI (and the anchorContent thing in general) is not needed.  You can just pass aAnchorElement directly to MoveToAnchor.

>+nsXULPopupElement::SizeTo(int32_t aWidth, int32_t aHeight)

The changes made here may not be safe.  What prevents the SetAttr calls from running script (via mutation events) and killing "this"?

It used to be OK because we held a stack strong ref to "element".  We should keep doing that with a kungFuDeathGrip for now, and annotate MOZ_CAN_RUN_SCRIPT in a followup.

>+++ b/dom/xul/nsXULPopupElement.h
>+nsXULElement* NS_NewXULPopupElement(already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo);

Newline after return type.

>+  virtual JSObject* WrapNode(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) override;

Per code style, drop the "virtual".

You need to audit places that use the string "XULElement" in our code to see whether they should be testing for one of these two possible classes now.  This includes stuff using ChromeUtils.getClassName(), stuff using .class in devtools code, and things using ExtensionUtils.instanceOf.  Stuff using "instanceof XULElement" will keep working for now, I guess...

r=me if the above issues are addressed.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8968871 [details] [diff] [review]
Part 6 - support two special cases used in popup.xml

r=me, though I have only middling confidence that I understand the popup/menu interactions here...

Comment 39

[Neil Deakin]

> Why isn't the triggerEvent in the dictionary just an "Event? triggerEvent =
> null;"?

I tried that and got a error about OpenPopupOptions not being cycle collectable.

Comment 40

[Boris Zbarsky [:bzbarsky]]

> I tried that and got a error about OpenPopupOptions not being cycle collectable.

What was the exact error you got?  What was the stack to it?

Comment 41

[Neil Deakin]

> What was the exact error you got?  What was the stack to it?

The error is at https://searchfox.org/mozilla-central/source/dom/bindings/Codegen.py#95

 0:06.80   File "/builds/working/dom/bindings/Codegen.py", line 13783, in __init__
 0:06.80     unlinkMethods, unionStructs) = UnionTypes(unionTypes, config)
 0:06.80   File "/builds/working/dom/bindings/Codegen.py", line 1416, in UnionTypes
 0:06.80     if idlTypeNeedsCycleCollection(t):
 0:06.80   File "/builds/working/dom/bindings/Codegen.py", line 88, in idlTypeNeedsCycleCollection
 0:06.80     return any(idlTypeNeedsCycleCollection(t) for t in type.flatMemberTypes)
 0:06.80   File "/builds/working/dom/bindings/Codegen.py", line 88, in <genexpr>
 0:06.80     return any(idlTypeNeedsCycleCollection(t) for t in type.flatMemberTypes)
 0:06.80   File "/builds/working/dom/bindings/Codegen.py", line 95, in idlTypeNeedsCycleCollection
 0:06.80     raise TypeError("Cycle collection for type %s is not supported" % type)

Comment 42

[Boris Zbarsky [:bzbarsky]]

Ah, because you're using the dictionary in a union, even though you never store the union anywhere that needs cycle collection...

I filed bug 1456261 on making that work.  Patch should be up in a few minutes.

Comment 43

[Neil Deakin]

Attachment: Part 5.2 - add support for dictionary argument to openPopup as popup.xml#openPopup does

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8970844 [details] [diff] [review]
Part 5.2 - add support for dictionary argument to openPopup as popup.xml#openPopup does

r=me.  Thank you!

Comment 45

[Neil Deakin]

Attachment: Fixes devtools cases

This is to get the devtools cases to work, although it is unclear when this check would be done.

Other cases where XULElement is checked are never popups/panels.

Comment 46

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8971220 [details] [diff] [review]
Fixes devtools cases

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

Up to you whether you want to address the nits.

::: devtools/server/actors/inspector/node.js
@@ +407,5 @@
>      let type = listener.type || "";
>      let url = "";
>  
>      // If the listener is an object with a 'handleEvent' method, use that.
> +    if (listenerDO.class === "Object" || listenerDO.class.match(/^XUL\w*Element$/)) {

NIT:

if (listenerDO.class === "Object" || /^XUL\w*Element$/.test(listenerDO.class)) {

Would be slightly more performant.

::: devtools/server/actors/thread.js
@@ +1150,5 @@
>          }
>          // Get the Debugger.Object for the listener object.
>          let listenerDO = this.globalDebugObject.makeDebuggeeValue(listener);
>          // If the listener is an object with a 'handleEvent' method, use that.
> +        if (listenerDO.class == "Object" || listenerDO.class.match(/^XUL\wElement$/)) {

NIT:
 
if (listenerDO.class === "Object" || /^XUL\w*Element$/.test(listenerDO.class)) {
 
Would be slightly more performant.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e872787876a5
remove obsolete calls to showPopup and replace usages of the popup box object with the same methods defined on popups, r=paolo
https://hg.mozilla.org/integration/mozilla-inbound/rev/5be83a2594fb
restructuring to allow nsXULElement to be subclassed. Rename nsXULElement::Create to make it clearer it creates from the prototype element, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/5659ad69e145
remove deprecated showPopup method of PopupBoxObject as well as unused GetPopupSetFrame method, and move some methods' positions to group related methods together better, r=paolo,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/962c081b1314
remove unused popup frame methods now that showPopup has been removed, r=paolo
https://hg.mozilla.org/integration/mozilla-inbound/rev/46d7f3cc7102
add dictionary second argument directly to PopupBoxObject::OpenPopup as supported in popup.xml#openPopup, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/12e768652a88
add two special cases in PopupBoxObject as supported in popup.xml, r=paolo,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b49df2389f
move PopupBoxObject to XULPopupElement, a new subclass of XULElement. Remove popup.xml methods, r=paolo,bz

Comment 48

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/e872787876a5
https://hg.mozilla.org/mozilla-central/rev/5be83a2594fb
https://hg.mozilla.org/mozilla-central/rev/5659ad69e145
https://hg.mozilla.org/mozilla-central/rev/962c081b1314
https://hg.mozilla.org/mozilla-central/rev/46d7f3cc7102
https://hg.mozilla.org/mozilla-central/rev/12e768652a88
https://hg.mozilla.org/mozilla-central/rev/19b49df2389f