Closed
Bug 614220
Opened 14 years ago
Closed 14 years ago
Port Deferred Session restore - Bug 588482
Categories
(SeaMonkey :: Session Restore, defect)
SeaMonkey
Session Restore
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: misak.bugzilla, Assigned: misak.bugzilla)
References
Details
Attachments
(2 files, 2 obsolete files)
42.92 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
2.05 KB,
patch
|
misak.bugzilla
:
review+
|
Details | Diff | 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 1•14 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•14 years ago
|
||
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•14 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•14 years ago
|
||
>>+ 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•14 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•14 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•14 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•14 years ago
|
||
Pushed with comment 6 nit fixed: http://hg.mozilla.org/comm-central/rev/73479070b3a1
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 9•14 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•14 years ago
|
||
Localization name fix. r+ by KaiRo
Attachment #497147 -
Flags: review+
Assignee | ||
Comment 11•14 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.
Description
•