490309 - Implement asynchronous ical parsing

Status: RESOLVED FIXED

(Calendar :: Internal Components, enhancement)

Description

[Daniel Boelzle [:dbo]]

This should allow releasing the UI thread from some load. I have a work in progress patch for this (though not ready for review yet).

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Since this greatly improves visual performance, I think we should block 1.0 on it.

Comment 2

[Daniel Boelzle [:dbo]]

Attachment: patch - v1

This patch implements asynchronous ical parsing up to the point where calEvent/calTodo objects are created. I haven't found a clean way to create the latter without running into deadlocks.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 378070 [details] [diff] [review]
patch - v1

>-      calICSServiceConstructor,
>-      NULL,
>-      NULL,
>-      NULL,
>-      NS_CI_INTERFACE_GETTER_NAME(calICSService),
>-      NULL,
>-      &NS_CLASSINFO_NAME(calICSService)
Why no class info?


>+                icsParser.parseString(data, null, null);
>+                parser.parseString(data, null, null);
>+                            parser.parseString(str, null, null);
I must have missed this file on review, but I assume you changed calIIcsParser.idl somewhere. Instead of adding another null argument, I'd suggest to use the idl [optional] tag for the latter two arguments. This way you are not forced to provide the argument each time.


>+        let worker = { // nsIRunnable:
>+            run: function worker_run() {
>+                let res = null;
>+                try {
>+                    actionFunc(callingThread);
>+                } catch (exc) {
>+                    res = exc;
>+                }
>+
>+                if (!respFunc) {
>+                    return;
>+                }
>+
>+                let response = { // nsIRunnable:
>+                    run: function response_run() {
>+                        respFunc(res);
>+                    }
This can be shortende a small bit using the new function syntax of js 1.7 or 1.8:
let response = { run: function() respFunc(res) };


>+    void onParsingDone(in nsresult rc, in calIIcsParser parser);
Only a small nit, but lets align this with our other listeners: onParsingComplete.

>   void parseString(in AString aICSString,
>-                   in calITimezoneProvider aTzProvider);
>+                   in calITimezoneProvider aTzProvider,
>+                   in calIIcsParsingListener aAsyncParsing);
Is there any way to interupt this? i.e what happens if the current xul window is closed and the aAsyncParsing listener goes out of context?


>+var gIcsParserClassInfo = {
> };
Maybe you can move the class info into the component itself instead of providing an extra object.


>+        // Do the actual string reading and ical parsing on a athread, but process the parsed ical
s/athread/thread/


r=philipp, looking forward to this patch!

Comment 4

[Daniel Boelzle [:dbo]]

(In reply to comment #3)
> (From update of attachment 378070 [details] [diff] [review])
> >-      calICSServiceConstructor,
> >-      NULL,
> >-      NULL,
> >-      NULL,
> >-      NS_CI_INTERFACE_GETTER_NAME(calICSService),
> >-      NULL,
> >-      &NS_CLASSINFO_NAME(calICSService)
> Why no class info?
covered by NS_IMPL_CI_INTERFACE_GETTERn

> >+                icsParser.parseString(data, null, null);
> >+                parser.parseString(data, null, null);
> >+                            parser.parseString(str, null, null);
> I must have missed this file on review, but I assume you changed
> calIIcsParser.idl somewhere. Instead of adding another null argument, I'd
> suggest to use the idl [optional] tag for the latter two arguments. This way
> you are not forced to provide the argument each time.
good idea, changed latter two args to be optional

> >+    void onParsingDone(in nsresult rc, in calIIcsParser parser);
> Only a small nit, but lets align this with our other listeners:
> onParsingComplete.
done

> >   void parseString(in AString aICSString,
> >-                   in calITimezoneProvider aTzProvider);
> >+                   in calITimezoneProvider aTzProvider,
> >+                   in calIIcsParsingListener aAsyncParsing);
> Is there any way to interupt this? i.e what happens if the current xul window
> is closed and the aAsyncParsing listener goes out of context?
I cannot imagine a current situation where this would happen, because the async API is by now only used from within the ics provider. However, such calls would bounce which ~might~ be OK since the program will not break. We might suppress errors logs or even opt for additional API.

Should we cover further optimizations (caldav, wcap) in follow-up bugs?

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/aaa44b859559>

-> FIXED

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

> Should we cover further optimizations (caldav, wcap) in follow-up bugs?

Yes, please file some as needed.

Comment 6

[Stefan Sitter [:ssitter]]

I think this checkin regressed Bug 494783.