[de-xbl] convert/rework the tabmail-alltabs-popup binding
Categories
(Thunderbird :: Toolbars and Tabs, task, P2)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: aleca)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
aleca
:
review+
|
Details | Diff | Splinter Review |
For Firefox this got removed and reworked starting with bug 1446101.
Looks like most of it is currently in https://searchfox.org/comm-central/source/mozilla/browser/base/content/browser-allTabsMenu.js
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
First patch to simply de-xbl the all tabs popup.
This CE extends the menupopup
element, which is a CE itself and doesn't depend on the soon to be removed popup parent.
Next step is to see if the Firefox approach can be integrated.
Assignee | ||
Updated•5 years ago
|
Reporter | ||
Comment 3•5 years ago
|
||
Comment on attachment 9079154 [details] [diff] [review] 1554627-tabmail-alltabs-popup.patch Review of attachment 9079154 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good. All things considering I wouldn't spend too much time following Firefox here (our tabs impl. is anyway too different) ::: mail/base/content/tabmail.js @@ +8,5 @@ > + > +{ > + /** > + * The MozTabmailAlltabsPopup widget is used as a menupopup > + * to list all the currently opened tabs. please utilize the full 80 ch @@ +11,5 @@ > + * The MozTabmailAlltabsPopup widget is used as a menupopup > + * to list all the currently opened tabs. > + * > + * @extends {MozElements.MozMenuPopup} > + */ and @implements {EventListener} @@ +12,5 @@ > + * to list all the currently opened tabs. > + * > + * @extends {MozElements.MozMenuPopup} > + */ > + no blank line before the class decl please @@ +65,5 @@ > + } > + > + this.tabmail = document.getElementById("tabmail"); > + > + this._mutationObserver = null; remove @@ +67,5 @@ > + this.tabmail = document.getElementById("tabmail"); > + > + this._mutationObserver = null; > + > + this._mutationObserver = new MutationObserver((aRecords, aObserver) => { let's also remove the a-namings in this class, while we're here @@ +83,5 @@ > + } > + > + _tabOnTabClose(aEvent) { > + let menuItem = aEvent.target.mCorrespondingMenuitem; > + if (menuItem) and add braces where missing even for one line ifs @@ +128,5 @@ > + } > + > + _createTabMenuItem(aTab) { > + let menuItem = document.createElementNS( > + "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "menuitem"); createXULElement @@ +176,5 @@ > + } > + } > + } > + > + customElements.define("tabmail-alltabs-popup", MozTabmailAlltabsPopup, let's name it tabmail-alltabs-menupopup since it is actaully a menupopup (and update the class name accordingly)
Assignee | ||
Comment 4•5 years ago
|
||
Sounds good to me in going with our own and more simpler approach.
Ready for a full review.
Assignee | ||
Updated•5 years ago
|
Comment 5•5 years ago
|
||
Comment on attachment 9079417 [details] [diff] [review] 1554627-tabmail-alltabs-popup.patch Review of attachment 9079417 [details] [diff] [review]: ----------------------------------------------------------------- I'd r+ this but I'd like some changes and want to see them before they land. ::: mail/base/content/tabmail.js @@ +17,5 @@ > + class MozTabmailAlltabsMenuPopup extends MozElements.MozMenuPopup { > + constructor() { > + super(); > + > + this.addEventListener("popupshowing", (event) => { I think it makes more sense for these listeners to be added in connectedCallback. @@ +114,5 @@ > + return; > + > + let tabstripBO = tabContainer.arrowScrollbox; > + for (let i = 0; i < this.childNodes.length; i++) { > + let curTabBO = this.childNodes[i].tab; While we're here, let's rename these variables to tabstrip (tabStrip?) and currentTab. The "BO" tells of an implementation detail long gone that really doesn't matter any more. @@ +117,5 @@ > + for (let i = 0; i < this.childNodes.length; i++) { > + let curTabBO = this.childNodes[i].tab; > + if (curTabBO.screenX >= tabstripBO.screenX && > + curTabBO.screenX + curTabBO.getBoundingClientRect().width <= > + tabstripBO.screenX + tabstripBO.getBoundingClientRect().width) { Oh, yuck. I know you didn't write this but let's make it better. :-) Get the bounding rectangle of the strip and the tab and just compare the .left and .right properties.
Assignee | ||
Comment 6•5 years ago
|
||
Thanks for the detailed review.
Here's the updated patch.
Comment 7•5 years ago
|
||
Comment on attachment 9080074 [details] [diff] [review] 1554627-tabmail-alltabs-popup.patch Review of attachment 9080074 [details] [diff] [review]: ----------------------------------------------------------------- Nice, just one minor change. ::: mail/base/content/tabmail.js @@ +112,5 @@ > + } > + > + for (let i = 0; i < this.childNodes.length; i++) { > + let currentTabBox = this.childNodes[i].tab.getBoundingClientRect(); > + let tabStripBox = tabStrip.getBoundingClientRect(); This line can go outside the loop so that it only runs once.
Assignee | ||
Comment 8•5 years ago
|
||
Sweet, thanks.
Doing a try run now.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b3d2fcdf48dd72e2788dc756e603924b50194623
Assignee | ||
Comment 9•5 years ago
|
||
Patch rebased from trunk and the new try push looks good, other than the intermittent X2
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=88654cc7058ec86844bc7592599b5374ef34c0c4
Assignee | ||
Updated•5 years ago
|
Comment 10•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c04d43d17059
[de-xbl] convert/rework the tabmail-alltabs-popup binding. r=mkmelin
Updated•5 years ago
|
Description
•