Closed Bug 589324 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, P2)

defect

Tracking

(blocking2.0 final+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: ashughes, Assigned: raymondlee)

References

Details

(Whiteboard: [4b4])

Attachments

(1 file, 4 obsolete files)

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)
Whiteboard: [4b4]
This seems like an issue with loading the Panorama sessionstore/group data fast enough.
Assignee: nobody → raymond
Priority: -- → P2
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.
Blocks: 598154
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — Splinter Review
Attachment #484795 - Flags: feedback?(ian)
Blocks: 589261
Comment on attachment 484795 [details] [diff] [review]
WIP

It breaks the browser_visibleTabs.js test.  Working on fix the test.
Attachment #484795 - Attachment description: v1 → WIP
Attachment #484795 - Flags: feedback?(ian)
Attached patch v1.1 (obsolete) — Splinter Review
Just updated the browser_visibleTabs.js so the test passes.  Sent it to try: 11a8ad41e85b
Attachment #484795 - Attachment is obsolete: true
Attachment #486023 - Flags: feedback?(ian)
Comment on attachment 486023 [details] [diff] [review]
v1.1

Looks good to me.
Attachment #486023 - Flags: review?(dao)
Attachment #486023 - Flags: feedback?(ian)
Attachment #486023 - Flags: feedback+
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
Attachment #486023 - Flags: review?(dao)
Raymond, try adding this to your patch and pushing to try. This fixed this issue for me locally.
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. :)
Attached patch v1.2 (obsolete) — Splinter Review
Thanks mitcho, your patch works!  

Passed try!
Attachment #486023 - Attachment is obsolete: true
Attachment #487277 - Attachment is obsolete: true
Attachment #487299 - Flags: feedback?(ian)
Comment on attachment 487299 [details] [diff] [review]
v1.2

Looks good.
Attachment #487299 - Flags: review?(dolske)
Attachment #487299 - Flags: feedback?(ian)
Attachment #487299 - Flags: feedback+
Duplicate of this bug: 609126
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.
blocking2.0: --- → final+
Comment on attachment 487299 [details] [diff] [review]
v1.2

Load balancing review to dietrich now that this is blocking.
Attachment #487299 - Flags: review?(dolske) → review?(dietrich)
Attachment #487299 - Flags: review?(dietrich) → review+
Attachment #487299 - Flags: approval2.0?
Attachment #487299 - Flags: approval2.0?
Doesn't need approval as it's blocking. Raymond, please package for check-in.
Attachment #487299 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/9b8ee895f234
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.