Closed Bug 641802 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 5

People

(Reporter: Bebe, Assigned: ttaubert)

Details

(Whiteboard: [4rc])

Attachments

(1 file, 2 obsolete files)

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.
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
Whiteboard: [4rc]
Priority: -- → P3
Target Milestone: --- → Future
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
Hardware: x86 → All
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+
(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.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #523819 - Attachment is obsolete: true
Attachment #524275 - Attachment is obsolete: true
Tim, thank you for the explanation; makes sense now.
http://hg.mozilla.org/mozilla-central/rev/6d32268b9792
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Future → Firefox4.2
Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
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.