Magnus Melin [:mkmelin] 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)

	Geoff Lankow (:darktrojan)
	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.

	Geoff Lankow (:darktrojan)
	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.