455232 - Query for supported-calendar-component-set in case servers don't support tasks/events

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] Fix - v1

As far as I read, a calendar-multiget query MUST return a 207 status, but there are some servers that just don't. Examples for this are google and egroupware 1.6. I'm not quite sure this is the way to go, but this patch is a bit more graceful and instead of failing the whole report, it disables tasks if the query for tasks is countered with a 4xx message. It also removes a previously needed specialcasing for google.

Comment 1

[Bruno Browning]

Comment on attachment 338532 [details] [diff] [review]
[checked in] Fix - v1

Looks good. I'd probably change the comment to indicate that the 4xx response is an error on the part of the server that we're working around.
r=browning

What I'd suggest is that we apply this patch but leave the bug open as a place to do the supported-calendar-compenent-set work mentioned on the newsgroup shortly after 0.9. We'll also want the PROPFIND change I mentioned in the newsgroup, which has been filed as bug 455262

Comment 2

[Christoph Kaulich]

It is working for me, as I have started the task in then newsgroup.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

Checked in on HEAD and MOZILLA_1_8_BRANCH and SUNBIRD_0_9_BRANCH

Keeping open for supported-calendar-component-set work.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] Additional fix - v1

This additional fix is needed, or else scheduling servers will bork. r=dbo, issue remains open.

Comment 5

[Bruno Browning]

I have draft code that does the supported-calendar-component-set bit, so I'll take (the remainder of) this one.

Comment 6

[Bruno Browning]

Attachment: query supported-calendar-component-set

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 409556 [details] [diff] [review]
query supported-calendar-component-set

>+            } catch(ex) {
Missing space

>+            if (haveSCCSReport && supportedComponentsXml.*.length()) {
I believe e4x has something like hasChildren()


>+                for (let sc = 0 ; sc < supportedComponentsXml.*.length(); sc++) {
>+                    let comp = supportedComponentsXml.*[sc][e-mail]();
I know e4x can iterate this better, i.e
for each (let sc in supportedComponentsXml) {
  let comp = [e-mail]();


>+                    if (thisCalendar.mGenerallySupportedItemTypes.indexOf(comp) >= 0) {
>+                    thisCalendar.mSupportedItemTypes.push(comp);
>+                    }
Aside from the indent problem here, if we do this then types will only be added to mSupportedItemTypes. Does this work if i.e VTODO is not supported? Wouldn't it just stay in the array? Or did I miss the clearing of the array?

I'd suggest something like

thisCalendar.mSupportedItemTypes = [ [e-mail]() for each (sc in supportedComponentsXml) ];


r- for now, I'd appreciate a new patch with the nits fixed and comments considered.

Comment 8

[Bruno Browning]

Attachment: [already reviewed code] address reviewer comments

> I believe e4x has something like hasChildren()

Indeed it does, .hasChildren rather than .hasChildren() but it's there and afaik not mentioned on devmo. Can you point me at some better e4x docs?

>I know e4x can iterate this better, i.e

Changed this. Is there some technical reason to prefer this style of iteration, or is it personal preference? From a functional standpoint the two methods seem identical to me, and the way I originally did it is the way this is done elsewhere in this file. Just curious.

> Or did I miss the clearing of the array?

The array is cleared (.length = 0) just above at line 1391, iff we have a response from the server on the supported-calendar-component-set query

>I'd suggest something like
>thisCalendar.mSupportedItemTypes = [ [e-mail]() for each (sc in
>supportedComponentsXml) ];

Some servers will include e.g. VJOURNAL and VTIMEZONE in their supported-calendar-component-set responses, so I think we want to make sure that "comp" is in mGenerallySupportedItemTypes before adding it to the (emptied) .mSupportedItemTypes

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 410973 [details] [diff] [review]
[already reviewed code] address reviewer comments

I'll be postponing this to after the beta, to make sure we don't risk regressions. The patch looks fine though, I'll r+ it as soon as we are through.

(In reply to comment #8)
> afaik not mentioned on devmo. Can you point me at some better e4x docs?
Unfortunately not. I read some docs on about.com I believe, but rather in howto format. e4x is really underdocumented.

> Changed this. Is there some technical reason to prefer this style of iteration,
> or is it personal preference? 
I don't have proof for the technical reason, but I can imagine that a construct like foo.*.length() will first retrieve all subnodes of foo, then check their length. This needs to happen on each for() iteration to check the counter. Using the for each () construct might only create an iterator which might be a bit more lightweight.

Aside from that I just think it looks a bit cleaner to use for each ().


> The array is cleared (.length = 0) just above at line 1391, iff we have a
> response from the server on the supported-calendar-component-set query
Ah I see. Personally I'd prefer assigning an empty array instead, but I can live with assigning the length.

> Some servers will include e.g. VJOURNAL and VTIMEZONE in their
> supported-calendar-component-set responses, so I think we want to make sure
> that "comp" is in mGenerallySupportedItemTypes before adding it to the
> (emptied) .mSupportedItemTypes
Ok

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 410973 [details] [diff] [review]
[already reviewed code] address reviewer comments

r=philipp. Will take care of checkin myself in about 20 minutes.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/b8ca7daa20b3>

-> FIXED