Remove the tabbrowser-tabbox xbl binding

RESOLVED FIXED in Firefox 57

Status

()

P5
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [xbl-flatten-inheritance])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
The tabbrowser-tabbox binding extends tabbox and overrides a single property ("tabs"). There is only one instance of this binding, which is used in #tabbrowser.

Here's what it extends:

#tab-base (tabbox.xml)
   #tabbox (tabbox.xml)
      #tabbrowser-tabbox (tabbrowser.xml)

The binding itself and the CSS attaching it to .tabbrowser-tabbox could be removed relatively easily by allowing the #tabbox binding to receive a [tabcontainer] attribute and adding a conditional to its "tabs" property.
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
Not sure if this is something we'd want to land while we still have an option to enable legacy addons, as there are references to the binding in the addons repo: https://dxr.mozilla.org/addons/search?q=tabbrowser-tabbox&redirect=false
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Not sure if this is something we'd want to land while we still have an
> option to enable legacy addons, as there are references to the binding in
> the addons repo:
> https://dxr.mozilla.org/addons/search?q=tabbrowser-tabbox&redirect=false

My understanding is that the option to enable legacy add-ons will remain around for some time, but it's completely unsupported, and we don't need to maintain any compatibility just for their sake.

So, I believe you can ignore them here.
(Assignee)

Comment 4

2 years ago
Comment on attachment 8902475 [details]
Bug 1394975 - Remove the tabbrowser-tabbox xbl binding;

For context, this isn't required to land for the prototype in Bug 1392352, it's just something I noticed when debugging that issue and changed as a refactor in that series. IMO this is an improvement since it removes one extra binding and a layer of inheritence by sending an already existing attribute down to the tabbox instead. I don't expect any noticeable perf change from this - the old code did a `document.getBindingParent(this)` and the new code does a `this.hasAttribute()` instead for the "tabs" getter. If you like this change I'm happy to land it, but we can also wontfix if you like the current implementation more.
Attachment #8902475 - Flags: review?(dtownsend)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8902475 [details]
Bug 1394975 - Remove the tabbrowser-tabbox xbl binding;

https://reviewboard.mozilla.org/r/174040/#review179744
Attachment #8902475 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 7

2 years ago
A talos push [0] shows some small improvements in "tabpaint opt e10s stylo" and "tp5o_scroll opt e10s linux64-stylo" but I wouldn't be surprised if that was just noise. There's a concerning 90% regression in "bloom_basic opt e10s windows7-32". That test "ensures that we correctly fast-reject ancestor selectors that clearly don't match any ancestor elements" [1].  It does look like there's a fair bit of noise in that test in the last 14 days on m-c [2] and the 3.38 value in the push seems within the normal range there, so maybe it was just bad luck.

[0]: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=210465f03b58ab8119c1c801188cb751fb55c708&newProject=try&newRevision=a1cb684659e666a355a03e704331808a8a88e2cd&framework=1&showOnlyImportant=0&showOnlyConfident=1
[1]: https://wiki.mozilla.org/Buildbot/Talos/Tests#bloom_basic
[2]: https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,1498303,1,1
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED

Comment 9

2 years ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58f4ecda0d4a
Remove the tabbrowser-tabbox xbl binding;r=mossop
https://hg.mozilla.org/mozilla-central/rev/58f4ecda0d4a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Updated

2 years ago
Blocks: 1397874
(Assignee)

Updated

a year ago
Whiteboard: [xbl-flatten-inheritance]
You need to log in before you can comment on or make changes to this bug.