Closed
Bug 580847
Opened 15 years ago
Closed 15 years ago
Lots of unnecessary TabMove events
Categories
(Firefox Graveyard :: Panorama, defect)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: Mardak, Assigned: raymondlee)
References
Details
Attachments
(1 file, 2 obsolete files)
6.93 KB,
patch
|
iangilman
:
review-
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•15 years ago
|
||
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?
Blocks: 574217
Assignee | ||
Comment 2•15 years ago
|
||
This patch would reduce the tab move events and only move visible tabs when necessary.
Attachment #459312 -
Flags: review?(ian)
Attachment #459312 -
Flags: feedback?(edilee)
Reporter | ||
Comment 3•15 years ago
|
||
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. ?
Assignee | ||
Comment 4•15 years ago
|
||
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?
Reporter | ||
Comment 5•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 7•15 years ago
|
||
- 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.
Attachment #459312 -
Attachment is obsolete: true
Attachment #459765 -
Flags: review?(ian)
Attachment #459312 -
Flags: review?(ian)
Attachment #459312 -
Flags: feedback?(edilee)
Reporter | ||
Comment 8•15 years ago
|
||
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?
Assignee | ||
Comment 9•15 years ago
|
||
Based on Mardak's suggestions, here is another attempt.
Attachment #459765 -
Attachment is obsolete: true
Attachment #459765 -
Flags: review?(ian)
Assignee | ||
Updated•15 years ago
|
Attachment #459865 -
Flags: review?(ian)
Comment 10•15 years ago
|
||
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.
Attachment #459865 -
Flags: review?(ian) → review-
Reporter | ||
Comment 11•15 years ago
|
||
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?
Assignee | ||
Comment 12•15 years ago
|
||
(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.
Assignee | ||
Comment 13•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → raymond
Comment 14•15 years ago
|
||
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.
Assignee | ||
Comment 15•15 years ago
|
||
(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•15 years ago
|
||
Raymond, thanks!
Mardak, would you consider this bug resolved?
Reporter | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 17•15 years ago
|
||
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy. Filter the bugmail spam with "tabcandymassmove".
Product: Mozilla Labs → Firefox
Target Milestone: -- → ---
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•