Last Comment Bug 614220 - Port Deferred Session restore - Bug 588482
: Port Deferred Session restore - Bug 588482
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Misak Khachatryan
:
Mentors:
Depends on: 588482
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-23 05:24 PST by Misak Khachatryan
Modified: 2010-12-12 08:26 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v 1 (42.90 KB, patch)
2010-11-23 05:24 PST, Misak Khachatryan
no flags Details | Diff | Review
v 1.1, nits fixed (42.99 KB, patch)
2010-12-08 02:41 PST, Misak Khachatryan
neil: review-
Details | Diff | Review
v 1.2 (42.92 KB, patch)
2010-12-09 05:57 PST, Misak Khachatryan
neil: review+
Details | Diff | Review
localisation fix (2.05 KB, patch)
2010-12-12 08:22 PST, Misak Khachatryan
misak.bugzilla: review+
Details | Diff | Review

Description Misak Khachatryan 2010-11-23 05:24:52 PST
Created attachment 492629 [details] [diff] [review]
v 1

Implement Deferred session restore functionality. Here in the patch i leaved app tab support logic, IMHO it's worth keep it. App tab support will be added one day sooner or later.
Comment 1 neil@parkwaycc.co.uk 2010-12-06 05:19:27 PST
Comment on attachment 492629 [details] [diff] [review]
v 1

>+        <menuitem id="historyRestoreLastSession"
>+                  label="&historyRestoreLastSession.label;"
>+                  oncommand="restoreLastSession();"
>+                  observes="canRestoreLastSession"/>
I don't think this is getting disabled properly. (I wish I could think of a better place to put this, but there isn't one as far as I can see.)

>+  set canRestoreLastSession(val) {
>+    // Cheat a bit; only allow false.
>+    if (val)
>+      return;
>+    this._lastSessionState = null;
Nit: the early return looks odd. Perhaps
if (!val)
  this._lastSessionState = null;

>+      if (ss.doRestore() ||
>+          ss.sessionType == Components.interfaces.nsISessionStartup.DEFER_SESSION)
>         iniString = ss.state;
Would it be simpler to write this:
if (ss._sessionType != Components.interfaces.nsISessionStartup.NO_SESSION)

>+  _extractHostsForCookies:
>+    function sss__extractHostsForCookies(aEntry, aHosts, aCheckPrivacy, aIsPinned) {
Nit: most of these are OK but this is one of three with two __s
Comment 2 Misak Khachatryan 2010-12-08 02:41:29 PST
Created attachment 496116 [details] [diff] [review]
v 1.1, nits fixed

I've found working solution, don't know if it's good or not.
Comment 3 neil@parkwaycc.co.uk 2010-12-08 04:10:04 PST
Comment on attachment 496116 [details] [diff] [review]
v 1.1, nits fixed

> function InitSessionStoreCallback()
> {
>   try {
>     var ss = Components.classes["@mozilla.org/suite/sessionstore;1"]
>                        .getService(Components.interfaces.nsISessionStore);
>     ss.init(window);
>   } catch(ex) {
>     dump("nsSessionStore could not be initialized: " + ex + "\n");
>-  }
>+  };
Don't need this semicolon.

>+  toggleRestoreLastSession();
I assume here that we're not going to worry about an extension clearing the last session?

>+function restoreLastSession() {
Nit: this probably belongs after undoCloseWindow.

>+  let ss = Components.classes["@mozilla.org/suite/sessionstore;1"]
>+                     .getService(Components.interfaces.nsISessionStore);
>+  ss.restoreLastSession();
>+}

>+  let restoreItem = document.getElementsByClassName("restoreLastSession")[0];
The menuitem has an id, so you might as well use that (and drop the class).

>+  let ss = Components.classes["@mozilla.org/suite/sessionstore;1"]
>+                     .getService(Components.interfaces.nsISessionStore);
InitSessionStoreCallback already gets the session store service.
It might be worthwhile combining the two methods to avoid duplication.

>+        <menuitem id="historyRestoreLastSession"
>+                  class="restoreLastSession"
>+                  label="&historyRestoreLastSession.label;"
I've just noticed there's no access key :-(
Comment 4 Misak Khachatryan 2010-12-09 05:57:18 PST
Created attachment 496479 [details] [diff] [review]
v 1.2

>>+  toggleRestoreLastSession();
>I assume here that we're not going to worry about an extension clearing the
>last session?

Please explain what can be done.

Except this one, all nits fixed.
Comment 5 neil@parkwaycc.co.uk 2010-12-09 06:34:53 PST
(In reply to comment #4)
> (From update of attachment 496479 [details] [diff] [review])
>>>+  toggleRestoreLastSession();
>>I assume here that we're not going to worry about an extension clearing the
>>last session?
>Please explain what can be done.
Well, we could decide to ignore it, or you could rewrite the patch again... which would you prefer ;-)
Comment 6 neil@parkwaycc.co.uk 2010-12-09 06:35:58 PST
Comment on attachment 496479 [details] [diff] [review]
v 1.2

>+    else
>+      restoreItem.setAttribute("disabled", true);
Nit: don't have to do this, because it's already disabled.
Comment 7 Misak Khachatryan 2010-12-09 08:37:51 PST
(In reply to comment #5)
> >Please explain what can be done.
> Well, we could decide to ignore it, or you could rewrite the patch again...
> which would you prefer ;-)

I'll wait when FF guys came up with solution :P
Comment 8 Misak Khachatryan 2010-12-09 08:49:18 PST
Pushed with comment 6 nit fixed: http://hg.mozilla.org/comm-central/rev/73479070b3a1
Comment 9 Robert Kaiser (not working on stability any more) 2010-12-12 07:09:00 PST
Comment on attachment 496479 [details] [diff] [review]
v 1.2

>+<!ENTITY historyRestoreLastSession.label "Restore Previous Session">
>+<!ENTITY historyRestoreLastSessionCmd.accesskey "R">

Ouch, that's a tiny, but nasty L10n bug. All our tools and localizers expect an accesskey to have the same name as the label, with .accesskey instead of .label etc. - you should name this historyRestoreLastSession.accesskey - with the "Cmd" in it. :(

It would be nice if you checked in that as a change, rs=me for correcting it.
Comment 10 Misak Khachatryan 2010-12-12 08:22:37 PST
Created attachment 497147 [details] [diff] [review]
localisation fix

Localization name fix. r+ by KaiRo
Comment 11 Misak Khachatryan 2010-12-12 08:26:34 PST
Pushed - http://hg.mozilla.org/comm-central/rev/b952a88135d7

Note You need to log in before you can comment on or make changes to this bug.