608405 - GroupItem.add should auto remove from previous group

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P4)

Description

[Ian Gilman [:iangilman]]

We do a pretty good job of making sure we never add a child to a group that's already got another parent, but sometimes it still happens. Might as well auto remove from the previous group when this happens.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

What should the arguments for .remove() be that is called in .add()? There is a dontClose e.g. for drag/drop so that group is not closed before its last child is dropped somewhere out of the group. But there will be surely some situation where the group should have been close when removing the last child?

Comment 2

[Ian Gilman [:iangilman]]

(In reply to comment #1)
> What should the arguments for .remove() be that is called in .add()? There is a
> dontClose e.g. for drag/drop so that group is not closed before its last child
> is dropped somewhere out of the group. But there will be surely some situation
> where the group should have been close when removing the last child?

Leave options empty when calling remove. This is just a safeguard; we should just stick with the default behavior.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Pushed to try.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Passed try.

Comment 5

[Ian Gilman [:iangilman]]

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

>+      is(tabItem.parent, targetGroup, 'tabItem changed groups');
>+      is(gBrowser.tabs.length, 2, 'there are two tabs');

You should explicitly check to make sure both groups have the correct number of children. 

>+      tabItem.close();
>+      sourceGroup.close();

Actually, won't removing the tab from sourceGroup automatically close it? Perhaps also verify that that has happened instead of closing it here.

... and don't forget to test your end state, so you know you left it clean for the next test!

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

You're absolutely right :)

Comment 7

[Ian Gilman [:iangilman]]

Comment on attachment 507572 [details] [diff] [review]
patch v2

Lovely :)

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 507572 [details] [diff] [review]
patch v2

Please fix the license block of the test to use public domain before landing.  You won't need to get extra approval for that.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Old license block replaced with public domain header.

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/fd7962a45868