340949 - (wcap) WCAP provider implementation

Status: VERIFIED FIXED

(Calendar :: Provider: WCAP, enhancement)

Description

[Daniel Boelzle [:dbo]]

Issue to introduce a provider for the Sun Java System Calendar Server (http://www.sun.com/software/products/calendar_srvr/index.xml). The server can be accessed using the Web Calendar Access Protocol (WCAP).

Comment 1

[Daniel Boelzle [:dbo]]

I have tossed together a wiki page:
 http://wiki.mozilla.org/Calendar:WCAP_Provider

Comment 2

[Daniel Boelzle [:dbo]]

Attachment: IDL additions to calIErrors

Comment 3

[Daniel Boelzle [:dbo]]

Attachment: switch on WCAP provider

Master switch to build with WCAP provider when everything is reviewed.

Comment 4

[Daniel Boelzle [:dbo]]

Attachment: preview of files to be added to the calendar tree

Preview of files I would like to check in and further work on until everything works and all issues are fixed. When everything has been reviewed, the final "switch on WCAP provider" patch can land. Ok?

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 225701 [details] [diff] [review]
IDL additions to calIErrors

>+  /**
>+   * Cannot perform operation, because user is offline.
>+   */
>+  const unsigned long WCAP_OFFLINE_MODE = WCAP_ERROR_BASE + 0;

I think this probably wants to be just OFFLINE, and defined to be the same hex number as NS_ERROR_OFFLINE from nsNetErrors.h, with an explanatory comment pointing to the authoritative source.

>+  /**
>+   * Base for all WCAP error codes, e.g.
>+   * CREATECALENDAR_ALREADY_EXISTS_FAILED (25) is mapped to
>+   * WCAP_WRAPPED_ERROR_BASE + 25
>+   *
>+   * Range of codes is limited to
>+   * [WCAP_WRAPPED_ERROR_START, WCAP_WRAPPED_ERROR_END).
>+   */
>+  const unsigned long WCAP_WRAPPED_ERROR_START = WCAP_ERROR_BASE + 0x10;
>+  const unsigned long WCAP_WRAPPED_ERROR_END = WCAP_ERROR_BASE + 0x100;

I would suggest dropping the WRAPPED_ from this file, and just assume that all WCAP-specific errors are going to be defined in the JS, and differentiate between server and client errors in that file, if necessary.

Comment 6

[Daniel Boelzle [:dbo]]

(In reply to comment #5)
> (From update of attachment 225701 [details] [diff] [review] [edit])
> >+  const unsigned long WCAP_OFFLINE_MODE = WCAP_ERROR_BASE + 0;
> 
> I think this probably wants to be just OFFLINE, and defined to be the same hex
> number as NS_ERROR_OFFLINE from nsNetErrors.h, with an explanatory comment
> pointing to the authoritative source.

Ok. Who do I have to convince to define that in IDL?

> >+  const unsigned long WCAP_WRAPPED_ERROR_START = WCAP_ERROR_BASE + 0x10;
> >+  const unsigned long WCAP_WRAPPED_ERROR_END = WCAP_ERROR_BASE + 0x100;
> 
> I would suggest dropping the WRAPPED_ from this file, and just assume that all
> WCAP-specific errors are going to be defined in the JS, and differentiate
> between server and client errors in that file, if necessary.

How could client code determine the range of server error codes without IDL? Assuming that the provider component is loaded into an own js context, there is no shared js. [Though, I can live without that IDL, because onError() is currently called with -1 only.]

Thanks for your comments!

Comment 7

[Daniel Boelzle [:dbo]]

Attachment: preview of files to be added to the calendar tree

Dan, I have moved all WCAP specific IDL to providers/wcap/public which emits into an own xpt file. Moreover I have defined error codes for all WCAP errors, and hard-coded NS_ERROR_OFFLINE's value into js.

Comment 8

[Daniel Boelzle [:dbo]]

Attachment: switch on WCAP provider

removed IDL files from base/public/Makefile.in

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 226152 [details] [diff] [review]
preview of files to be added to the calendar tree

For what it is worth, I think there are a small enough number of files in the wcap provider that it would be reasonable to just put them all in the wcap directory rather than splitting them into public and src subdirs.  Its up to you whether you want to do this, however.

>+XPIDLSRCS       = calIWcapCalendar.idl \
>+                  calIWcapErrors.idl \
>+                  calIFreeBusyEntry.idl \
>+                  calIFreeBusyListener.idl \
>

How about renaming calIFreeBusy* to wcapIFreeBusy* for now?  The reasoning behind this is that I suspect these files are probably goimg to want to move into the base dir at some point, and I think our build system is likely to be fragile in the face of an interface moving to another directory but keeping the same name. 

>+[scriptable, uuid(2ADC008C-A7A6-4f9a-91C8-A99742B68F3D)]
>+interface calIWcapErrors : calIErrors
>+{
>+    /* WCAP specific errors */
>+    const unsigned long WCAP_ERROR_BASE = ERROR_BASE + 0x200;

Sorry, I did a poor job communicating the last time around.  The above statement should stay in calIErrors.idl along with a comment about how much space youre reserving (how much space do we have before bumping up against the next module defined in nsError.h, anyway?), and where the reserved codes are defined.

Comment 10

[Daniel Boelzle [:dbo]]

(In reply to comment #9)
> (From update of attachment 226152 [details] [diff] [review] [edit])
> For what it is worth, I think there are a small enough number of files in the
> wcap provider that it would be reasonable to just put them all in the wcap
> directory rather than splitting them into public and src subdirs.  Its up to
> you whether you want to do this, however.

I would like to stick to an extra directory for IDL.

> How about renaming calIFreeBusy* to wcapIFreeBusy* for now?  The reasoning
> behind this is that I suspect these files are probably goimg to want to move
> into the base dir at some point, and I think our build system is likely to be
> fragile in the face of an interface moving to another directory but keeping the
> same name. 

Ok, done.

> >+[scriptable, uuid(2ADC008C-A7A6-4f9a-91C8-A99742B68F3D)]
> >+interface calIWcapErrors : calIErrors
> >+{
> >+    /* WCAP specific errors */
> >+    const unsigned long WCAP_ERROR_BASE = ERROR_BASE + 0x200;
> 
> Sorry, I did a poor job communicating the last time around.  The above
> statement should stay in calIErrors.idl along with a comment about how much
> space youre reserving (how much space do we have before bumping up against the
> next module defined in nsError.h, anyway?), and where the reserved codes are
> defined.

Ok, done.
IMO no problem, each module has 16 bits for error codes. I have claimed [ERROR_BASE + 0x200, ERROR_BASE + 0x300) in calIError.idl for WCAP.

Thanks for reviewing!

Comment 11

[Daniel Boelzle [:dbo]]

Attachment: preview of files to be added to the calendar tree

Comment 12

[Daniel Boelzle [:dbo]]

Attachment: switch on WCAP provider

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 226626 [details] [diff] [review]
preview of files to be added to the calendar tree

r=dmose for landing in CVS, though the code itself has not yet been reviewed. Sorry for the delay.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

> Ok, done.
> IMO no problem, each module has 16 bits for error codes. I have claimed
> [ERROR_BASE + 0x200, ERROR_BASE + 0x300) in calIError.idl for WCAP.

If you attach a patch for that to this bug, I'll review that soon as well.

Comment 15

[Daniel Boelzle [:dbo]]

(In reply to comment #14)
> > Ok, done.
> > IMO no problem, each module has 16 bits for error codes. I have claimed
> > [ERROR_BASE + 0x200, ERROR_BASE + 0x300) in calIError.idl for WCAP.
> 
> If you attach a patch for that to this bug, I'll review that soon as well.

I have shifted that into the "switch on" patch; IMO can be reviewed with that (last) patch.

Comment 16

[Daniel Boelzle [:dbo]]

Comment on attachment 226626 [details] [diff] [review]
preview of files to be added to the calendar tree

checked in, review ongoing.

Comment 17

[Joey Minta]

(In reply to comment #16)
> (From update of attachment 226626 [details] [diff] [review] [edit])
> checked in, review ongoing.
> 
From what I can tell, this didn't land on MOZILLA_1_8_BRANCH.  We need to keep calendar/ in sync to avoid merging problems.

Comment 18

[Daniel Boelzle [:dbo]]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 226626 [details] [diff] [review] [edit] [edit])
> > checked in, review ongoing.
> > 
> From what I can tell, this didn't land on MOZILLA_1_8_BRANCH.  We need to keep
> calendar/ in sync to avoid merging problems.

cross-commit went thru successfully yesterday committing to both branches. cvs also tells me the files are in, and tagged for MOZILLA_1_8_BRANCH. Seems that only lxr for MOZILLA_1_8_BRANCH needs some time... but is finally showing wcap.

Comment 19

[Daniel Boelzle [:dbo]]

*** Bug 196396 has been marked as a duplicate of this bug. ***

Comment 20

[Daniel Boelzle [:dbo]]

Attachment: switch on WCAP provider

revised to apply again

Comment 21

[Daniel Boelzle [:dbo]]

Attachment: general switch on WCAP provider (unconditional, without pref)

Comment 22

[Daniel Boelzle [:dbo]]

Attachment: switch on WCAP provider

using the "calendar.enterprise" pref now

Comment 23

[Matthew (lilmatt) Willis]

Drive-by comments from me. None are binding :)

(In reply to comment #22)
> Created an attachment (id=230762) [edit]

> using the "calendar.enterprise" pref now
Seems like that's a pretty general prefname. Maybe calendar.enterprise.wcap.enabled = true?

+   * Range claimed is [ERROR_BASE + 0x200, ERROR_BASE + 0x300)
Ending at 0x299 rather than 0x300 is cleaner for the next person.

+        var prefService =
+            Components.classes["@mozilla.org/preferences-service;1"]
+                      .getService(Components.interfaces.nsIPrefService);
+        var prefCalBranch = prefService.getBranch("calendar.");
+        bShowWcap = prefCalBranch.getBoolPref("enterprise");

We've got preference helpers now for this.


+    checkRequired();
+  }
-               onpageshow="checkRequired();"
+               onpageshow="initLocationPage();"

Maybe onpageshow="initLocationPage(); checkRequired();" is cleaner than coupling your new function to checkRequired()?

Comment 24

[Daniel Boelzle [:dbo]]

(In reply to comment #23)
> > using the "calendar.enterprise" pref now
> Seems like that's a pretty general prefname. Maybe
> calendar.enterprise.wcap.enabled = true?

For simplicity, I would like to stick to the general "enterprise" pref (at least for now). If necessary, we can diverse the prefs later on, though IMO you hardly want to use a multi-user calendar server without the "enterprise" features like free-busy preview.

> +   * Range claimed is [ERROR_BASE + 0x200, ERROR_BASE + 0x300)
> Ending at 0x299 rather than 0x300 is cleaner for the next person.

The range includes ERROR_BASE + 0x200, but excludes ERROR_BASE + 0x300, indicated by the braces. So the next range can cleanly start at ERROR_BASE + 0x300.

> +        var prefService =
> +            Components.classes["@mozilla.org/preferences-service;1"]
> +                      .getService(Components.interfaces.nsIPrefService);
> +        var prefCalBranch = prefService.getBranch("calendar.");
> +        bShowWcap = prefCalBranch.getBoolPref("enterprise");
> 
> We've got preference helpers now for this.

Good to know, I will have a look and revise the code.

> +    checkRequired();
> +  }
> -               onpageshow="checkRequired();"
> +               onpageshow="initLocationPage();"
> 
> Maybe onpageshow="initLocationPage(); checkRequired();" is cleaner than
> coupling your new function to checkRequired()?

I would like to stick to calling just one initing function. initLocationPage() is intended to init the page, so all necessary code ought to be triggered from there.

Thanks for your comments!

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 230762 [details] [diff] [review]
switch on WCAP provider

In general, this looks good.  There is one straightforward structural change I would like to see.  

One of the sweet spots of this prototypes/ plan is that it allows us to get a good feel for how good the calendar extension hooks are by attempting to use them.  In the future, XPCOM categories in combination with calICalendarProvider are going to be the way to hook in to calendarCreation.xul.  

For now, however, the way an extension would do this should be similarly easy to what you've done and cleaner.  In particular it would use a XUL overlay to add the collapsed wcap-radio as well as an event handler to calendarCreation.xul.  The event handler (onload? ondialogshowing? I'm not sure offhand) would uncollapse based on the pref.

Comment 26

[Joey Minta]

WCAP wasn't included in any of the previous roadmapping discussions and wasn't approved by drivers for inclusion in 0.3 planning.  blocking0.3-, if only because there's no way we have the resources to do a full review of the wcap code in the 0.3 timeframe.

Comment 27

[Stephan Schaefer]

WCAP support will allow a lot of people to use and give feedback on Lightning 0.3. Especially at this stage of the project user feedback is very important, so enabling the provider code is more than reasonable. The fact that Sun's QA is not aware of any provider specific issues should be sufficient enough to enable this code, especially for a 0.3 release. I cannot imagine anybody having a bigger interest in publishing an error free provider than we have. Therefore I request the blocking flag again.

Comment 28

[Joey Minta]

Everything about "switch on with a pref" patch says it perfectly belongs in the would-accept-patch bucket.  It's significantly low risk.  It doesn't meet one of the main feature areas.  It's a segment of code that some people are interested in driving, but not all.  That describe's precisely the would-accept-patch area.  Get it in before the slush and you're fine.

The unconditional patch still cannot block 0.3.  We landed the code in the tree under the agreement that it would need to be fully reviewed before it was turned on by default.  This was the path your organization chose to take, rather than submitting smaller, easier to review patches.  As such, it shouldn't come as a surprise that, this late in the game, the resources do not exist to review the whole patch in time for the release.

In either case, this is still blocking 0.3-, because would-accept-patch bugs don't block the release.

Comment 29

[Marek Stępień [:marcoos, inactive]]

After the latest check-in, the insufficientWcapVersionConfirmation.text property contains a grammatical mistake:

"doesn't supports a sufficient WCAP version!" should be "doesn't support a sufficient WCAP version!" (remove '-s' from 'supports').

Comment 30

[Matthew (lilmatt) Willis]

(In reply to comment #29)
> After the latest check-in, the insufficientWcapVersionConfirmation.text
> property contains a grammatical mistake:
> 
> "doesn't supports a sufficient WCAP version!" should be "doesn't support a
> sufficient WCAP version!" (remove '-s' from 'supports').

Thanks for catching that.
Fixed on branch and trunk.

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 230762 [details] [diff] [review]
switch on WCAP provider

Daniel convinced me that this is a more pragmatic solution.  This is partially because that UI needs some clean up anyway, and that would just cause the overlay to break.

Comment 32

[Daniel Boelzle [:dbo]]

Comment on attachment 230762 [details] [diff] [review]
switch on WCAP provider

landed switch on by pref.

Comment 33

[Stefan Sitter [:ssitter]]

(In reply to comment #32)
> (From update of attachment 230762 [details] [diff] [review] [edit])
> landed switch on by pref.

Please remember that none of the new wcap files is shipped with win32 Sunbird unless they are listed in [http://lxr.mozilla.org/mozilla/source/calendar/installer/windows/packages-static].

Comment 34

[Daniel Boelzle [:dbo]]

Attachment: Windows installer entries [checked in]

Thanks for pointing this out, Stefan.
Seems that you are the right one to review... thanks in advance.

Comment 35

[Stefan Sitter [:ssitter]]

Comment on attachment 234597 [details] [diff] [review]
Windows installer entries [checked in]

r=ssitter
Asking lilmatt for 2nd review as he knows more about the hidden installer pitfalls.

Comment 36

[Matthew (lilmatt) Willis]

Comment on attachment 234597 [details] [diff] [review]
Windows installer entries [checked in]

I'm getting "Unable to load XPCOM component" errors with this. Spinoff that to another bug (to fix like we fixed calItemBaseModule), and r+

r2=lilmatt

Comment 37

[Matthew (lilmatt) Willis]

Comment on attachment 234597 [details] [diff] [review]
Windows installer entries [checked in]

this patch checked in on branch and trunk.

I don't want to wait and have half-baked installers

Comment 38

[Daniel Boelzle [:dbo]]

(In reply to comment #37)
> (From update of attachment 234597 [details] [diff] [review] [edit])
> this patch checked in on branch and trunk.
> 
> I don't want to wait and have half-baked installers

I would have checked it in today, Matt.

Comment 39

[Simon Paquet [:sipaq]]

First of all, it would be great if you could attach all patches here, that you check into the mozilla cvs repository.

Second, regarding the latest checkin to calWcapErrors.js:
Are these error messages supposed to be hardcoded? If not, these should be moved to a calWcapErrors.properties file, so they can be localized.

Comment 40

[Daniel Boelzle [:dbo]]

(In reply to comment #39)
> First of all, it would be great if you could attach all patches here, that you
> check into the mozilla cvs repository.

The provider is still under development and it is agreed with dmose that I can check in without the patches/review cycle at this stage. Once the code has been reviewed and becomes stable, we will slip into the common review cycle.
But right now, I consider doing so nothing more than an obstacle without use. I even wouldn't know anyone familiar with the WCAP protocol.
And: No, I don't check in without patches/review on other places than the WCAP provider files.

> Second, regarding the latest checkin to calWcapErrors.js:
> Are these error messages supposed to be hardcoded? If not, these should be
> moved to a calWcapErrors.properties file, so they can be localized.

Yes, all of the hard-coded errors in calWcapErrors.js are considered as technical expert errors without use to end-users; common day-to-day errors are localized in wcap.properties.
Though maybe some of the adopted netwerk errors taken from nsNetErrors.h are useful to be localized (there is not API for this), e.g. NS_ERROR_OFFLINE, NS_ERROR_NET_TIMEOUT.

Comment 41

[Simon Paquet [:sipaq]]

(In reply to comment #40) 
> The provider is still under development and it is agreed with dmose that I 
> can check in without the patches/review cycle at this stage. Once the code 
> has been reviewed and becomes stable, we will slip into the common review 
> cycle. But right now, I consider doing so nothing more than an obstacle 
> without use. I even wouldn't know anyone familiar with the WCAP protocol.
> And: No, I don't check in without patches/review on other places than the 
> WCAP provider files.

My comment was not about the lack of review (which has been agreed with dmose) but only about the lack of patch attachments in this bug. Since bonsai offers nearly no search capabilities, I would definitely prefer, that all patches that checked in are attached to this bug, so that interested developers can take a look at them and trace the development of the WCAP provider if they wish to do that. I would not consider that an obstacle without use.

Comment 42

[Daniel Boelzle [:dbo]]

(In reply to comment #41)
> My comment was not about the lack of review (which has been agreed with dmose)
> but only about the lack of patch attachments in this bug. Since bonsai offers

Sorry, it seems that I got you wrong. But for me patches correspond to reviews...

> nearly no search capabilities, I would definitely prefer, that all patches that
> checked in are attached to this bug, so that interested developers can take a
> look at them and trace the development of the WCAP provider if they wish to do
> that. I would not consider that an obstacle without use.

IMO an avoidable overhead bloating this bug. You can conveniently use lxr to search the code (if you don't want to check out) or bonsai cvs query to get diff information, e.g. changes of the last week (on trunk):
<http://bonsai.mozilla.org/cvsquery.cgi?branch=HEAD&file=mozilla/calendar/providers/wcap/&date=week>

Comment 43

[Stefan Sitter [:ssitter]]

The last check in for this bug (2006-11-21 04:53, daniel.boelzle%sun.com) broke all Sunbird and Lightning Tinderboxes: 
> gmake[8]: *** No rule to make target `calIWcapSearchListener.idl'.  Stop.

Comment 44

[Michiel van Leeuwen (email: mvl+moz@)]

I backed out the makefile.in changes to try to get the tinderboxes back to green. When you add the missing file, you will also need to update Makefile.in

Comment 45

[Michiel van Leeuwen (email: mvl+moz@)]

I had to back out the entire patch to get back to green...

Comment 46

[Daniel Boelzle [:dbo]]

Dooh! I forgot to cvs add a new file...
Fixed that and re-checked everything in again.
Big SORRY for causing trouble and THANKS to Michiel for taking care!

Comment 47

[Matthew (lilmatt) Willis]

Not going to make the 0.5 train.

Comment 48

[Stefan Sitter [:ssitter]]

Daniel, it seems that there was an unscheduled checkin to mozilla/calendar/locales/en-US/chrome/calendar/wcap.properties today breaking the l10n freeze. Now the l10n teams are asking how to handle this checkin for their locale.

Comment 49

[Simon Paquet [:sipaq]]

(In reply to comment #48)
> Daniel, it seems that there was an unscheduled checkin to
> mozilla/calendar/locales/en-US/chrome/calendar/wcap.properties today breaking
> the l10n freeze. Now the l10n teams are asking how to handle this checkin for
> their locale.

Stefan, looking at bonsai the change did not affect any strings and only fixed the license header. Therefore I see no immediate need to fix this on the l10n front.

Comment 50

[Daniel Boelzle [:dbo]]

(In reply to comment #48)
> Daniel, it seems that there was an unscheduled checkin to
> mozilla/calendar/locales/en-US/chrome/calendar/wcap.properties today breaking
> the l10n freeze. Now the l10n teams are asking how to handle this checkin for
> their locale.

Sorry guys, I wasn't aware that this breaks l10n string freeze. I have just fixed the license header in that file, no l10n work needed of course (as Simon alreaday mentioned). I should not have commited to SUNBIRD_0_5_BRANCH... BIG SORRY!

Comment 51

[Daniel Boelzle [:dbo]]

Attachment: for review

Requesting overdue review...

Comment 52

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

Comment on attachment 321486 [details] [diff] [review]
for review

>+        return calWcapCalendarModule.WcapCalendarInfo.classDescription;
you might want to camelCase wcapCalendarInfo to be consistant with JS style.

>+    get contractID() {
>+    get classID() {
Please make sure all getters/setters are not anonymous in all files

>+    getInterface: function calWcapCalendar_getInterface(iid, instance) {
>+        if (iid.equals(Components.interfaces.nsIAuthPrompt)) {
>+            // use the window watcher service to get a nsIAuthPrompt impl
>+            return getWindowWatcher().getNewAuthPrompter(null);
>+        } else if (iid.equals(Components.interfaces.nsIPrompt)) {
>+            // use the window watcher service to get a nsIPrompt impl
>+            return getWindowWatcher().getNewPrompter(null);
>+        }
>+        Components.returnCode = Components.results.NS_ERROR_NO_INTERFACE;
>+        return null;
>+    },
Are there not more interfaces you want to return something for?
nsIAuthPrompt2 (in case it exists), nsIAuthPromptProvider

Also, why not just throw NS_NOINTERFACE/NS_ERROR_NO_INTERFACE?

>+                log("error: " + msg, context);
Although there is no conflict, since we have LOG(), maybe you want a different name for it?

>+    m_uri: null,
>+    get uri() {
>+        return this.m_uri;
>+    },
Why not use the base provider's mUri? I know you can't use the getter from base, but you could use the variable.

>+                    this_.assureAccess(accessRights);
Not important at all, but I think the word "ensure" fits better here, see grammar nazi discussions about ensure vs assure vs insure :-)



>+    m_calId: null,
>+    get calId() {
>+        if (this.m_calId) {
>+            return this.m_calId;
>+        }
>+        return this.session.defaultCalId;
>+    },
Could be shortened to: 
return this.m_calId || this.session.defaultCalId;


>+    get isOwnedCalendar() {
>+    get isDefaultCalendar() {
We should get this out into a common api for gdata/wcap/etc, but of course in a separate bug.

>+    getCalendarProperties: function calWcapCalendar_getCalendarProperties(propName, out_count) {
>+    getCalProps: function calWcapCalendar_getCalProps(propName) {
Similar name, but one is used with an out parameter? Why? I guess if some properties return an array? If there is an easy way to consolidate these two methods, go ahead.

>+            throw new Components.Exception(errorToString(calIErrors.CAL_IS_READONLY),
If there is a way we could move the errorsToString et al methods into a general calendar error handler, that would be great. It sucks to need to do this :-/

>+# The Initial Developer of the Original Code is
>+# Sun Microsystems, Inc.
>+# Portions created by the Initial Developer are Copyright (C) 2007
Indent SMI by two spaces in all files


>+DEPTH           = ../../..
>+topsrcdir       = @top_srcdir@
>+srcdir          = @srcdir@
>+VPATH           = @srcdir@
No need for alignment in this file, remove if you like (in all Makefiles)

>+    m_ifaces: [calIWcapCalendar,
>+               calICalendar,
>...
>+               nsISupports],

I assume you have constants somewhere, but for sake of alignment please add Components.interfaces.


err...do you have calWcapCalendar.js in there three times with different cvs versions?

>+var calWcapCalendar;
>+if (!calWcapCalendar) {
>+    calWcapCalendar = {};
>+    calWcapCalendar.prototype = {};
>+}
I believe if calWcapCalendar is not defined, then something wasn't loaded in the correct order and something should fail?

>+    if (cn) {
>+        params = encodeAttr(cn.replace(/[;:]/g, " "), "CN", params);
>+    }
Should [;:] just be removed, or can it be escaped?

>+        function sameSet(list, list_) {
>+            return (list.length == list_.length &&
>+                    list.every( function everyFunc(x) {
>+                                    return list_.some(
>+                                        function someFunc(y) { return x == y; } );
>+                                }
>+                        ));
>+        }
Wow, this looks like intense processing. Maybe we need a way to make this easier on an interface level?

>+    function encodeList(list) {
>+        var ret = "";
>+        for each ( var str in list ) {
>+            if (ret.length > 0)
>+                ret += ";";
>+            ret += str;
>+        }
>+        return ret;
>+    }
str.join(";").replace(/;(?=;)/g, "").replace(/(^;|;$)/g, "");

>+        function getAttachments(item) {
>+            var attachments = item.attachments;
>+                for each (var att in attachements) {
I wasn't aware its possible to loop through attachments like this? Also, don't we currently use the URL field?
>+        if (LOG_LEVEL > 2) {
>+            log("old item:\n" + oldItem.icalString + "\n\nnew item:\n" + item.icalString, this);
Wouldn't it make sense to add the required log level as a parameter and then filter out in the LOG function?

>+        var someDate = (item.startDate || item.entryDate || item.dueDate);
>+        if (someDate) {
>+            someTz = someDate.timezone;
>+            if (!someTz.isUTC && !someTz.isFloating) {
If the timezone is floating, maybe you want to pass calendarDefaultTimezone()?

>+    // xxx todo: temp workaround for bug in calItemBase.js
>+    if (!isParent(srcItem)) {
>+        return;
>+    }
Is a bug filed for whatever problem this is? If so, please add bug#

>+    forEachIcalComponent(
>+        icalRootComp, componentType,
>+        function(subComp) {
>+
>+            function patchTimezone(subComp, attr, xprop) {
Instead of patching the timezone, maybe its possible to let the wcap timezone provider take care? Not quite sure what forEachIcalCompoent does yet though.

>+            }
>+            catch (exc) {
>+            }
} catch () {

>+// ctors:
>+var CalEvent;
>+var CalTodo;
>+var CalDateTime;
>+var CalDuration;
>+var CalPeriod;
>+var Timer;
Since we have createEvent(), createTodo()...do you need the ctors?
>+                    return ("[0x" + err.toString(0x10) + "] unknown error.");
tricky way of saying toString(16) :-) Fooled me for a second

>+    // nsIUnicharStreamLoaderObserver:
The unichar streamloader has problems when the response body is empty, iirc. Make sure this isn't the case for wcap?

>+    try {
>+        loader.init(channel, netRequest, null /*context*/, 0 /*segment size*/);
>+    } catch (exc) {
>+        netRequest.execRespFunc(exc);
>+    }
Does the unichar streamload not also work differently on trunk? See gdata for an example with a normal streamloader

>+                // pw mgr host names must not have a trailing slash
>+                var passwordManager = Components.classes["@mozilla.org/passwordmanager;1"]
>+                                                .getService(Components.interfaces.nsIPasswordManager);
Don't forget to switch to loginmanager (trunk). See helper functions in gdata, we might want to generalize them.

>+                    onResult: function search_onResult(request, result) {
>+                        try {
>+                            if (!Components.isSuccessCode(request.status)) {
>+                                throw request.status;
>+                            }
throw new Components.Exception(result, request.status) ?

>+function getConsoleService() {
>+    if (!getConsoleService.m_obj) {
>+        getConsoleService.m_obj = Components.classes["@mozilla.org/consoleservice;1"]
>+                                            .getService(Components.interfaces.nsIConsoleService);
>+    }
>+    return getConsoleService.m_obj;
>+}
I believe you recently moved this to calUtils?

>+loginDialog.text=Enter your username and password for %1$S.
>+loginDialog.check.text=Use Password Manager to remember this password.
>+loginDialogPasswordOnly.text=Enter password for user %1$S on %2$S.

These can be taken from gecko, see gdata.


r=philipp with comments considered.

Comment 53

[Daniel Boelzle [:dbo]]

(In reply to comment #52)
> (From update of attachment 321486 [details] [diff] [review])
> >+        return calWcapCalendarModule.WcapCalendarInfo.classDescription;
> you might want to camelCase wcapCalendarInfo to be consistant with JS style.
done

> >+    get contractID() {
> >+    get classID() {
> Please make sure all getters/setters are not anonymous in all files
done

> >+    getInterface: function calWcapCalendar_getInterface(iid, instance) {
> >+        if (iid.equals(Components.interfaces.nsIAuthPrompt)) {
> >+            // use the window watcher service to get a nsIAuthPrompt impl
> >+            return getWindowWatcher().getNewAuthPrompter(null);
> >+        } else if (iid.equals(Components.interfaces.nsIPrompt)) {
> >+            // use the window watcher service to get a nsIPrompt impl
> >+            return getWindowWatcher().getNewPrompter(null);
> >+        }
> >+        Components.returnCode = Components.results.NS_ERROR_NO_INTERFACE;
> >+        return null;
> >+    },
> Are there not more interfaces you want to return something for?
> nsIAuthPrompt2 (in case it exists), nsIAuthPromptProvider
Actually this seems to be a leftover; I've removed the code.

> Also, why not just throw NS_NOINTERFACE/NS_ERROR_NO_INTERFACE?
done

> >+                log("error: " + msg, context);
> Although there is no conflict, since we have LOG(), maybe you want a different
> name for it?
Long term, I'd suggest we consolidate/enhance logging.

> >+    m_uri: null,
> >+    get uri() {
> >+        return this.m_uri;
> >+    },
> Why not use the base provider's mUri? I know you can't use the getter from
> base, but you could use the variable.
This would make it part of calBaseProvider's API which I dislike.

> >+    m_calId: null,
> >+    get calId() {
> >+        if (this.m_calId) {
> >+            return this.m_calId;
> >+        }
> >+        return this.session.defaultCalId;
> >+    },
> Could be shortened to: 
> return this.m_calId || this.session.defaultCalId;
done

> >+    get isOwnedCalendar() {
> >+    get isDefaultCalendar() {
> We should get this out into a common api for gdata/wcap/etc, but of course in a
> separate bug.
IMO bug 370150.

> >+    getCalendarProperties: function calWcapCalendar_getCalendarProperties(propName, out_count) {
> >+    getCalProps: function calWcapCalendar_getCalProps(propName) {
> Similar name, but one is used with an out parameter? Why? I guess if some
> properties return an array? If there is an easy way to consolidate these two
> methods, go ahead.
consolidated

> >+# The Initial Developer of the Original Code is
> >+# Sun Microsystems, Inc.
> >+# Portions created by the Initial Developer are Copyright (C) 2007
> Indent SMI by two spaces in all files
done

> err...do you have calWcapCalendar.js in there three times with different cvs
> versions?
> 
> >+var calWcapCalendar;
> >+if (!calWcapCalendar) {
> >+    calWcapCalendar = {};
> >+    calWcapCalendar.prototype = {};
> >+}
> I believe if calWcapCalendar is not defined, then something wasn't loaded in
> the correct order and something should fail?
seems to be a leftover, removed

> >+    if (cn) {
> >+        params = encodeAttr(cn.replace(/[;:]/g, " "), "CN", params);
> >+    }
> Should [;:] just be removed, or can it be escaped?
should be "removed", added comment

> >+    function encodeList(list) {
> >+        var ret = "";
> >+        for each ( var str in list ) {
> >+            if (ret.length > 0)
> >+                ret += ";";
> >+            ret += str;
> >+        }
> >+        return ret;
> >+    }
> str.join(";").replace(/;(?=;)/g, "").replace(/(^;|;$)/g, "");
Why the latter replace stuff? IMO a simple return list.join(";") is sufficient, isn't it?

> >+        function getAttachments(item) {
> >+            var attachments = item.attachments;
> >+                for each (var att in attachements) {
> I wasn't aware its possible to loop through attachments like this? Also, don't
> we currently use the URL field?
There's yet no attachment support, this is just sketched code.
URL field is covered in the same function.

> >+        if (LOG_LEVEL > 2) {
> >+            log("old item:\n" + oldItem.icalString + "\n\nnew item:\n" + item.icalString, this);
> Wouldn't it make sense to add the required log level as a parameter and then
> filter out in the LOG function?
The log function checks the LOG_LEVEL again. I just try to avoid any string concats up front. Maybe this is premature optimization ;)

> >+        var someDate = (item.startDate || item.entryDate || item.dueDate);
> >+        if (someDate) {
> >+            someTz = someDate.timezone;
> >+            if (!someTz.isUTC && !someTz.isFloating) {
> If the timezone is floating, maybe you want to pass calendarDefaultTimezone()?
Doesn't matter really, because the server couldn't cope with floating times.

> >+    // xxx todo: temp workaround for bug in calItemBase.js
> >+    if (!isParent(srcItem)) {
> >+        return;
> >+    }
> Is a bug filed for whatever problem this is? If so, please add bug#
I can't remember it :/

> >+    forEachIcalComponent(
> >+        icalRootComp, componentType,
> >+        function(subComp) {
> >+
> >+            function patchTimezone(subComp, attr, xprop) {
> Instead of patching the timezone, maybe its possible to let the wcap timezone
> provider take care? Not quite sure what forEachIcalCompoent does yet though.
The patching needs to be done, because cs separates date-time (always in UTC) from the timezone (no TZID param, but X-NSCP-DTSTART-TZID etc property). Seems the cs developers wouldn't want to send the timezone data with the VCALENDAR data, so they chose this way.

> >+// ctors:
> >+var CalEvent;
> >+var CalTodo;
> >+var CalDateTime;
> >+var CalDuration;
> >+var CalPeriod;
> >+var Timer;
> Since we have createEvent(), createTodo()...do you need the ctors?
resolved except for period and duration

> >+    // nsIUnicharStreamLoaderObserver:
> The unichar streamloader has problems when the response body is empty, iirc.
> Make sure this isn't the case for wcap?
> 
> >+    try {
> >+        loader.init(channel, netRequest, null /*context*/, 0 /*segment size*/);
> >+    } catch (exc) {
> >+        netRequest.execRespFunc(exc);
> >+    }
> Does the unichar streamload not also work differently on trunk? See gdata for
> an example with a normal streamloader
I'll come up with a follow-up patch for that.

> >+                // pw mgr host names must not have a trailing slash
> >+                var passwordManager = Components.classes["@mozilla.org/passwordmanager;1"]
> >+                                                .getService(Components.interfaces.nsIPasswordManager);
> Don't forget to switch to loginmanager (trunk). See helper functions in gdata,
> we might want to generalize them.
I'll consolidate the password manager stuff and come up with a follow-up patch.

> >+                    onResult: function search_onResult(request, result) {
> >+                        try {
> >+                            if (!Components.isSuccessCode(request.status)) {
> >+                                throw request.status;
> >+                            }
> throw new Components.Exception(result, request.status) ?
> 
> >+function getConsoleService() {
> >+    if (!getConsoleService.m_obj) {
> >+        getConsoleService.m_obj = Components.classes["@mozilla.org/consoleservice;1"]
> >+                                            .getService(Components.interfaces.nsIConsoleService);
> >+    }
> >+    return getConsoleService.m_obj;
> >+}
> I believe you recently moved this to calUtils?
yes, removed

> >+loginDialog.text=Enter your username and password for %1$S.
> >+loginDialog.check.text=Use Password Manager to remember this password.
> >+loginDialogPasswordOnly.text=Enter password for user %1$S on %2$S.
> 
> These can be taken from gecko, see gdata.
> 
> 
> r=philipp with comments considered.
> 

Checked changes in on HEAD and MOZILLA_1_8_BRANCH => FIXED.

Comment 54

[Andreas Treumann]

Verified in lightning 2008061918 and Sunbird 20080619 -> VERIFIED.