Magnus Melin [:mkmelin] Reporter
	Comment 5 • 5 years ago

Comment on attachment 9082722 [details] [diff] [review]
1554629-dexbl-tabmail.patch

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

Added a patch to apply on top of this.
Anyway, it's very possibly following firefox would be better. I suspect a rather large change unfortunately.

Should hg mv tabmail to preserve some history.

::: mail/base/content/tabmail.js
@@ +1319,5 @@
> +      return aIndex;
> +    }
> +
> +    persistTab(tab) {
> +      /* Returns null in case persist fails */

put this comment into the function documentation instaed

@@ +1375,5 @@
> +       *  safe to use in an auto-save style so that if we crash we can
> +       *  restore the (approximate) state at the time of the crash.
> +       *
> +       * @return {Object} The persisted tab states.
> +       */

move this document up where it belongs

@@ +1435,5 @@
> +       * Attempts to restore tabs persisted from a prior call to
> +       *  |persistTabs|.  This is currently a synchronous operation, but in
> +       *  the future this may kick off an asynchronous mechanism to restore
> +       *  the tabs one-by-one.
> +       */

this too: move up

@@ +1521,5 @@
> +    }
> +
> +    /**
> +     * getBrowserForDocumentId is used to find the browser for a specific
> +     * document via its id attribute

add . and remove the function name from the wordings

@@ +1559,5 @@
> +      this.tabContainer.selectedIndex = iTab;
> +    }
> +
> +    /**
> +     * UpdateCurrentTab - called in response to changing the current tab

same here

	Magnus Melin [:mkmelin] Reporter
	Comment 9 • 5 years ago

Comment on attachment 9083176 [details] [diff] [review]
1554629-dexbl-tabmail.patch

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

::: calendar/lightning/content/messenger-overlay-sidebar.js
@@ +168,5 @@
>  
>  /**
>   * For details about tab info objects and the tabmail interface see:
>   * comm-central/mail/base/content/mailTabs.js
> + * comm-central/mail/base/content/tabmail.js

can we change these two to just "comm", not comm-central

::: mail/base/content/tabmail.js
@@ +1163,5 @@
> +    closeOtherTabs(aTabNode, aNoUndo) {
> +      /**
> +       * Given a tabNode (or tabby thing), close all of the other tabs
> +       *  that are closeable.
> +       */

this should also be moved to the function documentation

@@ +1184,5 @@
> +      if (!tab.canClose) {
> +        return null;
> +      }
> +
> +      // We use Json and session restore transfer the tab to the new window.

JSON

@@ +1793,5 @@
> +      window.controllers.removeController(this.tabController);
> +    }
> +  }
> +
> +  customElements.define("tabmail", MozTabmail);

usually CSs should have hypen in the name. But, I'm not sure it's worth changing in this case. Longer term I think we just need to align with firefox, but I think we can leave it for now - it's not worse than before..

Can you update the comment here not to talk about XBL binging but Custom Element: https://searchfox.org/comm-central/rev/b781990cc914c195aa2b1fdfb78dc2fe470b8c04/mail/base/content/messenger.xul#479