Closed Bug 388926 Opened 14 years ago Closed 5 years ago
Use If-Modified-Since when checking for updates in ICS calendars
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:18.104.22.168) Gecko/20070713 Firefox/22.214.171.124 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
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).
The previous patch missed a parameter on the setRequestHeader.
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)
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
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.
Comment on attachment 285147 [details] [diff] [review] Patch v2 It's very unlikely that I will ever get around to reviewing this. Sorry.
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.
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.
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: 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+
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
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+
I have moved up the accept header and add a commit message to the patch.
Attachment #285147 - Attachment is obsolete: true
url: https://hg.mozilla.org/comm-central/rev/84b210b8fa1defa4f2633e02b87e568bfbd65eae changeset: 84b210b8fa1defa4f2633e02b87e568bfbd65eae user: Lukas Rechberger <firstname.lastname@example.org> 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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.4
You need to log in before you can comment on or make changes to this bug.