The default bug view has changed. See this FAQ.

Use If-Modified-Since when checking for updates in ICS calendars

RESOLVED FIXED in 4.4

Status

Calendar
Provider: ICS/WebDAV
--
enhancement
RESOLVED FIXED
10 years ago
2 years ago

People

(Reporter: Andrew Daviel, Assigned: Lukas Rechberger)

Tracking

unspecified

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
Build Identifier: 

Many ICS calendars are available on the Web, mostly read-only.
HTTP Last-Modified is typically set when these are downloaded.
It would save some time and bandwidth if Sunbird used HTTP GET
If-Modified-Since when refreshing or checking these, unless cache control
or expires headers on the server prohibit it.



Reproducible: Always

Steps to Reproduce:
1. Sunbird issues GET to refresh an existing calendar.

Actual Results:  
2. Server responds with entire ICS calendar, even if it has not changed.

Expected Results:  
1. Sunbird issues GET with If-Modified-Since header cached from last download 
2. Server responds with status 304 Not Modified if the calendar is unchanged

Comment 1

10 years ago
Created attachment 284704 [details] [diff] [review]
Patch v1

This patch stores the Last-Modified http header in the onAfterGet http hook and then uses it to set the If-Modified-Since header in the onBeforeGet http hook.  This really seemed to speed things up for calendars that are fairly static (like holiday calendars).
Attachment #284704 - Flags: review?(mvl)

Comment 2

10 years ago
Created attachment 285147 [details] [diff] [review]
Patch v2

The previous patch missed a parameter on the setRequestHeader.
Attachment #284704 - Attachment is obsolete: true
Attachment #285147 - Flags: review?(mvl)
Attachment #284704 - Flags: review?(mvl)
Comment on attachment 285147 [details] [diff] [review]
Patch v2

Doesn't necko already do the caching, using the last-modified header? If so, can't you look at the returned status codes? (if the file came from cache, don't bother parsing it)

Comment 4

10 years ago
Looking at the http channel source code it looks like it already does this, but several components set it.

http://lxr.mozilla.org/mozilla/source/browser/components/microsummaries/src/nsMicrosummaryService.js#2017
http://lxr.mozilla.org/mozilla/source/browser/components/search/nsSearchService.js#1041
http://lxr.mozilla.org/mozilla/source/mail/extensions/newsblog/content/Feed.js#168
Assignee: nobody → richwklein
OS: Linux → All
Hardware: PC → All
Richard, what's the status with your patch?

mvl, what's the status with your review here? It's been in your queue for over half a year now.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

9 years ago
I haven't been keeping the patch up to date, so it is probably bitrotted.  I'll see if I can get sometime this week to make sure it is up to date.
Status: NEW → ASSIGNED
Comment on attachment 285147 [details] [diff] [review]
Patch v2

It's very unlikely that I will ever get around to reviewing this. Sorry.
Attachment #285147 - Flags: review?(mvl)
Comment on attachment 285147 [details] [diff] [review]
Patch v2

mvl has reset the review flag; next according to http://wiki.mozilla.org/Calendar:Module_Ownership to ask for review is Daniel.
Attachment #285147 - Flags: review?(daniel.boelzle)
We already issue If-none-match against the etag and handle 304. Is it worth the effort to check for last-modified, too?
Comment on attachment 285147 [details] [diff] [review]
Patch v2

Removing me from review until the above question is resolved.
Attachment #285147 - Flags: review?(daniel.boelzle)
Whiteboard: [needs decision]

Comment 11

7 years ago
It's absolutely worth the effort to check If-Modified-Since. Not all hosts implement ETag. Virtually every web hosts implements sending last modified-date, and handling If-Modified-Since requests. 

I would really like to see this implemented. My company offers a hosted calendar and recently automated hits from subscriptions have started to use substantial bandwidth. 

As a benefit, this reduces latency/CPU utilization on the client.
Assignee: richwklein → nobody
Status: ASSIGNED → NEW
Whiteboard: [needs decision]
(Assignee)

Comment 12

2 years ago
Created attachment 8635297 [details] [diff] [review]
Patch
Attachment #8635297 - Flags: review?(philipp)
Assignee: nobody → lukas
Status: NEW → ASSIGNED
Comment on attachment 8635297 [details] [diff] [review]
Patch

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

Hi Lukas, thank you very much for updating the patch here!

Too bad we don't have a unit test specifically for the ics provider to make sure this works. Would you be interested in creating one? You could do it similar to the test_gdata_provider which also uses a mock http server to check if the requests are made correctly.

I am wondering what happens on servers that support both ETag and last modified date. Would the servers prefer the ETag or the last modified date? Maybe it would make sense to check if an etag can is set on retrieved items, and if so do not send an if-modified-since header. Can you take a look at this to see if its easily possible?

I'd appreciate if you could at least investigate the second point and then either attach a new patch with results, or comment and re-request review on this one.
Attachment #8635297 - Flags: review?(philipp) → feedback+
(Assignee)

Comment 14

2 years ago
Created attachment 8636541 [details] [diff] [review]
Patch v2

I think it depends on the implementation of the server or the script which generates the calender.

Now it will only send the 'If-None-Match' if an ETag was provided and only if not it sends 'If-Modified-Since' if 'Last-Modified' was provided.

I also think that the 'Accept'-Header should be always send, not only if a ETag is available, but that is another Bug
Attachment #8635297 - Attachment is obsolete: true
Attachment #8636541 - Flags: review?(philipp)
Comment on attachment 8636541 [details] [diff] [review]
Patch v2

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

r=philipp. I'm fine with moving the accept header up so it is always specified in this bug. If you want to do that here go ahead and upload a new patch with review+.

For checkin, can you upload a patch with your user and a sensible commit message set? I'd usually use something like "Bug 388926 - Use If-Modified-Since when checking for updates in ICS calendars. r=philipp". 

If you are using mq, this would be hg qref -D -U -m "Bug ...". For future patches you can also put this into your .hgrc:
[defaults]
qnew = -D -U
qrefresh = -D
Attachment #8636541 - Flags: review?(philipp) → review+
(Assignee)

Comment 16

2 years ago
Created attachment 8645440 [details] [diff] [review]
Patch v3

I have moved up the accept header and add a commit message to the patch.
Attachment #8636541 - Attachment is obsolete: true
Attachment #8645440 - Flags: review+
Attachment #285147 - Attachment is obsolete: true
Keywords: checkin-needed

Comment 17

2 years ago
url:        https://hg.mozilla.org/comm-central/rev/84b210b8fa1defa4f2633e02b87e568bfbd65eae
changeset:  84b210b8fa1defa4f2633e02b87e568bfbd65eae
user:       Lukas Rechberger <lukas@rechberger.priv.at>
date:       Tue Jul 21 14:04:35 2015 +0200
description:
Bug 388926 - Use If-Modified-Since when checking for updates in ICS calendars. r=philipp

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.4
You need to log in before you can comment on or make changes to this bug.