Closed Bug 308567 Opened 15 years ago Closed 14 years ago

wire up nsIPromptService to CalDAV


(Calendar :: Provider: CalDAV, defect)

Not set


(Not tracked)

Lightning 0.3


(Reporter: dmose, Assigned: garyvdm)




(1 file, 2 obsolete files)

Right now the only way to authenticate is to specify username and password in
the URI.
No longer blocks: lightning-0.1
Target Milestone: --- → Lightning 0.2
Whiteboard: [cal relnote]
*** Bug 322437 has been marked as a duplicate of this bug. ***
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
Assignee: dmose → garyvdm
Attached patch Patch rev0 (obsolete) — Splinter Review
Attachment #218675 - Flags: first-review?(dmose)
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?
Attachment #218675 - Flags: first-review?(dmose) → first-review-
Attached patch Patch rev1 (obsolete) — Splinter Review
As per discussion, pass notificationCallbacks as parameter to each method.
Attachment #218675 - Attachment is obsolete: true
Attachment #218881 - Flags: first-review?(dmose)
Attachment #218881 - Attachment description: Pach rev1 → Patch rev1
Blocks: 323952
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|.
Attachment #218881 - Flags: first-review?(dmose) → first-review-
Attached patch Patch rev2Splinter Review
Address nits :-)
Attachment #218881 - Attachment is obsolete: true
Attachment #219526 - Flags: first-review?(dmose)
Attachment #219526 - Flags: first-review?(dmose) → first-review+
Landed on both trunk and branch; thanks for the patch, Gary!
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [cal relnote]
You need to log in before you can comment on or make changes to this bug.