1042125 - Make the CalDAV provider async safe

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Reid Anderson]

Currently CalDAV assumes some storage calendar functions are synchronous, which will no longer be the case.

Comment 1

[Reid Anderson]

Attachment: AsyncCalDAV

Comment 2

[Reid Anderson]

Attachment: AsyncCalDAV

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8460307 [details] [diff] [review]
AsyncCalDAV

Review of attachment 8460307 [details] [diff] [review]:
-----------------------------------------------------------------

Code looks good, r=philipp. I will give this a test run after my rebuild finishes. I'd appreciate if you could upload a new patch with the below comments fixed.

Please pull the latest changes from comm-central, there are some minor conflicts in the first chunk of calDavCalendar.js after bug 1021684.

::: calendar/base/modules/calAsyncUtils.jsm
@@ +3,5 @@
> +     * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +     
> +    Components.utils.import("resource://calendar/modules/calUtils.jsm");
> +    Components.utils.import("resource://gre/modules/Promise.jsm");
> +     

Looks like I left a few redundant whitespaces in this file, could you remove them?

::: calendar/providers/caldav/calDavCalendar.js
@@ +1036,2 @@
>  
> +        Task.spawn(function() {

function*() {

@@ +1103,5 @@
>       * @param path Path of the item to delete, must not be encoded
>       */
>      deleteTargetCalendarItem: function caldav_deleteTargetCalendarItem(path) {
> +        let self = this;
> +        return Task.spawn(function*() {

I just found a pretty cool feature of Task.jsm thats not documented on MDN. Its very useful for functions like this where all that is run is a Task. It also saves us the let self = this crutch.


deleteTargetCalendaItem: Task.async(function*() {
    let pcal = promisifyCalendar(this.mOfflineStorage);
    ....

}),

See here for more details:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/modules/Task.jsm#168


On a side note, you should also use this in the async storage patch.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8460307 [details] [diff] [review]
AsyncCalDAV

As discussed via IRC, there are still some issues with the patch. First of all the missing cal.async prefix, then the item is not removed from the calendar. Setting r- to reflect this, looking forward to the new patch.

Comment 5

[Reid Anderson]

Attachment: AsyncCalDAV

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8460765 [details] [diff] [review]
AsyncCalDAV

Review of attachment 8460765 [details] [diff] [review]:
-----------------------------------------------------------------

Great, works as expected and code looks good. r=philipp

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Setting checkin-needed as the tree currently has APPROVAL REQUIRED. https://tbpl.mozilla.org/?tree=Thunderbird-Trunk

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: AsyncCalDAV

Ah I found a minor issue with calAsyncUtils.jsm. First of all it was indented by one level, then we forgot to change this.itemStatus when the onGetResult fails. This patch fixes both issues.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/74269aa54a84