GroupItem.add should auto remove from previous group

RESOLVED FIXED in Firefox 4.0b12

Status

defect
P4
normal
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: iangilman, Assigned: ttaubert)

Tracking

Trunk
Firefox 4.0b12
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-][cleanup][good first bug][API])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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.
Blocks: 627096
No longer blocks: 608028
Depends on: 600812
Whiteboard: [qa-][cleanup][good first bug][API]
(Assignee)

Updated

8 years ago
Assignee: nobody → tim.taubert
Version: unspecified → Trunk
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
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?
(Reporter)

Updated

8 years ago
Priority: P3 → P4
(Reporter)

Comment 2

8 years ago
(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.
Posted patch patch v1 (obsolete) — Splinter Review
Pushed to try.
Attachment #507382 - Flags: review?(ian)
Passed try.
(Reporter)

Comment 5

8 years ago
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!
Attachment #507382 - Flags: review?(ian) → review-
Posted patch patch v2 (obsolete) — Splinter Review
You're absolutely right :)
Attachment #507382 - Attachment is obsolete: true
Attachment #507572 - Flags: review?(ian)
(Reporter)

Comment 7

8 years ago
Comment on attachment 507572 [details] [diff] [review]
patch v2

Lovely :)
Attachment #507572 - Flags: review?(ian) → review+
(Assignee)

Updated

8 years ago
Attachment #507572 - Flags: approval2.0?
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.
Attachment #507572 - Flags: approval2.0? → approval2.0+
Old license block replaced with public domain header.
Attachment #507572 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/fd7962a45868
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.