Last Comment Bug 547406 - Port Bug 485976 [Move writing sessionstore.js off the main thread] to SeaMonkey
: Port Bug 485976 [Move writing sessionstore.js off the main thread] to SeaMonkey
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Misak Khachatryan
:
Mentors:
Depends on: 485976
Blocks: 525720 573384
  Show dependency treegraph
 
Reported: 2010-02-20 03:53 PST by Misak Khachatryan
Modified: 2010-06-20 15:29 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (4.90 KB, patch)
2010-02-20 03:53 PST, Misak Khachatryan
neil: review+
Details | Diff | Review
v2 (4.82 KB, patch)
2010-02-21 22:05 PST, Misak Khachatryan
misak.bugzilla: review+
Details | Diff | Review
final patch [Checkin: Comment 7] (4.83 KB, patch)
2010-02-22 04:44 PST, Misak Khachatryan
misak.bugzilla: review+
neil: superreview+
Details | Diff | Review

Description Misak Khachatryan 2010-02-20 03:53:31 PST
Created attachment 427927 [details] [diff] [review]
patch

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.
Comment 1 neil@parkwaycc.co.uk 2010-02-20 11:12:18 PST
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.]
Comment 2 Misak Khachatryan 2010-02-21 06:08:46 PST
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 neil@parkwaycc.co.uk 2010-02-21 06:57:21 PST
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.)
Comment 4 Misak Khachatryan 2010-02-21 22:05:00 PST
Created attachment 428143 [details] [diff] [review]
v2

Nits fixed, carrying forward r+ from Neil.
Comment 5 neil@parkwaycc.co.uk 2010-02-22 01:16:29 PST
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.
Comment 6 Misak Khachatryan 2010-02-22 04:44:09 PST
Created attachment 428177 [details] [diff] [review]
final patch
[Checkin: Comment 7]

renamed variable, carrying forward r+ from Neil.
Comment 7 Serge Gautherie (:sgautherie) 2010-02-23 00:34:09 PST
Comment on attachment 428177 [details] [diff] [review]
final patch
[Checkin: Comment 7]


http://hg.mozilla.org/comm-central/rev/37123caa84ab

Note You need to log in before you can comment on or make changes to this bug.