Closed
Bug 547406
Opened 14 years ago
Closed 14 years ago
Port Bug 485976 [Move writing sessionstore.js off the main thread] to SeaMonkey
Categories
(SeaMonkey :: Session Restore, defect)
SeaMonkey
Session Restore
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.1a1
People
(Reporter: misak.bugzilla, Assigned: misak.bugzilla)
References
Details
Attachments
(1 file, 2 obsolete files)
4.83 KB,
patch
|
misak.bugzilla
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
From parent bug: Right now as far as I can tell session store writes data to disk synchronously on the UI thread. It might make sense to move this to a worker thread.
Attachment #427927 -
Flags: superreview?(neil)
Attachment #427927 -
Flags: review?(neil)
Comment 1•14 years ago
|
||
Comment on attachment 427927 [details] [diff] [review] patch >+__defineGetter__("NetUtil", function() { >+ delete this.NetUtil; >+ Components.utils.import("resource://gre/modules/NetUtil.jsm"); >+ return NetUtil; >+}); Don't bother with this; just import NetUtil after XPCOMUtils. (Unless for some strange reason you want to reimplement asyncCopy!) >- >+ // Asynchronously copy the data to the file. Nit: don't delete that blank line >+ var self = this; >+ NetUtil.asyncCopy(istream, ostream, function(rc) { >+ if (Components.isSuccessCode(rc)) { >+ self._observerService.notifyObservers(null, >+ "sessionstore-state-write-complete", >+ ""); Nit: you only need a temporary for the observer service, not for this. [I blame the callback mechanism for forcing this awkward workaround.]
Attachment #427927 -
Flags: review?(neil) → review+
Assignee | ||
Comment 2•14 years ago
|
||
I got error in console when used this._observerService.notifyObservers(null, Error: this._observerService is undefined Source File: file:///home/misak/workspace/src/suite-opt/mozilla/dist/bin/components/nsSessionStore.js Line: 2686
Comment 3•14 years ago
|
||
That's why you need a temporary for the observer service itself. (You only removed the "self" temporary for "this", which obviously doesn't work.)
Assignee | ||
Comment 4•14 years ago
|
||
Nits fixed, carrying forward r+ from Neil.
Attachment #427927 -
Attachment is obsolete: true
Attachment #428143 -
Flags: superreview?(neil)
Attachment #428143 -
Flags: review+
Attachment #427927 -
Flags: superreview?(neil)
Comment 5•14 years ago
|
||
Comment on attachment 428143 [details] [diff] [review] v2 Almost there, but >+ var self = this._observerService; you need a new name for this variable >+ self.notifyObservers(null, >+ "sessionstore-state-write-complete", >+ ""); and you need to realign these.
Assignee | ||
Comment 6•14 years ago
|
||
renamed variable, carrying forward r+ from Neil.
Attachment #428143 -
Attachment is obsolete: true
Attachment #428177 -
Flags: superreview?(neil)
Attachment #428177 -
Flags: review+
Attachment #428143 -
Flags: superreview?(neil)
Updated•14 years ago
|
Attachment #428177 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 7•14 years ago
|
||
Comment on attachment 428177 [details] [diff] [review] final patch [Checkin: Comment 7] http://hg.mozilla.org/comm-central/rev/37123caa84ab
Attachment #428177 -
Attachment description: final patch → final patch
[Checkin: Comment 7]
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a1
You need to log in
before you can comment on or make changes to this bug.
Description
•