Closed Bug 1571336 Opened 2 years ago Closed 2 years ago

Call startBatch and endBatch on calDAV calendar even if it isn't cached


(Calendar :: Provider: CalDAV, enhancement)

Not set


(Not tracked)

Thunderbird 70.0


(Reporter: darktrojan, Assigned: darktrojan)



(1 file, 1 obsolete file)

For some reason I don't understand, we don't call startBatch or endBatch on a calDAV calendar if it's uncached. We should.

Attached patch 1571336-caldav-batch-1.diff (obsolete) — Splinter Review
Attachment #9082921 - Flags: review?(paul)
Comment on attachment 9082921 [details] [diff] [review]

Review of attachment 9082921 [details] [diff] [review]:

I don't know the reason why we weren't calling startBatch/endBatch for cached calDAV calendars, so these changes seem good to me.  r+ with my one comment addressed.  I'm also curious why you are preferring 'calendar.offlineStorage' rather than 'calendar.superCalendar' for getting at these startBatch/endBatch functions, since that wasn't clear to me.

::: calendar/providers/caldav/calDavRequestHandlers.js
@@ +93,5 @@
>              this._reader = null;
>          }
>          // Now that we are done, check which items need fetching.
> +        this.calendar.mOfflineStorage.startBatch();

Since we are using these "m" prefixes to signal a "private" property, only to be accessed internally, I think we should use the "public" `this.calendar.offlineStorage` instead.
Attachment #9082921 - Flags: review?(paul) → review+

In this case the "m" really seems to mean "not exposed through XPCOM". It's far from the only usage in this file, so I think it's okay to use.

Keywords: checkin-needed
Keywords: checkin-needed

I've changed my mind about whether to use this.calendar.superCalendar or this.calendar.mOfflineStorage so many times. this.calendar itself is also an option. I'm pretty sure they all work fine. But on the whole I guess superCalendar makes more sense.

Attachment #9082921 - Attachment is obsolete: true
Attachment #9083582 - Flags: review+
Keywords: checkin-needed

Pushed by
Call startBatch and endBatch on calDAV calendar even if it isn't cached. r=pmorris

Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 70
You need to log in before you can comment on or make changes to this bug.