The default bug view has changed. See this FAQ.

Port Deferred Session restore - Bug 588482

RESOLVED FIXED

Status

SeaMonkey
Session Restore
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Misak Khachatryan, Assigned: Misak Khachatryan)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

42.92 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
2.05 KB, patch
Misak Khachatryan
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
Attachment #492629 - Flags: review?(neil)

Comment 1

6 years ago
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
(Assignee)

Comment 2

6 years ago
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.
Attachment #492629 - Attachment is obsolete: true
Attachment #496116 - Flags: review?(neil)
Attachment #492629 - Flags: review?(neil)

Comment 3

6 years ago
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-
(Assignee)

Comment 4

6 years ago
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.
Attachment #496116 - Attachment is obsolete: true
Attachment #496479 - Flags: review?(neil)

Comment 5

6 years ago
(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

6 years ago
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+
(Assignee)

Comment 7

6 years ago
(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
(Assignee)

Comment 8

6 years ago
Pushed with comment 6 nit fixed: http://hg.mozilla.org/comm-central/rev/73479070b3a1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 9

6 years ago
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.
(Assignee)

Comment 10

6 years ago
Created attachment 497147 [details] [diff] [review]
localisation fix

Localization name fix. r+ by KaiRo
Attachment #497147 - Flags: review+
(Assignee)

Comment 11

6 years ago
Pushed - http://hg.mozilla.org/comm-central/rev/b952a88135d7
You need to log in before you can comment on or make changes to this bug.