576110 - Browser restart behavior for TabCandy

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Aza Raskin [:aza]]

This is a re-working of bug #575903.

Please see the design specifications here: https://wiki.mozilla.org/Firefox/Projects/TabCandy/Design/BrowserRestart

The guiding principal for Firefox restart is to put the user back to exactly where they were before. If Tab Candy was open on quit/restart, we bring them back to Tab Candy. If they were on a particular tab, bring them back there. There are three cases for browser restart that we need to handle for Tab Candy. It is important that restart remain highly performant. 

'''(1) Tab Candy was last open.'''

In this case, Tab Candy is opened at launch. The loading of TabCandy must feel near instant and we should be minimizing startup time regressions. While a spec should not dictate implementation directions, at the very least we should be saving the thumbnails of tabs from last time to make layout super fast. If that isn't fast enough, we can potentially save an image of the entire TabCandy interface.

'''(2) A tab was last open.'''

In this case, we return to that Tab. There should be no "loading" phase where there are extra tabs in the tabstrip, and tabcandy should be loaded in the background.

'''(3) The session restore page is invoked'''

See the design wiki.

Comment 2

[Aza Raskin [:aza]]

I've updated the wiki to include design specifications for session restore.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: Patch for 2

Collapse restored tabs so no extra tabs are displayed during the "loading" phrase and tabcandy would show the correct tabs for the active group.

Comment 4

[Ian Gilman [:iangilman]]

Patch applied in:

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/df9bbe8b6d78

There's still a brief moment when all the tabs are visible, but it's much quicker than before. 

Is it possible to get rid of even that initial moment?

Comment 5

[Raymond Lee [:raymondlee]]

When the window.TabItems.init() is called and Groups.newTab(item) is triggered by the window.TabMirror.customize().  The "active" group is not set and the stored info (e.g. group id) for browser tabs are not ready, therefore all tabItems are added to the "new tab" group.  When the "urlChanged" event for a tab would trigger TabItems.reconnect() to move the tab into the right group afterward.

If you are in the "new tab" group when you exist Fx, you would see all the tabs when Fx starts.  However, if you are on other group, the issue should be fixed.

One possible solution is to create a "temp" group.  When Groups.newTab(tabItem) is called and no active group is null at startup, put the tab into the "temp" group and then the tab would be moved to the right group when the "urlChanged" event is fired.   We need to ensure that the "temp" group is not visible in the tabcandy interface.  Furthermore, when a orphan tab is selected, we should set the active group to something else e.g. -1 instead of null to distinguish the difference. 

What do you think?

Comment 6

[Ian Gilman [:iangilman]]

Interesting, yes the tabs do stick around longer if I was in the "new tab" group. The brief flash I mentioned  is if you're in one of the other groups, but it's very brief indeed. 

One thing to note is that the "new tab" group will be going away soon.

I propose we let it be for the time being, and focus on the other top-level items in this bug.

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #5)
> One possible solution is to create a "temp" group.  When Groups.newTab(tabItem)
> is called and no active group is null at startup, put the tab into the "temp"
> group and then the tab would be moved to the right group when the "urlChanged"
> event is fired.   We need to ensure that the "temp" group is not visible in the
> tabcandy interface.  Furthermore, when a orphan tab is selected, we should set
> the active group to something else e.g. -1 instead of null to distinguish the
> difference. 
> 
> What do you think?

The temp group might work, but seems very very hacky. In the long run, we probably want to assign tabs to the right groups maybe even before they start loading?

We could imagine that this could be used advantageously... for example, I believe work has been done so that the currently active tab gets network priority for loading on startup... maybe the active tab group could get priority?

Comment 8

[Raymond Lee [:raymondlee]]

(In reply to comment #7)
> (In reply to comment #5)
> > One possible solution is to create a "temp" group.  When Groups.newTab(tabItem)
> > is called and no active group is null at startup, put the tab into the "temp"
> > group and then the tab would be moved to the right group when the "urlChanged"
> > event is fired.   We need to ensure that the "temp" group is not visible in the
> > tabcandy interface.  Furthermore, when a orphan tab is selected, we should set
> > the active group to something else e.g. -1 instead of null to distinguish the
> > difference. 
> > 
> > What do you think?
> 
> The temp group might work, but seems very very hacky. In the long run, we
> probably want to assign tabs to the right groups maybe even before they start
> loading?
> 
> We could imagine that this could be used advantageously... for example, I
> believe work has been done so that the currently active tab gets network
> priority for loading on startup... maybe the active tab group could get
> priority?

You are right.  In the long run, we should really integrate the tabcandy startup code into the session store so we can get the info (e.g. group id and other values) for each stored immediately when they are available.  At the moment, we are replying on setTimeout and url change event.

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: Patch for 1 Part 1

The patch shows the tab candy at startup if it was last open. There is a delay to show the tab candy interface because session store data is not available immediately at startup and also need to make the layout faster; those will come in as another patches.

Comment 10

[Aza Raskin [:aza]]

(In reply to comment #7)
Will it help that we will be adding a unique identifier to each tab, so that we do not need to wait for the urlChanged event?

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #9)
> Created attachment 457003 [details] [diff] [review]
> Patch for 1 Part 1

Patch accepted.

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/23ee7e46a0b4

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: Patch for 1 Part 2

This patch shows saved thumbnails and titles at browser startup if tab candy was last open when quitting Fx.

As Aza suggested in comment 0, an image of the entire TabCandy
interface can be saved to make the loading of Tab candy near instant but there several things to consider:
1) User resizes the window to make it larger and the Tab Candy screenshot doesn't fit the whole window. We can stretch the screenshot but everything would look odd.
2) User is not patient and clicks on a thumb to zoom in but he is actually clicking on the Tab Candy screenshot and everything is still loading at the background.

The best thing is to integrate tab candy into the session restore code and minimises the loading time of tab candy at startup.  Any other suggestions?

Comment 13

[Aza Raskin [:aza]]

The more we can do without resorting to tricks the better. That said, if we do need to use an image then:

(1) Ian has been proposing a resolution independent layout of tabcandy which then has a view in each window. That solves this problem.
(2) In this rare event, we just show a "throbber" saying "loading... please wait" or some such and then after the second of waiting, continue on with the animation.

Comment 14

[Ian Gilman [:iangilman]]

Yes, let's not save a snapshot of the entire window until we've exhausted all other possibilities. 

Raymond, I've applied your patch, and I have a couple of comments: 

- How about sticking the image data into the tab's existing canvas rather than adding a temporary canvas?
- I see you're also saving the page title, but you don't seem to be loading it.
- All of the new tab watching mechanism in reconnect() seems pretty messy right there. Is there somewhere to encapsulate it and/or combine it with our other tab watching tech (such as the stuff in mirror.js). 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/0acf67333c04

Comment 15

[Raymond Lee [:raymondlee]]

Attachment: Patch for 1 Part 3

- Tried to use the existing canvas but sometimes image isn't loaded correctly into the canvas. Also, the code e.g. paint() and send the urlChange event in _heartbeat() make the logic more complex to use the canvas to handle the cached image as well as the view of content window.  Now, it's using "img" tag to show the cached image and switch back to the canvas when DOMContentLoaded event is received.
- Move some code to the mirror.js to keep the reconnect() cleaner.
- The page titles are used in the previous patch as well as this one.  See the last line of showCachedData()

Comment 16

[Ian Gilman [:iangilman]]

Comment on attachment 457865 [details] [diff] [review]
Patch for 1 Part 3

Let's give it a try; please apply. One comment: in the Tabs.onReady function, we don't need two timeouts... please combine them.

Comment 17

[Ian Gilman [:iangilman]]

btw, thanks for the refactoring... definitely feels cleaner.

Comment 18

[Raymond Lee [:raymondlee]]

(In reply to comment #16)
> Comment on attachment 457865 [details] [diff] [review]
> Patch for 1 Part 3
> 
> Let's give it a try; please apply. One comment: in the Tabs.onReady function,
> we don't need two timeouts... please combine them.

Applied
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/78590b6c9948

Another one:
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/28a1ab977bf5

I've made some minor changes so it doesn't call the second timeout every time when Tabs.onReady is invoked. A second timeout with few seconds delay is needed because "DOMContentLoaded" event only means the DOM is ready but referenced stylesheets, images, and subframes may not be fully loading.
https://developer.mozilla.org/en/Gecko-Specific_DOM_Events

Alternatively, we can listen to the "load" event when a page is fully-loaded and then run the hideCachedData() method.  This seems to be more reliable than using the second timeout in the Tabs.onReady.

Comment 19

[Ian Gilman [:iangilman]]

Yes, if we can do it based on a reliable event, let's do that instead of guessing with the timeout. 

Also, what I'm seeing when TabCandy starts up is the cached images come into place right away, and then they get replaced by the canvas, which looks different and then the canvas updates a moment later and looks a lot more like what the cached image looked like. Is this related? At any rate, the goal should be a smooth (ideally imperceptible) transition from the cache to the live ones.

Comment 20

[Raymond Lee [:raymondlee]]

(In reply to comment #19)

Now switched to the load event and also added animated transition from the cache to the live ones. 
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/f3311956828c

Comment 21

[Ian Gilman [:iangilman]]

Raymond, 

That animation didn't really do the trick; it created a white flash that drew even more attention to itself. I've gone ahead and removed it along with some additional changes in: 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/192b9886313a

* We're now switching away from the cached thumbnail right on repaint rather than guessing about when it will happen. 
* Instead of passing in tab and then using tab.mirror, we're now using "this" (since the routine is already in the mirror's scope).
* Hiding the canvas and then showing it later was causing some weird size jumps after the cached thumbnail went away; we're now doing zero opacity instead, which fixes the problem.

Comment 22

[Glen A.]

(In reply to comment #7)
> We could imagine that this could be used advantageously... for example, I
> believe work has been done so that the currently active tab gets network
> priority for loading on startup... maybe the active tab group could get
> priority?
See bug 561149.

Comment 23

[Trent Waddington]

I searched, it doesn't look like it has been reported: Tab Candy doesn't restore tabs in a tab group in the same order as before the restart.  Selecting a tab in a tab group then results in an out-of-order tab bar.

Comment 24

[Ian Gilman [:iangilman]]

(In reply to comment #23)
> I searched, it doesn't look like it has been reported: Tab Candy doesn't
> restore tabs in a tab group in the same order as before the restart.  Selecting
> a tab in a tab group then results in an out-of-order tab bar.

Thank you for the report, Trent! Can you enter a new bug for this? This bug is starting to lose focus.

Comment 25

[Trent Waddington]

See bug 583085.

Comment 26

[(no longer active)]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".

Comment 27

[John Bird]

Too slow - on restart each tab takes around 1 second to be reloaded into
groups.   100 tabs -> 100 seconds.

Also 100 tabs  uses 900MB memory.

Comment 28

[Ian Gilman [:iangilman]]

What's left to do on this bug? If there's anything that's not already covered by another bug, lets file follow-ups and close this bug out.

We already have: 

Bug 591704 - Loading the Panorama (tab view) view can be very slow with many tabs 

Bug 597248 - Make sure Panorama's thumbnail cache is solid

Do we need anything else?

Comment 29

[juan becerra [:juanb]]

Preserving state. I know we don't have solid steps to reproduce issues like bug 598707, but they do seem to happen after restart.

Comment 30

[Ian Gilman [:iangilman]]

(In reply to comment #29)
> Preserving state. I know we don't have solid steps to reproduce issues like bug
> 598707, but they do seem to happen after restart.

But we are preserving state; that feature exists, it just has bugs. We don't need this bug open for that, we just need bugs for the individual issues (such as bug 598707).

Comment 31

[Raymond Lee [:raymondlee]]

Bug 599213 Update the session restore page for Panorama.

I believe we have all the bugs filed.

Comment 32

[Ian Gilman [:iangilman]]

(In reply to comment #31)
> Bug 599213 Update the session restore page for Panorama.
> 
> I believe we have all the bugs filed.

Excellent; closing this bug.

Comment 33

[Tracy Walker [:tracy]]

verified with recent builds of minefield