578927 - Re-order tabs in a group in Tab Candy doesn't reflect on the tab bar

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Raymond Lee [:raymondlee]]

Steps to reproduce:
1) Go to tab candy interface
2) Re-order tabs in a group 
3) Zoom into a tab in that group
4) The order of tabs on tab bar isn't in the same order as in the tab candy

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: Patch

Comment 2

[Ian Gilman [:iangilman]]

Comment on attachment 457799 [details] [diff] [review]
Patch

Looks good, except the original creation of visibleTabs: it uses "for each", which we've recently discovered is not guaranteed to maintain order. Can you rewrite that to use [].forEach and then apply the patch?

Comment 3

[Raymond Lee [:raymondlee]]

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

Removed the "for each"
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/bc7f40208dbf

Comment 4

[Ian Gilman [:iangilman]]

Raymond, thanks for removing the for each. Unfortunately now the routine modifies the "tabs" argument directly, which is a bad situation waiting to happen. Can you please fix that?

Comment 5

[Raymond Lee [:raymondlee]]

Ah, I wasn't aware that we shouldn't modify the tabs argument passed into the showOnlyTheseTabs()

Patched: 
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/651983832b51

Ian: please mark this bug as resolved if everything is OK.

Comment 6

[Ed Lee :Mardak]

     1.7 +      var visibleTabs = [];
     1.8 +
     1.9 +      tabs.forEach(function(tab) {
    1.10 +        visibleTabs.push(tab.tab.raw);
    1.11 +      });

Curious, why not

let visibleTabs = tabs.map(function(tab) tab.tab.raw);

?

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to comment #6)
Thanks Mardak.  

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

Comment 8

[Ian Gilman [:iangilman]]

(In reply to comment #5)
> Ah, I wasn't aware that we shouldn't modify the tabs argument passed into the
> showOnlyTheseTabs()

I think it's a good general rule that you should never modify an argument unless it's an explicit part of the function. In other words, addStuffTo(obj) should modify obj, but tellMeAbout(obj) should not. 

In this particular case, the array passed into showOnlyTheseTabs() is just meant to guide the operation, not be the result of the operation, so modifying it shouldn't be a side effect. In fact, in a number of cases, the routine is called with a group's master list of children, so modifying the array would have disastrous effects. 

Anyway, thanks for the fix... looks good!

Comment 9

[(no longer active)]

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