Re: [PATCH 2/5] youtube: Make browse() a cancellable operation

[Iago Toral <itoral igalia com>]

On Tue, 19 Apr 2011 07:34:48 +0000, Iago Toral <itoral igalia com> 
wrote:
> On Fri, 15 Apr 2011 09:55:12 +0200, "Juan A. Suarez Romero"
> <jasuarez igalia com> wrote:
> > @@ -1046,6 +1045,8 @@ search_cb (GObject *object, GAsyncResult
> > *result, OperationSpec *os)
> >      GRL_DEBUG ("Search operation has been cancelled");
> >      os->callback (os->source, os->operation_id, NULL, 0,
> > os->user_data, NULL);
> >      operation_spec_unref (os);
> > +    /* needs an extra unref */
> > +    operation_spec_unref (os);
> >      return;
> >    }
>
> We have to be much more descriptive with this kind of things. "Needs
> double unref" does not say anything, I already know that when i see 2
> unrefs in a row, the thing to explain here is *why* it needs the
> double unref.
>
> A simple:
> /* Look for OPERATION_SPEC_REF_RATIONALE for details on the reason
> for this double-unref */
> or
> /* Since we are not freeing the spec in the emission callback we have
> to do it here */
>
> would do the trick.

The rest of the patches look good to me, but please test this well, 
since bugs dealing with cancellation are usually crashers and difficult 
to track once the code is committed.

Iago