Closed Bug 580512 Opened 10 years ago Closed 10 years ago

App tabs should outlive normal sessions

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b4
Tracking Status
blocking2.0 --- beta4+

People

(Reporter: jruderman, Assigned: dao)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: relnote, Whiteboard: [4b2] [Input][in-litmus-bug-week])

Attachments

(1 file, 5 obsolete files)

Steps:
1. Turn a tab into an app tab
2. Restart Firefox

Result:
* Blake has session restore enabled.  His app tabs turn into normal tabs.
* I have session restore disabled.  My app tabs are just forgotten.

Expected:
* Blake should have his app tab restored using session store.
* I should have my app tab restored from its URL.
No longer blocks: pinnedtabs
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 575560
It's still not working for me.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
We need more precise STR then. I assume you're running a recent nightly...
(In reply to comment #3)
> I assume you're running a recent nightly...

Also, without add-ons which could break this?
App tabs are only remembered if I enable session restore.  I expect them to be remembered (URL only) even if I have session restore disabled, and to not be counted toward the session restore prompt (which by default appears if you have 2+ tabs open).

STR:
1. Launch Firefox with a new profile.
2. Make sure only one tab is shown.
3. Right-click on the tab and select "Make into App Tab".
4. Quit using Cmd+Q.
5. Relaunch Firefox.

Result: app tab is gone.
I just confirmed Jesse's steps (and kind of wish I hadn't, since now I have to recreate all my app tabs)
Blocks: pinnedtabs
Status: REOPENED → NEW
OS: Mac OS X → All
Hardware: x86 → All
Summary: App tabs are not remembered between sessions → App tabs are not remembered independently from sessions
Adding whiteboard keyword to get it into triage list.
Whiteboard: [4b2]
It seems like we erred when we made App Tabs part of Session Restore; they shouldn't be considered part of a user's browsing session, they're a user's declared applications and should always be restored.

This blocks a future beta for testing.
blocking2.0: ? → betaN+
Duplicate of this bug: 578796
Duplicate of this bug: 582756
On OSX 10.5.8 and Firefox 4.0b2 even when I simply close the window, without quitting the application, when I reopen a new window the app tabs disappear
I do agree with Jesse that an app tab should be restored once it's app-tabbed.

But, would this be a paradigm-conflict between an app tab and a bookmark?
--> Dao, but not sure if there's a better App Tab owner. Maybe fryn can help?
Assignee: nobody → dao
Duplicate of this bug: 583261
Duplicate of this bug: 583284
Whiteboard: [4b2] → [4b2] [Input]
Attached patch wip (obsolete) — Splinter Review
this doesn't open the home page when restoring app tabs -- I don't know yet which code prevents this
Component: Tabbed Browser → Session Restore
QA Contact: tabbed.browser → session.restore
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #462374 - Attachment is obsolete: true
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #462784 - Attachment is obsolete: true
Keywords: relnote
Attached patch patch with test (obsolete) — Splinter Review
Attachment #463903 - Attachment is obsolete: true
Attachment #464190 - Flags: review?(dietrich)
Comment on attachment 464190 [details] [diff] [review]
patch with test

>-  saveState: function sss_saveState(aUpdateAll) {
>+  saveState: function sss_saveState(aUpdateAll, aPinnedOnly) {
>     // if crash recovery is disabled, only save session resuming information
>     if (!this._resume_from_crash && this._loadState == STATE_RUNNING)
>       return;
> 
>     // if we're in private browsing mode, do nothing
>     if (this._inPrivateBrowsing)
>       return;
> 
>-    var oState = this._getCurrentState(aUpdateAll);
>+    var oState = this._getCurrentState(aUpdateAll, aPinnedOnly);
>+    if (!oState)
>+      return;
>+
>+    if (aPinnedOnly)
>+      this._prefBranch.setBoolPref("sessionstore.resume_session_once", true);
>+
>     oState.session = {
>       state: this._loadState == STATE_RUNNING ? STATE_RUNNING_STR : STATE_STOPPED_STR,
>       lastUpdate: Date.now()
>     };
>     if (this._recentCrashes)
>       oState.session.recentCrashes = this._recentCrashes;
> 
>     this._saveStateObject(oState);

At the very least, this isn't going to save app tabs if you crash with browser.sessionstore.resume_from_crash set to false. I would still expect those to persist.

This also doesn't solve the problem with app tabs not being restored after closing the window on OS X. I'd be ok fixing that one in a followup though.

As an aside, I'm not a huge fan of app tabs being tacked onto session restore. I guess it'll stay, but it seems as though it wasn't planned out too well and is requiring more underlying changes to session restore than was probably assumed. Doing it in session restore has its advantages too though... Not much to do about it now.
(In reply to comment #22)
> At the very least, this isn't going to save app tabs if you crash with
> browser.sessionstore.resume_from_crash set to false. I would still expect those
> to persist.

Not sure why we'd draw a line here if the user wants his data blown away. I guess I don't see the point of the pref... Does it go back to the days before we had about:sessionrestore?
> As an aside, I'm not a huge fan of app tabs being tacked onto session restore.
> I guess it'll stay, but it seems as though it wasn't planned out too well and
> is requiring more underlying changes to session restore than was probably
> assumed. Doing it in session restore has its advantages too though... Not much
> to do about it now.

Where else makes more sense? They are tabs, albeit with special properties in how they're displayed in chrome, and are treated as such in all the tab-related APIs. Their content needs to be restored like any other tab's content: form data, storage, whatnot, which is SS's primary job.
(In reply to comment #23)
> (In reply to comment #22)
> > At the very least, this isn't going to save app tabs if you crash with
> > browser.sessionstore.resume_from_crash set to false. I would still expect those
> > to persist.
> 
> Not sure why we'd draw a line here if the user wants his data blown away. I
> guess I don't see the point of the pref... Does it go back to the days before
> we had about:sessionrestore?

Requesting UX input on this.
Keywords: uiwanted
(In reply to comment #24)
> Their content needs to be restored like any other tab's content: form
> data, storage, whatnot, which is SS's primary job.

I had kind of assumed they would work that way, but, FWIW, that's probably going to put me off using them. If I'm not restoring my session, I'd quite like a clean start. If I've got Gmail or Facebook or Twitter or whatever in an app tab, I'd be happy to see it load up the latest content on my "home page" on that site when I click on it, rather than reloading the state of my news feed or inbox or whatever other page on that "app" site I had moved to. Not to mention taking up memory and CPU time with something I may not even look at in this session.

Anyway, my individual view doesn't matter, my point is that it shouldn't be an assumption that app tabs have to preserve their exact state between sessions - actual desktop apps usually don't.
Would it not just have made more sense to duplicate the session restore and use a tweaked version for the App Tabs. To simply tack them onto the existing session restore seems a bit ****.
(In reply to comment #27)
> Would it not just have made more sense to duplicate the session restore and use
> a tweaked version for the App Tabs. To simply tack them onto the existing
> session restore seems a bit half-arsed.

Why would that have made more sense?

It would've entailed duplication of code, more constantly-running XPCOM services, and more XPCOM cruft at startup.

And please try to contribute to the conversation without verbally deriding the work of others. The guy who wrote the patch isn't stupid and wasn't drunk (I hope!). Sometimes it takes >1 try to get things right.

(In reply to comment #26)
> Anyway, my individual view doesn't matter, my point is that it shouldn't be an
> assumption that app tabs have to preserve their exact state between sessions -
> actual desktop apps usually don't.

True. I'm not 100% about restoring full state for app tabs. But these are web apps like Gmail, or Wordpress, where you might have a half-written document that you want restored, so I'm advocating the route of zero dataloss for now, until we get some more input from the UX team about how they envisioned app tabs to work.

However, this conversation is straying from the original bug. File a new bug if you don't want app tabs' state restored.
(In reply to comment #28)
> (In reply to comment #27)
> > Would it not just have made more sense to duplicate the session restore and use
> > a tweaked version for the App Tabs. To simply tack them onto the existing
> > session restore seems a bit ****.
> 
> Why would that have made more sense?
> 
> It would've entailed duplication of code, more constantly-running XPCOM
> services, and more XPCOM cruft at startup.
> 
> And please try to contribute to the conversation without verbally deriding the
> work of others. The guy who wrote the patch isn't stupid and wasn't drunk (I
> hope!). Sometimes it takes >1 try to get things right.
> 

First of all, my apologies to Dao. It was disrespectful and that wasn't my intention.

Back on topic I think it's as simple a question as "Can App Tabs outgrow the usage of the session storage" and the answer is yes, so why not lay the foundations for that potential growth now? That way, should thinks go the other way, it's as easy to disconnect the App Tabs logic.
(In reply to comment #23)
> (In reply to comment #22)
> > At the very least, this isn't going to save app tabs if you crash with
> > browser.sessionstore.resume_from_crash set to false. I would still expect those
> > to persist.
> 
> Not sure why we'd draw a line here if the user wants his data blown away. I
> guess I don't see the point of the pref... Does it go back to the days before
> we had about:sessionrestore?

I don't know the exact origin of that pref but it indicates that the user doesn't want their session restored if the browser crashes. So if app tabs are truly independent from sessions, then the app tabs should *always* be restored.

(In reply to comment #24)
> > As an aside, I'm not a huge fan of app tabs being tacked onto session restore.
> > I guess it'll stay, but it seems as though it wasn't planned out too well and
> > is requiring more underlying changes to session restore than was probably
> > assumed. Doing it in session restore has its advantages too though... Not much
> > to do about it now.
> 
> Where else makes more sense? They are tabs, albeit with special properties in
> how they're displayed in chrome, and are treated as such in all the tab-related
> APIs. Their content needs to be restored like any other tab's content: form
> data, storage, whatnot, which is SS's primary job.

I realize that it fits best here. And I agree that we want the data, history, etc restored. It just seems like we might be chasing this down the rabbit hole. This is not the last thing that's going need to change in session restore. If that's what we need to do, then we need to do it. I didn't mean to derail this conversation, I just wanted to voice my concern.
answer for ui-wanted request: The value of browser.sessionrestore.resume_from_crash shouldn't have an effect on restoring app tabs. Making a page an "app tab" should be thought of like installing an application on your (web) desktop. It's a permanent installation action. To undo it, you should need to take an explicit uninstall action.

Any chance this could be made ready for beta 4? Not sure how close this patch is given all the comments.
Keywords: uiwanted
Attached patch patch v3 (obsolete) — Splinter Review
Could somebody please run sessionstore tests with this on OS X? Don't want to stress the tryserver too much. An OS X tryserver run with an older checkpoint had a failure in browser_394759.js and a timeout in browser_580512.js. I think I've got the latter covered. Not sure what was going on with the former.

All tests pass for me locally on Linux.
Attachment #464190 - Attachment is obsolete: true
Attachment #464190 - Flags: review?(dietrich)
Attachment #464803 - Flags: review?(dietrich)
(In reply to comment #33)
> Run OSX only?

Yes, I did that, still eats resources and takes half a day to complete though.
Summary: App tabs are not remembered independently from sessions → App tabs should outlive normal sessions
Comment on attachment 464803 [details] [diff] [review]
patch v3

>+    if (aPinnedOnly) {
>+      // fold pinned tabs from all windows into a single window
>+      let tabArrays = total.map(function (w) w.tabs.filter(function (t) t.pinned));
>+      total[0].tabs = tabArrays.reduce(function (a, b) a.concat(b));
>+      total.length = 1;
>+      if (total[0].tabs.length == 0)
>+        return null;
>+
>+      ix = -1;
>+      lastClosedWindowsCopy = [];
>+    } else {

This doesn't seem right. As a user, I'd expect the same number of windows to be restored as closed. In this app-tabs-aren't-mirrored world, we can definitely expect users to have task-specific windows with app tabs that are particular to those tasks on them. (Note: I think we might be shifting to app-tab mirroring across windows.)

The other session restore changes look ok though. Please get a browser peer to review the changes outside of SS.
Attachment #464803 - Flags: review?(dietrich) → review-
(In reply to comment #35)
> This doesn't seem right. As a user, I'd expect the same number of windows to be
> restored as closed.

That's true, definitely.

> In this app-tabs-aren't-mirrored world, we can definitely
> expect users to have task-specific windows with app tabs that are particular to
> those tasks on them. (Note: I think we might be shifting to app-tab mirroring
> across windows.)

I believe that's the case, indeed. I agree that until that change is made, your comment here is valid.
Attached patch patch v4Splinter Review
Addressed comment 38. Requesting review from vlad for non-sessionstore bits.
Attachment #464803 - Attachment is obsolete: true
Attachment #465317 - Flags: review?(vladimir)
Attachment #465317 - Flags: review?(dietrich)
(In reply to comment #37)
> 
> Addressed comment 38.

That's fantastic, thanks. I expect my account should show the €1,000,000 any moment then?
I trusted you, carelessly. Won't do it again.
Comment on attachment 465317 [details] [diff] [review]
patch v4

>-#ifndef XP_MACOSX
>     case "browser-lastwindow-close-granted":
>       // last browser window is quitting.
>       // remember to restore the last window when another browser window is openend
>       // do not account for pref(resume_session_once) at this point, as it might be
>       // set by another observer getting this notice after us

while you're here, can you fix the typo s/openend/opened/?

>         delete state.hidden;
>-        state = { windows: [state] };
>-        this._restoreCount = 1;
>-        this.restoreWindow(aWindow, state, this._isCmdLineEmpty(aWindow));
>+#ifndef XP_MACOSX
>+        if (!this._doResumeSession())
>+#endif
>+          state.tabs = state.tabs && state.tabs.filter(function (tab) tab.pinned);
>+        if (state.tabs && state.tabs.length > 0) {
>+          this._restoreCount = 1;
>+          state = { windows: [state] };
>+          this.restoreWindow(aWindow, state, this._isCmdLineEmpty(aWindow, state));
>+        }

state.tabs being either false or tabs is kind of wonky - maybe just make it an empty array?
Attachment #465317 - Flags: review?(dietrich) → review+
We should get this for beta4, prioritizing based on user feedback!
blocking2.0: betaN+ → beta4+
http://hg.mozilla.org/mozilla-central/rev/3137ecdfdb60
Status: NEW → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b4
Depends on: 587299
Depends on: 587464
Depends on: 589246
Added to Litmus:
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=12816
Flags: in-litmus+
Whiteboard: [4b2] [Input] → [4b2] [Input][in-litmus-bug-week]
Depends on: 600545
No longer depends on: 600545
Depends on: 601161
This change is causing a pretty major breakage of the SessionStore API when crash recovery is disabled since non-pinned tabs are getting removed during API calls.  See bug 600545 and bug 601161.  It also breaks the reliability of the restore closed window functionality in certain instances as well (bug 589246).
Depends on: 601198
You need to log in before you can comment on or make changes to this bug.