641802 - "Move to group" option stays populated with the old data even after the groups have been deleted in panorama

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Florin Strugariu [:Bebe]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0

If we try to use the  move to group option after all the groups have been deleted, the menu option shows all the deleted groups


Reproducible: Always

Steps to Reproduce:
1. In a clean profile open a few tabs
2. Open Panorama(ctrl+sift+e)
3. Create some Groups, name them , and put a tab in every group
4. Exit Panorama and verify that the move to group option is populated 
5. Open Panorama (ctrl+sift+e) and delete all the created groups
6. Close panorama
7. Verify the move to tab option

Actual Results:  
The old groups are present in the menu.


Expected Results:  
The menu should be empty.

Comment 1

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:2.0b13pre) Gecko/20110314 Firefox/4.0b13pre

Confirming issue on Ubuntu 10.04. Setting resolution to New.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Pushed to try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=9c20ead3182b

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 523819 [details] [diff] [review]
patch v1

Passed try.

Comment 4

[Raymond Lee [:raymondlee]]

Comment on attachment 523819 [details] [diff] [review]
patch v1

Looks good to me!

Comment 5

[Ian Gilman [:iangilman]]

Comment on attachment 523819 [details] [diff] [review]
patch v1

Looks good, except I don't really understand how this patch fixes the bug described in comment 0. Can you explain what the problem was?

Minor point:

>+  let assertValidStart = function () {
>+    let cw = TabView.getContentWindow();
>+    is(cw.GroupItems.groupItems.length, 1, "there is one groupItem");
>+    is(gBrowser.tabs.length, 1, "there is one tab");
>+    ok(TabView.isVisible(), "tabview is visible");
>+  }
>+
>+  let assertValidEnd = function () {
>+    let cw = TabView.getContentWindow();
>+    is(cw.GroupItems.groupItems.length, 1, "there is one groupItem");
>+    is(gBrowser.tabs.length, 1, "there is one tab");
>+    ok(!TabView.isVisible(), "tabview is hidden");
>+  }

These are almost identical... perhaps combine them somehow?

r+ with the above addressed

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #5)
> Looks good, except I don't really understand how this patch fixes the bug
> described in comment 0. Can you explain what the problem was?

Sure :) So the problem was that "updateContextMenu()" is only run if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0) => if there are any hidden tabs.

>// there are hidden tabs so initialize the iframe and update the context menu
>if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
>  this.updateContextMenu(TabContextMenu.contextTab, event.target);

So once this is run the menu gets filled with menuitems. If we do now invalidate the condition (i.e. removing all groups but one) the popmenu is not updated and the menuitems stay there untouched. The condition exists to prevent any unwanted initialization of Panorama - if that's already initialized we can just call it.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 9

[Ian Gilman [:iangilman]]

Tim, thank you for the explanation; makes sense now.

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6d32268b9792

Comment 11

[George Carstoiu]

Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre