588217 - Integrate Tab Groups data into Session Restore proper

Status: RESOLVED WONTFIX

(Firefox :: Session Restore, defect, P2)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Right now Tab Candy is using Session Restore to stash data. However this approach is falling short in a few ways:

Undo Close Tab knows nothing about tab sets. So recently closed tabs are shown from all tab sets. This should be local to the current tab set (and stored independently). If we're going to handle this inside session restore, then we need to actually acknowledge that Tab Candy exists & is using session restore for data.

If tab sets become window-independent, then we need to restructure how session data is stored.

Current:
> {
>   "windows":[
>     {
>       "tabs":[
>         { ... },
>         { ... }
>       ],
>       "_closedTabs":[...],
>     }
>   ],
>   "_closedWindows":[...]
> }

Proposed (haven't necessarily covered all details)
> {
>   "tabsets":[
>     {
>       "id":100100101010,
>       "title": "foo",
>       "tabs":[
>         { ... },
>         { ... }
>       ],
>       "_closedTabs":[...],
>     },
>   ]
>   "windows":[
>     {
>       "tabset":100100101010,
>     },
>     { ... }
>   ]
> }

This would break consumers, hard. If it's going to happen, it should happen now.

We'd need new APIs. We'd need to get rid of some old APIs

Comment 1

[Dietrich Ayala (:dietrich)]

If tab sets become window independent, we could save them as window objects, with a type=set property. Not only would that not fully break add-on consumers (worst case they report more windows. but no dataloss!), it would also keep the data backwards compatible*.

(* If it even is, anymore. I'm not sure.)

Comment 2

[Ian Gilman [:iangilman]]

By the way, this bug is related to bug 578512 (manage all windows).

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I'll add another reason why this would be good...

This makes it much easier (in association with bug 586068 and maybe something else that we'd need to file) to prioritize loading a specific group. We would be able to only load a tabset once it's actually being used. I know it's been pointed out by John Bird that loading all the tabs (a) takes long time and (b) uses a lot of memory. This gets worse with more tabs being "hidden" behind tab sets. Should definitely be a new bug, but could happen later (since it wouldn't be stringy or really impact behavior too much)...

Comment 4

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

Ian: we probably need to put a bug on file about the APIs required for this and similar add-on requests to gain awareness of tab candy constructs like "groups" and which tabs are visible in which groups. I've seen several requests for this as a cleaner way of dealing with the hidden/visible aspect of things.

Comment 5

[Ian Gilman [:iangilman]]

(In reply to comment #4)
> Ian: we probably need to put a bug on file about the APIs required for this and
> similar add-on requests to gain awareness of tab candy constructs like "groups"
> and which tabs are visible in which groups. I've seen several requests for this
> as a cleaner way of dealing with the hidden/visible aspect of things.

Done, bug 588899.

Comment 6

[Justin Dolske [:Dolske]]

--> dietrich, since apparently he volunteered to do the session restore piece of this.

Comment 7

[Benjamin Smedberg]

What APIs will be changed as a part of this? This sounds offhand like it will touch extension-visible APIs and should block beta5, but I'd love to hear that it doesn't.

Comment 8

[Aza Raskin [:aza]]

(In reply to comment #1)
> If tab sets become window independent, we could save them as window objects,
> with a type=set property. Not only would that not fully break add-on consumers
> (worst case they report more windows. but no dataloss!), it would also keep the
> data backwards compatible*.
> 
> (* If it even is, anymore. I'm not sure.)

Just double checking that this does not mean that each tab group will be represented by creating a physical window?

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to comment #8)
> Just double checking that this does not mean that each tab group will be
> represented by creating a physical window?

Yes and no. I was describing a way to model groups in session restore's internal storage, such that the group data wouldn't get lost if a user's migration path looked like: 3.* -> 4.* -> 3.* -> 4.*.

A side-effect of this approach is that groups *would* appear as actual windows in 3.*. There are a lot of other problems with the approach. We can probably write-off that upgrade scenario and relnote it. If the group data is synced via Weave then we can call that a workaround.

Comment 10

[Aza Raskin [:aza]]

The thought of 3 and 4 integration hadn't even crossed my mind. *shudders*.

+1 to the approach.

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: wip 1

* at data collection time, collect group data and add to the object that is serialized and written to disk.

TODO:

* remove all the manual storage code in tab candy, outside of saving the group id on each tab (we can fix that later)

* update groups.jsm with an api for bulk-setting the group data when a session is restored

Comment 12

[Dietrich Ayala (:dietrich)]

The approach I'm taking with the tabview part of this:

1. when a session is restored, we add all the group/tab data from the ss data via TabViewGroup.createGroup(). the first time tabview is loaded, the data is already there, ready to go.

2. remove the load/save code entirely from tabs.jsm/groups.jsm. the only thing that the tabview code will need to do is get/set the group id on each tab. it's currently doing that when loading/saving all a tab's data. instead i'll integrate it into the groupID getter/setter on each TabViewTab. wrt migration, i'll leave the loading apis in place for a while (until beta after next probably), so that the first time this code is run, we pull the data from the old location (per-window), but subsequent loadings willl use the pre-populated data from the session.

3. remove groups.jsm's dependency on windows altogether. it was only there because the data was stored per-window. now that it'll be re/stored by nsSessionStoreService, it's no longer necessary for groups.jsm to be window-aware at all.

Comment 13

[Dietrich Ayala (:dietrich)]

Attachment: wip2 - changes to ss for restoration

Comment 14

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

Somehow I doubt this will make b5, moving to b6

Comment 15

[Dietrich Ayala (:dietrich)]

(In reply to comment #14)
> Somehow I doubt this will make b5, moving to b6

Correctomundo, hard dependency on big tabview changes in bug 578512 which got pushed to b6.

Comment 16

[Ian Gilman [:iangilman]]

Comment on attachment 469911 [details] [diff] [review]
wip2 - changes to ss for restoration

Dietrich, the patch looks good so far, and I agree with the plan you've outlined.  Thanks!

Comment 17

[Dietrich Ayala (:dietrich)]

Changing to blocking- and unassigning for now, since bug 578512 isn't going to be fixed for Fx4.

The only bug this blocks that we might want for Fx4 is bug 587874. However, it doesn't seem like a ship-blocker to me.

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

Saw this in the Panorama b8 dependency tree. Questions:

1. Will/should this work be pursued before bug 578512, which is now out of the scope of fx4?
2. Alternatively, should we not expect further work on this bug until much later, in which case we should drop its blocking of bug 591704?

Comment 19

[Ian Gilman [:iangilman]]

(In reply to comment #18)
> Saw this in the Panorama b8 dependency tree. Questions:
> 
> 1. Will/should this work be pursued before bug 578512, which is now out of the
> scope of fx4?
> 2. Alternatively, should we not expect further work on this bug until much
> later, in which case we should drop its blocking of bug 591704?

I believe number 2; removing block.

Comment 20

[Aza Raskin [:aza]]

Unless people feel strongly about this, I'm punting to the future as it doesn't seem like we need this to ship.

Comment 21

[Vish]

(In reply to comment #20)
> Unless people feel strongly about this, I'm punting to the future as it doesn't
> seem like we need this to ship.

Not sure why this has been punted. Without being able to save & restore the grouped tabs on next start, Tab Candy seems more work and just eye-candy than actually being useful. 

All the sorting/naming of the groups seems to be in vain, when it needs to be redone again. 
More users getting confused by the behavior > http://support.mozilla.com/en-US/questions/758762

Can we get this for firefox4 final?

Comment 22

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #21)
> Can we get this for firefox4 final?

At this point, no.

Comment 23

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I should have said this before instead of going for the multi-step email spam. Sorry.

(In reply to comment #21)
> Not sure why this has been punted. Without being able to save & restore the
> grouped tabs on next start, Tab Candy seems more work and just eye-candy than
> actually being useful. 

Grouped tabs are saved and restored on next start. While Panorama won't load the group data immediately at startup, the last visible group should be shown and group data (names, positions, etc) is available when Panorama loads.

> All the sorting/naming of the groups seems to be in vain, when it needs to be
> redone again. 
> More users getting confused by the behavior >
> http://support.mozilla.com/en-US/questions/758762

The top answer there is wrong. That data is restored and AFAIK has been since before b6. I know it is for b7+.

Comment 24

[Alex Faaborg [:faaborg] (Firefox UX)]

>Unless people feel strongly about this, I'm punting to the future as it doesn't
>seem like we need this to ship.

We can't really do these three things together:

-punt on this
-punt on the home tab
-remove the save on close dialog box

the result of all three is that users are relying on a history menu command (or a preference they have to manually flip) in order to restore all of the data that they have created in tab candy (groups, names, positions, etc).

Possible solutions in order of difficulty:

1) add the restore session command to tab view (for when the user is wondering where all their stuff went)
2) automatically restore, but use bar-tab like loading for tab groups
3) automatically restore, use bar-tab like loading for tab groups, and introduce a distinction between groups that the user wants to permanently save or not (star icon instead of pencil, only way to name a group is to star it)

I think 3 is the best interface, but realistically we probably only have time for 1 or 2.

Comment 25

[Vish]

(In reply to comment #23)
> I should have said this before instead of going for the multi-step email spam.
> Sorry.
> 
> (In reply to comment #21)
> > Not sure why this has been punted. Without being able to save & restore the
> > grouped tabs on next start, Tab Candy seems more work and just eye-candy than
> > actually being useful. 
> 
> Grouped tabs are saved and restored on next start. While Panorama won't load
> the group data immediately at startup, the last visible group should be shown
> and group data (names, positions, etc) is available when Panorama loads.
>

No, it does *not* . Well atleast in Ubuntu Linux, running 4.0~b8~hg20101128r58314+nobinonly-0ubuntu1~umd1~maverick.

Everytime I close using the Minefield > 'Quit' menu option.
And Start FF4 using a launcher where the command is "firefox-4.0" .

I have waited a while after starting FF4 too, but the groups are *never* available in Panorama. I have tried saving the groups alone, tried saving the groups with pages loaded in the tabs. I have even waited 5mins after starting FF4 waiting for Panorama to 'load' and not performing any other action, not once were the groups available the next time. Hence I mentioned that there seems no use in doing any action with the groups. It just isnt working here.

If this isnt the bug, should I open a separate bug regarding this problem?

Comment 26

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #25)
> If this isnt the bug, should I open a separate bug regarding this problem?

Please do. Make sure I'm CCed and we can discuss your issue there.

This bug is mostly just a change in who is in control of the data when we startup/quit. Instead of Panorama telling sessionstore to hold data, sessionstore should query Panorama for data or tell Panorama it has data when it's ready. While this does have an effect on user-facing features, it's not something that would change the game. It would just make many of the internals easier.

Comment 27

[Vish]

(In reply to comment #26)
> (In reply to comment #25)
> > If this isnt the bug, should I open a separate bug regarding this problem?
> 
> Please do. Make sure I'm CCed and we can discuss your issue there.
> 
Gah! it was a PEBKAC! not a bug. ;-)
With FF3 and earlier, I had set the Start-Up Preferences as "Show my Home Page".
And when I had migrated to FF4 it carried over the prefs, and hence the 'issue' of not storing tab groups. 

When I changed it to "Show windows and tabs from last time" it works.. Sorry for the noise.  :s

Comment 28

[Alex Faaborg [:faaborg] (Firefox UX)]

>and hence the 'issue'

yeah, that's not an "issue" but rather an issue.  We can't ship with a default pref that eliminates user data.

Comment 29

[Vish]

(In reply to comment #28)
> >and hence the 'issue'
> 
> yeah, that's not an "issue" but rather an issue.  We can't ship with a default
> pref that eliminates user data.

Let's not give up. :-)

I filed Bug 618243 , maybe we can prevent this scenario.


(In reply to comment #24)
> Possible solutions in order of difficulty:
> 2) automatically restore, but use bar-tab like loading for tab groups

I tried bar-tab and it works awesome with TabGroups! Not loading the tabs/groups on startup saves a lot of on startup time.

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)