614220 - Port Deferred Session restore - Bug 588482

Status: RESOLVED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Misak Khachatryan]

Attachment: 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

[[e-mail]]

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]

Attachment: v 1.1, nits fixed

I've found working solution, don't know if it's good or not.

Comment 3

[[e-mail]]

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]

Attachment: 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

[[e-mail]]

(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

[[e-mail]]

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]

(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]

Pushed with comment 6 nit fixed: http://hg.mozilla.org/comm-central/rev/73479070b3a1

Comment 9

[Robert Kaiser]

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]

Attachment: localisation fix

Localization name fix. r+ by KaiRo

Comment 11

[Misak Khachatryan]

Pushed - http://hg.mozilla.org/comm-central/rev/b952a88135d7