1554629 - [de-xbl] convert the tabmail binding

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

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

FIrefox re-did this a long time ago in bug 1392352. https://hg.mozilla.org/mozilla-central/rev/d5be59a9f5fb

It should be mostly in tabbrowser.js now - https://searchfox.org/comm-central/source/mozilla/browser/base/content/tabbrowser.js

Unclear if we want to semi-copy that or convert the current binding to custom element.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: 1554629-dexbl-tabmail.patch

Here's a first draft of this patch.
It almost works with our tabs system working via customElement.
Still, some errors to take care of, code cleanup, and some bugs caused on the first group of toolbarbuttons that turn out disabled.

I'm asking a quick feedback on this as I'm having some difficulties in understanding how the custom interfaces are implemented, and how/why the _callTabListeners() method is used instead of regular event listeners.

Comment 2

[Alessandro Castellani [:aleca]]

I'm making progress here and I think I figured out how to properly implement controllers and listeners.
I'll upload a new patch for feedback soon.

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: 1554629-dexbl-tabmail.patch

And I'm stuck again.
I'm having issues in implementing the progressListener, and I'm starting to wonder if maybe what's on m-c is a better approach to this.

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: tabmail-changes.patch

This seems to help.

Comment 5

[Magnus Melin [:mkmelin]]

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

Comment 6

[Alessandro Castellani [:aleca]]

Attachment: 1554629-dexbl-tabmail.patch

Thanks for the helper patch, I keep forgetting to use the arrow function when implementing interfaces. I'll remember some day.

I updated the patch, used hg mv to preserve history, and fixed other things.
Everything seems to work now, and I just launched a push to try to see how many red flags we get.

Let me know what you think, if this approach is good or I should start a separated patch to copy what's on m-c.

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1554629-dexbl-tabmail.patch

Some spaces and indentation fixes.

Comment 8

[Alessandro Castellani [:aleca]]

The greenest of them all: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=798066b313bd8624a06a26427b66c2c34917feba

Comment 9

[Magnus Melin [:mkmelin]]

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

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: 1554629-dexbl-tabmail.patch

Longer term I think we just need to align with firefox, but I think we can leave it for now

Agree as this moves forward the de-xbl effort and shouldn't introduce any regression.
The conversion to what FF did can be done in a follow up bug.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/16c3cbd61751
[de-xbl] convert the tabmail binding to a custom element. r=mkmelin