1554627 - [de-xbl] convert/rework the tabmail-alltabs-popup binding

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, task, P2)

Description

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/rev/5a670c59f9004ef9be4874cfbfe57ec2ef3b260f/mail/base/content/tabmail.xml#2778

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

Comment 1

[Magnus Melin [:mkmelin]]

The parent-class popup is getting removed in bug 1555497.

Comment 2

[Alessandro Castellani [:aleca]]

Attachment: 1554627-tabmail-alltabs-popup.patch

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.

Comment 3

[Magnus Melin [:mkmelin]]

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)

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: 1554627-tabmail-alltabs-popup.patch

Sounds good to me in going with our own and more simpler approach.
Ready for a full review.

Comment 5

[Geoff Lankow (:darktrojan)]

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.

Comment 6

[Alessandro Castellani [:aleca]]

Attachment: 1554627-tabmail-alltabs-popup.patch

Thanks for the detailed review.
Here's the updated patch.

Comment 7

[Geoff Lankow (:darktrojan)]

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.

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1554627-tabmail-alltabs-popup.patch

Sweet, thanks.
Doing a try run now.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b3d2fcdf48dd72e2788dc756e603924b50194623

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: 1554627-tabmail-alltabs-popup.patch

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c04d43d17059
[de-xbl] convert/rework the tabmail-alltabs-popup binding. r=mkmelin