The default bug view has changed. See this FAQ.

Add single tab save/restore to session store API

RESOLVED FIXED in Firefox 3 beta4

Status

()

Firefox
Session Restore
--
enhancement
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Manish Singh, Assigned: Simon Bünzli)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
Firefox 3 beta4
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
I propose the following API addition to nsISessionStore:

  AString getTabState(in nsIDOMNode aTab);
  void setTabState(in nsIDOMNode aTab, in AString aState);

analogous to {get,set}{Browser,Window}State, except it operates on a single tab.

If this API is acceptable, I can cook up a patch.
(Assignee)

Comment 1

10 years ago
What use case(s) did you have in mind for these two methods?

And please take into consideration that cookies are currently saved/restored per window and not per tab.
(Reporter)

Comment 2

10 years ago
An extension that does a full state bookmark and restore of tabs (not just URL and title, but also sessionHistory, scroll position, i.e. all the stuff session saver saves).

This is already working, but what I've done was use getWindowState and pull out the the tab I want out of the tabs array, which I feel is an abuse of the API, not to mention the wastefulness of snapshotting the other tabs I don't care about. For restore, I cut'n'pasted code from nsSessionStore.js, so that's obviously suboptimal.

I'm aware that cookies are per window. In the current implementation I just ignore them, need to think through that interaction further anyway.
(Assignee)

Comment 3

10 years ago
CCing Mike Shaver for a second opinion on the usefulness of the proposal (see comment #0).

(In reply to comment #2)
You can actually restore the tab also with only three or four lines of code (see the attachment at bug 298571 comment #18 and bug 344640 comment #24).

In fact, the only difference between the proposed setTabState and the existing setWindowState is that setWindowState will create a new tab for you whereas setTabState reuses an existing one.

So the main gain would be a slightly more obvious interface and a certain performance win when getting the tab state. Might still be worth it, though.

Manish: If you want to implement this, please have a look at the attachment at bug 298571 comment #23 for how to update the data related to a single tab only (just ignore the privacy override bits!). And as long as cookies aren't per tab (bug 117222) and you don't get another clever idea for what to do about them, adding a note to the interface definition might be enough.
(Reporter)

Comment 4

10 years ago
(In reply to comment #3)
> You can actually restore the tab also with only three or four lines of code
> (see the attachment at bug 298571 comment #18 and bug 344640 comment #24).
> 
> In fact, the only difference between the proposed setTabState and the existing
> setWindowState is that setWindowState will create a new tab for you whereas
> setTabState reuses an existing one.

In the extension, I've implemented the (slightly) higher level interface to take the same semantics as openUILinkIn, so I want the option to replace a current tab.

> Manish: If you want to implement this, please have a look at the attachment at
> bug 298571 comment #23 for how to update the data related to a single tab only
> (just ignore the privacy override bits!). And as long as cookies aren't per tab
> (bug 117222) and you don't get another clever idea for what to do about them,
> adding a note to the interface definition might be enough.

Will do, once I get buy in from shaver and whoever else that the API is acceptable.

I think the intent of this API is on the side of the angels.  It will let the caller express more clearly the intended scope of changes, which means that any subsequent optimizations we do to avoid writing the whole session state out on every load, etc., will be more effective.  It also aids in understanding the calling code and reducing the amount of data that needs to be considered when analyzing the behaviour.
(Assignee)

Updated

9 years ago
Depends on: 407162
(Assignee)

Comment 6

9 years ago
Created attachment 292624 [details] [diff] [review]
getTabState/setTabState/duplicateTab

This patch adds three new methods to our API:
* getTabState returns a JSON representation of the data concerning a single tab (really just the subset of what get(Window|Browser)State returns for a tab)
* setTabState loads that state into an existing tab
* duplicateTab copies a tab while ignoring the privacy_level pref (so that form data, etc. is retained for https pages as well)

The first two would now allow to easily implement undoCloseTab like functionality (in fact, our own undoCloseTab feature gets the very same data) or to move individual tabs between sessions/computers/whatever the original use case was. The third one is needed mainly for bug 298571 but will benefit extensions as well, simplifying e.g. window-merging/-splitting.

Mike: Any concerns about these additions?
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
Attachment #292624 - Flags: superreview?(shaver)
Attachment #292624 - Flags: review?(dietrich)
(Assignee)

Updated

9 years ago
Blocks: 298571
Target Milestone: --- → Firefox 3 M11
(Reporter)

Comment 7

9 years ago
Thanks for implementing this! I hadn't found the time yet to flesh out an implementation myself.
(Assignee)

Comment 8

9 years ago
Dietrich, Mike: Please make sure that you get to this bug before Beta 3, as the UI for this back-end change has already landed...

(In reply to comment #7)
In case you find time to actually use it (or consider whether it's fit for your use case), please provide some feedback as to whether this is what you originally requested.
(Reporter)

Comment 9

9 years ago
Yes, this is exactly what I wanted! I've ported over the extension to use it (after applying the patch in the bug), and it works great.

For reference, the extension is now on AMO: https://addons.mozilla.org/en-US/firefox/addon/5756

Obviously the version there doesn't contain the code to use the new API here, but it was trivial to plug in.
Comment on attachment 292624 [details] [diff] [review]
getTabState/setTabState/duplicateTab

>Index: browser/components/sessionstore/nsISessionStore.idl
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/sessionstore/nsISessionStore.idl,v
>retrieving revision 1.8
>diff -u -6 -d -p -r1.8 nsISessionStore.idl
>--- browser/components/sessionstore/nsISessionStore.idl	11 Dec 2006 09:45:24 -0000	1.8
>+++ browser/components/sessionstore/nsISessionStore.idl	11 Dec 2007 18:12:33 -0000
>@@ -44,13 +44,13 @@ interface nsIDictionary;
> /**
>  * nsISessionStore keeps track of the current browsing state - i.e.
>  * tab history, cookies, scroll state, form data, POSTDATA and window features
>  * - and allows to restore everything into one window.
>  */
> 
>-[scriptable, uuid(11852a90-20de-11db-a98b-0800200c9a66)]
>+[scriptable, uuid(58d17e12-a80f-11dc-8314-0800200c9a66)]
> interface nsISessionStore : nsISupports
> {
>   /**
>    * Initialize the service
>    */
>   void init(in nsIDOMWindow aWindow);
>@@ -82,12 +82,35 @@ interface nsISessionStore : nsISupports
>    * @param aState     is a JSON string representing a session state.
>    * @param aOverwrite boolean overwrite existing tabs
>    */
>   void setWindowState(in nsIDOMWindow aWindow, in AString aState, in boolean aOverwrite);
> 
>   /**
>+   * @param aTab is the tab whose state is to be returned.
>+   * 
>+   * @return a JSON string representing the state of the tab (note: this is no
>+   *         complete session state and is to be used only with setTabState).
>+   */

can you describe what is missing and why in the comment?

s/no/not/

actually, i don't think that the note part is really necessary.

>+
>+  setTabState: function sss_setTabState(aTab, aState) {
>+    var window = aTab.ownerDocument.defaultView;
>+    
>+    var tabState = this._safeEval("(" + aState + ")");
>+    tabState._tab = aTab;

can you add some basic validation of the state data here?

nit: could get |window| after the eval lines.

also, please include xpcshell tests for the new apis.
Attachment #292624 - Flags: review?(dietrich)
(Assignee)

Comment 11

9 years ago
Created attachment 295254 [details] [diff] [review]
nits fixed

(In reply to comment #10)
> can you describe what is missing and why in the comment?
> actually, i don't think that the note part is really necessary.

Most importantly, cookie data is missing (which we store per window), so I've left the (clarified) comment in.

> can you add some basic validation of the state data here?
> nit: could get |window| after the eval lines.

Done and done.

> also, please include xpcshell tests for the new apis.

Added basic tests as we've already got for the rest of SessionStore. Since I currently don't have a complete build environment with the test harnesses, I couldn't test them, though...
Attachment #292624 - Attachment is obsolete: true
Attachment #295254 - Flags: review?(dietrich)
Attachment #292624 - Flags: superreview?(shaver)
(Reporter)

Comment 12

9 years ago
Created attachment 299095 [details] [diff] [review]
Slightly cleaned up patch

The patch didn't apply for me, as it creates a new test_bug350525.xul which already exists. I renamed to test_bug393716.xul which is what it should have been.  I also had to stick the testbody inside CDATA. I've attached a patch with these changes.

However, but the duplicateTab test fails because calling that function results in:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "aData is undefined" {file: "file:///home/manish/ffdev/objectdir/dist/bin/components/nsSessionStore.js" line: 1178}]' when calling method: [nsISessionStore::duplicateTab]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://mochikit/content/chrome/browser/components/sessionstore/test/chrome/test_bug393716.xul :: <TOP_LEVEL> :: line 86"  data: yes]
************************************************************

I haven't looked at the code in detail enough to figure out why aData is undefined there.
Attachment #295254 - Attachment is obsolete: true
Attachment #295254 - Flags: review?(dietrich)
(Assignee)

Comment 13

9 years ago
Created attachment 299165 [details] [diff] [review]
fixed test breakage

Thanks for the clean up. Turns out that we didn't properly handle tabs which were loading with an incomplete tab state (e.g. missing tab index). Now all the added tests pass (at least in my ad hoc testing environment).
Attachment #299095 - Attachment is obsolete: true
Attachment #299165 - Flags: review?(dietrich)
(Reporter)

Comment 14

9 years ago
Tests pass here now, but later on in the test run (outside of the sessionsaver tests) I get:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "aEntry is undefined" {file: "file:///home/manish/ffdev/objectdir/dist/bin/components/nsSessionStore.js" line: 1195}]' when calling method: [nsIObserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

It looks like the same problem as above, not handling incomplete tab state, so easy fix would be to check if aEntry is defined in extractHosts and bail if it isn't.
(Assignee)

Comment 15

9 years ago
This looks like a different issue. aEntry in that situation should never be undefined. If it were, you should've got an error when trying to restore that defective state (because in sss_deserializeHistoryEntry we make the exact same assumption). Do you have a testcase for me to reproduce this new issue?
(Reporter)

Comment 16

9 years ago
Not really. This is spewed out during the full chrome mochitest run, but many tests after the sessionsaver tests, and it doesn't appear to result in any test failures either.
(Assignee)

Comment 17

9 years ago
Can't debug that myself. Could you please enclose the last line of _updateCookieHosts in a try-catch block and Cu.reportError(uneval(aWindow)); in case of an exception? This should copy such a faulty window state to the Error Console and hopefully gives us a clue about what might have gone wrong.

OTOH: Do these exceptions happen without the test to this bug added to the test suite as well? If not, you could also try to comment out the ss.setTabState(...) line and the two following ok(ss.getTabValue(...)) lines (and should that not help the duplicate-tab one as well) to make sure that it's really this test which causes the issue in the first place.

Thanks for your help!
(Reporter)

Comment 18

9 years ago
Yeah, they don't happen with the test removed.
(Reporter)

Comment 19

9 years ago
Or now even with the test added again. There's other test failures and exceptions in the current code, maybe that's interfering...
(Assignee)

Comment 20

9 years ago
(In reply to comment #19)
> There's other test failures and exceptions in the current code

"current code" hopefully not referring to SessionStore code... (otherwise please file new bugs)
(Reporter)

Comment 21

9 years ago
Yes, sorry I wasn't clear, the session store tests all pass still.
Comment on attachment 299165 [details] [diff] [review]
fixed test breakage

r=me, thanks for the corrections, and tests.
Attachment #299165 - Flags: review?(dietrich) → review+
(Assignee)

Updated

9 years ago
Attachment #299165 - Flags: approval1.9?

Updated

9 years ago
Attachment #299165 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in browser/components/sessionstore/nsISessionStore.idl;
/cvsroot/mozilla/browser/components/sessionstore/nsISessionStore.idl,v  <--  nsISessionStore.idl
new revision: 1.10; previous revision: 1.9
done
Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.95; previous revision: 1.94
done
Checking in browser/components/sessionstore/test/chrome/Makefile.in;
/cvsroot/mozilla/browser/components/sessionstore/test/chrome/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/browser/components/sessionstore/test/chrome/test_bug393716.xul,v
done
Checking in browser/components/sessionstore/test/chrome/test_bug393716.xul;
/cvsroot/mozilla/browser/components/sessionstore/test/chrome/test_bug393716.xul,v  <--  test_bug393716.xul
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4

Updated

9 years ago
Depends on: 416115
It looks likely this caused bug 416115
(Assignee)

Updated

9 years ago
Keywords: dev-doc-needed
Updated the documentation at http://developer.mozilla.org/en/docs/nsISessionStore
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

9 years ago
Blocks: 426045
You need to log in before you can comment on or make changes to this bug.