Closed Bug 598600 Opened 9 years ago Closed 9 years ago

New tabs should be added to the last active group after restart

Categories

(Firefox Graveyard :: Panorama, defect, P2)

defect

Tracking

(blocking2.0 beta7+)

VERIFIED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: micmon, Assigned: raymondlee)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached image Screenshot
Set Firefox to remember and restore tabs on restart. Have at least one tab group. This group should be empty or only have a blank tab (app tabs are fine to have). Now close and restart the browser and start one or two new tabs. Go to panorama view: the tabs are NOT created inside the tab group you would assume them to be in, they are not associated to any group and show up on top of each other on the bottom right and may even be covered by a tab group.

=> after restart, tabs should always be created in the last active tab group
More information about my usage pattern: I currently have 3 or 4 tab groups. One is the "session group" e.g. normal browsing, the others are special topic groups ("university", "fedora"). I often close all tabs of the "session group" before quitting. The group is still there, though, and creating new tabs before restarting the browser creates them in there correctly. However, after the restart firefox does not seem to remember this tab group was active.
FWIW I should also mention that this works fine as long as the session group has at least one non-app tab in it when closing.
Raymond, please verify that your work on bug 595893 and friends fixes this scenario as well. This is probably a dupe of one of those bugs.
Assignee: nobody → raymond
Blocks: 597043
Priority: -- → P2
I see that this bug is describing two  things.

1) Orphan tabs appear after restart. This would be handled by Bug 595893
2) If the last active group has no non-app tab and after restart, new tab might be added to another group when there are more than one group.

Changed the bug for point 2.
Depends on: 595893
Summary: Creates tabs outside tab group → New tabs should be added to the last active group after restart
Status: NEW → ASSIGNED
The patch in bug
Status: ASSIGNED → NEW
(In reply to comment #5)
> The patch in bug
Please ignore this
Duplicate of this bug: 603483
Attached patch v1 (obsolete) — Splinter Review
Attachment #482483 - Flags: feedback?(ian)
Comment on attachment 482483 [details] [diff] [review]
v1

Pushed to try. 6edcdd3f1f11
This seems to be happening more lately. The fact that it happens at all after bug 595893 means there's something wrong with the patch. Since bug 595893 was a b7 blocker, nominating this as a b7 blocker as well.
blocking2.0: --- → ?
Comment on attachment 482483 [details] [diff] [review]
v1

Nice catch!

The test doesn't really seem like it tests the bug (at least not as described in comment 0). On the other hand I'm not sure exactly how you would test that... you'd need to create a new window (which therefore hasn't yet loaded Panorama), stuff some group/tab data into it as if it had run Panorama before in a previous session, create some new tabs, and switch to Panorama (loading it up for the first time this session) and see if any of the tabs are orphaned. Sounds hard. 

Also: 

>-    GroupItems.groupItems.forEach(function(group) {
>-      group.close();
>-    });
>-    
>     let options = {
>       bounds: box,
>       immediately: true
>     };
>+
>     let groupItem = new GroupItem([], options);
>     let items = TabItems.getItems();
>     items.forEach(function(item) {
>       if (item.parent)
>         item.parent.remove(item);
>       groupItem.add(item, null, {immediately: true});
>     });
>-    
>+
>+    // ensure items are moved before closing the groups
>+    GroupItems.groupItems.forEach(function(group) {
>+      if (groupItem != group) 
>+        group.close();
>+    });

I don't think this change is necessary; closing all the groups ahead of time removes all of their tabs anyway.
Attachment #482483 - Flags: feedback?(ian) → feedback-
(In reply to comment #11)
> The test doesn't really seem like it tests the bug (at least not as described
> in comment 0). On the other hand I'm not sure exactly how you would test
> that... you'd need to create a new window (which therefore hasn't yet loaded
> Panorama), stuff some group/tab data into it as if it had run Panorama before
> in a previous session, create some new tabs, and switch to Panorama (loading it
> up for the first time this session) and see if any of the tabs are orphaned.
> Sounds hard. 

Nah, not too hard. A little bit of listening for the state to be restored and that's about it.

> let state = { windows[{ tabs: [{ ..., extData:{ that stuff panorama does} }], ..., extData: { more panorma... } }] };
> let win2 = openDialog(...);
> sessionstore.setWindowState(win2, JSON.stringify(state), true);

Now you have your window set up with a bunch of tabs & panorama data that's never been accessed.

> win2.gBrowser.addTab()

Now you *should* have an orphaned tab. I never tried to repro with a new window, just starting firefox, so this might not even be an accurate test.
Attached patch v1 (obsolete) — Splinter Review
* Updated the test using setWindowState().  Thanks Paul
* Added code to ensure that we are not saving group id after the group item is deleted, this prevents empty group being created after a group is closed.
Attachment #482483 - Attachment is obsolete: true
Attachment #482791 - Flags: feedback?(ian)
Status: NEW → ASSIGNED
OS: Linux → Windows CE
Comment on attachment 482791 [details] [diff] [review]
v1

Looking good. Just a couple comment nits:

>-    if (!this._inited) // too soon to save now
>+    if (!this._inited || this._uninited) // too soon to save now

Update the comment ("too soon/late to save" or something). 

>+  // set the preference of first run to true

This comment confused me at first; I thought it meant we were setting it as if it was the first run (until I read the code below it). Perhaps the comment could read something like "make sure we don't trigger the 'first run' behavior".
Attachment #482791 - Flags: review?(dietrich)
Attachment #482791 - Flags: feedback?(ian)
Attachment #482791 - Flags: feedback+
Attached patch v1 (obsolete) — Splinter Review
f+=ian
Attachment #482791 - Attachment is obsolete: true
Attachment #482891 - Flags: review?(dietrich)
Attachment #482791 - Flags: review?(dietrich)
Attached patch v1 (obsolete) — Splinter Review
Fixed a type

f+=ian
Attachment #482891 - Attachment is obsolete: true
Attachment #483070 - Flags: review?(dolske)
Attachment #482891 - Flags: review?(dietrich)
This is a re-emergence of a b7 blocker, which wasn't all the way fixed, as this bug is testament to. so this should block b7 as well.
blocking2.0: ? → beta7+
OS: Windows CE → All
Hardware: x86 → All
Attachment #483070 - Flags: review?(dolske) → review?(dietrich)
Comment on attachment 483070 [details] [diff] [review]
v1

>   // ----------
>   // Function: save
>   // Saves this groupItem to persistent storage.
>   save: function GroupItem_save() {
>-    if (!this._inited) // too soon to save now
>+    if (!this._inited || this._uninited) // too soon/late to save
>       return;

when would this happen?

i don't like the idea of callers being able call this and think it did something when it actually did not.
(In reply to comment #18)
> Comment on attachment 483070 [details] [diff] [review]
> v1
> 
> >   // ----------
> >   // Function: save
> >   // Saves this groupItem to persistent storage.
> >   save: function GroupItem_save() {
> >-    if (!this._inited) // too soon to save now
> >+    if (!this._inited || this._uninited) // too soon/late to save
> >       return;
> 
> when would this happen?
> 
> i don't like the idea of callers being able call this and think it did
> something when it actually did not.

When a new tab item is restored/created, it would go into a group (based on the session storage data); if it doesn't belong to any group, a new group would be created by GroupItems.newTab().  

In the UI.init(), there are some code (UI.reset()) which removes all groups and creates a new group for all tab items if it's first run for Panorama or no group data exists

When a new browser window is opened with browser tab(s), and go into Panorama UI,  tab item(s) and a group item would be created.  Since there is no group data, the UI.reset() would remove the new group and move all tab Items into the a new group.  However, GroupItem.save() is called in multiple places, so we run into a situation which the old group item id is being saved to the session storage but the group item has already removed by UI.reset().  When user restarts the browser, Panorama would restore a blank group which should not exist.
Duplicate of this bug: 604758
Duplicate of this bug: 604923
Comment on attachment 483070 [details] [diff] [review]
v1

Reassigning review to Dolske, as Dietrich's away on vacation.
Attachment #483070 - Flags: review?(dietrich) → review?(dolske)
additional STR:

1. restart
2. add a new tab
3. switch tabs
4. right click on tab
5. ctrl-tab back to previous tab

Results: Previous tab is selected, other tabs disappear from tab-strip. 

Expected: Nothing changed, switches back to previous tab.
Duplicate of this bug: 599754
Blocks: 594644
Blocks: 598795
Here's a script you can run from your error console to pull all of your tabs (whether they've been grouped or are orphaned) into a single group: 

http://gist.github.com/637583

I could probably whip one up that only does it for orphaned tabs if anyone wants.
(In reply to comment #25)
> Here's a script you can run from your error console to pull all of your tabs
> (whether they've been grouped or are orphaned) into a single group: 
> 
> http://gist.github.com/637583
> 
> I could probably whip one up that only does it for orphaned tabs if anyone
> wants.

Thanks!
I tried it, and it does its job, but without updating the tab bar (you have to enter and exit TabView). Is it possible to add that to the script?
this type of functionality would be pretty useful to have included in TabView itself. Maybe as a button somewhere.

Alternatively, a jetpack to add this function would be a cool trick.
cc'ing the reviewer...
(In reply to comment #26)
> (In reply to comment #25)
> > Here's a script you can run from your error console to pull all of your tabs
> > (whether they've been grouped or are orphaned) into a single group: 
> > 
> > http://gist.github.com/637583
> > 
> > I could probably whip one up that only does it for orphaned tabs if anyone
> > wants.
> 
> Thanks!
> I tried it, and it does its job, but without updating the tab bar (you have to
> enter and exit TabView). Is it possible to add that to the script?

Forked & added that. Not sure if there's a way with a single tabview call (I feel like there might be), but this just calls showTab for each tab in the window. http://gist.github.com/638855
Dolske: really need this review. Can we get an ETA or can you toss it to someone else?
Whiteboard: [needs review dolske]
Blocks: 606435
Blocks: 606429
Attachment #483070 - Flags: review?(dolske) → review+
Attachment #483070 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [needs review dolske]
No longer blocks: 606435
Flags: in-litmus?(twalker)
Ian: we can haz checkin to mozilla-central default and the b7relbranch?
I landed this on trunk:
https://hg.mozilla.org/mozilla-central/rev/e2124a8f65ed

It doesn't apply cleanly to the b7 relbranch. I'd like to avoid us doing the work of figuring out why and merging it, which would be easy to do if we decided to kill the relbranch now :)
Word is we're no longer using the b7 branch (shipping from tip instead), so this is resolved.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
verified with latest build of minefield 20101029
Status: RESOLVED → VERIFIED
Duplicate of this bug: 587368
Test added in Litmus https://litmus.mozilla.org/show_test.cgi?id=15225
Flags: in-litmus?(twalker) → in-litmus+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.