Closed Bug 1554629 Opened 6 months ago Closed 3 months ago

[de-xbl] convert the tabmail binding

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 70.0

People

(Reporter: mkmelin, Assigned: aleca)

References

Details

Attachments

(1 file, 5 obsolete files)

Assignee: nobody → alessandro
Attached patch 1554629-dexbl-tabmail.patch (obsolete) — Splinter Review

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.

Attachment #9082473 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attachment #9082473 - Flags: feedback?(mkmelin+mozilla)

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.

Attached patch 1554629-dexbl-tabmail.patch (obsolete) — Splinter Review

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.

Attachment #9082473 - Attachment is obsolete: true
Attachment #9082722 - Flags: feedback?(mkmelin+mozilla)
Attached patch tabmail-changes.patch (obsolete) — Splinter Review

This seems to help.

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
Attachment #9082722 - Flags: feedback?(mkmelin+mozilla)
Attached patch 1554629-dexbl-tabmail.patch (obsolete) — Splinter Review

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.

Attachment #9082722 - Attachment is obsolete: true
Attachment #9082981 - Attachment is obsolete: true
Attachment #9083175 - Flags: review?(mkmelin+mozilla)
Attached patch 1554629-dexbl-tabmail.patch (obsolete) — Splinter Review

Some spaces and indentation fixes.

Attachment #9083175 - Attachment is obsolete: true
Attachment #9083175 - Flags: review?(mkmelin+mozilla)
Attachment #9083176 - Flags: review?(mkmelin+mozilla)
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
Attachment #9083176 - Flags: review?(mkmelin+mozilla) → review+

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.

Attachment #9083176 - Attachment is obsolete: true
Attachment #9083398 - Flags: review+
Keywords: checkin-needed

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

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
You need to log in before you can comment on or make changes to this bug.