Review of attachment 187457 [details] [review]:

The changes to gdata_documents_service_query_documents() are fine, but I'm afraid most of the changes to gdata_documents_service_query_documents_async() aren't possible, and could cause crashes due to closure data being freed too early by language runtimes.

Could you please split this up into two patches: one which just contains the accepted changes to the two methods, and one which adds a GDestroyNotify parameter to gdata_documents_service_query_documents_async() as described below. If you could extend them both to apply to all the other relevant methods (such as the other ones modified in http://git.gnome.org/browse/libgdata/commit/?id=9fe511b3fcb5fca3698f897a503cd24ec74fa4ed) that would be amazing.

Thanks!

::: gdata/services/documents/gdata-documents-service.c
@@ +338,3 @@
  * @query: (allow-none): a #GDataDocumentsQuery with the query parameters, or %NULL
+ * @cancellable: (allow-none): optional #GCancellable object, or %NULL
+ * @progress_callback: (allow-none) (scope call) (closure progress_user_data): a #GDataQueryProgressCallback to call when an entry is loaded, or %NULL

These look fine.

@@ +386,2 @@
 /**
+ * gdata_documents_service_query_documents_async:

See below for why this can't be done.

(The (skip) annotation was added in http://git.gnome.org/browse/libgdata/commit/?id=9fe511b3fcb5fca3698f897a503cd24ec74fa4ed, which gives some explanation of why it's necessary.)

@@ +388,3 @@
  * @self: a #GDataDocumentsService
  * @query: (allow-none): a #GDataDocumentsQuery with the query parameters, or %NULL
+ * @cancellable: (allow-none): optional #GCancellable object, or %NULL

This is fine.

@@ +389,3 @@
  * @query: (allow-none): a #GDataDocumentsQuery with the query parameters, or %NULL
+ * @cancellable: (allow-none): optional #GCancellable object, or %NULL
+ * @progress_callback: (allow-none) (scope async) (closure progress_user_data): a #GDataQueryProgressCallback to call when an entry is loaded, or %NULL

It's incorrect to set (scope async) on this parameter, as it can (and will) be called multiple times during the async query operation, whereas (scope async) requires that the callback be called exactly once.

The proper fix is either to:
 • add a new (scope) type to GIR to allow specifying that closure data should be freed at the same time as another, specified, closure's data (in that case we could then set the progress_user_data's closure to be destroyed at the same time as the main user_data closure); or to
 • add a GDestroyNotify destroy_progress_user_data parameter and use (scope notified).

@@ +390,3 @@
+ * @cancellable: (allow-none): optional #GCancellable object, or %NULL
+ * @progress_callback: (allow-none) (scope async) (closure progress_user_data): a #GDataQueryProgressCallback to call when an entry is loaded, or %NULL
+ * @progress_user_data: (closure): data to pass to the @progress_callback function

This is fine.

Ok, looks like I didn't properly understand the scope annotation, thanks for clarifying it. I will split the patch up and extend the fixes to the other functions, and get back to you.

Created attachment 189358 [details] [review]
New patch with only the easy fixes

Okay, here's the (allow-none) part of the patch. I applied it to all the functions in the API that take nullable GCancellable parameters. I also made all the progress callbacks (allow-none) and marked their (closure)s. I went ahead and added (allow-none) and (closure) to the progress callbacks in the async functions too, but they're still marked (skip).

I will get around to the GDestroyNotify business later.

Review of attachment 189358 [details] [review]:

Great! Committed to master, thanks.

commit 84060afb583fc5c9642861eb9ccd82488000f906
Author: P. F. Chimento <philip.chimento@gmail.com>
Date:   Mon Jun 6 21:28:53 2011 +0200

    introspection: Fix introspection for GCancellable parameters
    
    ...and progress callbacks in non-async functions
    See bug #649728

 gdata/gdata-access-handler.c                       |    8 +++---
 gdata/gdata-batch-operation.c                      |    4 +-
 gdata/gdata-buffer.c                               |    2 +-
 gdata/gdata-client-login-authorizer.c              |    4 +-
 gdata/gdata-service.c                              |   24 ++++++++--------
 gdata/services/calendar/gdata-calendar-service.c   |   28 ++++++++++----------
 gdata/services/contacts/gdata-contacts-contact.c   |    8 +++---
 gdata/services/contacts/gdata-contacts-service.c   |   24 ++++++++--------
 gdata/services/documents/gdata-documents-service.c |   18 ++++++------
 gdata/services/picasaweb/gdata-picasaweb-service.c |   22 ++++++++--------
 gdata/services/youtube/gdata-youtube-service.c     |   28 ++++++++++----------
 11 files changed, 85 insertions(+), 85 deletions(-)

I'm working on the GDestroyNotify patch, any suggestions on how to go about testing the changes?

(In reply to comment #5)
> I'm working on the GDestroyNotify patch, any suggestions on how to go about
> testing the changes?

What we want to do is guarantee that the GDestroyNotify callback is called exactly once for a given gdata_*_async() function call; ideally just before the GAsyncReadyCallback callback is called.

How about adding some tests to the relevant test suites. e.g. In gdata/tests/documents.c, add a test something like:

typedef struct {
    guint progress_destroy_notify_count;
    guint async_ready_notify_count;
} ProgressClosure;

static void
progress_callback (blah, ProgressClosure *data)
{
    /* No-op */
}

static void
progress_closure_free (blah, ProgressClosure *data)
{
    /* Check that this callback is called first */
    g_assert_cmpuint (data->async_ready_notify_count, ==, 0);
    data->progress_destroy_notify_count++;
}

static void
finish_callback (blah, ProgressClosure *data)
{
    /* Check that this callback is called second */
    g_assert_cmpuint (data->progress_destroy_notify_count, ==, 1);
    data->async_ready_notify_count++;

    g_main_loop_quit (foo); // As in the other async tests
}

static void
my_destroy_notify_test ()
{
    ProgressClosure *data = g_slice_new (ProgressClosure);
    data->progress_destroy_notify_count = 0;
    data->async_ready_notify_count = 0;

    gdata_documents_service_something_async (blah, progress_callback, progress_closure, (GDestroyNotify) progress_closure_free, (GAsyncReadyCallback) finish_callback, blah);

    g_main_loop_run (foo);

    /* Check that both callbacks were called exactly once */
    g_assert_cmpuint (data->progress_destroy_notify_count, ==, 1);
    g_assert_cmpuint (data->async_ready_notify_count, ==, 1);

    g_slice_free (ProgressClosure, data);
}

If you're adding GDestroyNotify callbacks to all *_async() operations in libgdata, this ProgressClosure code could mostly be factored out into gdata/tests/common.c, which should make it easy to add tests using it.

Created attachment 189819 [details] [review]
Patch with API-breaking fixes

Here's the patch adding GDestroyNotify parameters to the async query functions, and adding tests to the test suite. Will you be breaking API in 1.0?

I had a little trouble with the test suite - it seems that on the master branch, not all the existing tests passed even without my modifications, is this normal? However, all the /calendar/query/*, /contacts/query/*, etc., tests passed both before and after applying this patch.

Review of attachment 189819 [details] [review]:

I've made a few minor changes to the patch, the largest of which being the modification of gdata_access_handler_get_rules_async().

I've listed the modifications briefly below, but made them myself and committed the patch. Thank you!

commit fafa68352959512861bcbc8c9d3f0295a70e767c
Author: Philip Chimento <philip.chimento@gmail.com>
Date:   Sat Jun 18 18:31:28 2011 +0100

    introspection: Add GDestroyNotify to progress callbacks
    
    All query_async functions that take a progress callback should have a
    GDestroyNotify parameter for freeing the progress callback data, since
    there is no suitable (scope) annotation.
    
    This breaks API by changing the signatures for:
     • gdata_access_handler_get_rules_async()
     • gdata_service_query_async()
     • gdata_calendar_service_query_all_calendars_async()
     • gdata_calendar_service_query_own_calendars_async()
     • gdata_calendar_service_query_events_async()
     • gdata_contacts_service_query_contacts_async()
     • gdata_contacts_service_query_groups_async()
     • gdata_documents_service_query_documents_async()
     • gdata_picasaweb_service_query_all_albums_async()
     • gdata_picasaweb_service_query_files_async()
     • gdata_youtube_service_query_standard_feed_async()
     • gdata_youtube_service_query_videos_async()
     • gdata_youtube_service_query_related_async()
    
    Closes: bgo#649728

 HACKING                                            |    3 +
 gdata/gdata-access-handler.c                       |   13 +++-
 gdata/gdata-access-handler.h                       |    1 +
 gdata/gdata-service.c                              |   14 +++-
 gdata/gdata-service.h                              |    2 +-
 gdata/services/calendar/gdata-calendar-service.c   |   27 +++++--
 gdata/services/calendar/gdata-calendar-service.h   |    6 +-
 gdata/services/contacts/gdata-contacts-service.c   |   20 ++++--
 gdata/services/contacts/gdata-contacts-service.h   |    2 +
 gdata/services/documents/gdata-documents-service.c |   10 ++-
 gdata/services/documents/gdata-documents-service.h |    1 +
 gdata/services/picasaweb/gdata-picasaweb-service.c |   23 ++++--
 gdata/services/picasaweb/gdata-picasaweb-service.h |    2 +
 gdata/services/youtube/gdata-youtube-service.c     |   30 ++++++--
 gdata/services/youtube/gdata-youtube-service.h     |    3 +
 gdata/tests/calendar.c                             |   82 +++++++++++++++++++-
 gdata/tests/common.c                               |   26 ++++++
 gdata/tests/common.h                               |   10 +++
 gdata/tests/contacts.c                             |   52 ++++++++++++-
 gdata/tests/documents.c                            |   26 ++++++-
 gdata/tests/picasaweb.c                            |   68 ++++++++++++++++-
 gdata/tests/youtube.c                              |   56 +++++++++++++-
 22 files changed, 427 insertions(+), 50 deletions(-)

::: gdata/gdata-service.c
@@ +735,3 @@
+
+	if (data->destroy_progress_user_data != NULL)
+		data->destroy_progress_user_data(data->progress_user_data);

This needs a space before the opening bracket in the function call, and braces around the body of the if block.

@@ +749,3 @@
  * @progress_user_data: (closure): data to pass to the @progress_callback function
+ * @destroy_progress_user_data: (allow-none): the function to call when @progress_callback will not be called anymore, or %NULL. This function will be
+ * called with @progress_user_data as a parameter and can be used to free any memory allocated for it.

s/anymore/any more/

@@ +752,3 @@
  * @callback: a #GAsyncReadyCallback to call when the query is finished
  * @user_data: (closure): data to pass to the @callback function
  *

The “Since” tag needs updating for all the modified functions to be “Since: 0.9.1”.

::: gdata/tests/youtube.c
@@ +1513,3 @@
 		g_test_add_data_func ("/youtube/query/related", service, test_query_related);
 		g_test_add_data_func ("/youtube/query/related_async", service, test_query_related_async);
+		g_test_add_data_func ("/youtube/query/related_async_profress_closure", service, test_query_related_async_progress_closure);

s/profress/progress/

Awesome! Thanks for the help.