Neil Deakin Assignee
	Description • 7 years ago

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.

	Neil Deakin Assignee
	Comment 2 • 7 years ago

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.

	Neil Deakin Assignee
	Comment 4 • 7 years ago

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

	Neil Deakin Assignee
	Comment 5 • 7 years ago

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

	Neil Deakin Assignee
	Comment 8 • 7 years ago

This version should pass all tests.

	:Paolo Amadini
	Comment 9 • 7 years ago

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?

	Neil Deakin Assignee
	Comment 10 • 7 years ago

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

	:Paolo Amadini
	Comment 11 • 7 years ago

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.

	:Paolo Amadini
	Comment 12 • 7 years ago

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?

	:Paolo Amadini
	Comment 13 • 7 years ago

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

	:Paolo Amadini
	Comment 14 • 7 years ago

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?

	Neil Deakin Assignee
	Comment 15 • 7 years ago

> 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"

	:Paolo Amadini
	Comment 16 • 7 years ago

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

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

Thanks!

	Neil Deakin Assignee
	Comment 17 • 6 years ago

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?

	:Gijs (he/him)
	Comment 18 • 6 years ago

(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.

	Neil Deakin Assignee
	Comment 20 • 6 years ago

Bug 1445912 removed some of the other unneeded methods.

	Neil Deakin Assignee
	Comment 24 • 6 years ago

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.

	Neil Deakin Assignee
	Comment 25 • 6 years ago

Changes needed for some tests.

	Neil Deakin Assignee
	Comment 26 • 6 years ago

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

dom/webidl changes

	Neil Deakin Assignee
	Comment 27 • 6 years ago

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

	Neil Deakin Assignee
	Comment 28 • 6 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 6 years ago

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

Why ConstructElement instead of just Construct?

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 6 years ago

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

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

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 6 years ago

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;"?

	:Paolo Amadini
	Comment 32 • 6 years ago

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.

	:Paolo Amadini
	Comment 33 • 6 years ago

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?

	:Paolo Amadini
	Comment 34 • 6 years ago

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.

	:Paolo Amadini
	Comment 35 • 6 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 6 years ago

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

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

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 6 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 38 • 6 years ago

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...

	Neil Deakin Assignee
	Comment 39 • 6 years ago

> 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.

	Boris Zbarsky [:bzbarsky]
	Comment 40 • 6 years ago

> 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?

	Neil Deakin Assignee
	Comment 41 • 6 years ago

> 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)

	Boris Zbarsky [:bzbarsky]
	Comment 42 • 6 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 44 • 6 years ago

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!

	Neil Deakin Assignee
	Comment 45 • 6 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 46 • 6 years ago

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.

	Pulsebot
	Comment 47 • 6 years ago

Pushed by neil@mozilla.com:
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

	Andreea Pavel [:apavel]
	Comment 48 • 6 years ago
bugherder

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