580847 - Lots of unnecessary TabMove events

Status: RESOLVED FIXED

Description

[Ed Lee :Mardak]

I'm in the process of writing a Tabs jsmodule to provide an API to get tab events from any browser window and I noticed doing anything with tabs results in a ton of TabMove events.

Disabling tabcandy reduces the number of TabMoves to 0 for things like new tab or switching tabs. It seems like the number of moves is order of number of tabs in the group.

Comment 1

[Ed Lee :Mardak]

This seems to be triggered by the stack..

Tabs.onFocus
Page.tabsOnFocus
Groups.setActiveGroup
Groups.updateTabBarForActiveGroup
UI.tabBar.showOnlyTheseTabs
visibleTabs.forEach(function(visibleTab) tabbrowser.moveTabTo(visibleTab, ..

Is this even necessary if the tabs outside of the current group are collapsed?

Comment 2

[Raymond Lee [:raymondlee]]

Created attachment 459312 [details] [diff] [review]
v1

This patch would reduce the tab move events and only move visible tabs when necessary.

Comment 3

[Ed Lee :Mardak]

I don't see why the tabs need to be reordered at this point. Why wouldn't the tabs be moved to the right spot in the tabbar when it's moved in tabcandy? The only thing setActiveGroup should then need to do is make sure only the tabs matching the active group are visible. ?

Comment 4

[Raymond Lee [:raymondlee]]

You are right.  It would be better to do that when tabs be moved in TabCandy.  

Ian, Mitcho: do you know why we have code in showOnlyTheseTabs()?  Was it because tab candy was a browser tab?

Comment 5

[Ed Lee :Mardak]

These extra move events are causing tabcandy to do a ton of unnecessary work from its onMove handler -> activeGroup.reorderBasedOnTabOrder -> Group.arrange -> each tabitem.setBounds -> tabitem.save -> disk

Comment 6

[Ian Gilman [:iangilman]]

I think it's the way it is simply due to prototyping expedience at the time (Aza wrote it, so you'd have to ask him to be sure). I agree that moving them in the tab bar when they're moved in the Tab Candy UI is a cleaner solution.

Comment 7

[Raymond Lee [:raymondlee]]

Created attachment 459765 [details] [diff] [review]
v2

- When a tab item is moved in the Tab Candy interface, it would also be moved in the tab strip
- Only call group.reorderBasedOnTabOrder() just before switching to Tab Candy interface if needed.  This should remove all the unnecessary calls.

Comment 8

[Ed Lee :Mardak]

Does reordering the tabs need to put them adjacent to each other? It should be ok if there are hidden tabs between the visible tabs as long as they're in the right order yeah?

Your approach is to instead eagerly move tabs as they're dragged in tabcandy to instead batch them up when exiting tabcandy? Perhaps saving some moves if the user drags the same tab back and forth and maybe ends up in the same state they started in?

Comment 9

[Raymond Lee [:raymondlee]]

Created attachment 459865 [details] [diff] [review]
v3

Based on Mardak's suggestions, here is another attempt.

Comment 10

[Ian Gilman [:iangilman]]

Comment on attachment 459865 [details] [diff] [review]
v3

Rearranging the tabs only when we switch out of the Tab Candy UI is a decent optimization, but how about when we start supporting multiple windows? Then you could have a Tab Candy in one window and be moving tabs around in another window. If we're already updating xul:tabs vs TabItems eagerly, then that just works. Otherwise we have to do more special casing. 

Raymond, Mardak, thoughts? 

At any rate, comments on this patch:

* Drag isn't the right place for calling setReorderTabsOnHide; other places move tabs around in groups as well (though they're more obscure). I think if you stick it in Group.add and Group.remove, you'll be set.
* Have you made sure _isTabCandyVisible() is false during the hideTabCandy() reordering? Otherwise the reordering will trigger more adds to the reorder list.
* The documentation for reorderTabsBasedOnTabItemOrder is wrong; appears to just be a duplicate of the previous routine's documentation. 
* Looks like the rearrange algorithm will potentially generate a move event for every tab in the group even if only one tab has moved. Maybe this is fine for now, but if the goal here is to reduce move events, perhaps it could be improved.

Comment 11

[Ed Lee :Mardak]

With the reordering of tabs either on tabcandy-item-move or tabcandy-close, Tabbar.showOnlyTheseTabs probably doesn't even need the logic to reorganize the tabs?

Comment 12

[Raymond Lee [:raymondlee]]

(In reply to comment #10)
> Comment on attachment 459865 [details] [diff] [review]
> v3
> 
> Rearranging the tabs only when we switch out of the Tab Candy UI is a decent
> optimization, but how about when we start supporting multiple windows? Then you
> could have a Tab Candy in one window and be moving tabs around in another
> window. If we're already updating xul:tabs vs TabItems eagerly, then that just
> works. Otherwise we have to do more special casing. 
> 

When we support multiple windows, we would have singleton JS module to organize everything, hence  when moving tabs in one window and the JS module gets notified, the JS module would update other windows directly or using observer notification.

Comment 13

[Raymond Lee [:raymondlee]]

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/4ba7f9d665dd

- The setReorderTabsOnHide() would only be called in Group.add().  It doesn't seem necessary in Group.delete() because when a tab is removed from the tab, the tabs in the group are still in the same order (from left to right).
- setReorderTabsOnHide() only sets to re-order tabs for group if  _isTabCandyVisible() is true.
- Will refine the efficiency of the rearrange algorithm.  Just push the commit so it doesn't block other bugs depend on it.

Comment 14

[Ian Gilman [:iangilman]]

Raymond, thanks!

I noticed you killed the timeout in the Tabs.onMove() callback... was that intentional? I've found that marshaling the event to the DOM thread keeps things cleaner in general. 

Also, I think the setReorderTabsOnHide() in Groups.add() should be called regardless of the "dontArrange" flag... dontArrange is used simply to defer the visual update; it doesn't change the fact that the children have probably been reordered in memory.

Comment 15

[Raymond Lee [:raymondlee]]

(In reply to comment #14)
> Raymond, thanks!
> 
> I noticed you killed the timeout in the Tabs.onMove() callback... was that
> intentional? I've found that marshaling the event to the DOM thread keeps
> things cleaner in general. 
> 

I don't think it's necessary because we are not doing anything related to DOM when Tabs.onMove is called. We just set an array flag for later use. But I've added the timeout back in.

> Also, I think the setReorderTabsOnHide() in Groups.add() should be called
> regardless of the "dontArrange" flag... dontArrange is used simply to defer the
> visual update; it doesn't change the fact that the children have probably been
> reordered in memory.

Now, setReorderTabsOnHide() is called whenever Groups.add() is called.

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/7521f4782eef

Comment 16

[Ian Gilman [:iangilman]]

Raymond, thanks! 

Mardak, would you consider this bug resolved?

Comment 17

[:Ehsan Akhgari]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".