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

RESOLVED FIXED in seamonkey2.1a1

Status

SeaMonkey
Session Restore
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Misak Khachatryan, Assigned: Misak Khachatryan)

Tracking

Trunk
seamonkey2.1a1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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

Description

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

Comment 1

7 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

7 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

7 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

7 years ago
Created attachment 428143 [details] [diff] [review]
v2

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)
(Assignee)

Updated

7 years ago
Blocks: 525720

Comment 5

7 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

7 years ago
Created attachment 428177 [details] [diff] [review]
final patch
[Checkin: Comment 7]

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

7 years ago
Attachment #428177 - Flags: superreview?(neil) → superreview+
(Assignee)

Updated

7 years ago
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
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a1

Updated

7 years ago
Blocks: 573384
You need to log in before you can comment on or make changes to this bug.