606657 - moveTabToGroupItem fails if move the only tab in a group to the same group

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[ithinc]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101022 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101022 Firefox/4.0b8pre

When implementing a "Move Tabs to New Group" feature, I need to move the context tab to a new group first, then move all needed tabs to that group. If the context tab happens to be the first tab in the tabs, it leads to a failure.

Reproducible: Always

Steps to Reproduce:
Supposing you have not opened an app tab:
TabView.moveTabTo(gBrowser.mTabs[0], null);
TabView.moveTabTo(gBrowser.mTabs[0], gBrowser.mTabs[0].tabItem.parent.id);

Comment 1

[Kevin Hanes]

This would be great to have for beta 8, though I'm not sure we'll get to it. We currently plan to do our most intensive API work post-ff4.0.

I'll leave this bug blocking beta8 for the time being to keep it on our minds.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

This should be as simple as comparing the tab's current group id with the target group's id. If they're equal do nothing.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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

I'm not familiar with this code.

Comment 4

[ithinc]

Yes, it's simple enough. The patch should work.

Comment 5

[Ian Gilman [:iangilman]]

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

>+  TabView.moveTabTo(tab, null);

This line shouldn't be necessary; entering the test we should have one tab and one group, and the tab should be in the group. Is this not what you're finding?

Otherwise looks good. R+ with that addressed.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2 (test corrected)

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3 (test now correctly cleans up its environment)

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Pushed to try. Passed.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 501368 [details] [diff] [review]
patch v3 (test now correctly cleans up its environment)

a=beltzner

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Passed try.

Comment 11

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/041947ac6cbb

Comment 12

[Tracy Walker [:tracy]]

verified with recent minefield nightly build