575560 - Hook up session restore with app tabs

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Dão Gottwald [:dao]]

Bug 563730 added a "pinned" property to tabs. Session restore should probably store this and re-pin these tabs on startup, although Blair thought session restore should ignore these tabs entirely, in which case they would have to be restored in some other way.

Comment 1

[Dão Gottwald [:dao]]

Attachment: rough patch for restoring the 'pinned' state

Comment 2

[Dão Gottwald [:dao]]

Attachment: rough patch for restoring the 'pinned' state

just spotted that aTabs[0].clientWidth isn't what we want if the first tab is pinned

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Are the unpinTab calls needed? There should never be pinned tabs when this code is loaded, right?

Comment 4

[Dão Gottwald [:dao]]

It could be needed if restoreWindow is called with aOverwriteTabs and a window with pinned tabs. unpinTab returns very early when it has nothing to do, so this shouldn't be a problem.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

This seems to treat pinned tabs just like normal tabs which take up less space, and not like AppTabs. Specifically, that they are meant to be shared between all windows (ie, all windows show the same AppTabs, and only one instance is ever loaded for all windows), and shouldn't load until first selected.

(In reply to comment #0)
> although Blair thought session restore should ignore these tabs entirely

To clarify: I think things like the form values & the state object should still remain in session restore (although, not tied to a window, since they're meant to be single-instance). But remembering which AppTabs are configured should be outside of session restore, because they're more permanent than sessions.

Comment 6

[Please Ignore This Troll (Account Disabled)]

undo close tab - this action also restores pinned tabs as ordinary ones.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #5)
> Specifically, that they are meant to be shared between
> all windows (ie, all windows show the same AppTabs, and only one instance is
> ever loaded for all windows), and shouldn't load until first selected.

I'm not sure that proposed behavior is ideal, but even assuming it is, this change doesn't preclude us from implementing that behavior (it's entirely orthogonal to it, AFAICT).

Comment 11

[Please Ignore This Troll (Account Disabled)]

There is an extension that does faviconizing tabs (e.g. make tabs into app tabs), and it has not this (and other) bugs that current built-in minefield realisation has. Maybe it's better to copy the code of the extension?

Comment 12

[Cork]

The reason for this is, that the FaviconizeTab does much less then what the app tab is meant to do when its finned. FaviconizeTab only changes the size of a normal tab, while the app tab is meant to let web application work more like normal computer programs. Cross browser windows and much, much, more.

Comment 13

[Please Ignore This Troll (Account Disabled)]

Well, I didn't know that something was planned to be added to "app tab" feature.
What is "cross browser windows"? since when MoFo cares about other browsers?
tab is a page opened in a browser, how can it be a "normal computer program"? the site itself should represent those features (like meebo is a good IM), what has browser to do with web pages than just displaying them?
I'm curious, since such kind of information seems to be kept in a top secret.

Comment 14

[Johnathan Nightingale [:johnath]]

This definitely blocks final, but it would be kind of swell to get this into beta2 since that's the first appearance of the context menu UI. If we agree with Gavin that the question of cross-window app tab singletons is orthogonal, can we get a version of this patch together for review?

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 454815 [details] [diff] [review]
rough patch for restoring the 'pinned' state

I think we should take this simple patch and file a new bug for the more involved cross-window persistence.

Comment 16

[Johnathan Nightingale [:johnath]]

Probably want a different reviewer here, zpao's out for a few weeks. Dietrich, or Simon maybe?

Comment 19

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

This needs beta feedback, and I'd really really prefer that it be done for beta2. I like Dao's suggestion in comment 15, but needs a quick review and a resummarization. Dao: can you resummarize the bug?

Dietrich: can you review?

Comment 20

[:Gavin Sharp [email: [e-mail]]]

The summary's fine - the other issue is not really related.

Comment 21

[Dietrich Ayala (:dietrich)]

Comment on attachment 454815 [details] [diff] [review]
rough patch for restoring the 'pinned' state


>     
>     if (aTabs.length > 0) {
>       // Determine if we can optimize & load visible tabs first
>       let maxVisibleTabs = Math.ceil(tabbrowser.tabContainer.mTabstrip.scrollClientSize /
>-                                     aTabs[0].clientWidth);
>+                                     aTabs[aTabs.length - 1].clientWidth);

why this change?

patch looks ok otherwise. i'm concerned that pinning tabs moves them, so run the unit tests to make sure there's not adverse affect there. r=me with tests passing and the above change addressed.

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #21)
> >       // Determine if we can optimize & load visible tabs first
> >       let maxVisibleTabs = Math.ceil(tabbrowser.tabContainer.mTabstrip.scrollClientSize /
> >-                                     aTabs[0].clientWidth);
> >+                                     aTabs[aTabs.length - 1].clientWidth);
> 
> why this change?

The first tab might be an app tab, which would cause us to assume a nonsensical high number of "max. visible tabs".

> patch looks ok otherwise. i'm concerned that pinning tabs moves them, so run
> the unit tests to make sure there's not adverse affect there. r=me with tests
> passing and the above change addressed.

Tests are passing, but we don't test with pinned tabs, so I don't think this means much...

Comment 23

[Dietrich Ayala (:dietrich)]

> > patch looks ok otherwise. i'm concerned that pinning tabs moves them, so run
> > the unit tests to make sure there's not adverse affect there. r=me with tests
> > passing and the above change addressed.
> 
> Tests are passing, but we don't test with pinned tabs, so I don't think this
> means much...

hah, good point :) please add a test then - we need to be sure restoring app tabs doesn't break other restoration bits.

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/8e8042431c86

Comment 26

[Tanner M. Young [:tmyoung]]

Verified Fixed on Mozilla/5.0 (Windows; Windows NT 6.0; WOW64; en-US; rv:2.0b2pre) Gecko/20100717 Minefield/4.0b2pre ID:20100717054110