329034 - async calICalendar methods and callbacks need request handles or ids

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Two important uses for such things:

* the front-end should be able to cancel an in-progress request.
* callbacks should be able to determine which particular request a set of results belong to (so that, for example, they can be discarded if they belong to a cancelled request)

One possible way to do this would be to make the appropriate method return an nsICancelable.  However, given the upcoming plans for calICachingCalendar, using nsIRequest may end up making more sense.  We'll probably also need an integer id for identity-testing purposes (cf bug 325636).  We could either put that on another interface on the object that is handed back, or we could perhaps just hand back an integer and provide a method for converting that into an nsI{Request,Cancelable} on demand.

Comment 1

[Reed Loden [:reed]]

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!

Comment 2

[Daniel Boelzle [:dbo]]

Isn't nsIRequest tied too much against networking for the demanded purpose? IMO it's a bad idea if providers cannot implement that pieces like loadFlags.
I think it makes more sense to taylor one towards calICalendar, e.g.

interface calIRequest : nsISupports {
    // similar to nsIRequest:
    boolean isPending();
    readonly attribute nsresult statusCode;
    readonly attribute string statusString; // ?
    void cancel(in nsresult status);

    readonly attribute unsigned long id;
    readonly attribute calICalendar calendar;

    // constants from calIOperationListener:
    readonly attribute unsigned long operationType;
};

Asynchronous calICalendar calls would return those request objects:

    calIRequest addItem(...);
    calIRequest getItems(...);
    ...

calIOperationListener could be simplified like

    // do we still need aId?
    void onOperationComplete(in calIRequest request,
                             in nsIVariant aDetail);

    void onGetResult(in calIRequest request,
                     in nsIIDRef aItemType, 
                     in PRUint32 aCount, 
                     [array, size_is(aCount), iid_is(aItemType)] 
                     in nsQIResult aItems ); 

Further on (though slightly off topic I admit), we could merge the latter two callbacks to just

    void onOperation(in calIRequest request,
                     in nsIVariant aDetail);

+ the callee could check whether the request is still in progress
  (-> request.statusCode).
+ arrays of items could be provided via nsIVariant

Comment 3

[Daniel Boelzle [:dbo]]

Implemented <http://lxr.mozilla.org/mozilla/source/calendar/providers/wcap/public/calIWcapRequest.idl> for WCAP provider and it seems to work quite well. We are using it to cancel pending requests for invitations and free-busy.

Comment 4

[Daniel Boelzle [:dbo]]

Attachment: adding calIRequest

Proposing calIRequest and calIRequestResultListener. The latter generic result listener will be used for further API enhancements, e.g. asynchronously searching for calendars, free-busy.

Comment 5

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

Comment on attachment 254802 [details] [diff] [review]
adding calIRequest

>+interface calIRequestResultListener : nsISupports

I don't understand this interface. What does it do? All the async calls already have callback interfaces. Why need another? I don't see it being referenced anywhere.

Is implementing calIRequest optional, or do we need to implement it for all other providers?

Comment 6

[Daniel Boelzle [:dbo]]

(In reply to comment #5)
> (From update of attachment 254802 [details] [diff] [review])
> >+interface calIRequestResultListener : nsISupports
> 
> I don't understand this interface. What does it do? All the async calls already
> have callback interfaces. Why need another? I don't see it being referenced
> anywhere.

You currently run into problems when dialogs are closed before the listener (implemented in the dialog) has been called. You run into dead XUL contexts, see e.g. bug 360111. Thus dialogs that have open requests need to cancel those requests, e.g. in unload, making sure no listener is called anymore. That's the purpose of the returned request object.

> Is implementing calIRequest optional, or do we need to implement it for all
> other providers?

Synchronous providers need not implement it, so they can return null.

Comment 7

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

I understand the purpose of the calIRequest interface, but not that of the calIRequestResultListener.

Comment 8

[Daniel Boelzle [:dbo]]

(In reply to comment #7)
> I understand the purpose of the calIRequest interface, but not that of the
> calIRequestResultListener.

As I wrote in comment #4, I have put a generic listener into calIRequest.idl, too. I use it in further API enhancements (bug 370146 and bug 370148). IMO a generic listener for free-busy and search requests is ok, not bloating the IDL too much. However we can alternatively define specific listener interfaces for search and free-busy. What do you favor?

Comment 9

[Matthew (lilmatt) Willis]

This doesn't block 0.5, but it sure would be nice to have this settled for the bugs daniel mentioned, as well as the autocomplete bug 181249.

Comment 10

[Matthew (lilmatt) Willis]

Not going to make the 0.5 train.

Comment 11

[Matthew (lilmatt) Willis]

Comment on attachment 254802 [details] [diff] [review]
adding calIRequest

>+++ mozilla/calendar/base/public/Makefile.in	2007-02-12 11:27:46.860084800 +0100
>@@ -70,13 +70,14 @@ XPIDLSRCS	= calIAlarmService.idl \
> 		  calIRecurrenceDateSet.idl \
> 		  calIRecurrenceItem.idl \
> 		  calIRecurrenceRule.idl \
> 		  calIImportExport.idl \
> 		  calITodo.idl       \
> 		  calIDateTimeFormatter.idl \
> 		  calIWeekTitleService.idl \
> 		  calIPrintFormatter.idl \
>+		  calIRequest.idl \
Please put this in alpha order rather than adding to the end.  It also doesn't need tabs.  We should stop using tabs for new entries to the IDL list.


>+++ mozilla/calendar/base/public/calICalendar.idl	2007-02-12 11:30:54.840387200 +0100
>@@ -132,16 +133,17 @@ interface calICalendar : nsISupports
>    *
>    */
> 
>   /**
>    * addItem adds the given calIItemBase to the calendar.
>    *
>    * @param aItem       item to add
>    * @param aListener   where to call back the results
>+   * @return            request object to track operation
I think "request ticket..." might be a better way to phrase the comment.  It's up to you.

>@@ -152,27 +154,27 @@ interface calICalendar : nsISupports
>    * - aDetail: the calIItemBase corresponding to the immutable
>    *            version of the newly added item
>    *
>    * If an item with a given ID already exists in the calendar,
>    * onOperationComplete is called with an aStatus of NS_ERROR_XXXXX,
>    * and aDetail set with the calIItemBase of the internal already
>    * existing item.
>    */
>-  void addItem( in calIItemBase aItem, in calIOperationListener aListener );
>+  calIRequest addItem( in calIItemBase aItem, in calIOperationListener aListener );
Since you're touching these lines, please remove the extra spaces in the ()s and wrap the line where necessary.  This comment applies throughout this file.


>+++ mozilla/calendar/base/public/calIRequest.idl	2007-02-12 11:27:01.164377600 +0100
>@@ -0,0 +1,87 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
Set the tab-width to 20 so you can easily see if you accidentally add tabs.

>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: NPL 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.
Shouldn't this be "Sun Microsystems code"?

>+ *
>+ * The Initial Developer of the Original Code is Sun Microsystems, Inc.
>+ * Portions created by Sun Microsystems are Copyright (C) 2006 Sun
>+ * Microsystems, Inc. All Rights Reserved.
>+ *
>+ * Original Author: Daniel Boelzle ([e-mail])
Move your name and email to the next line, indented 2 spaces from "Original Author". Also, use < > around your email address rather than ( ).
>+ *
>+ * 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 NPL, 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 NPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
It looks like this license block isn't formatted like the normal one.  Grab the "official" one from http://www.mozilla.org/MPL/boilerplate-1.1/

>+
>+#include "nsIVariant.idl"
>+
>+[scriptable, uuid(6586B48D-3FF6-4b82-B9F6-5B561D416E36)]
>+interface calIRequest : nsISupports
>+{
>+    /**
>+     * For easy testing for equality.
>+     */
>+    readonly attribute unsigned long id;
>+    
Trailing spaces ^^

>+    /**
>+     * Determines whether the request is pending, i.e. has not been completed.
>+     */
>+    readonly attribute boolean isPending;
>+    
Trailing spaces ^^

>+    /**
>+     * Determines whether the request has succeeded, i.e. it has successfully
>+     * been completed.
>+     * XXX todo: remove this and favor
>+     * !request.isPending && Components.isSuccessCode(request.status) ?
>+     */
>+    readonly attribute boolean succeeded;
I'd rename this to success just because it's shorter.  It's up to you.
Should your todo comment be removed and simply be the actual implementation of "get succeeded()" in calRequest.js?  I don't want to type the line in your comment a bunch of times.

>+    
Trailing spaces ^^

>+    /**
>+     * Status of the request, e.g. NS_OK while pending or after successful
>+     * completion, or NS_ERROR_FAILED when failed.
>+     */
>+    readonly attribute nsIVariant status;
We all discussed that status is unnecessary.  succeeded is probably enough for now and we can add "status" later.

>+    
Trailing spaces ^^

>+    /**
>+     * Cancels a pending request and changes status.
>+     */
>+    void cancel(in nsIVariant status);
>+};
>+
>+[scriptable, uuid(D0BC007F-D0B5-4352-A32A-8F7A9F55A713)]
>+interface calIRequestResultListener : nsISupports
>+{
>+    /**
>+     * Callback receiving results.
>+     *
>+     * @param request object to track operation
>+     * @param result optional request result
>+     */
>+    void onRequestResult(in calIRequest request,
>+                         in nsIVariant result);
This line doesn't need to wrap. ^^


Clint and I both think the interface and listener are useful, and that calIOperationListener is overly complex for simply providing a ticket to a request so that this is useful.  Given our conversations, I'm ok giving r+ on this with the above formatting nits addressed.

Comment 12

[Daniel Boelzle [:dbo]]

Rethinking this interface, I think
- a better name would possibly be "calIOperation" (sticking to the term "operation" seems more appropriate to me); renaming calIRequestResultListener to e.g. calIGenericOperationListener.
- status is indeed useful to signal an operation's error; the alternative would be to pass errors to the listeners, IMO only bloating their signature

Opinions?

Comment 13

[Daniel Boelzle [:dbo]]

Attachment: adding calIOperation

Since returning an operation handle is optional, there is no immediate need to add support to the providers. However regarding mid term we should do so for the async ones.

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 277114 [details] [diff] [review]
adding calIOperation

r+ with these comments considered:


Maybe it would be good to have a comment somewhere in calICalendar.idl, saying in which cases its optional to return a calIOperation.

>+  calIOperation modifyItem(in calIItemBase aNewItem, in calIItemBase aOldItem, 
>+                           in calIOperationListener aListener);

>+  calIOperation getItems(in unsigned long aItemFilter, in unsigned long aCount, 
>+                         in calIDateTime aRangeStart, in calIDateTime aRangeEndEx,
>+                         in calIOperationListener aListener);
While you are at it, one line per argument


>+    readonly attribute nsIVariant status;
>+    void cancel(in nsIVariant aStatus);

Do we need to have a variant here? If status should be a Components.results.* status, then PRUint32 or unsigned long should be sufficient.


>+
>+    /**
>+     * Determines whether the request has succeeded, i.e. it has successfully
>+     * been completed.
>+     */
>+    readonly attribute boolean success;
I don't think we really need success if we have isPending and status. Using (!op.isPending && Components.isSuccessCode(op.status)) should be sufficient for anyone using calIOperation. I'm fine with keeping it though.

>+     * @param request object to track operation
>+     * @param result request result or null in case of an error
Please use the complete argument name and if you like add some whitespaces. This may apply to other comment blocks, I didn't see this until now though. Example:
+     * @param aRequest    Object to track operation
+     * @param aResult     Request result or null in case of an error

Comment 15

[Daniel Boelzle [:dbo]]

(In reply to comment #14)
> >+    readonly attribute nsIVariant status;
> >+    void cancel(in nsIVariant aStatus);
> 
> Do we need to have a variant here? If status should be a Components.results.*
> status, then PRUint32 or unsigned long should be sufficient.
A variant could transport nsIException which has some message.

> >+    /**
> >+     * Determines whether the request has succeeded, i.e. it has successfully
> >+     * been completed.
> >+     */
> >+    readonly attribute boolean success;
> I don't think we really need success if we have isPending and status. Using
> (!op.isPending && Components.isSuccessCode(op.status)) should be sufficient for
> anyone using calIOperation. I'm fine with keeping it though.
I commented exactly that in the first attachment of the bug, but like it because of its brevity.

Comment 16

[Daniel Boelzle [:dbo]]

- renamed onRequestResult() to onResult()
- landed calIOperation on HEAD and MOZILLA_1_8_BRANCH
- adopted free-busy lookup of event dialog
- adopted wcap provider
- follow-up bug 392901 to generally track UI changes
- follow-up bug 392902 to implement calIOperation for all async providers
=> FIXED