Closed Bug 614220 Opened 14 years ago Closed 14 years ago

Port Deferred Session restore - Bug 588482

Categories

(SeaMonkey :: Session Restore, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: misak.bugzilla, Assigned: misak.bugzilla)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch v 1 (obsolete) — Splinter Review
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.
Attachment #492629 - Flags: review?(neil)
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
Attached patch v 1.1, nits fixed (obsolete) — Splinter Review
I've found working solution, don't know if it's good or not.
Attachment #492629 - Attachment is obsolete: true
Attachment #496116 - Flags: review?(neil)
Attachment #492629 - Flags: review?(neil)
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 :-(
Attachment #496116 - Flags: review?(neil) → review-
Attached patch v 1.2Splinter 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.

Except this one, all nits fixed.
Attachment #496116 - Attachment is obsolete: true
Attachment #496479 - Flags: review?(neil)
(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 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.
Attachment #496479 - Flags: review?(neil) → review+
(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
Pushed with comment 6 nit fixed: http://hg.mozilla.org/comm-central/rev/73479070b3a1
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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.
Attached patch localisation fixSplinter Review
Localization name fix. r+ by KaiRo
Attachment #497147 - Flags: review+
You need to log in before you can comment on or make changes to this bug.