355117 - Add Minimal Support for Google Calendar

Status: RESOLVED FIXED

(Calendar :: Provider: GData, enhancement)

Description

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

Adds a minimal version of the google calendar Provider.

   KNOWN ISSUES
    * Alarms are not implemented
    * Optimistic concurrency is not handled correctly, 409 Conflict responses
      have unforseen results.
    * Recurring events are skipped. Google just changed the way this was handled
      and I also haven't looked into how sunbird does it.
    * If no timezone was set by the user, editing events gives the events
      the first timezone that has the same offset. Since there are still some
      open timezone issues, this will most likely break things.
    * All day event keep moving back one day when you edit them, since google
      does not correctly parse <gd:when> elements. A bug has been filed. This is
      also the reason why one-day allday events show up as two day events
    * Setting Status "Cancelled" on an Event deletes it from google after a while,
      I need some input on this, what should be done with those.
    * Google's has an event privacy "default", which has no represenation in
      sunbird. Sunbird could check google's calendar meta-feed to check what
      the default value is.
    * Error catching is rudimental. This needs a huge makeover. Google's errors
      are not catched, http server errors are not catched...
    * Cancelling the login to google does not tell the listener something went
      wrong. This means that calIOperationListener::onOperationComplete is
      not called, which can delay things.

   KNOWN LIMITATIONS
    * Google does not support TODO items
    * Google does not support Categories and URLs (correctly)
    * There is no "not specified" in event statii. Defaults to confirmed
    * Google does not have a priority property on events
    * This provider only handles private calendars withouht magic cookies.
      For all other types please use the ICS provider.

Comment 1

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

Attachment: Patch for Basic Google Provider UI Elements

Comment 2

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

Attachment: Add Error Range for Google Calendar Provider

Comment 3

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

Attachment: Minimal functionality of the google provider

Comment 4

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

Attachment: IDL definitions

Comment 5

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

Attachment: Basic UI added to the Add calendar dialog

Comment 6

[Joey Minta]

Comment on attachment 240919 [details] [diff] [review]
Patch for Basic Google Provider UI Elements

+    var bShowGoogle = false;
     try {
         bShowWcap = getPrefSafe("calendar.prototypes.wcap", false);
+        bShowGoogle = getPrefSafe("calendar.google.enabled",false);
Couple of comments here.  I thought I've mentioned elsewhere that normal calendar style doesn't include a prefix for variable types (since js is weakly typed.)  Also, there's no way for getPrefSafe to throw, so you don't need a try/catch here.

Comment 7

[Joey Minta]

Comment on attachment 240923 [details] [diff] [review]
Add Error Range for Google Calendar Provider

this review should go to dmose, since he dipped into the error-code for wcap.

Comment 8

[Joey Minta]

Comment on attachment 240926 [details] [diff] [review]
IDL definitions

+    attribute AUTF8String googleCalendarName;
What's the difference between googleCalendarName and calICalendar's name property?  Since it's defined here to be read/write, perhaps we ought to just make the two identical?

Also, I'm reluctant to see more fragmentation in the calICalendar idl.  Is there a strong reason to have these implemented separately, rather than using the flexible set/getCalendarPref methods on calICalendarManager?

Similarly, it might be nice to try to unify some of the errors here into more generic calendar errors that other providers can share.

Comment 9

[Joey Minta]

Comment on attachment 240924 [details] [diff] [review]
Minimal functionality of the google provider

Just some first pass comments.  Please pick a specific reviewer, otherwise I doubt anyone will be willing to give this the in-depth look it needs.

+function calGoogleCalendar ( session ) {
+
+	this.mObservers = new Array();
+	this.mSession = session;
As an FYI, normal calendar style is:
function foo(aArg1, aArg2) {
with no spaces around arguments and an a- prefix on arguments.  This is your file though, so I won't dictate what you need to do, just be consistent.

+	QueryInterface: function (aIID) {
This is my own pet-peeve: anonymous functions.  Debugging can be a lot easier if you go ahead and give these functions names like cgc_qi, or whatever.

+		if (!aIID.equals(Components.interfaces.nsISupports) &&
+			!aIID.equals(Components.interfaces.calIGoogleCalendar) &&
+			!aIID.equals(Components.interfaces.calICalendar)) {
+			throw Components.results.NS_ERROR_NO_INTERFACE;
tabs have been known to cause problems on some platforms and are generally discouraged.  Expand to spaces, please.

+	get googleUser() { return this.mSession.googleUser; },
+	set googleUser(value) { this.mSession.googleUser = value; },
setters need to |return value;| at the end of them.

+	get name() {
+        return getCalendarManager().getCalendarPref(this,"NAME");
+	},
+
+	set name(name) {
+		getCalendarManager().setCalendarPref(this, "NAME", name);
+	},
You've got some weird alignment going on here.  Also http://lxr.mozilla.org/mozilla/source/calendar/base/src/calCalendarManager.js#332 will make NAME lowercase, so it's not necessary to make them caps.  I don't much care either way, though.

+			var ioService = Components.classes["@mozilla.org/network/io-service;1"]
+                                   .getService(Components.interfaces.nsIIOService);
Since you're loading the calUtils.js file, you can shorten these to use Cc and Ci.

+        this.mObservers.forEach(
+            function ( obj ) {
+                try {
+                    obj[event].apply(obj,args);
+                } catch (e) {
+                    Components.utils.reportError(e);
+                }
+            }
+        );
I find the .forEach style slightly hard to read, plus it creates an anonymous function.  More importantly, you should add something specific to the .reportError call, so that it's easier to tell where it's coming from.

+		if(this.mObservers.indexOf( aObserver ) == -1 ) {
+			this.mObservers.push(aObserver);
+		}
In this file you mix |if (foo)| spacing styles.

+	adoptItem: function ( aItem, aListener ) {
+		LOG({add_item: aItem.id});
Hmm, that's an interesting use of LOG that I didn't expect.  Why not just LOG("add item:"+aItem.id)?

+			// XXX Is this really needed? I saw it on some other provider, but
+			// theoretically, addItem should not be called if the calendar is
+			// readonly. this also applies for delete/modify
Right now the front end doesn't do many checks to see if a calendar is read-only.  These are here to truly enforce the read-only property.

+	addItem: function ( aItem, aListener ) {
+		this.adoptItem( aItem.clone(), aListener );
+	},
Note that items passed in via addItem aren't generally required to have their .id set.  Will the google server side assign an id?  A comment about that would be nice.

+			if(relevantFieldsMatch(aOldItem,aNewItem)) {
+                LOG({action:"Not requesting item modification, relevant fields match"});
This seems like an expensive check to make every time there is a modification.  Why not go ahead and pass the new item on to google anyway?

+
+			this.mSession.asyncItemRequest("PUT",uri,"application/atom+xml",
+					xmlentry,this,this.modifyItem_response,extradata);
+		} catch(e) {
+			if(aListener != null) {
+				aListener.onOperationComplete(this,
+						e.result,
+						Components.interfaces.calIOperationListener.MODIFY,
+						null, e.message);
+			}
What are the expected errors here?  I'm starting to notice a tendency to over-protect code here.  Can you include some comments about expected failure modes here?

+            var extradata = { listener: aListener, item: aItem.clone() };
+            extradata.item.makeImmutable();
Why do you make the item immutable here, but not in modifyItem?

That's as far as I got for now...  This will be a lot more readable if you could tidy up the whitespace so that there's consistent indenting at various block levels.

Comment 10

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

Thank you for the comments. I will keep them in Mind for the next patches. The tabs/spacing issues were not intended, I like to blame it on my editor.

#6: I just copied this from what wcap did, I must admit I didn't even test it. This should be changed seperatly, so it follows a common scheme.

#8: I am working on a more transparent interface definition. googleCalendarName was intended to be the email part in http://www.google.com/calendar/feeds/*email*/private/full while name should be the name that the user has set in the calendar tab.

I like the idea of using get/setCalendarPref, I just discovered it recently but haven't looked into it yet.

By now I have much more errors, as noted in this patches release notes the error handling was something that was really bugging me. I will try to find CAL_ or NS_ errors that could fit for the next patch.

#9: I am working on a new patch, since I have changed some fundamental things, specifically on how the session fits in with the calendar. Before I pick a specific reviewer I would like to bring out that patch, since it would probably be as much work to review the new one as it would be to fully review this one. I didn't take out the first-review flags to not create too much bugspam. 

About the upercaes "NAME" as a calendar pref, I copied that line from another provider, I'll just keep it that way to be consistant. 

About using Ci and Cc, I know of that possibility, but I'm not sure if I like it though. I can go ahead and change those though if that makes it easier to read.

About my usage of LOG, I for some reason had all strings show up as objects with one character per line when using LOG the conventional way. I started using that notation and since then never checked if the other way worked.

About relevantFieldsMatch, is it not much more expensive to send each change that google dosen't really value over the network? The delay is much longer than calling that function.


Thank you for reviewing the code so far, to make it easier for you (all), it would probably be enough to only look for conceptional errors until I prepare the next patch.

Comment 11

[Joey Minta]

(In reply to comment #10)
> About my usage of LOG, I for some reason had all strings show up as objects
> with one character per line when using LOG the conventional way. I started
> using that notation and since then never checked if the other way worked.
> 
A fix was checked in for that just a few days ago.

Comment 12

[cmtalbert]

Looks like most of the comments I had in mind have been addressed, I only have one more to add. You might be addressing this in your new patch, but when you try to build this one, it fails because calIGoogleSession.idl doesn't exist. I had to comment it out of calendar/providers/google/public/Makefile.in. After that, it built fine.

And so far, basic functionality seems to be there.

Comment 13

[Damian Szczepanik]

mark as test case wanted

for this feature we need to prepare at least couple of test cases...

Comment 14

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

Attachment: Patch for Basic Google Provider UI elements

Nothing changed, diff'd against newest revision.

Comment 15

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

Attachment: Basic Provider Files

This is the new updated patch of the basic files in /m/c/provider/google

Fixes since last release:
    * Session handeling has been improved
    * All day events are shown correctly, as long as they are multi-day
    * Error handeling has been improved
    * Canceling the login dialog has been improved.
 
New Known Limitations:
    * This patch does not have support for alarms or attendees
    * If you cancel all login dialogs, then there is no chance to login again without restarting.

Comment 16

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

Attachment: IDL Interface definitions

I have minimized the interface definitions a lot. I think there is no use for other interfaces since there is not much use for the other classes outside of the provider.

If anyone has more ideas on what else should go in the interface, feel free to comment.

Comment 17

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

Attachment: Makefile.in for various directories

Comment 18

[cmtalbert]

Comment on attachment 246623 [details] [diff] [review]
Makefile.in for various directories

>+EXTRA_SCRIPTS = \
>+				calGoogleCalendar.js \
>+				calGoogleSession.js \
>+				calGoogleRequest.js \
>+				calGoogleUtils.js \
>+				calGoogleErrors.js \
>+				$(NULL)
>+
These should only be one tab and then spaces for alignment

>+EXTRA_PP_COMPONENTS = \
>+				calGoogleCalendarModule.js \
>+				$(NULL)
These should also be one tab and then spaces

Going to mark r=ctalbert with these fixed.

Comment 19

[cmtalbert]

(In reply to comment #18)
> (From update of attachment 246623 [details] [diff] [review] [edit])
> >+EXTRA_SCRIPTS = \
> >+				calGoogleCalendar.js \
> >+				calGoogleSession.js \
> >+				calGoogleRequest.js \
> >+				calGoogleUtils.js \
> >+				calGoogleErrors.js \
> >+				$(NULL)
> >+
> These should only be one tab and then spaces for alignment
> 
> >+EXTRA_PP_COMPONENTS = \
> >+				calGoogleCalendarModule.js \
> >+				$(NULL)
> These should also be one tab and then spaces
> 
> Going to mark r=ctalbert with these fixed.
> 
Actually, there should be no tabs here since these are not defining a target. These define variables, so these should be spaces. Down below where you have libs:: and install:: those define targets and the tabs are required there (those are correct in your patch).

So, change the above tabs to spaces. Still r+. (Thanks to lilmatt for correcting me on this!)

Comment 20

[cmtalbert]

Comment on attachment 246622 [details] [diff] [review]
IDL Interface definitions


>+/** Adds Google Calendar specific capabilities.
>+ */
I'm not too sure this comment is needed, but there is no reason why it shouldn't conform
to the usual style:
/**
 * <blah>
 */

>+/**
>+ * Google Error Codes
>+ */
It might be nice to mention in this comment, that GOOGLE_ERROR_BASE is defined in calIError.idl.
I don't feel too strongly about that, so whatever you think. It is pretty obvious if you know
IDL syntax.

>+[scriptable, uuid(d1a6e988-4b4d-45a5-ba46-43e501ea96e3)]
>+interface calIGoogleCalendar : calICalendar
>+{
>+    /**
>+     * Google's Calendar Name
>+     * This represents the <calendar name> in
>+     * http://www.google.com/calendar/feeds/<calendar name>/private/full
>+     */
>+    attribute AUTF8String googleCalendarName;
>+
>+};
Remove the blank line between the attribute and the closing brace, please.

>+[scriptable, uuid(2f1a0143-10b7-4f0f-a7a9-8a1229b071b8)]
>+interface calIGoogleErrors : calIErrors
>+{
>+
>+    const unsigned long GOOGLE_NOT_SUPPORTED = GOOGLE_ERROR_BASE + 1;
Remove blank line between opening brace and const line. It's uneeded and the code is 
readble since the const line is indented.

>+    const unsigned long GOOGLE_ITEM_INVALID = GOOGLE_ERROR_BASE + 10;
>+
>+
>+
>+
>+
>+};
Please remove the blank lines between the last const and the closing brace, they are not needed.

These are just style nits, so r+ with them fixed.

Comment 21

[cmtalbert]

Comment on attachment 246621 [details] [diff] [review]
Basic Provider Files

calGoogleCalendarModule.js:
>+    registerSelf: function cGCM_registerSelf(aComponentManager, aFileSpec, aLocation, aType) {
>+        aComponentManager = aComponentManager.QueryInterface(Components.interfaces.nsIComponentRegistrar);
Create consts of Cc and Ci (const Cc = Components.classes and const Ci = Components.interfaces) to shorten some of these lines. Also the parameters of cGCM_registerSelf should be wrapped so they don't go over 80 chars.

>+    mFactory: {
>+        createInstance: function cGCM_mFactory_createInstance(aOuter, iid) {
>+
>+            if (aOuter != null)
>+                throw Components.results.NS_ERROR_NO_AGGREGATION;
>+            var cal = new calGoogleCalendar();
>+
>+            return cal.QueryInterface(aIID);
>+        }
>+    },
It QueryInterface should use iid, not aIID. But in order to be consistent with the function above this, you may want to change the parameter to aIID.

>+function getSessionCredentials(aUsername, aPassword, aSavePassword) {
>+ ..snip..
>+    if(prompter.promptPassword(
>+       getFormattedString("google","loginDialogTitle"),
getFormattedString will be looking for a google.properties file in the standard locale chrome. 
However, google.properties is not part of the patch. So, we need an updated patch with this file in place. Don't forget you will also have to edit the proper jar.mn to pick it up too.

>+function getCalendarCredentials(aCalendarName, aUsername, aPassword, aSavePassword) {
>+ ..snip..    
>+    if(prompter.promptUsernameAndPassword(
>+       getFormattedString("google","loginDialogTitle"),
getCalendarCredentials also makes use of google.properties file

By fixing or circumventing both of the above errors, I was able to create a google calendar in Lightning. However, I could not log into the google calendar. I used my proper user name and password (password blanked out below). I'm not sure what I may have done wrong, I was following your instructions as closely as I could. Here is a log of the login error
Log of Login Error:
   Getting item feed
   Adding item http://www.google.com/calendar/feeds/4n85a57j523kdr26rovd0i1evs%40group.calendar.google.com/private/full to queue
   Logging in to [e-mail]
   Logging object...
   action: Setting Upload Data:
   content: application/x-www-form-urlencoded
   data: Email=cmtalbert%40gmail.com&Passwd=xxxx&source=-Thunderbird-2.0b1pre&service=cl
   End object
   
   calGoogleRequest: Requesting POST https://www.google.com/accounts/ClientLogin
   Logging object...
   action: Streamloader failed
   result: 2153185281
   message: null
   End object
   
Login failed. Status: 2153185281

Due to the missing google.properties, the login error, and the minor param error, I am going to r- this version of this patch. Please provide an updated "Basic Provider Files" patch to address these issues and I will continue the review. Please let me know I can help you debug the log in error or if I did something stupid. Either way, we need a new patch with the missing properties file, so still r-.

Comment 22

[Joey Minta]

Comment on attachment 246621 [details] [diff] [review]
Basic Provider Files

+
+    mFactory: {
+        createInstance: function cGCM_mFactory_createInstance(aOuter, iid) {
A quick drive-by comment, that I only learned by chance.  If the factory for creating an xpcom component is included in the same object as the module that registers it, the module will be leaked.  You should define kFactory outside the module, and return it in getClassObject as a normal variable.

Comment 23

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

Attachment: Makefile.in for various directories (with correct tabs)

Tabs corrected.

Comment 24

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

Attachment: IDL Interface definitions

Redundant blank lines removed,
comments fixed.

I decided against adding the comment about GOOGLE_ERROR_BASE. Even if one dosen't know idl's syntax, due to the include line the next place he will look is calIErrors.idl.

Comment 25

[cmtalbert]

Comment on attachment 246619 [details] [diff] [review]
Patch for Basic Google Provider UI elements


> function initLocationPage()
> {
>     var bShowWcap = false;
>+    var bShowGoogle = false;
>     try {
>         bShowWcap = getPrefSafe("calendar.prototypes.wcap", false);
>+        bShowGoogle = getPrefSafe("calendar.google.enabled",false);
>     }
>     catch (exc) {} // calendar.prototypes.wcap currently not present in Sunbird
>     bShowWcap = (bShowWcap || getPrefSafe("calendar.wcap.enabled", false));
>     document.getElementById("wcap-radio").setAttribute(
>         "collapsed", bShowWcap ? "false" : "true");
>+    document.getElementById("google-radio").setAttribute(
>+        "collapsed", bShowGoogle ? "false" : "true");
>     checkRequired();
> }

Please put a blank line between the variables and the try statement, just to keep this block from 
appearing so dense.

That's a very minor nit, this patch looks good, so r+ with that.

Comment 26

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

Attachment: Adds properties file for locale-specific strings

Comment 27

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

Attachment: Basic UI Elements

newline added

Comment 28

[Matthew (lilmatt) Willis]

Comment on attachment 247084 [details] [diff] [review]
Makefile.in for various directories (with correct tabs)

>Index: calendar/providers/Makefile.in
>===================================================================

>-DIRS = memory storage composite caldav ics wcap
>+DIRS = memory storage composite caldav ics wcap google
At this point, I'd prefer this to be switched to one dir per line so cvs blame makes more sense moving forward.

It should be like so (without tabs):

DIRS = \
    memory \
    storage \
    composite \
    caldav \
    ics \
    wcap \
    google \
    $(NULL)


>+++ calendar/providers/google/Makefile.in	2006-11-30 20:05:46.270859200 +0100
>@@ -0,0 +1,72 @@
>+# 
No empty first line please

>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public
>+# License Version 1.1 (the "License"); you may not use this file
>+# except in compliance with the License. You may obtain a copy of
>+# the License at http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS
>+# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
>+# implied. See the License for the specific language governing
>+# rights and limitations under the License.
>+#
>+# The Original Code is mozilla.org code.
The Original Code is the Google Calendar provider.
>+#
>+# The Initial Developer of the Original Code is
>+# Philipp Kewisch <[e-mail]>
Indent your name 2 spaces please.

>+# Portions created by the Initial Developer are Copyright (C) 2006
>+# the Inital Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#
>+#
Only one blank line here.

>+# Alternatively, the contents of this file may be used under the terms of
>+# either the GNU General Public License Version 2 or later (the "GPL"), or
>+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+# in which case the provisions of the GPL or the LGPL are applicable instead
>+# of those above. If you wish to allow use of your version of this file only
>+# under the terms of either the GPL or the LGPL, and not to allow others to
>+# use your version of this file under the terms of the MPL, indicate your
>+# decision by deleting the provisions above and replace them with the notice
>+# and other provisions required by the GPL or the LGPL. If you do not delete
>+# the provisions above, a recipient may use your version of this file under
>+# the terms of any one of the MPL, the GPL or the LGPL.
>+#
>+# ***** END LICENSE BLOCK *****
The license block is wrapped in a non-standard fashion.  Be sure to use the boilerplate from http://www.mozilla.org/MPL/boilerplate-1.1/  rather than just copy/pasting this from another file.

>+
>+DEPTH           = ../../..
>+topsrcdir       = @top_srcdir@
>+srcdir          = @srcdir@
>+VPATH           = @srcdir@
Align the = beneath the L in LICENSE BLOCK.

>+
>+include $(DEPTH)/config/autoconf.mk
>+
>+DIRS            = public
>+MODULE          = google
These don't need to align with the block above. Just put one space before and after the =.

>+
>+EXTRA_SCRIPTS = \
>+                calGoogleCalendar.js \
>+                calGoogleSession.js \
>+                calGoogleRequest.js \
>+                calGoogleUtils.js \
>+                calGoogleErrors.js \
>+                $(NULL)
Only indented these 4 spaces.

>+
>+EXTRA_PP_COMPONENTS = \
>+                calGoogleCalendarModule.js \
>+                $(NULL)
Only indented these 4 spaces as well.
Use http://lxr.mozilla.org/mozilla/source/calendar/prototypes/wcap/lightning-wcap/Makefile.in  as an example.

>+++ calendar/providers/google/public/Makefile.in	2006-11-25 22:02:47.073232000 +0100
>@@ -0,0 +1,53 @@
>+# 
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public
>+# License Version 1.1 (the "License"); you may not use this file
>+# except in compliance with the License. You may obtain a copy of
>+# the License at http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS
>+# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
>+# implied. See the License for the specific language governing
>+# rights and limitations under the License.
>+#
>+# The Original Code is mozilla.org code.
>+#
>+# The Initial Developer of the Original Code is
>+# Philipp Kewisch <[e-mail]>
>+# Portions created by the Initial Developer are Copyright (C) 2006
>+# the Inital Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#
>+#
>+# Alternatively, the contents of this file may be used under the terms of
>+# either the GNU General Public License Version 2 or later (the "GPL"), or
>+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+# in which case the provisions of the GPL or the LGPL are applicable instead
>+# of those above. If you wish to allow use of your version of this file only
>+# under the terms of either the GPL or the LGPL, and not to allow others to
>+# use your version of this file under the terms of the MPL, indicate your
>+# decision by deleting the provisions above and replace them with the notice
>+# and other provisions required by the GPL or the LGPL. If you do not delete
>+# the provisions above, a recipient may use your version of this file under
>+# the terms of any one of the MPL, the GPL or the LGPL.
>+#
>+# ***** END LICENSE BLOCK *****
Same license block comments here as before.

>+
>+DEPTH           = ../../../..
>+topsrcdir       = @top_srcdir@
>+srcdir          = @srcdir@
>+VPATH           = @srcdir@
Same spacing comments here as before. Align beneath L in LICENSE BLOCK.

>+
>+include $(DEPTH)/config/autoconf.mk
>+
>+MODULE          = google
>+XPIDL_MODULE    = google
Same spacing comments here as before. One space either side of =.

>+
>+XPIDLSRCS       = calIGoogleErrors.idl \
>+                  calIGoogleCalendar.idl \
>+                  $(NULL)
Move the calIGoogleErrors.idl to the next line down, indented 4 spaces, like so:
XPIDLSRCS = \
    calIGoogleErrors.idl \
    calIGoogleCalendar.idl \
    $(NULL)


These are all stylistic nits. r=lilmatt with them fixed.

Comment 29

[cmtalbert]

Comment on attachment 246621 [details] [diff] [review]
Basic Provider Files

*** The remainder of my first review comments on the "Basic Provider" patch ***

In general, this is a very readable code. It is well documented and clearly written. Many of my comments are stylistic. Please put spaces after commas in parameter lists, wrap long parameter lists with one parameter per line. Eventually, I quit commenting about the line wrapping, even though it still needs to be done. Please go through the patch and look at each file w.r.t. that.

I mention several places for follow-on bugs that are for functionality that this google provider doesn't provide. In reality all of the "google provider needs to handle x" follow-on bugs can be one bug, unless it makes sense to you to separate those out.

** calGoogleCalendar.js **
>+    set readOnly(v) { 
There is a space after this line, please remove it

>+    set googleCalendarName(v) { 
Here too

if(getCalendarCredentials(this.googleCalendarName,oUsername,oPassword,oSavePassword)) {
Wrap the parameters of this function so that they are less than 80 chars

>+            throw googleException(Components.interfaces.calIGoogleErrors.GOOGLE_INVALID_URI,aUri);
Use Ci constant definition to shorten this line

>+    adoptItem: function cGC_adoptItem(aItem, aListener) {
>+        LOG("adding Item " + aItem.title);
>+
>+        try {
>+            // Check if calendar is readonly
>+            // XXX Is this really needed? I saw it on some other provider, but
>+            // theoretically, addItem should not be called if the calendar is
>+            // readonly. this also applies for delete/modify
>+            if (this.readOnly) {
>+                throw googleException(Components.interfaces.calIErrors.CAL_IS_READONLY);
You can use Ci here too.
I think that this is necessary, because if this is someone else's google calendar, and that
person changes the state of it from read/write to read only, the only way the view will find 
out about that is if it attempts the addItem and receives an error.

>+        throw googleException(Components.interfaces.calIGoogleErrors.GOOGLE_LOGIN_CANCELED);
and here too

>+        } catch(e) {
>+            // XXX I suggest throwing an error here should call onOperationComplete
>+            // automatically, since some uncaught error has occurred.
>+            // this would keep the code cleaner here
Please file a follow on bug to update the framework to do this for you then. And change
"XXX" to Bug <bugnumber>"

>+            
>+            LOG("adoptItem failed before request (" + aItem.title + "): " +
>+                e.message);
>+            if(aListener != null) {
>+                aListener.onOperationComplete(this,
>+                        e.result,
>+                        Components.interfaces.calIOperationListener.ADD,
>+                        null, e.message);
>+            }
Align these parameters with the t in this. Use Ci for the Components.interfaces and it will go 
over 80 chars but not by that much. It's more readable to go over and have the params aligned.

>+
>+    addItem: function cGC_addItem(aItem, aListener) {
>+        // Google assigns an ID to every event added. Any id set here or in
>+        // adoptItem will be overridden.
>+        return this.adoptItem( aItem.clone(), aListener );
>+    },
>+
>+    modifyItem: function cGC_modifyItem(aNewItem, aOldItem, aListener) {
>+        LOG("Modify Item " + aOldItem.title);
>+
>+        try {
>+            if (this.readOnly) {
>+                throw googleException(Components.interfaces.calIErrors.CAL_IS_READONLY);
Same comment here about Ci...

>+            
>+            // Check if we have a session. If not, then the user has canceled
>+            // the login prompt.
>+            if(!this.mSession) {
>+                throw googleException(Components.interfaces.calIGoogleErrors.GOOGLE_LOGIN_CANCELED);
and here too...

>+                // XXX does the new item need to be cloned or not?
>+                var newitem = aNewItem.clone();
>+                newitem.makeImmutable();
lilmatt or jminta: opinon on this ^^^^

>+                if(aListener != null) {
>+                    aListener.onOperationComplete(this,Components.results.NS_OK,
>+                        Components.interfaces.calIOperationListener.MODIFY,
>+                        newitem.id,newitem)
Again, please align these and use Ci


>+            this.mSession.modifyItem(this,aNewItem,this.modifyItem_response,extradata)
please wrap these parameters

>+
>+        } catch(e) {
>+            LOG("modifyItem failed before request (" + aNewItem.title + "): " +
>+                e.message);
>+            if(aListener != null) {
>+                aListener.onOperationComplete(this,
>+                        e.result,
>+                        Components.interfaces.calIOperationListener.MODIFY,
>+                        null, e.message);
Ci, alignment...

>+            // We need the item in the response, since google dosen't return any
>+            // item XML data on delete, and we need to call the observers.
>+            // XXX does the item really need to be cloned here?
>+            var extradata = { listener: aListener, item: aItem.clone() };
>+            extradata.item.makeImmutable();
jminta, lilmatt: your opinions here. It seems to me that we should only clone the item if it was
not mutable to start with. So if (!newItem.isMutable) newItem = newItem.clone(); But, let's 
get the final word from those guys.

>+            if(aListener != null) {
>+                aListener.onOperationComplete(this,    e.result,
>+                        Components.interfaces.calIOperationListener.DELETE,
>+                        null, e.message);
alignment, Ci

>+    getItem: function cGC_getItem(aId, aListener) {
>+        // XXX note I have never had a case where this function was called. I
>+        // implemented it to my best knowledge, but it could have errors.
Please file a follow-on bug with the Joey's test tool that we need a test implmented
for this functionality then. That way we can ensure it works in case future versions
use this call.

>+    getItems: function cGC_getItems(aItemFilter, aCount, aRangeStart, aRangeEnd, aListener) {
Both the function header and several places inside this function need wrapping.
Be sure to use the Ci const where appropriate.

>+    refresh: function cGC_refresh() { },
This should return Components.results.NS_ERROR_NOT_IMPLEMENTED, since refresh
is not currently supported.

>+    addItem_response: function cGC_addItem_response(aRequest, aStatus, aResult) {
Wrapping issues in this function.

>+    modifyItem_response: function cGC_modifyItem_response(aRequest, aStatus, aResult) {
Wrapping issues here too

>+
>+    /**
>+     * deleteItem_response 
>+     * Response callback, called by the session object when an Item was deleted
>+     *
>+     * @param aRequest  The request object that initiated the request
>+     * @param aStatus   The response code. This is a Components.results.* Code
>+     * @param aResult   In case of an error, this is the error string, otherwise
>+     *                  this may be empty.
>+     */
>+    deleteItem_response: function cGC_deleteItem_response(aRequest, aStatus, aResult) {
Wrapping and Ci issues in this function

>+    getItem_response: function cGC_getItem_response(aRequest, aStatus, aResult) {
>+        // our general response does it all for us. I hope.
>+        this.general_response(Components.interfaces.calIOperationListener.GET,
>+            aResult,aStatus,aRequest.extraData.listener);
These parameters should be wrapped, and aligned with the C of the Ci: like so:
this.general_response(Ci.calIOperationListener.GET,
                      aResult,
                      aStatus,
                      aRequest.extraData.listener);

>+    getItems_response: function cGC_getItems_response(aRequest, aStatus, aResult) {
..snip..
googleException(Components.interfaces.calIGoogleErrors.GOOGLE_XML_MISSING_FEED);
Wrap!

googleException(Components.interfaces.calIGoogleErrors.GOOGLE_XML_MISSING_CHILDREN);
here too

>+                        if(item)
>+                        {
>+                            var itemReturnOccurrences = ((aRequest.extraData.itemfilter & Components.interfaces.calICalendar.ITEM_FILTER_CLASS_OCCURRENCES) != 0);
Wow! Please wrap this line

>+                            if( (itemReturnOccurrences && itemIsOccurrence) || !itemIsOccurrence) {
Wrap and no blank line after the if

>+                                aRequest.extraData.listener.onGetResult(this,Components.results.NS_OK,
>+                                                        Components.interfaces.calIEvent,null,1,[item]);
Remember, spaces between commas and params (param1, param2, param3, ...), and this should be wrapped too. 

>+
>+                    case "gd:where":
>+                    case "id":
>+                    case "updated":
>+                    case "title":
>+                    case "subtitle":
>+
>+                    case "generator":
>+                    case "openSearch:itemsPerPage":
>+                    case "openSearch:startIndex":
>+                        // known but unparsed/unimportant elements
>+                    break;
Why are these unimportant? It would seem that id is important, as well as where, assuming that where==location.

>+
>+            // operations that have multiple elements
>+            if(!canPost) {
>+                // There was no
>+                // <link rel="http://schemas.google.com/g/2005#post">
>+                // Element, set calendar readonly.
>+                // XXX do we have to notify the user?
>+                // XXX if this feed is readonly, we need to force readOnly
>+                //     to always be set, until the feed is not readonly?
>+                this.readOnly = true;
>+            }
Ideally, there should be some kind of call you can make to alert the views that a given
calendar has shifted to readonly. Please open a follow on bug for this functionality.

>+    general_response: function cGC_general_response(aOperation, aItemString, aStatus, aListener) {
Several wrapping issues here.

>--- /dev/null	2006-11-26 15:45:53.178228800 +0100
>+++ calendar/providers/google/calGoogleCalendarModule.js	2006-11-26 14:56:55.794478400 >+// This global keeps the session Objects for the usernames
>+var g_sessionMap;
>+
>+
>+var calGoogleCalendarModule = {
>+
>+        const scripts = [ "calGoogleSession.js" , "calGoogleCalendar.js",
>+                          "calGoogleRequest.js", "calGoogleUtils.js", 
>+                          "calGoogleErrors.js", "calUtils.js" ];
Remove the space after calGoogleSession.js" and after "calGoogleUtils.js", and after "calUtils.js"
>+    registerSelf: function cGCM_registerSelf(aComponentManager, aFileSpec, aLocation, aType) {
wrap these params

>+        aComponentManager = aComponentManager.QueryInterface(Components.interfaces.nsIComponentRegistrar);
Ci will help here.

>+    mFactory: {
>+        createInstance: function cGCM_mFactory_createInstance(aOuter, iid) {
>+
>+            if (aOuter != null)
>+                throw Components.results.NS_ERROR_NO_AGGREGATION;
>+            var cal = new calGoogleCalendar();
>+
>+            return cal.QueryInterface(aIID);
>+        }
>+    },
aIID vs iid issue, mentioned previously

>--- /dev/null	2006-11-26 15:46:01.740540800 +0100
>+++ calendar/providers/google/calGoogleSession.js	2006-11-26 15:00:50.041308800 +0100
   
>+    /**
>+     * attribute savePassword
>+     *
>+     * Sets if the password for this user should be saved or not
>+     */
>+    get savePassword() { return this.mSavePassword; },
>+    set savePassword(v) { 
>+        this.mSavePassword = v;
>+        return v;
>+    },
This is a nice effect, but I think you do need a blank line between a get and set (per the style guidelines). Please insert a blank line between the getter/setter on all of these.

>+    loginAndContinue: function cGS_loginAndContinue(aRequest) {
>+
>+        if(this.mLoggingIn) {
>+            return;
>+        }
>+        try {
>+            LOG("Logging in to " + this.googleUser);
>+
>+            // XXX Some of this code may be better kept somewhere else.
>+            // XXX Especially code regarding the get*Credentials calls.
>+            // XXX Comments?
I think most of the code that is in here seems rather google specific. I'd be interested to hear what you think can be moved into a more generalized location. Perhaps you see something I don't. In general, we will need to file a follow-on bug to create a location for any generalized code, and to refactor existing code to use that location.

>+if(getCalendarCredentials(aRequest.calendar.googleCalendarName,oUsername,oPassword,oSavePassword)) 
You need spaces after params on the same line and these should probably wrapped so that each one 
has its own line.

>+                        function cGS_loginAndContinue_moveToNewSession(element,index,arr) {
This is a nice name, but it is a bit long, can you shorten it and keep a meaningful name? Perhaps cGS_lgnAndCont_mvToNewSession -- it's up to you, but if the name were shorter it would help the spacing out down below where it is called.

>+this.failQueue(Components.interfaces.calIGoogleErrors.GOOGLE_LOGIN_CANCELED,aRequest.calendar);
Wrap!

>+// Start logging in
Right above this comment is an extra line

>+// Get Version info
Right above this comment is an extra line of spaces

>+request.addQueryParameter("max-results",aCount ? aCount : 2147483647);
Shouldn't that number (INT_MAX) be a const? It could change on different systems, for instance.
Use this: /js/src/liveconnect/jsj_convert.c, line 339 -- #define jint_MAX_VALUE 2147483647.0

>--- /dev/null	2006-11-26 15:46:12.506020800 +0100
>+++ calendar/providers/google/calGoogleRequest.js	2006-11-26 14:46:38.026171200 +0100
>+    get type() { return this.mType; },
>+    set type(v) {
>+        this.mType = (v<1 || v>5 ? 0 : v);
This obviously works, but it's completely unreadable. Please insert some parenthesis.

>+    get method() { return this.mMethod; },
>+    set method(v) { this.mMethod = v; return v; },
Blank line between getter/setter please

>+    /**
>+     * attribute extraData
Remove one of the blank lines above this comment.

>+    fail: function cGR_fail(aCode, aMessage) {
>+        if(!aMessage) {
>+            aMessage = getGoogleExceptionMessage(aCode);
>+        }
>+        this.mResponseListenerFunc.call(this.mResponseListenerObject,this,aCode,aMessage);
Are we certain that the caller always sets up this.mResponseListenerFunc? I don't see
that it is required in the constructor. You might want a try catch around this unless this
is only called by the internal functions of the Google Provider (i.e. no one can grab this 
interface and start sending items up to google on their on).
PS spaces after commas and wrap those long param lists.

>+    succeed: function cGR_succeed(aResult) {
>+        // Succeeding is nothing more than failing with the result code set to
>+        // NS_OK.
lol isn't that true of life?

>+    prepareChannel: function cGR_prepareChannel(aChannel) {
>+ ..snip..
>+            var inputStream = Components.classes["@mozilla.org/io/string-input-stream;1"]
>+                              .createInstance(Components.interfaces.nsIStringInputStream);
Nice wrapping, Cc and Ci will help too

>+    onStreamComplete: function cGR_onStreamComplete(aLoader, aContext, aStatus, aResultLength, aResult) {
>+
>+        var httpChannel = aLoader.request.QueryInterface(Components.interfaces.nsIHttpChannel);
>+        var uploadChannel = aLoader.request.QueryInterface(Components.interfaces.nsIUploadChannel);
>+
>+
>+        // Handle all (documented) error codes
Remove the two blank lines above here. And wrap those long lines (Ci will help)

>+            case 409: /* Specified version number doesn't match resource's
>+                         latest version number. */
>+
>+                // 409 Conflict. The client should get a newer version of the event
>+                // and edit that one.
>+
>+                // XXX I haven't quite looked into this yet.
>+
>+            default:
>+                /* The following codes are caught here:
>+                    400 BAD REQUEST: Invalid request URI or header, or
>+                                     unsupported nonstandard parameter.
>+                    404 NOT FOUND: Resource (such as a feed or entry) not found.
>+                    500 INTERNAL SERVER ERROR: Internal error. This is the
>+                                               default code that is used for
>+                                               all unrecognized errors.
>+                 */
>+
>+                // Something else went wrong
>+                var error = "A request Error Occurred. Status Code: " +
>+                    httpChannel.responseStatus + " " +
>+                    httpChannel.responseStatusText;
>+
>+               this.fail(Components.results.NS_ERROR_FAILURE,error);
Is it your intention here to fall through to the "default" case from case 409 (there is no break)?
Please also file a follow on bug to look into case 409 and replace // XXX with // Bug <number>

>+++ calendar/providers/google/calGoogleErrors.js	2006-11-26 14:44:11.395326400 +0100
>+
>+function getGoogleExceptionMessage(aCode) {
>+
>+    const kGE = Components.interfaces.calIGoogleErrors;
yes - this is nice!!!!

>+
>+    var message;
>+    switch(aCode) {
>+        /*
>+         * Errors from calIGoogleErrors
>+         */
>+        case  kGE.GOOGLE_NOT_SUPPORTED:
>+            message = "This feature is not supported by Google.";
Woah! If these are going to be displayed to the user, then they need to go into a properties file. And you should load them from a properties file. 

>+
>+
>+        /*
>+         * Errors from calIErrors
>+         */
Two blank lines above this comment remove the one with the extra spaces in it.

>+        case kGE.CAL_IS_READONLY:
>+            message = "Calendar is read-only.";
>+        break;
>+        default:
>+            message = "An unknown error occurred";
>+        break;
>+    }
>+    return message;
>+}
>+

>+++ calendar/providers/google/calGoogleUtils.js	2006-11-26 20:12:39.153691200 +0100

>+function getSessionCredentials(aUsername, aPassword, aSavePassword) {
>+    
>+    // XXX need to add throws if args are not objects
>+    
>+    var oUsername = { value:aUsername.value };
>+    var oPassword = { value:aPassword.value };
>+    var oCheck = { value: aSavePassword.value };
Please add those throws before resubmitting the patch for review.

>+
>+    if(prompter.promptPassword(
>+       getFormattedString("google","loginDialogTitle"),
>+       getFormattedString("google","loginDialogText",[aUsername]),
>+       oPassword,
>+       getFormattedString("google","loginDialogCheckText"),
>+       oCheck)) {
>+        aPassword.value = oPassword.value;
>+        aSavePassword.value = oCheck.value;
>+        return true;
>+    } 
This if statement has real issues with alignment and spaces in parameter lists. Please fix it.

>+function getCalendarCredentials(aCalendarName, aUsername, aPassword, aSavePassword) {
>+
>+    // XXX need to add throws if args are not objects
>+
>+    var oUsername = {value:aUsername.value};
>+    var oPassword = {value:null};
>+    var oCheck = { value: aSavePassword };
Again please add throws or at least add a non-catastrophic exit from this function if the parameters are not objects.
>+    
>+    var watcher = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>+        .getService(Components.interfaces.nsIWindowWatcher);
>+    var prompter = watcher.getNewPrompter(null);
>+    
>+    if(prompter.promptUsernameAndPassword(
>+       getFormattedString("google","loginDialogTitle"),
>+       getFormattedString("google","loginDialogText",[aCalendarName]),
>+       oUsername,oPassword,
>+       getFormattedString("google","loginDialogCheckText"),
>+       oCheck)) {
>+        aUsername.value = oUsername.value;
>+        aPassword.value = oPassword.value;
>+        aSavePassword.value = oCheck.value;
>+        return true;
>+    }
Yikes, that if statement again!

On a more general note, why can't getSessionCredentials and getCalendarCredentials be the same function? It looks like a lot of duplicated code.

>+function getMozillaTimezone(aICALTimezone) {
>+
>+    if(aICALTimezone == "UTC" || aICALTimezone == "floating") {
>+        return aICALTimezone;
>+    }
>+
>+    // XXX I am waiting for a timezone patch to handle the /mozilla.org/<date>/
>+    // prefix for now we need to go through all timezones and see which timezone
>+    // ends with aICALTimezone
Please include bug number here, and if the bug doesn't exist, please file it.

>+function fromRFC3339(aStr, aTimezone) {
>+
>+    // XXX I have not covered leapseconds (matches[8]), this might need to
>+    // be done. The only reference to leap seconds I found is bug 227329.
>+
>+    // Create a DateTime instance (calUtils.js)
>+    var datetime = createDateTime();
>+
>+    // Killer regex to parse RFC3339 dates
>+    var re = new RegExp("^([0-9]{4})-([0-9]{2})-([0-9]{2})" +
>+        "([Tt]([0-9]{2}):([0-9]{2}):([0-9]{2})(\.[0-9]+)?)?" +
>+        "(([Zz]|([+-])([0-9]{2}):([0-9]{2})))?");
Wow

>+        if(datetime.timezoneOffset != offset_in_s) {
>+            // XXX I have no better solution as of now. Feel free to discuss
This is where the timezone database comes in. I wonder how often we attempt to guess timezones like this in the codebase. This is at least the second time we attempt to do this. There is some other timezone guessing code in http://lxr.mozilla.org/seamonkey/source/calendar/resources/content/calendarUtils.js#319

Please file a follow on bug to rationalize this. That way when we switch to the timezone database, we only have to change code in one place.

>+        } else if(aDateTime.timezone == "floating") {
>+            // RFC3339 �4.3 Unknown Local Offset Convention
An odd character in that comment. Please remove

>+function ItemToXMLEntry(aItem, aAuthorName, aAuthorEmail) {
In here add a comment with a bug number of a follow on bug to include the remaining attributes: attendees, alarms etc.

Also, how do you handle recurring events?

>+/**
>+ * relevantFieldsMatch
>+ * Tests if all google supported fields match
>+ *
>+ * @param a The reference item
>+ * @param b The comparing item
>+ * @return  true if all relevant fields match, otherwise false
>+ */
>+function relevantFieldsMatch(a, b) {
>+
In this function, you need to add SEQUENCE and RECURRENCE-ID to your propery list to check. iTIP (RFC 2446) holds that an event is uniquely defined by UID then SEQUENCE then Recurerence ID. I believe that both SEQUENCE and RECURRENCE-IDs are members of the property bags in events, and of course RECURRENCE-ID does not exist for non-recurring items.

>+function XMLEntryToItem(aXMLEntry, aTimezone) {
..snip..
                    
>+                    if((!item.startDate.isValid && startDate) || (item.startDate.isValid && item.startDate.compare(startDate) > 0)) {
>+                        item.startDate = startDate;
>+                        item.endDate = endDate;
>+                    } else {
>+                        // We only need the chronologically first event
>+                        break;
>+                    }
What is the significance of this if statement? What do you mean by "we only need chronologically first event?" I'm just confused by this code block.

>+                case "gd:recurrence":
>+                    // child.firstChild.nodeValue contains ICAL recurrence information
>+                    
>+                    var recurrenceInfo = child.firstChild.nodeValue || "";
>+                    var lines = recurrenceInfo.split("\n");
>+
>+                    // Some items don't contain gd:when elements. Those have
>+                    // gd:reccurrence items, which contains some start date
>+                    // info. 
>+                    // XXX This code is somewhat preliminary
Can you define what types of recurrences work, and which don't, then write a bug concerning those that don't? also, if you think that this code is going to get much more involved or complicated, it would be good to factor it out of this function. This switch statement is already pretty gigantic. It might be a good idea to go ahead and factor some of the bodies of these case items to their own function calls or to several small functions, whatever makes sense.

>+                case "gd:visibility":
>+                    // XXX What to do with privacy "default"?
Currently, our code defaults to everything is "Public". Google defaults to everything is "Private". I tend to think that most people would prefer the "Private" setting for a default. I recommend taking that discussion to the newsgroup -- that's a much larger issue than this one line of code.

>+                case "gd:recurrenceException":
>+                    // TODO this will be patched on later
>+                break;
>+                case "gd:comments":
>+                    // TODO this will be patched on later
>+                break;
>+                case "gd:who":
>+                    // TODO this will be patched on later
>+                break;
Include these in your "next things to do on google" follow-on bug 


>+function LOGitem(item) {
>+
>+    if(item)
>+        LOG("Logging calIEvent:" +  
>+            "\n\tid:" + item.id +
>+            "\n\tediturl:" + item.getProperty("X-GOOGLE-EDITURL") +
>+            "\n\tcreated:" + item.getProperty("CREATED") +
>+            "\n\tupdated:" + item.getProperty("LAST-MODIFIED") +
>+            "\n\ttitle:" + item.title +
>+            "\n\tcontent:" + item.getProperty("DESCRIPTION") +
>+            "\n\ttransparency:" + item.getProperty("TRANSP") +
>+            "\n\tstatus:" + item.status +
>+            "\n\tstartTime:" + item.startDate.toString() +
>+            "\n\tendTime:" + item.endDate.toString() +
>+            "\n\tlocation:" + item.getProperty("LOCATION") +
>+            "\n\tprivacy:" + item.privacy +
>+            "\n\tisOccurrence:" + item.getProperty("x-MOZ-ITEM-IS-OCCURRENCE"));
>+            
>+}
Blank line between closing brace and last line of if statement

Comment 30

[Joey Minta]

In reply to Comment #29:
>+                // XXX does the new item need to be cloned or not?
>+                var newitem = aNewItem.clone();
>+                newitem.makeImmutable();
lilmatt or jminta: opinon on this ^^^^

The IDL indicates that it is possible to pass in both mutable and immutable items here.  The deciding factor, however, is that you're not modifying any properties of the new item, so a clone() should not be necessary at all.

>+            // We need the item in the response, since google dosen't return any
>+            // item XML data on delete, and we need to call the observers.
>+            // XXX does the item really need to be cloned here?
>+            var extradata = { listener: aListener, item: aItem.clone() };
>+            extradata.item.makeImmutable();
jminta, lilmatt: your opinions here.

This is another case where you're clone()ing and then immediately making the item immutable.  These are basically inverse calls.  You should only clone an item under when (a) you want to modify some of its properties AND (b) the item is immutable.

Comment 31

[Joey Minta]

Comment on attachment 247089 [details] [diff] [review]
IDL Interface definitions

It seems to me that there are existing error codes that convey approximately the same data you want to convey with these errors.

+    const unsigned long GOOGLE_NOT_SUPPORTED = GOOGLE_ERROR_BASE + 1;
NS_ERROR_NOT_IMPLEMENTED?

+    const unsigned long GOOGLE_INVALID_URI = GOOGLE_ERROR_BASE + 2;
NS_ERROR_MALFORMED_URI?

+    const unsigned long GOOGLE_XML_INVALID = GOOGLE_ERROR_BASE + 5;
NS_ERROR_DOM_SYNTAX_ERR

+    const unsigned long GOOGLE_XML_MISSING_FEED = GOOGLE_ERROR_BASE + 6;
NS_ERROR_DOM_HIERARCHY_REQUEST_ERR?

+    const unsigned long GOOGLE_GET_ITEM_UNKNOWN = GOOGLE_ERROR_BASE + 9;
NS_ERROR_UNEXPECTED? NS_ERROR_INVALID_ARG?

+    const unsigned long GOOGLE_ITEM_INVALID = GOOGLE_ERROR_BASE + 10;
NS_ERROR_INVALID_ARG?

At the very least, you should add some comments describing these errors a bit more.  For instance, the differences between the 3 XML_MISSING errors is difficult to understand simply from the names.

Comment 32

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

(In reply to comment #31)
> +    const unsigned long GOOGLE_GET_ITEM_UNKNOWN = GOOGLE_ERROR_BASE + 9;
> NS_ERROR_UNEXPECTED? NS_ERROR_INVALID_ARG?
> 
> +    const unsigned long GOOGLE_ITEM_INVALID = GOOGLE_ERROR_BASE + 10;
> NS_ERROR_INVALID_ARG?

NS_ERROR_INVALID_ARG is for invalide arguments to c++ functions (like a null pointer). It's not applicable here.

Comment 33

[Joey Minta]

(In reply to comment #32)
> NS_ERROR_INVALID_ARG is for invalide arguments to c++ functions (like a null
> pointer). It's not applicable here.
> 
It's also used for js arguments.  For instance, http://lxr.mozilla.org/mozilla/source/browser/components/search/nsSearchService.js#262

Comment 34

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

Attachment: Makefile.in for various directories

* tabs corrected
* various indents corrected
* licence boilerplate corrected

Comment 35

[cmtalbert]

Comment on attachment 247091 [details] [diff] [review]
Adds properties file for locale-specific strings

Looks good, just some minor notes...

> calendar-@AB_CD@.jar:
> % locale calendar @AB_CD@ %locale/@AB_CD@/calendar/
> *   locale/@AB_CD@/calendar/aboutDialog.dtd      (%chrome/calendar/aboutDialog.dtd)
>     locale/@AB_CD@/calendar/calendar.dtd         (%chrome/calendar/calendar.dtd)
>     locale/@AB_CD@/calendar/calendarCreation.dtd (%chrome/calendar/calendarCreation.dtd)
>     locale/@AB_CD@/calendar/calendar.properties  (%chrome/calendar/calendar.properties)
>     locale/@AB_CD@/calendar/wcap.properties      (%chrome/calendar/wcap.properties)
>+    locale/@AB_CD@/calendar/google.properties      (%chrome/calendar/google.properties)
Be sure that your chrome location string is aligned at column 50 in jar.mn files when possible. Here, you should align it with the wcap string above. 

>+++ calendar/locales/en-US/chrome/calendar/google.properties	2006-11-30 21:40:41.840686400 +0100
The license block in google.properties looks weird. It might be incorrectly wrapped again. Also, I don't think that this is original mozilla.org code, unless you just got hired. Include the standard "original author" copyright statement in the boilerplate. This part:
# The Initial Developer of the Original Code is
# ____________________________________________.
# Portions created by the Initial Developer are Copyright (C) 2___
# the Initial Developer. All Rights Reserved.

These are just alignment and comment changes. You have my r+ with those changes.

Comment 36

[Matthew (lilmatt) Willis]

Comment on attachment 247091 [details] [diff] [review]
Adds properties file for locale-specific strings

You'll need to add your .properties file to lightning's jar.mn as well.

Comment 37

[Matthew (lilmatt) Willis]

Comment on attachment 247174 [details] [diff] [review]
Makefile.in for various directories

+EXTRA_SCRIPTS = \
+    calGoogleCalendar.js \
+    calGoogleSession.js \
+    calGoogleRequest.js \
+    calGoogleUtils.js \
+    calGoogleErrors.js \
+    $(NULL)
+
+EXTRA_PP_COMPONENTS = \
+    calGoogleCalendarModule.js \

Remember to add these files to /m/cal/installer/windows/packages-static so that they are included in the Windows builds.  We often forget that step when adding new files to the build that aren't in some .jar file.

EXTRA_SCRIPTS files will be inside /js while EXTRA_PP_COMPONENTS files will be inside /components

Comment 38

[Matthew (lilmatt) Willis]

Comment on attachment 247089 [details] [diff] [review]
IDL Interface definitions

Moving this review to mvl.

Drive-by:
Be sure to add any resulting .xpt files to /m/cal/installer/windows/packages-static if necessary.

Comment 39

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

Attachment: Makefile.in for various directories

Corrected License (again)

Comment 40

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

Attachment: Adds properties file for locale-specific strings

Correct License boilerplate
correct alignment issues

Comment 41

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

Comment on attachment 247228 [details] [diff] [review]
Adds properties file for locale-specific strings

>+loginDialogText=Please enter your credentials for "%1$S".

What is a 'credential'? in other words: just say 'username and password'.

Comment 42

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

Comment on attachment 247089 [details] [diff] [review]
IDL Interface definitions


>+interface calIGoogleCalendar : calICalendar
>+{
>+    /**
>+     * Google's Calendar Name
>+     * This represents the <calendar name> in
>+     * http://www.google.com/calendar/feeds/<calendar name>/private/full
>+     */
>+    attribute AUTF8String googleCalendarName;
>+};

Why is this interface needed at all? You only need interfaces if something outside your module need access to the function or attribute. But because the providers are supposed to be pluggable, they need to follow a strict interface. Callers outside the provider can't know which provider is being used, so can never use the additional interfaces.
So, I think you can remove this entire interface. Nothing will be able to use it anyway.


>+interface calIGoogleErrors : calIErrors

jminta already suggested a bunch of build-in alternatives. It would be better to use those, because they can be translated from number to string in the error console, etc.

Comment 43

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

For next patches, please just create one file. This whole lot of files are really hard to review. I'm having a hard time figuring out how things declared in one file are used.

Comment 44

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

Comment on attachment 240952 [details]
Basic UI added to the Add calendar dialog

ui-review=mvl

Comment 45

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

Attachment: Adds properties file for locale-specific strings

Fixed loginDialogText per mvl's comment and discussion in IRC.

Comment 46

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

Comment on attachment 247089 [details] [diff] [review]
IDL Interface definitions

All GOOGLE_* errors were moved to more general errors.
GOOGLE_LOGIN_FAILED was moved to a constant later to be defined in calGoogleCalendarModule.js

Comment 47

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

Attachment: Makefile.in for various directories

Removed /m/c/providers/google/public/Makefile.in since there is no interface left.

Comment 48

[Joey Minta]

Comment on attachment 247093 [details] [diff] [review]
Basic UI Elements

ugh, initPageLocation() is a mess. :-(  getPrefSafe can't throw, b isn't an accepted prefix for variables, and .setAttribute() should be on the netx line with the .'s aligned.  In the interest of consistency, though, don't fix those for your code yet.

bShowGoogle = getPrefSafe("calendar.google.enabled",false);
Still nit here: space before |false|.  However, this gets to the heart of a couple of questions regarding providers, etc that we've been putting off.

(1) Does the google calendar provider belong in prototypes?
(2) What's our policy with regards to which providers will we ship?
(3) What's the proper architecture to expose these providers to users?

r2=jminta for the code, but there are larger policy questions that need to be answered before this lands.

Comment 49

[Joey Minta]

Comment on attachment 247448 [details] [diff] [review]
Adds properties file for locale-specific strings

r2=jminta

Comment 50

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

Attachment: Basic Provider Files

Corrected issues as discussed
Addded branch check for nsIStreamListener (for Lightning)

Comment 51

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

Attachment: Adds entries to /m/c/installer/windows/packages-static

Comment 52

[cmtalbert]

Comment on attachment 247972 [details] [diff] [review]
Basic Provider Files

Overall, the patch looks very very good. Nice job!

Some very nit picky comments on alignment and follow on bugs. 

--- /dev/null	2006-12-08 16:07:01.895160000 +0100
+++ calendar/providers/google/calGoogleCalendar.js	2006-12-08 10:58:50.676092800 +0100
+    set uri(aUri) {
+        // Parse google url, catch private cookies, public calendars,
..snip..
+        if(matches) {
+            // Set internal Calendar Name
+            this.googleCalendarName = matches[2];
+
+            // Set normalized url. We need the full xml stream and private
+            // access
+            var ioService = Cc["@mozilla.org/network/io-service;1"]
+                            .getService(Ci.nsIIOService);
+
+            // We need private visibility  both composite and full projection
+            // composite is readonly, but contains comments.
+            this.mUri = ioService.newURI("http://www.google.com/calendar/feeds/" +
+                                            this.googleCalendarName +
+                                            "/private/full", null, null);
^^^ This line here has some minor alignment issues
+            this.mCompositeUri = ioService.newURI("http://www.google.com/calendar/feeds/" +
+                                            this.googleCalendarName +
+                                            "/private/composite", null, null);
^^^ This one too. Go ahead an align with the beginning of first param even though that will 
    go over 80 chars.
+
+++ calendar/providers/google/calGoogleSession.js	2006-12-08 10:58:50.886395200 +0100
+    loginAndContinue: function cGS_loginAndContinue(aRequest) {
..snip..
+            request.setUploadData("application/x-www-form-urlencoded",
+                        "Email=" + encodeURIComponent(this.googleUser) +
+                        "&Passwd=" + encodeURIComponent(this.googlePassword) +
+                        "&accountType=HOSTED_OR_GOOGLE" +
+                        "&source=" + encodeURIComponent(source) +
+                        "&service=cl");
^^^ alignment here
..snip..
+        // Request Parameters
+        request.addQueryParameter("max-results",
+                                  aCount ? aCount : 2147483647);
^^ Could you not use INT_MAX here?

--- /dev/null	2006-12-08 16:07:02.105462400 +0100
+++ calendar/providers/google/calGoogleUtils.js	2006-12-08 11:07:30.974244800 +0100
+function getMozillaTimezone(aICALTimezone) {
+
+    if(!aICALTimezone ||
+       aICALTimezone == "UTC" ||
+       aICALTimezone == "floating") {
+        return aICALTimezone;
+    }
+
+    // XXX I am waiting for a timezone patch to handle the /mozilla.org/<date>/
+    // prefix for now we need to go through all timezones and see which timezone
+    // ends with aICALTimezone
^^^ If there isn't a bug for this already, you need to file one, or there will be 
    no patch.

Comment 53

[cmtalbert]

Comment on attachment 247973 [details] [diff] [review]
Adds entries to /m/c/installer/windows/packages-static

looks ok. Do not have working nsinstall on this box. Someone else needs to build and test this.

Comment 54

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

Attachment: Makefile.in for various directories

Removed calGoogleErrors.js from a Makefile.in since it was removed.

Comment 55

[Matthew (lilmatt) Willis]

Comment on attachment 247973 [details] [diff] [review]
Adds entries to /m/c/installer/windows/packages-static

>Index: calendar/installer/windows/packages-static
>===================================================================
>+bin\components\calGoogleCalendarModule.xpt
You don't have an .idl anymore, so you don't make an .xpt component.  This line is unnecessary.

>@@ -196,16 +197,17 @@ bin\components\calendarService.js
> bin\components\calAlarmMonitor.js
> bin\components\calCompositeCalendar.js
> bin\components\calDavCalendar.js
> bin\components\calICSCalendar.js
> bin\components\calImportExportModule.js
> bin\components\calItemModule.js
> bin\components\calMemoryCalendar.js
> bin\components\calStorageCalendar.js
>+bin\components\calGoogleCalendarModule.js
> bin\components\calWcapCalendarModule.js
> bin\components\FeedProcessor.js
Please move that line up so things are in alphabetical order.

>@@ -219,16 +221,20 @@ bin\js\calListFormatter.js
> bin\js\calMonthGridPrinter.js
> bin\js\calOutlookCSVImportExport.js
> bin\js\calProtocolHandler.js
> bin\js\calRecurrenceInfo.js
> bin\js\calTodo.js
> bin\js\calUtils.js
> bin\js\calWeekPrinter.js
> bin\js\calWeekTitleService.js
>+bin\js\calGoogleCalendar.js
>+bin\js\calGoogleSession.js
>+bin\js\calGoogleRequest.js
>+bin\js\calGoogleUtils.js
> bin\js\calWcapCachedCalendar.js
> bin\js\calWcapCalendar.js
> bin\js\calWcapCalendarItems.js
> bin\js\calWcapErrors.js
> bin\js\calWcapRequest.js
> bin\js\calWcapSession.js
> bin\js\calWcapUtils.js
Same with these lines. Alphabetical order.

Comment 56

[cmtalbert]

I've got a couple more style nits that I missed in my review. This doesn't change the review status, but they do need to be fixed in order to be in the proper mozilla style.

1. Ensure there is a single spaces after if/for/for each/switch statements
Example if (blah) not if(blah)

2. Ensure that a break in a case statement is aligned with the case block, not the case tag. 
Example:
case 2: 
    var blah
    dosomestuff();
    break;
case 3:
    ...

Comment 57

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

Attachment: Basic Provider Files

Fixed style nits as requested.
Added Constant for 2147483647 and a comment on that as discussed on IRC

I really don't like the

case 1:
    foo();
    break;
case2:

syntax, I think it makes the code a bit more unreadable and a bit easier to forget break statements, but if thats what mozilla style says, here be it.

Comment 58

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

Attachment: Makefile.in for various directories

Removed calGoogleErrors.js from the Makefiles.

Comment 59

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

Comment on attachment 248079 [details] [diff] [review]
Basic Provider Files

>+// This global keeps the session Objects for the usernames
>+var g_sessionMap;

You don't use that variable in this file. No need to define it here.

>+++ calendar/providers/google/calGoogleRequest.js	2006-12-09 13:20:04.175078400 +0100
>+    set type(v) {
>+        if (v < 1 || v > 5) {
>+            throw Cr.NS_ERROR_ILLEGAL_VALUE;
>+        }

You defined constants above, and now you use the values. hardcoded.

>+        switch (this.mType) {

Better to add a default case in this switch, and throw here.

Comment 60

[Joey Minta]

Attachment: Review comments, 1st pass

These are my comments from the first pass.  I haven't had a chance to dive into the network stuff, but that should be fairly straightforward, so I don't expect many comments there.

Comment 61

[Omar Bajraszewski]

(In reply to comment #0)

>    KNOWN LIMITATIONS
>     * Google does not support Categories and URLs (correctly)

When I received a google invitation there is a link to display the event in description. Somehow it should be supported by Mozilla Calendar

Comment 62

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

Attachment: GData Extension patches - v1

As discussed this will now be an extension.

This patch modifies the build environment to allow the extension to be built in the build system. The extension files are also contained.

Note that the calendar.png icon will be added as soon as I get an OK from the Google Team to use their icon. This should happen shortly.

Comment 63

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

To make review easier, I created a couple of diff's between the last patch and this patch for the files in /m/c/providers/gdata/content/js/ and the component file.

They can be found at http://mozilla.kewis.ch/googleprovider/

Comment 64

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

Comment on attachment 252927 [details] [diff] [review]
GData Extension patches - v1

>+++ calendar/providers/gdata/components/calGoogleCalendarModule.js	2007-01-26 >+++ calendar/providers/gdata/content/js/calGoogleCalendar.js	2007-01-26 

This second file is very much not content (UI stuff), but part of the components (backend stuff). So those files really must be in the components dir. You likely want to use the same trick used in m/c/base/content to put the js files that are not components in itself into dist/bin/js. But sourcefiles must be in the components dir.

Comment 65

[Matthew (lilmatt) Willis]

Comment on attachment 252927 [details] [diff] [review]
GData Extension patches - v1

Moving this request as I'm too busy with 0.3.1 to get to this, but I want it to move forward.

Comment 66

[Daniel Boelzle [:dbo]]

first wade-thru this beast...

>+++ calendar/providers/gdata/components/calGoogleCalendarModule.js	2007-01-26 18:38:13.957263900 +0100

>+// This constant is used internally to signal a failed login to the login
>+// handler's response function.
>+const kGOOGLE_LOGIN_FAILED = 1;
>+
>+var g_classInfo = {
>+     calGoogleCalendar: {
>+        getInterfaces: function (count) {
make unanonymous; grep thru your files for more

>+        category: "cal-provider",
>+        categoryEntry: "gdata-provider",
why add a category?

>+var calGoogleCalendarModule = {
>+
>+    mUtilsLoaded: false,
>+
>+    loadUtils: function cGCM_loadUtils() {
>+        if (this.mUtilsLoaded)
>+            return;
>+
>+        const scripts = ["calGoogleCalendar.js", "calGoogleSession.js",
>+                         "calUtils.js", "calGoogleRequest.js",
>+                         "calGoogleUtils.js"];
>+
>+        var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
>+                     .getService(Components.interfaces.mozIJSSubScriptLoader);
>+
>+        // Load provider specific extra scripts

>+        var scriptUri;
remove here, declare in loop

>+        const baseUri = "chrome://gdata-provider/content/js/";
Is there any specific reason why you place your extra js files in the chrome?

>+++ calendar/providers/gdata/content/js/calGoogleCalendar.js	2007-01-26 18:40:34.111808300 +0100

>+    function calAttrHelper(aAttr) {
>+        this.getAttr = function calAttrHelper_get() {
>+            // Note that you need to declare this in here, to avoid cyclic
>+            // getService calls.
>+            var calMgr = Cc["@mozilla.org/calendar/manager;1"]
>+                         .getService(Ci.calICalendarManager);
>+            return calMgr.getCalendarPref(calObject, aAttr);
>+        };
>+        this.setAttr = function calAttrHelper_set(aValue) {
>+            var calMgr = Cc["@mozilla.org/calendar/manager;1"]
>+                         .getService(Ci.calICalendarManager);
>+            calMgr.setCalendarPref(calObject, aAttr, aValue);
>+            return aValue;
>+        };
>+    }
>+
>+    var prefAttrs = ["name", "supressAlarms"];
>+    for each (var attr in prefAttrs) {
>+        var helper = new calAttrHelper(attr);
>+        this.__defineGetter__(attr, helper.getAttr);
>+        this.__defineSetter__(attr, helper.setAttr);
>+    }
IMO a bit over-engineered for just two attributes, but if you like...

>+    get uri() { return this.mUri; },
>+
>+    set uri(aUri) {
>+        // Parse google url, catch private cookies, public calendars,
>+        // basic and full types, bogus ics file extensions, invalid hostnames
>+        var re = new RegExp("/calendar/(feeds|ical)/" +
>+                            "([^/]+)/(public|private)-?([^/]+)?/" +
>+                            "(full|basic)(.ics)?$");
>+
>+        var matches = aUri.path.match(re);
>+
>+        if (matches) {
>+            // Set internal Calendar Name
>+            this.mCalendarName = decodeURIComponent(matches[2]);
>+
>+            // Set normalized url. We need the full xml stream and private
>+            // access
>+            var ioService = Cc["@mozilla.org/network/io-service;1"]
>+                            .getService(Ci.nsIIOService);
>+
>+            // We need private visibility and full projection
>+            this.mUri = ioService.newURI("http://www.google.com" +
>+                                         "/calendar/feeds/" +
>+                                         matches[2] +
>+                                         "/private/full",
>+                                         null,
>+                                         null);
>+            this.findSession(true);
>+        } else {
>+            throw googleException(Cr.NS_ERROR_MALFORMED_URI, aUri);
>+        }
>+        return this.mUri;
>+    },
The url getter may return a modified/normalized url; I wouldn't consider this a good idea, because the calendar manager could rely on it being "as is" (as passed to you), because AFAIK it's its only identity to separate calendar instances. So I would favor to save the unnormalized one, too, for the uri getter.

>+    adoptItem: function cGC_adoptItem(aItem, aListener) {
...
>+            // Make sure the item is an event
>+            try {
>+                aItem = aItem.QueryInterface(Ci.calIEvent);
>+            } catch(e) {
>+                throw googleException(Cr.NS_ERROR_NOT_IMPLEMENTED);
>+            }
why not just let NOINTERFACE pass?

>+        } catch(e) {
>+            LOG("adoptItem failed before request " + aItem.title + "(" +
>+                aItem.id + "):\n" + e);
aItem.id is never set for adoptItem/addItem, so just leave it out.

>+    refresh: function cGC_refresh() {
>+        // We will use refresh to reprompt the session credentials. Otherwise
>+        // there is no way to reload the calendar after the login prompt has
>+        // been canceled or the network connection is down
>+        LOG("Refreshing session object");
>+        this.findSession();
>+    },
I wouldn't do that. Besides the explicit "reload remote calendars" context menu, IIRC mvl has added support for automatic (timer driven) refresh in the calendar manager. So this is more a data refresh, I wouldn't ask  for relogin. So if your data is always consistent (because uncached), then just leave this out: canRefresh=>false.

>+    deleteItem_response: function cGC_deleteItem_response(aRequest,
...
>+                aRequest.extraData.listener.onOperationComplete(this,
>+                                                                Cr.NS_OK,
>+                                                                Ci.calIOperationListener.DELETE,
>+                                                                aRequest.extraData.item.id,
>+                                                                aRequest.extraData.item.QueryInterface(Ci.calIEvent));
why the query?

>+    getItems_response: function cGC_getItems_response(aRequest,
>+                                                      aStatus,
>+                                                      aResult) {
...
>+            if (!xml.link.(@rel == 'http://schemas.google.com/g/2005#post') &&
>+               !this.mReadOnly) {
>+                // Check if it is possible to post to the feed. This requires a
>+                // <link rel="http://schemas.google.com/g/2005#post">
>+                // element. If the calendar is not already readonly, set the
>+                // flag and notify the observers about this error.
>+                this.mReadOnly = true;
>+                this.notifyObservers("onError",
>+                                     [Ci.calIErrors.CAL_IS_READONLY]);
>+            }
Even if you cannot post to the calendar, why signal an error here? Is readOnly use generally forbidden?
I would use the optimistic way: assume the calendar is writable, if it's not, writing will fail (and you can set mReadOnly=true). But reading ought hardly cause errors, at least not CAL_IS_READONLY.

>+    general_response: function cGC_general_response(aOperation,
...
>+            if (!item) {
>+                // This can happen if the
>+                throw Cr.NS_ERROR_FAILURE;
>+            }
>+                LOGitem(item);
>+                item.calendar = this;
>+                item.makeImmutable();
...
indentation

>+++ calendar/providers/gdata/content/js/calGoogleRequest.js	2007-01-26 18:40:34.181905600 +0100

>+    onStreamComplete: function cGR_onStreamComplete(aLoader,
>+                                                    aContext,
>+                                                    aStatus,
>+                                                    aResultLength,
>+                                                    aResult) {
...
>+                // google always returns utf8 data
>+                unicodeConverter.charset = "UTF-8";
>+                var str;
>+                try {
>+                    str = unicodeConverter.convertFromByteArray(aResult,
>+                                                                aResultLength);
why not use a unichar-stream-loader?

>+++ calendar/providers/gdata/content/js/calGoogleSession.js	2007-01-26 18:40:34.241989000 +0100
>+    if (passwordManagerGet(aUsername, password)) {
>+        this.mGooglePass = password.value;
>+        this.savePassword = true;
why save the password if it's still saved?

>+calGoogleSession.prototype = {
>+    getItems: function cGS_getItems(aCalendar,
>+                                    aCount,
>+                                    aRangeStart,
>+                                    aRangeEnd,
>+                                    aItemReturnOccurrences,
>+                                    aResponseListener,
>+                                    aExtraData) {
>+
>+        var rfcRangeStart = (aRangeStart ? toRFC3339(aRangeStart) : null);
>+        var rfcRangeEnd =   (aRangeEnd ? toRFC3339(aRangeEnd) : null);
>+
>+        var request = new calGoogleRequest(this);
>+
>+        request.type = request.GET;
>+        request.uri = aCalendar.uri.spec;
>+        request.setResponseListener(aCalendar, aResponseListener);
>+        request.extraData = aExtraData;
>+        request.calendar = aCalendar;
>+
>+        // Request Parameters
>+        request.addQueryParameter("max-results",
>+                                  aCount ? aCount : kMANYEVENTS);
>+        request.addQueryParameter("singleevents",
>+                                  aItemReturnOccurrences ? "true" : "false");
>+        request.addQueryParameter("start-min", rfcRangeStart);

>+        request.addQueryParameter("start-max", rfcRangeEnd);
>+
>+        this.asyncItemRequest(request);
>+    },

I think I need some further information about this code:
When setting singleevents=true, does google return occurrences only? I.e. all recurring items are flattened, no parent item returned, but only occurrences (having a RECURRENCE-ID)?
If this is the case, I think the above approach is wrong. You should always let google return master [+ exception] items, i.e. we are flattening the items in the app, using recurrenceInfo.getOccurrences(). If ITEM_FILTER_CLASS_OCCURRENCES is set, it means that occurrences are to be returned in onGetResult, *but* all of these occurrences need to refer to their proper (unexpanded) parentItem. IMO code in getItems_response eventually needs rework then.

>+++ calendar/providers/gdata/content/js/calGoogleUtils.js	2007-01-26 18:40:34.312086300 +0100

>+    // If the username contains no @, assume @gmail.com
>+    // XXX Maybe use accountType=GOOGLE and just pass the raw username?
>+    if (aUsername.indexOf('@') == -1) {
>+        aUsername += "@gmail.com";
>+    }
Although function parameters ought not be modified, I think it's ok to do here.

>+function getCalendarCredentials(aCalendarName,
>+                                aUsername,
>+                                aPassword,
>+                                aSavePassword) {
...

mind pref signon.rememberSignons, ie
>+    return prompter.promptUsernameAndPassword(title,
>+                                              text,
>+                                              aUsername,
>+                                              aPassword,
     getPrefSafe("signon.rememberSignons", true) ? savepassword : null,
>+                                              aSavePassword);
>+}

>+function ItemToXMLEntry(aItem, aAuthorEmail, aAuthorName) {
please add comments about missing features to your code, if necessary:
- inviting attendees? organizer, partstats, ...
- popup-alarm support?
- priority
- attachments
[- in progress: recurrence/exception support]

>+                         !!aXMLEntry.gd::originalEvent.toString());
rather hacky style ;)
better aXMLEntry.gd::originalEvent.toString().length > 0


>+++ calendar/providers/gdata/content/js/calUtils.js	2007-01-26 18:40:34.362155800 +0100
Can we rely on this API staying stable? Then I could consolidate calWcapUtils.js, too.

>+++ calendar/providers/gdata/install.rdf	2007-01-26 18:37:23.377055000 +0100
>+        <em:id>{718e30fb-e89b-41dd-9da7-e25a45638b28}</em:id> <!-- sunbird -->
>+        <em:minVersion>0.3</em:minVersion>
>+        <em:maxVersion>0.6a1</em:maxVersion>
why limit the sunbird case?

>+++ calendar/providers/gdata/jar.mn	2007-01-26 18:25:18.961515100 +0100
>+    content/calendarCreation.xul        (content/calendarCreation.xul)
>+    content/js/calGoogleCalendar.js     (content/js/calGoogleCalendar.js)
>+    content/js/calGoogleRequest.js      (content/js/calGoogleRequest.js)
>+    content/js/calGoogleSession.js      (content/js/calGoogleSession.js)
>+    content/js/calGoogleUtils.js        (content/js/calGoogleUtils.js)
>+    content/js/calUtils.js              (content/js/calUtils.js)
why package up provider code into the chrome jars?

>+gdata-provider-en-US.jar:
>+% locale gdata-provider en-US %locale/en-US/
>+    locale/en-US/gdata.properties       (/calendar/locales/en-US/chrome/gdata-provider/gdata.properties)
>+    locale/en-US/gdata.dtd              (/calendar/locales/en-US/chrome/gdata-provider/gdata.dtd)

I am unsure whether we should put the dtd and properties files into calendar/locales/. Is this necessary for the localizers?
Matt/sipaq should comment on this.
I would prefer to keep all files of a provider in a single directory (same applies to WCAP dtd/properties).

Comment 67

[Simon Paquet [:sipaq]]

(In reply to comment #66)
>[...]
> >+gdata-provider-en-US.jar:
> >+% locale gdata-provider en-US %locale/en-US/
> >+    locale/en-US/gdata.properties       (/calendar/locales/en-US/chrome/gdata-provider/gdata.properties)
> >+    locale/en-US/gdata.dtd              (/calendar/locales/en-US/chrome/gdata-provider/gdata.dtd)
> 
> I am unsure whether we should put the dtd and properties files into
> calendar/locales/. Is this necessary for the localizers?
> Matt/sipaq should comment on this.
> I would prefer to keep all files of a provider in a single directory (same
> applies to WCAP dtd/properties).

IMO since this an extension, it should use its own namespace and not put files into the calendar namespace.

Comment 68

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

(In reply to comment #66)
> >+        category: "cal-provider",
> >+        categoryEntry: "gdata-provider",
> why add a category?

I guess this was in anticipation of making providers pluggable. jminta started writing a patch http://pantheon.yale.edu/~jpm76/pluggableProviders.diff which I had extended a bit http://mozilla.kewis.ch/pluggableProviders2.diff This uses categories to populate the combo box with the provider names.

Should I take this out until that patch is ready?

> remove here, declare in loop
> Is there any specific reason why you place your extra js files in the chrome?

mvl already pointed this out to me. The files now get put into a "js" subdir of the extension directory.

> >+    get uri() { return this.mUri; },
> >+
> >+    set uri(aUri) {
> >+        // Parse google url, catch private cookies, public calendars,
> >+        // basic and full types, bogus ics file extensions, invalid hostnames
> >+        var re = new RegExp("/calendar/(feeds|ical)/" +
> >+                            "([^/]+)/(public|private)-?([^/]+)?/" +
> >+                            "(full|basic)(.ics)?$");
> >+
> >+        var matches = aUri.path.match(re);
> >+
> >+        if (matches) {
> >+            // Set internal Calendar Name
> >+            this.mCalendarName = decodeURIComponent(matches[2]);
> >+
> >+            // Set normalized url. We need the full xml stream and private
> >+            // access
> >+            var ioService = Cc["@mozilla.org/network/io-service;1"]
> >+                            .getService(Ci.nsIIOService);
> >+
> >+            // We need private visibility and full projection
> >+            this.mUri = ioService.newURI("http://www.google.com" +
> >+                                         "/calendar/feeds/" +
> >+                                         matches[2] +
> >+                                         "/private/full",
> >+                                         null,
> >+                                         null);
> >+            this.findSession(true);
> >+        } else {
> >+            throw googleException(Cr.NS_ERROR_MALFORMED_URI, aUri);
> >+        }
> >+        return this.mUri;
> >+    },
> The url getter may return a modified/normalized url; I wouldn't consider this a
> good idea, because the calendar manager could rely on it being "as is" (as
> passed to you), because AFAIK it's its only identity to separate calendar
> instances. So I would favor to save the unnormalized one, too, for the uri
> getter.

I understand your concern here, but returning an unnormalized url would allow the user to add the same google calendar with multiple urls that all point to the same calendar.

The only alternative might be to only accept urls formatted in a certain way, but since the Google Calendar UI presents many different possibilities to the user all of which use a different projection and visibility value than I need, the URL has to be modified anyway. (i.e right clicking on the XML button in the Google Calendar UI gives me an url with http://www.google.com/calendar/feeds/[e-mail]/private-magicCookie/basic and I need http://www.google.com/calendar/feeds/[e-mail]/private/full )

> 
> >+    refresh: function cGC_refresh() {
> >+        // We will use refresh to reprompt the session credentials. Otherwise
> >+        // there is no way to reload the calendar after the login prompt has
> >+        // been canceled or the network connection is down
> >+        LOG("Refreshing session object");
> >+        this.findSession();
> >+    },
> I wouldn't do that. Besides the explicit "reload remote calendars" context
> menu, IIRC mvl has added support for automatic (timer driven) refresh in the
> calendar manager. So this is more a data refresh, I wouldn't ask  for relogin.
> So if your data is always consistent (because uncached), then just leave this
> out: canRefresh=>false.

The relevant bug for autorefresh is bug 215971. When the autorefresh timer fires, |if(cal.canRefresh) cal.refresh();| is called. Therefore, this mechanism is used on autorefresh.

How else should I allow the user to login again after he cancels the login dialog? If I would just ask for the username and password each time a request is made, then the user would be asked three times on calendar startup since three queries are made. I think this is far more intrusive than a login dialog every couple of minutes when the autorefresh is triggered.
> >+++ calendar/providers/gdata/content/js/calGoogleSession.js	2007-01-26 18:40:34.241989000 +0100
> >+    if (passwordManagerGet(aUsername, password)) {
> >+        this.mGooglePass = password.value;
> >+        this.savePassword = true;
> why save the password if it's still saved?
This member indicates that the password should be saved. If the password is changed externally, then the next request will fail. With |this.savePassword = true;| The password dialog will have the "Save Password?" checkbox already checked, which I assume is what the user wants when he last saved the password. Also, when the currently set password fails, then it is removed from the password manager. (|calGoogleSession.invalidate()|)

> I think I need some further information about this code:
> When setting singleevents=true, does google return occurrences only? I.e. all
> recurring items are flattened, no parent item returned, but only occurrences
> (having a RECURRENCE-ID)?
> If this is the case, I think the above approach is wrong. You should always let
> google return master [+ exception] items, i.e. we are flattening the items in
> the app, using recurrenceInfo.getOccurrences(). If
> ITEM_FILTER_CLASS_OCCURRENCES is set, it means that occurrences are to be
> returned in onGetResult, *but* all of these occurrences need to refer to their
> proper (unexpanded) parentItem. IMO code in getItems_response eventually needs
> rework then.

What you are describing will be taken care of as soon as I have the recurrence patch working. |singleevents=true| gives me a feed where each occurrence is returned as an item entry. This might make occurrences single, unconnected items but I did that more to be able to show the user his events until I have the recurrence working. With the current patch it is not possible to change occurrences.

The alternatives I have to offer would be to either make the minimal patch *ONLY * show items that are not recurring, or to take some parts of the upcoming recurrence patch into this patch. Note the latter alternative will still not allow modification of occurrences (for now).

> mind pref signon.rememberSignons,
I also checked for this pref in passwordManagerSave, just to make sure. Or does the password manager do this? Should I also check in passwordManagerGet() or does remeberSignons mean to not remember any future signons?


> >+function ItemToXMLEntry(aItem, aAuthorEmail, aAuthorName) {
> please add comments about missing features to your code, if necessary:
> - inviting attendees? organizer, partstats, ...
> - popup-alarm support?
> - priority
> - attachments
> [- in progress: recurrence/exception support]
Most of this is commented in XMLEntryToItem. 
* I added a note for alarms.
* I filed a bug on the google issue tracker to add priority support.
Google dosen't support attachments and I don't really think they will. I could try to use google base to save attachments, but I don't know if that works. I think the better alternative is to wait for a bug requesting attachments.
* recurrence, attendants is already noted in XMLEntryToItem

> >+++ calendar/providers/gdata/content/js/calUtils.js	2007-01-26 18:40:34.362155800 +0100
> Can we rely on this API staying stable? Then I could consolidate
> calWcapUtils.js, too.
I would like to include a version of calUtils.js in the extension until 0.5 is released since I have no way to access it right now, due to bug 366560 not being in 0.3.1 and me not finding a way to access the installation directory from an extension. I tried resource:/// but that didnt work out. Maybe you know how? 

The other possibility would be to copy the functions into calGoogleUtils.js until 0.5 is released, this would keep me from copying the whole file. 

> 
> >+++ calendar/providers/gdata/install.rdf	2007-01-26 18:37:23.377055000 +0100
> >+        <em:id>{718e30fb-e89b-41dd-9da7-e25a45638b28}</em:id> <!-- sunbird -->
> >+        <em:minVersion>0.3</em:minVersion>
> >+        <em:maxVersion>0.6a1</em:maxVersion>
> why limit the sunbird case?
Well, I guess I need at least 0.3 to make it work. Can I just leave out the maxVersion? I though it is needed or at least good style to add it.

> why package up provider code into the chrome jars?
Moved as described above.

The other issues I didnt note are either now fixed or need some special attention.

Comment 69

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

(In reply to comment #66)
> >+    getItems_response: function cGC_getItems_response(aRequest,
> >+                                                      aStatus,
> >+                                                      aResult) {
> ...
> >+            if (!xml.link.(@rel == 'http://schemas.google.com/g/2005#post') &&
> >+               !this.mReadOnly) {
> >+                // Check if it is possible to post to the feed. This requires a
> >+                // <link rel="http://schemas.google.com/g/2005#post">
> >+                // element. If the calendar is not already readonly, set the
> >+                // flag and notify the observers about this error.
> >+                this.mReadOnly = true;
> >+                this.notifyObservers("onError",
> >+                                     [Ci.calIErrors.CAL_IS_READONLY]);
> >+            }
> Even if you cannot post to the calendar, why signal an error here? Is readOnly
> use generally forbidden?
> I would use the optimistic way: assume the calendar is writable, if it's not,
> writing will fail (and you can set mReadOnly=true). But reading ought hardly
> cause errors, at least not CAL_IS_READONLY.

When posting to a read-only calendar, all I get is a 403 error with the body "This feed is read-only". If I just follow the 403 then it will show a login dialog on every request that fails because the feed is readonly. 

I could of course parse for this string and set the calendar readonly instead of prompting for user/password, but it sounds a bit hacked to do |if(aResult == "This feed is read-only")|.

I guess this issue should be looked into since shared calendars may become readonly at any point in time. If the user tries to add an item before the feed is refreshed, then the user might get the login-dialog loop anyway.

I'm going to ask the Google Calendar Newsgroup if this string is going to stay or if there is a different method to distinguish between a login failure and a read-only calendar at that point.

Comment 70

[Daniel Boelzle [:dbo]]

(In reply to comment #68)
> (In reply to comment #66)
> > >+        category: "cal-provider",
> > >+        categoryEntry: "gdata-provider",
> > why add a category?
> 
> I guess this was in anticipation of making providers pluggable. jminta started
> writing a patch http://pantheon.yale.edu/~jpm76/pluggableProviders.diff which I
> had extended a bit http://mozilla.kewis.ch/pluggableProviders2.diff This uses
> categories to populate the combo box with the provider names.
> 
> Should I take this out until that patch is ready?

IMO yes, it's no big deal to put it in again once Joey is ready.

> > The url getter may return a modified/normalized url; I wouldn't consider this a
> > good idea, because the calendar manager could rely on it being "as is" (as
> > passed to you), because AFAIK it's its only identity to separate calendar
> > instances. So I would favor to save the unnormalized one, too, for the uri
> > getter.
> 
> I understand your concern here, but returning an unnormalized url would allow
> the user to add the same google calendar with multiple urls that all point to
> the same calendar.

What's the problem then? If a users does so, it's her own fault (and doesn't make sense, just bloats her view with duplicate data...)
I just wanted to point out the (potential) problem that AFAIK the url currently states the only identity a calendar instance has; thus I would keep it constant.

> > I wouldn't do that. Besides the explicit "reload remote calendars" context
> > menu, IIRC mvl has added support for automatic (timer driven) refresh in the
> > calendar manager. So this is more a data refresh, I wouldn't ask  for relogin.
> > So if your data is always consistent (because uncached), then just leave this
> > out: canRefresh=>false.
> 
> The relevant bug for autorefresh is bug 215971. When the autorefresh timer
> fires, |if(cal.canRefresh) cal.refresh();| is called. Therefore, this mechanism
> is used on autorefresh.
> 
> How else should I allow the user to login again after he cancels the login
> dialog? If I would just ask for the username and password each time a request
> is made, then the user would be asked three times on calendar startup since
> three queries are made. I think this is far more intrusive than a login dialog
> every couple of minutes when the autorefresh is triggered.

I consider any unnecessary forced re-login being a bug. Even if only every half hour (autorefresh's default is 30 minutes).

You are actually working around an open problem I am facing in WCAP, too.
I am currently hacking this by suppressing identical errors within the last 2 secs: <http://lxr.mozilla.org/seamonkey/source/calendar/providers/wcap/calWcapSession.js#117> 
It's worth a bug, of course, I just didn't have the time to file it.
The main problem is that providers cannot put multiple getItems (or whatever) calls into relation that are logically one operation (e.g. a view refresh with "Workweek days only" checked issues two getItems calls).

A solution would be: The providers only fire straight-forward to the passed calIOperationListener (leaving out the global calIObserver). The calling code knows the overall logical operation and could decide what errors are notified to an error box and what are just follow-ups.

> What you are describing will be taken care of as soon as I have the recurrence
> patch working. |singleevents=true| gives me a feed where each occurrence is
> returned as an item entry. This might make occurrences single, unconnected
> items but I did that more to be able to show the user his events until I have
> the recurrence working. With the current patch it is not possible to change
> occurrences.

Ok, so for now, just assure that no recurrenceId and no recurrenceInfo is set at those unconnected occurrences.

> The alternatives I have to offer would be to either make the minimal patch
> *ONLY * show items that are not recurring, or to take some parts of the
> upcoming recurrence patch into this patch. Note the latter alternative will
> still not allow modification of occurrences (for now).

I would prefer a two-step. First bring this basic patch in (early), then continue work on the recurrences thing; easier to review then.

> > mind pref signon.rememberSignons,
> I also checked for this pref in passwordManagerSave, just to make sure. Or does
> the password manager do this? Should I also check in passwordManagerGet() or
> does remeberSignons mean to not remember any future signons?

IMO you can check the password manager independently of that pref.
My understanding of that pref is just that users just shouldn't have the option to save the password if that pref is explicitly false.

> > >+function ItemToXMLEntry(aItem, aAuthorEmail, aAuthorName) {
> > please add comments about missing features to your code, if necessary:
> > - inviting attendees? organizer, partstats, ...
> > - popup-alarm support?
> > - priority
> > - attachments
> > [- in progress: recurrence/exception support]
> Most of this is commented in XMLEntryToItem. 
> * I added a note for alarms.
> * I filed a bug on the google issue tracker to add priority support.
> Google dosen't support attachments and I don't really think they will. I could
> try to use google base to save attachments, but I don't know if that works. I
> think the better alternative is to wait for a bug requesting attachments.
> * recurrence, attendants is already noted in XMLEntryToItem

Attachments are open for WCAP, too. It's a diverse topic how those are stored, pure pointers or not, ...
Even if you commented in XMLEntryToItem, I would do it in ItemToXMLEntry, too.

> > >+++ calendar/providers/gdata/content/js/calUtils.js	2007-01-26 18:40:34.362155800 +0100
> > Can we rely on this API staying stable? Then I could consolidate
> > calWcapUtils.js, too.
> I would like to include a version of calUtils.js in the extension until 0.5 is
> released since I have no way to access it right now, due to bug 366560 not
> being in 0.3.1 and me not finding a way to access the installation directory
good point.

> from an extension. I tried resource:/// but that didnt work out. Maybe you know
> how? 
No, and I don't think it's sensible to dynamically get a calUtils from Sunbird or Lightning for the above reason. calUtils may not stay stable, but you want your extension working on multiple release versions (if possible). Thus I would second to include calUtils into your extension.

> The other possibility would be to copy the functions into calGoogleUtils.js
> until 0.5 is released, this would keep me from copying the whole file. 
IMO including calUtils is better, assuming gdata code is revised when changes are made to calUtils, which is currently the case.

> > >+++ calendar/providers/gdata/install.rdf	2007-01-26 18:37:23.377055000 +0100
> > >+        <em:id>{718e30fb-e89b-41dd-9da7-e25a45638b28}</em:id> <!-- sunbird -->
> > >+        <em:minVersion>0.3</em:minVersion>
> > >+        <em:maxVersion>0.6a1</em:maxVersion>
> > why limit the sunbird case?
> Well, I guess I need at least 0.3 to make it work. Can I just leave out the
> maxVersion? I though it is needed or at least good style to add it.
Yes, leave it as is.

Comment 71

[Daniel Boelzle [:dbo]]

(In reply to comment #69)
> (In reply to comment #66)
> > >+    getItems_response: function cGC_getItems_response(aRequest,
> > >+                                                      aStatus,
> > >+                                                      aResult) {
> > ...
> > >+            if (!xml.link.(@rel == 'http://schemas.google.com/g/2005#post') &&
> > >+               !this.mReadOnly) {
> > >+                // Check if it is possible to post to the feed. This requires a
> > >+                // <link rel="http://schemas.google.com/g/2005#post">
> > >+                // element. If the calendar is not already readonly, set the
> > >+                // flag and notify the observers about this error.
> > >+                this.mReadOnly = true;
> > >+                this.notifyObservers("onError",
> > >+                                     [Ci.calIErrors.CAL_IS_READONLY]);
> > >+            }
> > Even if you cannot post to the calendar, why signal an error here? Is readOnly
> > use generally forbidden?
> > I would use the optimistic way: assume the calendar is writable, if it's not,
> > writing will fail (and you can set mReadOnly=true). But reading ought hardly
> > cause errors, at least not CAL_IS_READONLY.
> 
> When posting to a read-only calendar, all I get is a 403 error with the body
> "This feed is read-only". If I just follow the 403 then it will show a login
> dialog on every request that fails because the feed is readonly. 
> 
> I could of course parse for this string and set the calendar readonly instead
> of prompting for user/password, but it sounds a bit hacked to do |if(aResult ==
> "This feed is read-only")|.
> 
> I guess this issue should be looked into since shared calendars may become
> readonly at any point in time. If the user tries to add an item before the feed
> is refreshed, then the user might get the login-dialog loop anyway.
> 
> I'm going to ask the Google Calendar Newsgroup if this string is going to stay
> or if there is a different method to distinguish between a login failure and a
> read-only calendar at that point.
> 

So your core problem is that you can't distinguish those two errors, whether authentication failed or the user has no access right to modify.
But is it necessary to distinguish this here at all? getItems is used only for reading, so it can only be an authentication failure.

Moreover, if you have a look at a different calendar, e.g. you view my one but login as philipp kewisch, wouldn't you constantly run into read-only error notifications?

Comment 72

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

(regarding uri setter)
> What's the problem then? If a users does so, it's her own fault (and doesn't
> make sense, just bloats her view with duplicate data...)
> I just wanted to point out the (potential) problem that AFAIK the url currently
> states the only identity a calendar instance has; thus I would keep it
> constant.
I understand the Problem, I just thought it might enhance the user experience sorting out duplicates beforehand (which does work fine). I'll keep my uri separate and return the unnormalized. I still think there should be some mechanism for providers to normalize their location uri for such cases, even if its as easy as using the getter instead of a local variable after setting the uri.

(regarding refresh() function)
> I consider any unnecessary forced re-login being a bug. Even if only every half
> hour (autorefresh's default is 30 minutes).
> 
> You are actually working around an open problem I am facing in WCAP, too.
> I am currently hacking this by suppressing identical errors within the last 2
> secs:
> <http://lxr.mozilla.org/seamonkey/source/calendar/providers/wcap/calWcapSession.js#117> 
> It's worth a bug, of course, I just didn't have the time to file it.
> The main problem is that providers cannot put multiple getItems (or whatever)
> calls into relation that are logically one operation (e.g. a view refresh with
> "Workweek days only" checked issues two getItems calls).
> 
> A solution would be: The providers only fire straight-forward to the passed
> calIOperationListener (leaving out the global calIObserver). The calling code
> knows the overall logical operation and could decide what errors are notified
> to an error box and what are just follow-ups.
I am not actually logging in again every xxx minutes. I only make sure the session object is not null. If there is already a session object for that calendar, then nothing is changed and the refresh does nothing. If the user previously canceled the login dialog (which effectively means that there is no session object), then he will be prompted again after xxx minutes, or when he choses reload remote calendars.

Even if the three requests at the beginning would only be one, after the user clicks cancel, a new dialog gets shown every time he changes a month. I think that degrades user experience, since the user clicked cancel for a reason. If he dosent want to enter user data, then he dosen't want to access that calendar and probably still dosen't want to when he changes to a different month. 

But he will want to when he reloads the calendar per context menu, and must live with it if he sets up auto-refresh (just as he would have to live with relogin in thunderbird when he cancels the mail server login dialog there).

I guess I'm fine with re-requesting each time a new request is made, but I think it is much smoother the way it is now. What do you say I keep it the way it is and add a comment to a follow on bug on one of the following:

- The issues you described above
- Add a way to re-login to the calendar server if the user cancels the dialog, possibly different from clicking "reload remote calendars".

Comment 73

[Daniel Boelzle [:dbo]]

(In reply to comment #72)
> (regarding refresh() function)
...
> > I consider any unnecessary forced re-login being a bug. Even if only every half
> > hour (autorefresh's default is 30 minutes).
> > 
> > You are actually working around an open problem I am facing in WCAP, too.
> > I am currently hacking this by suppressing identical errors within the last 2
> > secs:
> > <http://lxr.mozilla.org/seamonkey/source/calendar/providers/wcap/calWcapSession.js#117> 
> > It's worth a bug, of course, I just didn't have the time to file it.
> > The main problem is that providers cannot put multiple getItems (or whatever)
> > calls into relation that are logically one operation (e.g. a view refresh with
> > "Workweek days only" checked issues two getItems calls).
> > 
> > A solution would be: The providers only fire straight-forward to the passed
> > calIOperationListener (leaving out the global calIObserver). The calling code
> > knows the overall logical operation and could decide what errors are notified
> > to an error box and what are just follow-ups.
> I am not actually logging in again every xxx minutes. I only make sure the
> session object is not null. If there is already a session object for that
> calendar, then nothing is changed and the refresh does nothing. If the user
> previously canceled the login dialog (which effectively means that there is no
> session object), then he will be prompted again after xxx minutes, or when he
> choses reload remote calendars.
> 
> Even if the three requests at the beginning would only be one, after the user
> clicks cancel, a new dialog gets shown every time he changes a month. I think
> that degrades user experience, since the user clicked cancel for a reason. If
> he dosent want to enter user data, then he dosen't want to access that calendar
> and probably still dosen't want to when he changes to a different month. 

IMO the user should uncheck the calendar then, which eliminates the foremost getItems calls while view travelling (though not the agenda and alarm updates).
I have implemented your behaviour once for WCAP (as you do: once prompt has been cancelled => dead provider until refresh), but users wondered more than once why their calendars didn't react anymore.

> But he will want to when he reloads the calendar per context menu, and must
> live with it if he sets up auto-refresh (just as he would have to live with
> relogin in thunderbird when he cancels the mail server login dialog there).

I think Thunderbird's mail folder behaviour is different (at least for IMAP): it asks me over and over again for a password if I cancel the prompt and travel folders. I think this is correct.

> I guess I'm fine with re-requesting each time a new request is made, but I
> think it is much smoother the way it is now. What do you say I keep it the way
> it is and add a comment to a follow on bug on one of the following:
> 
> - The issues you described above
> - Add a way to re-login to the calendar server if the user cancels the dialog,
> possibly different from clicking "reload remote calendars".

I think refresh has been designed for a different purpose: refresh cached data (like an ics file cached by a memory cal). It wasn't meant to reset dead providers which have been the effect of cancelled password prompts. Moreover, I think the UI (reload remote calendars) is misleading, too.
That way I think canRefresh ought to return false and you should always try to establish a session if you don't have one to fulfill the call.
As stated in my previous comment, IMO it's the responsibility of the UI code how to react on login failures, it's not part of the provider.

I hope I don't get you wrong here, and maybe somebody else can add his understanding in this field here.

Comment 74

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

I changed the url setter issue. Adding multiple versions of the same calendar reveals duplicate events in the unifinder and since bug 325641 is not ready yet there are no duplicate events in the views. If you think this is ok, I guess I can live with it for now.

I also tested changing the behaviour of the refresh() function as requested and now have the three login dialogs and also one when the user deselects the calendar. To suppress this, I would have to add a delay to the request queue, which I don't really like. I really want a smooth solution for this issue, I am open for any suggestions.

Also I did some elaborate testing on the read-only issue. When doing a login request, I know it is definitly a login failure. When writing to the calendar, I get a 403 with "This feed is read-only" in the body. When I try to access a calendar to which I have no access at all, I get a 404 error telling me the feed cannot be accessed. This also happens when I revoke access to a calendar after logging in. I haven't been able to get a 401 error.

I could assume that a 403 that belongs to a write operation means the calendar is readonly. This works quite well currently. If this is ok with you I'm keeping it. The only drawback that emerges from that is the lack of possibilities to translate Ci.calIErrors.CAL_IS_READONLY into a localized error message, since triggering onError shows a dialog with either Google's "The feed is read-only" message or no message at all.

If this comment solves all issues I expressed my concerns about, drop me an email and I'll upload a new patch (which also has the other issues fixed). Don't want to clutter this bug even more than it already is.

Comment 75

[Daniel Boelzle [:dbo]]

Comment on attachment 252927 [details] [diff] [review]
GData Extension patches - v1

ok for me; r=dbo.

Comment 76

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

Attachment: GData Extension patches - v2

All changes made as discussed.

Comment 77

[Matthew (lilmatt) Willis]

Attachment: gdata extension v3

I did some very minor stylistic cleanup and moved the properties/dtd to the locales directory before checkin.  Due to the way the l10n folks work, having to check out an extra directory just to localize an extension isn't really feasible.

Anyway, this is the patch as checked in

Philipp: We're missing whatever icon you're specifying in jar.mn

Comment 78

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

Attachment: Missing icon

Oh, sorry about that.

Comment 79

[Matthew (lilmatt) Willis]

Icon added and truncated file fixed.

-> FIXED

Comment 80

[Richard Trout]

What's the relationship between this and bug # 335826 (which provides read/write support for Google Calendar via the Google Data API, and is currently implemented via an early verson XPI extension).