Move Threading to Chrome Worker

RESOLVED FIXED in 1.0b4

Status

Calendar
Internal Components
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

Bug Flags:
blocking-calendar1.0 +

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 502294 [details] [diff] [review]
Fix - v1
Attachment #502294 - Flags: review?(mschroeder)
(Assignee)

Updated

7 years ago
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?
(Assignee)

Comment 3

7 years ago
(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!
(Assignee)

Comment 4

7 years ago
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/9c767af63b3a>
-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
(Assignee)

Comment 5

7 years ago
(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.