396515 - ICS provider should use Accept: text/calendar on GET

Status: RESOLVED FIXED

(Calendar :: Provider: ICS/WebDAV, defect)

Description

[Bruno Browning]

In an email of 14 September bernard at oracle requested:

For calendars configured "On the Network" in Format
   "iCalendar (ICS)" Lightning issue an HTTP GET request
   with the following request header:

      Accept: text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5

   Could Lightning specify the following value instead?

      Accept: text/calendar

Comment 1

[Bruno Browning]

Taking this. The Accept: header probably should actually be "text/calendar; charset=utf-8", and we should properly set the Accept-Charset: header as well.

Comment 2

[Bruno Browning]

Attachment: set headers

tested w/ http:// and file:///; I want to test against ftp:// before requesting review.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Bruno, could you please provide an update on the status of this patch/bug?

Comment 4

[Martin Schröder [:mschroeder]]

Bruno, do you want to ask for review for the patch? Do we need the "; charset=utf-8" part if we specify a separate Accept-Charset header?

Comment 5

[Bernard Desruisseaux]

Actually, you can't specify ";charset=utf-8" in the Accept request header.
The semi-colon is used to separate the "media-range" from the "accept-params".
See: http://tools.ietf.org/html/rfc2616#section-14.1

Thanks!
Bernard

Comment 6

[Bruno Browning]

Attachment: patch rev 2

Comment 7

[Daniel Boelzle [:dbo]]

Comment on attachment 340799 [details] [diff] [review]
patch rev 2

>-        channel.loadFlags |= Components.interfaces.nsIRequest.LOAD_BYPASS_CACHE;
>-        channel.notificationCallbacks = this;
>+        this.prepareChannel(channel);
> 
>         var downloader = Components.classes["@mozilla.org/network/downloader;1"]
>                                    .createInstance(CI.nsIDownloader);
...

This will add the etag check to the backup download, but I don't see 304 handled in that code. As far as I see this may lead to empty backups.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Bernard, is this still an issue? Requesting text/calendar might be more correct, but for servers that serve such files as text/plain we break interop.

Comment 9

[Bernard Desruisseaux]

Philipp,

How about using the following?

Accept: text/calendar,text/plain;q=0.8,*/*;q=0.5

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Sounds good to me. We'd have to find out what happens if there is no accepted content type and display the right kind of error. Now we just need someone to put up a patch ;-)

Comment 11

[Malintha Fernando [:marlio]]

Attachment: AcceptHeaders_V1.patch

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8565562 [details] [diff] [review]
AcceptHeaders_V1.patch

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

Looks good, r=philipp

Comment 13

[Malintha Fernando [:marlio]]

Attachment: AcceptHeaders.patch

Adding patch headers

Comment 14

[aleth [:aleth]]

Comment on attachment 8565578 [details] [diff] [review]
AcceptHeaders.patch

Please mark r- patches as obsolete and patches to be checked in as r+ in the future. That makes handling checkin-needed easier. Thanks!

Comment 15

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/a6ce5db74705