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

VERIFIED FIXED in Firefox 5

Status

P3
normal
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: Bebe, Assigned: ttaubert)

Tracking

Trunk
Firefox 5

Details

(Whiteboard: [4rc])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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

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

Confirming issue on Ubuntu 10.04. Setting resolution to New.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Version: unspecified → Trunk

Updated

8 years ago
Whiteboard: [4rc]

Updated

8 years ago
Priority: -- → P3
Target Milestone: --- → Future
(Assignee)

Updated

8 years ago
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Hardware: x86 → All
(Assignee)

Comment 3

8 years ago
Comment on attachment 523819 [details] [diff] [review]
patch v1

Passed try.
Comment on attachment 523819 [details] [diff] [review]
patch v1

Looks good to me!
Attachment #523819 - Flags: review?(ian)
Attachment #523819 - Flags: feedback?(raymond)
Attachment #523819 - Flags: feedback+
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
Attachment #523819 - Flags: review?(ian) → review+
(Assignee)

Comment 6

8 years ago
(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.
(Assignee)

Comment 7

8 years ago
Created attachment 524275 [details] [diff] [review]
patch v2
Attachment #523819 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 8

8 years ago
Created attachment 524278 [details] [diff] [review]
patch for checkin
Attachment #524275 - Attachment is obsolete: true
Tim, thank you for the explanation; makes sense now.

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/6d32268b9792
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Future → Firefox4.2

Comment 11

8 years ago
Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED

Updated

8 years ago
Target Milestone: Firefox5 → Firefox 5
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.