589324 - Switch-to-Tab after Session Restore does not respect Tab Candy grouping

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[u279076]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b4) Gecko/20100818 Firefox/4.0b4

After restoring a session with tabs in multiple groupings, Switch-to-tab opens tabs from other groupings in the current displayed grouping.  It should be switching the window to display the respective grouping.

Steps to Reproduce:
1. Go to www.google.com
2. Open Images | Videos | Maps | News | Books in new tabs with CTRL+Click
3. Open the Tab Candy View and drag News to its own group
4. Click the main group (News should be gone)
5. Type "News" in the location bar and select Switch to Tab (Window should switch to the News grouping)
6. Switch back to the main grouping
7. Quit -> Save & Quit then start Firefox again (Session should restore to the main group)
8. Type "News" in the location bar and select Switch to Tab
9. Open the Tab Candy View and select the main grouping

Results:
Step 8 > News pops into the current window
Step 9 > News pops back into its own grouping

Expected:
Step 8 > Same behaviour as Step 5 (Window switches to display just News in its own grouping)

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

This seems like an issue with loading the Panorama sessionstore/group data fast enough.

Comment 2

[Ian Gilman [:iangilman]]

It's because Tab View hasn't loaded yet. Need to do the same thing we do for the "switch group" key combo or the "move tab to group" menu. 

Note that this can be done before bug 599852, but that that bug will make it faster.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: WIP

Comment 4

[Raymond Lee [:raymondlee]]

Comment on attachment 484795 [details] [diff] [review]
WIP

It breaks the browser_visibleTabs.js test.  Working on fix the test.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v1.1

Just updated the browser_visibleTabs.js so the test passes.  Sent it to try: 11a8ad41e85b

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 486023 [details] [diff] [review]
v1.1

Looks good to me.

Comment 7

[Raymond Lee [:raymondlee]]

Comment on attachment 486023 [details] [diff] [review]
v1.1

The tests didn't pass but I couldn't figure out why it causes problem in the tests for bug 580412.

@mitcho: could you give me some help please since you wrote the tests?


http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1288113133.1288114014.12933.gz

s: talos-r3-fed-017
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug580412.js | Move away from the edge
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug580412.js | Moving five pixels: shouldn't snap

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: 580412 test patch to make it slighly more robust

Raymond, try adding this to your patch and pushing to try. This fixed this issue for me locally.

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 486023 [details] [diff] [review]
v1.1

>+  // make sure we don't trigger the 'first run' behavior
>+  prefService.setBoolPref("experienced_first_run", true);

>+        // clean up and finish
>+        prefService.setBoolPref("experienced_first_run", false);

Raymond, also, note that the experienced_first_run should not be overridden in an individual test, as I mentioned on IRC before. :)

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v1.2

Thanks mitcho, your patch works!  

Passed try!

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 487299 [details] [diff] [review]
v1.2

Looks good.

Comment 13

[Dietrich Ayala (:dietrich)]

Blocking request was on this bug's dupe in bug 609126.

Blocking+ for interaction unexpectedness between two new features that a lot of eyes will be on.

Comment 14

[Ian Gilman [:iangilman]]

Comment on attachment 487299 [details] [diff] [review]
v1.2

Load balancing review to dietrich now that this is blocking.

Comment 15

[Ian Gilman [:iangilman]]

Doesn't need approval as it's blocking. Raymond, please package for check-in.

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 17

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/9b8ee895f234