Closed Bug 1446961 Opened 2 years ago Closed Last year

Replace popup boxobject properties with element methods

Categories

(Core :: XUL, task, P5)

task

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Blocks 2 open bugs)

Details

Attachments

(9 files, 7 obsolete files)

23.64 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
14.05 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.87 KB, patch
Paolo
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
6.04 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
4.40 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
50.09 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
13.96 KB, patch
Details | Diff | Splinter Review
8.07 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.00 KB, patch
miker
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → enndeakin
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.
For reference, here is an alternate version of using a different object returned from WrapNode instead of subclassing nsXULElement.
Summary: Try replacing panel boxobject properties with element methods → Try replacing popup boxobject properties with element methods
See Also: → 1437638
Some simple performance testing shows no different with these patches, or a variation with a really big if block checking other tags.
Attachment #8960141 - Attachment is obsolete: true
Attachment #8965007 - Flags: review?(paolo.mozmail)
See Also: → 1430028
This version should pass all tests.
Attachment #8960143 - Attachment is obsolete: true
Attachment #8960144 - Attachment is obsolete: true
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?
Flags: needinfo?(enndeakin)
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
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
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?
(I see that enableKeyboardNavigator has been removed, wondering if it's just this.)
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?
Depends on: 1445912
> 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"
Flags: needinfo?(enndeakin)
Comment on attachment 8965007 [details] [diff] [review]
Part 1 - replace obsolete usage of popup methods

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

Thanks!
Attachment #8965007 - Flags: review?(paolo.mozmail) → review+
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?
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8960146 - Attachment is obsolete: true
Attachment #8965013 - Attachment is obsolete: true
Attachment #8965014 - Attachment is obsolete: true
Bug 1445912 removed some of the other unneeded methods.
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.
Changes needed for some tests.
Attachment #8968867 - Flags: review?(bzbarsky)
Attachment #8968868 - Attachment is patch: true
Attachment #8968868 - Flags: review?(paolo.mozmail)
Comment on attachment 8968868 [details] [diff] [review]
Part 3 - remove deprecated showPopup method

dom/webidl changes
Attachment #8968868 - Flags: review?(bzbarsky)
Attachment #8968869 - Flags: review?(paolo.mozmail)
Attachment #8968870 - Flags: review?(bzbarsky)
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
Attachment #8968871 - Flags: review?(paolo.mozmail)
Attachment #8968875 - Flags: review?(paolo.mozmail)
Attachment #8968874 - Flags: review?(paolo.mozmail)
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?
Attachment #8968874 - Flags: review?(bzbarsky)
Priority: -- → P5
Comment on attachment 8968867 [details] [diff] [review]
Part 2 - add a means to subclass nsXULElement

Why ConstructElement instead of just Construct?
Attachment #8968867 - Flags: review?(bzbarsky) → review+
Comment on attachment 8968868 [details] [diff] [review]
Part 3 - remove deprecated showPopup method

s/method's/methods'/ in the commit message.
Attachment #8968868 - Flags: review?(bzbarsky) → review+
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;"?
Attachment #8968870 - Flags: review?(bzbarsky) → review-
Attachment #8968868 - Flags: review?(paolo.mozmail) → review+
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.
Attachment #8968869 - Flags: review?(paolo.mozmail) → review+
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?
Attachment #8968871 - Flags: review?(paolo.mozmail) → review?(bzbarsky)
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.
Attachment #8968874 - Flags: review?(paolo.mozmail)
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.
Attachment #8968875 - Flags: review?(paolo.mozmail)
> 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 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.
Attachment #8968874 - Flags: review?(bzbarsky) → review+
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...
Attachment #8968871 - Flags: review?(bzbarsky) → review+
> 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.
> 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?
Flags: needinfo?(enndeakin)
> 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)
Flags: needinfo?(enndeakin)
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 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!
Attachment #8970844 - Flags: review?(bzbarsky) → review+
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.
Attachment #8971220 - Flags: review?(mratcliffe)
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.
Attachment #8971220 - Flags: review?(mratcliffe) → review+
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
Summary: Try replacing popup boxobject properties with element methods → Replace popup boxobject properties with element methods
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.