[de-xbl] convert the tabmail binding
Categories
(Thunderbird :: General, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: aleca)
References
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
aleca
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
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.
Assignee | ||
Comment 3•5 years ago
|
||
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.
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
Assignee | ||
Comment 6•5 years ago
|
||
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.
Assignee | ||
Comment 7•5 years ago
|
||
Some spaces and indentation fixes.
Assignee | ||
Comment 8•5 years ago
|
||
The greenest of them all: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=798066b313bd8624a06a26427b66c2c34917feba
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
Assignee | ||
Comment 10•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Comment 11•5 years ago
|
||
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
Updated•5 years ago
|
Description
•