[Bug 63402] Avatar image caching is not asynchronous

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=63402

--- Comment #6 from Chandni Verma <chandniverma2112 at gmail.com> ---
(In reply to comment #5)
> Thanks for looking into this!
> 
> I'm reviewing a flattened version here. I'd suggest attaching a new,
> flattened version with these changes - it'll be easier to review that way.
> 
> +  write_data *avatar_data = g_slice_new (write_data);
> +  write_data *mime_data = g_slice_new (write_data);
> +  AvatarFileWrittenData *avatar_file_written_data = g_slice_new
> +      (AvatarFileWrittenData);
> 
> These don't need to be separate structs. I'd use a single struct to
> represent the whole operation:
> 
> typedef struct {
>     TpContact *self;
>     gchar *token;
>     gchar *mime_type;
>     GFile *mime_file;
> } WriteAvatarData;
> 
> (and if you discover you need more fields, you can add them).

Done.


> 
> You don't actually need to copy the avatar data into this struct, as far as
> I can see, because you only use it for the first
> g_file_replace_contents_async() call (which will, internally, copy it).

I am not quite acquainted with GTask but if I remove the avatar data the test
./tests/dbus/test-contacts which checks this code-path fails with error:

ERROR:contacts.c:599:create_contact_with_fake_avatar: assertion failed (content
== avatar_data): ("\210\177*\b-avatar-data" == "fake-avatar-data")



> 
> It looks as though you leak the memory for these structs in error code
> paths. I would suggest not allocating them until you need them (that would
> be just after the comment "Save avatar in cache..." here, I think) and then
> you won't need to worry about cleanup on errors.
> 
> +  avatar_file = g_file_new_for_path (filename);
> 
> You don't unref this, so it's leaked. Unref it after calling
> g_file_replace_contents_async().

Done, but how do you know g_file_replace_contents_async() keeps it's own
reference? Also, the parameter is called "input file".


> 
> +      g_free (avatar_data->str);
> +      g_slice_free (write_data, avatar_data);
> +      g_free (mime_data->str);
> +      g_slice_free (write_data, mime_data);
> +      g_object_unref (avatar_file_written_data->mime_file);
> +      g_slice_free (AvatarFileWrittenData, avatar_file_written_data);
> 
> This should be in a new function avatar_file_written_data_free() (or
> write_avatar_data_free() if you use the naming I suggested) so you don't
> have to keep duplicating it.
> 
> I haven't reviewed all the duplicate copies of freeing the struct and its
> contents, so there might be memory leaks there - but when you do this, it'll
> be obvious at a glance whether things are being freed correctly.

I guess this isn't required now.


> 
> +  if (avatar_data->self == NULL)
> +    {
> +      g_free (avatar_data->str);
> +      g_slice_free (write_data, avatar_data);
> +      g_free (mime_data->str);
> +      g_slice_free (write_data, mime_data);
> +      g_object_unref (avatar_file_written_data->mime_file);
> +      g_slice_free (AvatarFileWrittenData, avatar_file_written_data);
> +      return;
> +    }
> +  /* Update the avatar token if a newer one is given */
> +  contact_set_avatar_token (avatar_data->self, avatar_data->str, FALSE);
> +
> +  tp_clear_object (&avatar_data->self->priv->avatar_file);
> +  avatar_data->self->priv->avatar_file = (GFile *) file;
> +  g_free (avatar_data->str);
> +  g_slice_free (write_data, avatar_data);
> +
> +  /* Now that the avatar-file has been written, write the mime-file. */
> +  g_file_replace_contents_async (avatar_file_written_data->mime_file,
> +      mime_data->str, strlen (mime_data->str), NULL, FALSE,
> G_FILE_CREATE_NONE,
> +      NULL, &mime_file_written, mime_data);
> +
> +  g_slice_free (AvatarFileWrittenData, avatar_file_written_data);
> 
> There's a logic error here: if the TpContact is NULL, you don't write the
> MIME type to a file. If you're signalling the avatar token here, I'd do:
> 
>     if (data->self != NULL)
>       {
>         contact_set_avatar_token (data->self, data->token, FALSE);
>       }
> 
>     g_file_replace_contents_async(...)
> 
> but actually it'd make more sense to delay signalling the changed avatar
> token until the end as well - the application can't usefully act on it until
> you've cached the file.
> 
> If writing the avatar data fails, it's probably still worth trying to write
> out the MIME type, and if either or both fail, I think it's still worth
> signalling the token change (and perhaps also the MIME type and
> avatar-file), even though the current implementation didn't. That simplifies
> things down to one code path.
> 
> General points
> --------------
> 
> In both g_file_replace_contents_async calls, I think you should use
> G_FILE_CREATE_PRIVATE|G_FILE_CREATE_REPLACE_DESTINATION for the overwrite -
> we're potentially dealing with private data here, and REPLACE_DESTINATION
> gives the closest match for how g_file_set_contents() worked.
> 
> Minor nitpicking stuff
> ----------------------
> 
> +typedef struct{
> 
> Coding style: space before "{".
> 
> +static void mime_file_written (GObject *file,
> +                               GAsyncResult *res,
> +                               gpointer user_data){
> 
> Coding style: newlines after "static void" and ")" here. We also usually
> indent the second and subsequent arguments in function definitions by 4
> spaces, rather than lining them up with the "(" - please follow
> <http://telepathy.freedesktop.org/wiki/Style/> and/or the style of nearby
> code.
> 
> +  GError* error = NULL;
> +  write_data* mime_data = (write_data *) user_data;
> 
> Coding style: "type *value" not "type* value"
> (<http://lists.freedesktop.org/archives/systemd-devel/2013-April/010753.
> html> for the reason).
> 
> You don't need a cast when going from gpointer (or equivalently, void *) to
> SomeType *.
> 
> There are a lot of (GFile *) casts here. I'd prefer this:
> 
>     static void
>     something_written (GObject *source_object,
>         GAsyncResult *res,
>         gpointer user_data)
>     {
>       GFile *file = G_FILE (source_object);
>       WriteFileData *data = user_data;
>       GError *error = NULL;
> 
>       if (!g_file_replace_contents_finish (file, res, NULL, &error))
>         {
>           ...
>         }
> 
>        ... (and the same for all other uses of the GFile) ...
>     }
> 
> Coding style: blank line after the variable declarations, please (like in
> the example immediately before this comment).
> 
> Coding style: it's conventional to check the return from the _finish
> function, rather than the contents of 'error', as in the same example. If it
> fails, it's OK to assume that error is non-NULL.

Fixed.
Patch follows.
It seems that the test required some changes though, I haven't looked into it
yet.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.