1469902 - Migrate <tabbox> to a Custom Element

Status: RESOLVED FIXED

(Core :: XUL, task, P3)

Description

[Brian Grinstead [:bgrins]]

This looks like a good candidate for Custom Element conversion: https://bgrins.github.io/xbl-analysis/tree/#tabbox

- bound to a single element (tabbox)
- no <content>
- no [implements]

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1469902 - Remove the tab-base binding

Review commit: https://reviewboard.mozilla.org/r/251846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251846/

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1469902 - Migrate <tabbox> to a Custom Element

Review commit: https://reviewboard.mozilla.org/r/251848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251848/

Comment 3

[Brian Grinstead [:bgrins]]

This works, but leaks in debug builds. I think that's due to Bug 1413418: https://treeherder.mozilla.org/#/jobs?repo=try&revision=173e7ed0efd270f79933051d3a13e191f0602aba

Comment 4

[Brian Grinstead [:bgrins]]

Screenshots look good: https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=47e3cd1ce0e9cb5efcbfd0a8931a9ca378581cc6&newProject=try&newRev=1d41d015ceeb670112fc4ca60d4122ed1da939c6

Comment 5

[Brian Grinstead [:bgrins]]

Leak seems to go away with the patch in Bug 1413418 applied.

Before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f2682c84de93c905b8e98afa823270df9418603
After: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6375fb7cfb1001d0b145a101b914c9b4d8046a71

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: Bug 1469902 - Migrate <tabbox> to a Custom Element;r=dao

MozReview-Commit-ID: HNDiMYmKgkg

Comment 7

[Brian Grinstead [:bgrins]]

Arshad, there's one consumer in mail that extends the tabbox xbl binding. It looks like it only changes one property (`tabs`): https://searchfox.org/comm-central/rev/b0a715472ce3bfd64760f0091bddee2ac012434a/mail/base/content/tabmail.xml#1826

I'm fairly sure that could be achieved by setting the [tabcontainer] attribute on the DOM node that gets that binding attached (as per this getter https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/toolkit/content/widgets/tabbox.xml#44-46). If not, you should be able to set an expando property on the DOM node from JS (override node.tabs to be a getter that returns the right thing. Last option would be to port the current tabbox XBL binding over to comm-central and somehow avoid loading the Custom Element from toolkit/.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: Bug 1469902 - hg cp tabbox.xml -> tabbox.js to preserve blame

Comment 9

[Arshad Khan [:arshad]]

Ok! I'll work on this tomorrow. Thanks for letting me know the possible approaches of conversion.

Comment 10

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db833d1a587d1d2a3266b5ed52eb81b25bc542bb

Comment 11

[Brian Grinstead [:bgrins]]

Not seeing anything show up on Talos so far: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f2b4496207583ca8b9f6840f58714d1577a3a37c&newProject=try&newRevision=4e815cb8f9c0315d8c9db0e5fe0a9ef836986537&framework=1&showOnlyImportant=1.

Comment 12

[Brian Grinstead [:bgrins]]

In phab it still shows up as "This revision now requires changes to proceed." but I pushed an update to the patch which I thought would re-flag it for you. Marking needinfo to make sure it shows up on your radar.

Comment 13

[Dão Gottwald [::dao]]

Comment on attachment 9003177 [details]
Bug 1469902 - Migrate <tabbox> to a Custom Element;r=dao

Dão Gottwald [::dao] has approved the revision.

Comment 14

[Dão Gottwald [::dao]]

Comment on attachment 9003189 [details]
Bug 1469902 - hg cp tabbox.xml -> tabbox.js to preserve blame

Dão Gottwald [::dao] has approved the revision.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/373020acb086
Migrate <tabbox> to a Custom Element;r=dao

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/373020acb086

Comment 17

[Jorg K (CEST = GMT+2)]

Hmm, no tabs in Thunderbird any more and no warning :-(

Comment 18

[Tim Nguyen :ntim]

(In reply to Jorg K (GMT+2) from comment #17)
> Hmm, no tabs in Thunderbird any more and no warning :-(

See comment 7.

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to Tim Nguyen :ntim from comment #18)
> See comment 7.
Right, sorry. In the future please CC me and Richard as well.