Bug 340949 (wcap)

WCAP provider implementation

VERIFIED FIXED in 0.9

Status

enhancement
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: dbo, Assigned: dbo)

Tracking

unspecified
Dependency tree / graph

Details

Attachments

(1 attachment, 11 obsolete attachments)

272.26 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
Assignee

Description

13 years ago
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).
Assignee

Updated

13 years ago
Status: NEW → ASSIGNED
Assignee

Updated

13 years ago
Assignee: nobody → daniel.boelzle
Status: ASSIGNED → NEW
Assignee

Updated

13 years ago
Depends on: 341154
Assignee

Updated

13 years ago
Depends on: 341522
Assignee

Comment 1

13 years ago
I have tossed together a wiki page:
 http://wiki.mozilla.org/Calendar:WCAP_Provider
Status: NEW → ASSIGNED
Assignee

Comment 2

13 years ago
Posted patch IDL additions to calIErrors (obsolete) — Splinter Review
Attachment #225701 - Flags: first-review?(dmose)
Assignee

Comment 3

13 years ago
Posted patch switch on WCAP provider (obsolete) — Splinter Review
Master switch to build with WCAP provider when everything is reviewed.
Attachment #225702 - Flags: first-review?(dmose)
Assignee

Comment 4

13 years ago
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?
Assignee

Updated

13 years ago
Depends on: 341776
Assignee

Updated

13 years ago
Depends on: 257428
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.
Attachment #225701 - Flags: first-review?(dmose) → first-review-
Assignee

Comment 6

13 years ago
(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!
Assignee

Comment 7

13 years ago
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.
Attachment #225701 - Attachment is obsolete: true
Attachment #225703 - Attachment is obsolete: true
Attachment #226152 - Flags: first-review?(dmose)
Assignee

Comment 8

13 years ago
Posted patch switch on WCAP provider (obsolete) — Splinter Review
removed IDL files from base/public/Makefile.in
Attachment #225702 - Attachment is obsolete: true
Attachment #226153 - Flags: first-review?(dmose)
Attachment #225702 - Flags: first-review?(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.
Attachment #226152 - Flags: first-review?(dmose) → first-review-
Assignee

Comment 10

13 years ago
(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!
Assignee

Comment 11

13 years ago
Attachment #226152 - Attachment is obsolete: true
Attachment #226626 - Flags: first-review?(dmose)
Assignee

Comment 12

13 years ago
Posted patch switch on WCAP provider (obsolete) — Splinter Review
Attachment #226153 - Attachment is obsolete: true
Attachment #226627 - Flags: first-review?(dmose)
Attachment #226153 - Flags: first-review?(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.
Attachment #226626 - Flags: first-review?(dmose) → first-review+
> 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.
Assignee

Comment 15

13 years ago
(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.
Assignee

Comment 16

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

checked in, review ongoing.
Attachment #226626 - Attachment is obsolete: true

Comment 17

13 years ago
(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.
Assignee

Comment 18

13 years ago
(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.
Assignee

Comment 19

13 years ago
*** Bug 196396 has been marked as a duplicate of this bug. ***
Assignee

Comment 20

13 years ago
Posted patch switch on WCAP provider (obsolete) — Splinter Review
revised to apply again
Attachment #226627 - Attachment is obsolete: true
Attachment #229073 - Flags: first-review?(dmose)
Attachment #226627 - Flags: first-review?(dmose)
Assignee

Comment 21

13 years ago
Attachment #229073 - Attachment is obsolete: true
Attachment #229468 - Flags: first-review?(dmose)
Attachment #229073 - Flags: first-review?(dmose)
Assignee

Comment 22

13 years ago
Posted patch switch on WCAP provider (obsolete) — Splinter Review
using the "calendar.enterprise" pref now
Attachment #229468 - Attachment is obsolete: true
Attachment #230762 - Flags: first-review?(dmose)
Attachment #229468 - Flags: first-review?(dmose)
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()?

Assignee

Comment 24

13 years ago
(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!
Assignee

Updated

13 years ago
Attachment #229468 - Attachment description: switch on WCAP provider → general switch on WCAP provider (unconditional, without pref)
Attachment #229468 - Attachment is obsolete: false
Attachment #229468 - Flags: first-review?(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.
Attachment #230762 - Flags: first-review?(dmose) → first-review-
Assignee

Updated

13 years ago
Flags: blocking0.3?
Target Milestone: --- → Lightning 0.3

Comment 26

13 years ago
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.
Flags: blocking0.3? → blocking0.3-
Assignee

Updated

13 years ago
No longer depends on: 257428, 341776

Comment 27

13 years ago
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.
Flags: blocking0.3- → blocking0.3?

Comment 28

13 years ago
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.
Flags: blocking0.3? → blocking0.3-
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').
(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 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.
Attachment #230762 - Flags: first-review- → first-review+
Assignee

Updated

13 years ago
Attachment #229468 - Attachment is obsolete: true
Attachment #229468 - Flags: first-review?(dmose)
Assignee

Comment 32

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

landed switch on by pref.
Attachment #230762 - Attachment is obsolete: true
(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].
Assignee

Comment 34

13 years ago
Thanks for pointing this out, Stefan.
Seems that you are the right one to review... thanks in advance.
Attachment #234597 - Flags: first-review?(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.
Attachment #234597 - Flags: second-review?(mattwillis)
Attachment #234597 - Flags: first-review?(ssitter)
Attachment #234597 - Flags: first-review+
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
Attachment #234597 - Flags: second-review?(mattwillis) → second-review+
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
Attachment #234597 - Attachment description: Windows installer entries → Windows installer entries [checked in]
Assignee

Comment 38

13 years ago
(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.
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.
Alias: wcap
Component: General → Provider: WCAP
QA Contact: general → wcap-provider
Summary: WCAP provider → WCAP provider implementation
Assignee

Comment 40

13 years ago
(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.
(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.
Assignee

Comment 42

13 years ago
(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>
Target Milestone: Lightning 0.3 → Lightning 0.5
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.
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
I had to back out the entire patch to get back to green...
Assignee

Comment 46

13 years ago
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!
Not going to make the 0.5 train.
Target Milestone: Lightning 0.5 → ---
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.
(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.

Assignee

Comment 50

12 years ago
(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!
Assignee

Comment 51

11 years ago
Posted patch for reviewSplinter Review
Requesting overdue review...
Attachment #234597 - Attachment is obsolete: true
Attachment #321486 - Flags: review?(philipp)
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.
Attachment #321486 - Flags: review?(philipp) → review+
Assignee

Comment 53

11 years ago
(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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.9

Comment 54

11 years ago
Verified in lightning 2008061918 and Sunbird 20080619 -> VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.