Closed Bug 547406 Opened 10 years ago Closed 10 years ago

Port Bug 485976 [Move writing sessionstore.js off the main thread] to SeaMonkey

Categories

(SeaMonkey :: Session Restore, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a1

People

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

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — 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 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+
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
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.)
Attached patch v2 (obsolete) — Splinter Review
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)
Blocks: 525720
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.
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)
Attachment #428177 - Flags: superreview?(neil) → superreview+
Keywords: checkin-needed
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]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a1
Blocks: 573384
You need to log in before you can comment on or make changes to this bug.