Closed
Bug 1446961
Opened 7 years ago
Closed 6 years ago
Replace popup boxobject properties with element methods
Categories
(Core :: XUL, task, P5)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: enndeakin, Assigned: enndeakin)
References
Details
Attachments
(9 files, 7 obsolete files)
(deleted),
patch
|
Paolo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Paolo
:
review+
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Paolo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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 | ||
Comment 1•7 years ago
|
||
Assignee: nobody → enndeakin
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.
Assignee | ||
Comment 3•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Summary: Try replacing panel boxobject properties with element methods → Try replacing popup boxobject properties with element methods
Updated•7 years ago
|
Blocks: war-on-xbl
Updated•7 years ago
|
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.
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8960141 -
Attachment is obsolete: true
Attachment #8965007 -
Flags: review?(paolo.mozmail)
Updated•7 years ago
|
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
This version should pass all tests.
Attachment #8960143 -
Attachment is obsolete: true
Attachment #8960144 -
Attachment is obsolete: true
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?
Updated•7 years ago
|
Flags: needinfo?(enndeakin)
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
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
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?
Comment 13•7 years ago
|
||
(I see that enableKeyboardNavigator has been removed, wondering if it's just this.)
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?
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"
Flags: needinfo?(enndeakin)
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!
Attachment #8965007 -
Flags: review?(paolo.mozmail) → review+
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?
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 19•6 years ago
|
||
Attachment #8960146 -
Attachment is obsolete: true
Attachment #8965013 -
Attachment is obsolete: true
Attachment #8965014 -
Attachment is obsolete: true
Assignee | ||
Comment 20•6 years ago
|
||
Bug 1445912 removed some of the other unneeded methods.
Assignee | ||
Comment 21•6 years ago
|
||
Assignee | ||
Comment 22•6 years ago
|
||
Assignee | ||
Comment 23•6 years ago
|
||
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.
Assignee | ||
Comment 25•6 years ago
|
||
Changes needed for some tests.
Assignee | ||
Updated•6 years ago
|
Attachment #8968867 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•6 years ago
|
Attachment #8968868 -
Attachment is patch: true
Attachment #8968868 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 26•6 years ago
|
||
Comment on attachment 8968868 [details] [diff] [review] Part 3 - remove deprecated showPopup method dom/webidl changes
Attachment #8968868 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•6 years ago
|
Attachment #8968869 -
Flags: review?(paolo.mozmail)
Assignee | ||
Updated•6 years ago
|
Attachment #8968870 -
Flags: review?(bzbarsky)
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
Attachment #8968871 -
Flags: review?(paolo.mozmail)
Assignee | ||
Updated•6 years ago
|
Attachment #8968875 -
Flags: review?(paolo.mozmail)
Assignee | ||
Updated•6 years ago
|
Attachment #8968874 -
Flags: review?(paolo.mozmail)
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?
Attachment #8968874 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•6 years ago
|
Priority: -- → P5
![]() |
||
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?
Attachment #8968867 -
Flags: review?(bzbarsky) → review+
![]() |
||
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.
Attachment #8968868 -
Flags: review?(bzbarsky) → review+
![]() |
||
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;"?
Attachment #8968870 -
Flags: review?(bzbarsky) → review-
Updated•6 years ago
|
Attachment #8968868 -
Flags: review?(paolo.mozmail) → review+
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.
Attachment #8968869 -
Flags: review?(paolo.mozmail) → review+
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?
Attachment #8968871 -
Flags: review?(paolo.mozmail) → review?(bzbarsky)
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.
Attachment #8968874 -
Flags: review?(paolo.mozmail)
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.
Attachment #8968875 -
Flags: review?(paolo.mozmail)
![]() |
||
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().
![]() |
||
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.
Attachment #8968874 -
Flags: review?(bzbarsky) → review+
![]() |
||
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...
Attachment #8968871 -
Flags: review?(bzbarsky) → review+
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.
![]() |
||
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?
Flags: needinfo?(enndeakin)
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)
Flags: needinfo?(enndeakin)
![]() |
||
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.
Assignee | ||
Comment 43•6 years ago
|
||
Attachment #8968870 -
Attachment is obsolete: true
Attachment #8970844 -
Flags: review?(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!
Attachment #8970844 -
Flags: review?(bzbarsky) → review+
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.
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+
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
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
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Summary: Try replacing popup boxobject properties with element methods → Replace popup boxobject properties with element methods
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•