Closed Bug 624192 Opened 10 years ago Closed 10 years ago

Move Threading to Chrome Worker

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

Details

Attachments

(1 file)

Since bug 608142 its no longer possible to dispatch objects across threads the way we have been doing it now. The upcoming patch will fix this by using chrome workers, which (since yesterday) can access threadsafe XPCOM objects.

This is just a first step to get trunk working again, I hope we can move more things to that worker in the future.
Attached patch Fix - v1Splinter Review
Attachment #502294 - Flags: review?(mschroeder)
Flags: blocking-calendar1.0+
Comment on attachment 502294 [details] [diff] [review]
Fix - v1

First question and nits, otherwise looks good so far but haven't tested it functionally:

The removed function from calUtils.jsm is the only user of getWorkerThread(), so it can also be removed.

>diff -r 2b4b40c2f1d1 calendar/base/src/calIcsParser.js
>--- a/calendar/base/src/calIcsParser.js	Sat Jan 08 20:50:04 2011 +0100
>+++ b/calendar/base/src/calIcsParser.js	Sat Jan 08 23:31:24 2011 +0100
[...]
>@@ -221,70 +220,65 @@
[...]
>+            worker.onerror = {
>+                handleEvent: function(error) {
>+                    cal.ERRRO("Error Parsing ICS: " + error.message);
>+                    aAsyncParsing.onParsingComplete(Components.results.NS_ERROR_FAILURE, this_);
>+                }
>+            };

Typo: cal.ERROR(...)

[...]

>     parseFromStream: function ip_parseFromStream(aStream, aTzProvider, aAsyncParsing) {
[...]
>+        let stringData = unicodeConverter.convertFromByteArray(octetArray, octetArray.length);
>+
>+        this.parseString(stringData, aTzProvider, aAsyncParsing);
>     },

The reading of the string from the stream seems to be no longer on an extra thread/in the worker after this patch. Is this your intention?
(In reply to comment #2)
> The reading of the string from the stream seems to be no longer on an extra
> thread/in the worker after this patch. Is this your intention?

Yes and no. The unicode converter is not threadsafe, so we can't access it from a worker. This is something we'll have to fix another time.

Thanks for catching the other errors!
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/9c767af63b3a>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
(In reply to comment #2)
> Comment on attachment 502294 [details] [diff] [review]
> Fix - v1
> 
> First question and nits, otherwise looks good so far but haven't tested it
> functionally:
Oops, I missed the fact that you didn't actually say r+ ;-) Go ahead and do anything you still wanted to do and let me know if it fails.
Comment on attachment 502294 [details] [diff] [review]
Fix - v1

Patch was checked in long ago... so finally setting r+ (without testing because I have some build problems on my Mac like our tinderboxen).
Attachment #502294 - Flags: review?(mschroeder) → review+
You need to log in before you can comment on or make changes to this bug.