Closed Bug 1571336 Opened 3 months ago Closed 3 months ago

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

Categories

(Calendar :: Provider: CalDAV, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: darktrojan, Assigned: darktrojan)

Details

Attachments

(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]
1571336-caldav-batch-1.diff

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 mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1933f7704fb2
Call startBatch and endBatch on calDAV calendar even if it isn't cached. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 3 months 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.