Last Comment Bug 308567 - wire up nsIPromptService to CalDAV
: wire up nsIPromptService to CalDAV
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Provider: CalDAV (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Lightning 0.3
Assigned To: Gary van der Merwe
:
:
Mentors:
: 322437 (view as bug list)
Depends on:
Blocks: 323952
  Show dependency treegraph
 
Reported: 2005-09-14 15:02 PDT by Dan Mosedale (:dmose)
Modified: 2006-08-22 06:40 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch rev0 (9.53 KB, patch)
2006-04-17 04:15 PDT, Gary van der Merwe
dmose: first‑review-
Details | Diff | Splinter Review
Patch rev1 (28.66 KB, patch)
2006-04-18 13:58 PDT, Gary van der Merwe
dmose: first‑review-
Details | Diff | Splinter Review
Patch rev2 (39.10 KB, patch)
2006-04-23 13:08 PDT, Gary van der Merwe
dmose: first‑review+
Details | Diff | Splinter Review

Description Dan Mosedale (:dmose) 2005-09-14 15:02:14 PDT
Right now the only way to authenticate is to specify username and password in
the URI.
Comment 1 Bruno Browning 2006-03-25 21:40:26 PST
*** Bug 322437 has been marked as a duplicate of this bug. ***
Comment 2 Jack Bates [:nottheoilrig] 2006-04-14 09:55:35 PDT
I also experience this problem.  Have tried authenticating to my CalDAV server using Basic & Negotiate authentication.  Both work in Firefox, but not in Sunbird

Many thanks! - Jack
Comment 3 Gary van der Merwe 2006-04-17 04:15:59 PDT
Created attachment 218675 [details] [diff] [review]
Patch rev0
Comment 4 Dan Mosedale (:dmose) 2006-04-17 18:47:26 PDT
Comment on attachment 218675 [details] [diff] [review]
Patch rev0

There's a problem here, which is that with this setup, only one code-path is really supported for the callbacks at a time.  If someone else wanted to use the service concurrently with the CalDAV code, there'd be unavoidable races.

I'm not what the right design here is.  The most obvious one would be to add another parameter to all the various method calls on the interface.

That would make the parameter list kinda long on some of the calls, but that's not a new problem, and it could certainly be fixed some other time.

I'm open to other suggestions about ways to do this, however.  shaver, do you have any thoughts here?
Comment 5 Gary van der Merwe 2006-04-18 13:58:18 PDT
Created attachment 218881 [details] [diff] [review]
Patch rev1

As per discussion, pass notificationCallbacks as parameter to each method.
Comment 6 Dan Mosedale (:dmose) 2006-04-22 17:55:24 PDT
Comment on attachment 218881 [details] [diff] [review]
Patch rev1

This looks excellent.  Just a few nits, and then it will be ready to land.

* Please add your name and email address to the license boilerplate in each file that you're touching.

>     /**
>      * @param closure   caller-private data returned via listener
>      */
>     void lockResources(in PRUint32 count,
>                        [array, size_is(count)]
>                        in nsIWebDAVResource resources,
>                        in nsIWebDAVOperationListener listener,
>+                       in nsIInterfaceRequestor notificationCallbacks,
>                        in nsISupports closure);

Please add an @param doxygen comment for the notificationCallbacks param similar to the one for the closure parameter in each place that you're adding it.


>+    
>+    // nsIInterfaceRequestor impl
>+    getInterface: function(iid, instance) {

|instance| doesn't need to be listed as a parameter here since it is actually the |retval|.

>+        Components.returnCode = Components.results.NS_ERROR_NO_INTERFACE;
>+        return null;

In general, the only time it's necessary to touch Components.returnCode directly is if you're returning a success code other than NS_OK.  Since this is a failure code, the XPConnect way to do that is just to |throw| it directly instead of the set and |return|.
Comment 7 Gary van der Merwe 2006-04-23 13:08:06 PDT
Created attachment 219526 [details] [diff] [review]
Patch rev2

Address nits :-)
Comment 8 Dan Mosedale (:dmose) 2006-04-24 09:11:10 PDT
Landed on both trunk and branch; thanks for the patch, Gary!

Note You need to log in before you can comment on or make changes to this bug.