Closed Bug 1442582 Opened 6 years ago Closed 6 years ago

Remove the tabbrowser-tabpanels binding

Categories

(Firefox :: Tabbed Browser, task, P2)

Product:
Firefox
Component:
Tabbed Browser
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: dao, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1442582 - Remove the tabbrowser-tabpanels binding.
59 bytes, text/x-review-board-request
dao
: review+
Details 
      No description provided.
Comment on attachment 8955461 [details]
Bug 1442582 - Remove the tabbrowser-tabpanels binding.

Hmm, I think we can take a more radical approach: remove the tabbrowser-tabpanels binding
Attachment #8955461 - Attachment is obsolete: true
Attachment #8955461 - Flags: review?(mconley)
Blocks: war-on-xbl
Summary: Avoid creating an async tab switcher when attempting to select a tab that is already selected → Remove the tabbrowser-tabpanels binding
Priority: -- → P1
Blocks: 1443849
Comment on attachment 8955461 [details]
Bug 1442582 - Remove the tabbrowser-tabpanels binding.

https://reviewboard.mozilla.org/r/224630/#review231738

This is great, thank you!
Attachment #8955461 - Flags: review?(mconley) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d679c9a9afa0
Remove the tabbrowser-tabpanels binding. r=mconley
Comment on attachment 8955461 [details]
Bug 1442582 - Remove the tabbrowser-tabpanels binding.

https://reviewboard.mozilla.org/r/224630/#review231746

::: testing/mochitest/browser-test.js:247
(Diff revision 4)
>    // The selector for just this element
>    function immediateSelector(element) {
> -    if (element.localName == "notificationbox" && element.parentNode &&
> -        element.parentNode.classList.contains("tabbrowser-tabpanels")) {
> +    if (element.localName == "notificationbox" &&
> +        element.parentNode &&
> +        element.parentNode.parentNode &&
> +        element.parentNode.parentNode.classList.contains("tabbrowser-tabbox")) {

This should have been element.parentNode.id == "tabbrowser-tabpanels" since bug 1442651 landed in the meantime.

Not sure if this will cause a failure. I'd push a followup but autoland makes this hard. If this fails gracefully, I'll push a followup to inbound tomorrow.
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/af2dffa14ef9
Backed out changeset d679c9a9afa0 for mochitest browser chrome failures at browser_ext_tabs_create.js on a CLOSED TREE
I'm getting all kinds of failures that I didn't get last week:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=160d6494e22dfa09956a772d958e573d6097b7b6
Depends on: 1445949
Depends on: 1445948
No longer blocks: 1443849
Blocks: 1476769
I still see failures on a rebased version of the patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d02408fa0717d4cb4f2e18d5f00c8173dc66d60e&selectedJob=188819863.

Dão, is there any chance you could take a look at this and try to get it re-landed?
Flags: needinfo?(dao+bmo)
I couldn't figure out the failures last time I checked (let alone why I got suddenly a lot more of them). I was going to take another look but never got around to it... and I probably won't in the near future, so unassigning for now. I'll leave the needinfo in place as a reminder, but feel free to pick this up in the meantime.
Assignee: dao+bmo → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2
It's not actually clear to me that the `_getSwitcher().requestTab` code needs to run unconditionally as opposed to moving it into the existing "select" callback which only fires when the panel actually changes, but this patch keeps the current semantics.
I can try moving it into the "select" listener if you'd prefer it there.
(In reply to Brian Grinstead [:bgrins] from comment #21)
> It's not actually clear to me that the `_getSwitcher().requestTab` code
> needs to run unconditionally as opposed to moving it into the existing
> "select" callback which only fires when the panel actually changes,

Right, that wasn't clear to me either. My patch tried to change that and perhaps this uncovered some deeper AsyncTabSwitcher issues.

(In reply to Brian Grinstead [:bgrins] from comment #22)
> I can try moving it into the "select" listener if you'd prefer it there.

Certainly would be nice to get this cleaned up or at least understand why things need to be set up this way.
(In reply to Dão Gottwald [::dao] from comment #23)
> (In reply to Brian Grinstead [:bgrins] from comment #21)
> > It's not actually clear to me that the `_getSwitcher().requestTab` code
> > needs to run unconditionally as opposed to moving it into the existing
> > "select" callback which only fires when the panel actually changes,
> 
> Right, that wasn't clear to me either. My patch tried to change that and
> perhaps this uncovered some deeper AsyncTabSwitcher issues.
> 
> (In reply to Brian Grinstead [:bgrins] from comment #22)
> > I can try moving it into the "select" listener if you'd prefer it there.
> 
> Certainly would be nice to get this cleaned up or at least understand why
> things need to be set up this way.

Just confirmed locally it needs to happen before the select event fires or else the test fails. I'll see if I can figure out why.
My best guess at the moment is that the async switcher causes some focus-related side effects such that when you click on an already-selected tab but something else is focused (another window or a popup notification), we still need to call it.

I can spend a bit more time looking into it - but I think this change is an improvement and could be landed on its own (no more XBL binding flip at runtime, more explicit about what code runs when)
Comment on attachment 8994363 [details]
Bug 1442582 - Remove the tabbrowser-tabpanels binding.

https://reviewboard.mozilla.org/r/258942/#review266110

Okay, let's file a followup.

::: browser/base/content/tabbrowser.js:4400
(Diff revision 1)
>          hist.add(2 /* unblockByVisitingTab */ );
>          tab.finishMediaBlockTimer();
>        }
>      });
> +
> +    this.tabpanels.addEventListener("select", (event) => {

Can you please move this to the top of the _setupEventListeners method?

::: browser/base/content/tabbrowser.js:4402
(Diff revision 1)
>        }
>      });
> +
> +    this.tabpanels.addEventListener("select", (event) => {
> +      if (event.target == this.tabpanels) {
> +        gBrowser.updateCurrentBrowser();

s/gBrowser/this/

::: browser/base/content/tabbrowser.js:4408
(Diff revision 1)
> +      }
> +    });
> +
> +    this.tabpanels.addEventListener("preselect", (event) => {
> +      if (gMultiProcessBrowser) {
> +        gBrowser._getSwitcher().requestTab(event.detail);

s/gBrowser/this/
Attachment #8994363 - Flags: review?(dao+bmo) → review+
Assignee: nobody → bgrinstead
Flags: needinfo?(dao+bmo)
Attachment #8955461 - Attachment is obsolete: true
Comment on attachment 8994363 [details]
Bug 1442582 - Remove the tabbrowser-tabpanels binding.

https://reviewboard.mozilla.org/r/258942/#review266114

::: browser/base/content/tabbrowser.js:4400
(Diff revision 1)
>          hist.add(2 /* unblockByVisitingTab */ );
>          tab.finishMediaBlockTimer();
>        }
>      });
> +
> +    this.tabpanels.addEventListener("select", (event) => {

another nit: the parentheses around event can go away
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6b30047d863c
Remove the tabbrowser-tabpanels binding. r=dao
Blocks: 1478112
https://hg.mozilla.org/mozilla-central/rev/6b30047d863c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
See Also: → 1469191
Depends on: 1469191
See Also: 1469191 →
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: