1970541 - Precompute more drag-drop data structures on tab strip dragstart

Status: REOPENED

(Firefox :: Tabbed Browser, task, P4)

Description

[Stephen Thompson [:sthompson]]

The drag-drop code in the <tabs> component:

• stores movingTabs as an array of the moving tabs on the dragged tab's dragdata object on dragstart, but _animateTabMove will reverse that on every dragover if the locale is RTL
• has several places where we slice the array of tab strip items, filter out moving tabs, or otherwise process the array every dragover

Technically, we're wasting CPU cycles by doing the same work over and over during a drag, although we don't actually have hard evidence from profiles that this is a performance issue.

The larger opportunity is to be more explicit about:

1. what data is required to implement our drag-drop logic
2. how we use that data

For example, on https://searchfox.org/mozilla-central/rev/3b58bde321bf705a91c32c7a6574a3427ce81d3f/browser/components/tabbrowser/content/tabs.js#2363 we have tabs = tabs.filter(t => !movingTabs.includes(t) || t == draggedTab);. The main purpose for this data structure is to calculate the element index in getOverlappedElement where the dragged items should be dropped for purposes of animating how the non-moving tabs shift out of the way. It's not intuitive that we should exclude moving tabs but still have the dragged tab in the case where we are dragging several tabs at once. On dragstart, we could build this array once, call it something like dropIndexSearchArray, save it on the dragged tab's dragdata object, and use it without modification during the entire drag operation. Similarly, we could precompute an array of unmovingTabStripItems instead of reusing the tabs array from before and skipping the draggedTab https://searchfox.org/mozilla-central/rev/3b58bde321bf705a91c32c7a6574a3427ce81d3f/browser/components/tabbrowser/content/tabs.js#2661-2664

Comment 1

[Gabriel Astorgano[:astor]]

Attachment: Bug 1970541 Precompute unmovingTabStripItems and dropIndexSearchArray r=sthompson

Moves data structure creation to the on_dragstart event

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/bf027a10140b
https://hg.mozilla.org/integration/autoland/rev/3894e0ec2ad7
Precompute unmovingTabStripItems and dropIndexSearchArray r=sthompson,tabbrowser-reviewers

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/4cef53c557c8
https://hg.mozilla.org/integration/autoland/rev/5d3dd040050b
Revert "Bug 1970541 Precompute unmovingTabStripItems and dropIndexSearchArray r=sthompson,tabbrowser-reviewers" for causing bc failures in browser_move_unpinned_not_overflowing.js

Comment 4

[Cristian Tuns]

Backed out for causing bc failures

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/components/tabbrowser/test/browser/dragdrop/browser_move_unpinned_not_overflowing.js | Uncaught exception in test bound test_move_unpinned_horizontal_rtl - at chrome://global/content/customElements.js:676 - InternalError: too much recursion

Comment 5

[Gabriel Astorgano[:astor]]

Attachment: Bug 1970541 Add tests move unpinned non stacking horizontal tabs r=sthompson

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ffb83bad2eef
https://hg.mozilla.org/integration/autoland/rev/70b01d56b33c
Precompute unmovingTabStripItems and dropIndexSearchArray r=sthompson,tabbrowser-reviewers

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8537def8afc3
https://hg.mozilla.org/integration/autoland/rev/1020df6307c5
Revert "Bug 1970541 Precompute unmovingTabStripItems and dropIndexSearchArray r=sthompson,tabbrowser-reviewers" for causing failures @browser_multiselect_tabs_reorder.js.

Comment 8

[agoloman]

Backed out for causing bc failures @browser_multiselect_tabs_reorder.js.

• Backout link
• Push with failures
• Failure Log

Comment 9

[BMO Github Automation]

Authored by agoloman
https://github.com/mozilla-firefox/firefox/commit/28328852faef8df1ee41e277a56ec6855020cead
[main] Revert "Bug 1970541 Precompute unmovingTabStripItems and dropIndexSearchArray r=sthompson,tabbrowser-reviewers" for causing failures @browser_multiselect_tabs_reorder.js.

Comment 10

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/70b01d56b33c

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/28328852faef
https://hg.mozilla.org/mozilla-central/rev/b3e58208064e
Revert "Bug 1970541 Precompute unmovingTabStripItems and dropIndexSearchArray r=sthompson,tabbrowser-reviewers" for causing failures @browser_multiselect_tabs_reorder.js.

Comment 12

[Cristian Tuns]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/1020df6307c5