Last Comment Bug 388926 - Use If-Modified-Since when checking for updates in ICS calendars
: Use If-Modified-Since when checking for updates in ICS calendars
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Provider: ICS/WebDAV (show other bugs)
: unspecified
: All All
-- enhancement (vote)
: 4.4
Assigned To: Lukas Rechberger
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-20 01:03 PDT by Andrew Daviel
Modified: 2015-08-09 15:39 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (1.62 KB, patch)
2007-10-12 17:42 PDT, Richard Klein
no flags Details | Diff | Splinter Review
Patch v2 (1.63 KB, patch)
2007-10-16 14:45 PDT, Richard Klein
no flags Details | Diff | Splinter Review
Patch (1.29 KB, patch)
2015-07-17 07:36 PDT, Lukas Rechberger
philipp: feedback+
Details | Diff | Splinter Review
Patch v2 (1.41 KB, patch)
2015-07-21 05:17 PDT, Lukas Rechberger
philipp: review+
Details | Diff | Splinter Review
Patch v3 (1.84 KB, patch)
2015-08-09 03:44 PDT, Lukas Rechberger
lukas: review+
Details | Diff | Splinter Review

Description User image Andrew Daviel 2007-07-20 01:03:43 PDT
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 User image Richard Klein 2007-10-12 17:42:06 PDT
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).
Comment 2 User image Richard Klein 2007-10-16 14:45:24 PDT
Created attachment 285147 [details] [diff] [review]
Patch v2

The previous patch missed a parameter on the setRequestHeader.
Comment 3 User image Michiel van Leeuwen (email: mvl+moz@) 2007-10-17 11:32:49 PDT
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 5 User image Simon Paquet [:sipaq] 2008-05-14 07:26:50 PDT
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.
Comment 6 User image Richard Klein 2008-05-14 10:58:41 PDT
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 7 User image Michiel van Leeuwen (email: mvl+moz@) 2008-06-17 09:12:23 PDT
Comment on attachment 285147 [details] [diff] [review]
Patch v2

It's very unlikely that I will ever get around to reviewing this. Sorry.
Comment 8 User image Martin Schröder [:mschroeder] 2008-07-16 02:57:51 PDT
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.
Comment 9 User image Daniel Boelzle [:dbo] 2008-08-11 05:39:05 PDT
We already issue If-none-match against the etag and handle 304. Is it worth the effort to check for last-modified, too?
Comment 10 User image Daniel Boelzle [:dbo] 2008-08-13 01:43:05 PDT
Comment on attachment 285147 [details] [diff] [review]
Patch v2

Removing me from review until the above question is resolved.
Comment 11 User image George Sexton 2010-03-13 10:54:43 PST
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.
Comment 12 User image Lukas Rechberger 2015-07-17 07:36:02 PDT
Created attachment 8635297 [details] [diff] [review]
Patch
Comment 13 User image Philipp Kewisch [:Fallen] 2015-07-20 03:01:39 PDT
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.
Comment 14 User image Lukas Rechberger 2015-07-21 05:17:14 PDT
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
Comment 15 User image Philipp Kewisch [:Fallen] 2015-08-08 02:56:02 PDT
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
Comment 16 User image Lukas Rechberger 2015-08-09 03:44:37 PDT
Created attachment 8645440 [details] [diff] [review]
Patch v3

I have moved up the accept header and add a commit message to the patch.
Comment 17 User image aleth [:aleth] 2015-08-09 15:37:52 PDT
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

Note You need to log in before you can comment on or make changes to this bug.