Re: Vimeo plugin

[Víctor M. Jáquez L. <vjaquez igalia com>]

Hi Joaquim,

On Wed, Apr 21, 2010 at 11:42:41AM +0200, Joaquim Rocha wrote:
> I've been developing a Grilo plugin to prove search for videos in Vimeo.

I'll put my bad-ass coder hat :)

> From a076ab4a49d696cda66c7e5b96c3e1e1cfc7fe79 Mon Sep 17 00:00:00 2001
> From: Joaquim Rocha <jrocha igalia com>
> Date: Wed, 21 Apr 2010 10:33:00 +0200
> Subject: [PATCH] Added Vimeo plugin
> 
> The Vimeo plugin allows to search videos, retrieving, among other data, the video's play URL.
> ---
>  configure.ac          |   56 ++++++
>  src/Makefile.am       |    6 +-
>  src/vimeo/Makefile.am |   34 ++++
>  src/vimeo/grl-vimeo.c |  386 ++++++++++++++++++++++++++++++++++++
>  src/vimeo/grl-vimeo.h |   77 ++++++++
>  src/vimeo/gvimeo.c    |  519 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/vimeo/gvimeo.h    |  112 +++++++++++
>  7 files changed, 1189 insertions(+), 1 deletions(-)
>  create mode 100644 src/vimeo/Makefile.am
>  create mode 100644 src/vimeo/grl-vimeo.c
>  create mode 100644 src/vimeo/grl-vimeo.h
>  create mode 100644 src/vimeo/gvimeo.c
>  create mode 100644 src/vimeo/gvimeo.h
> 
> diff --git a/configure.ac b/configure.ac
> index 7a1fd95..e9b2561 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -101,6 +101,21 @@ AC_SUBST(SQLITE_LIBS)
>  AC_SUBST(HAVE_SQLITE)
>  AM_CONDITIONAL(HAVE_SQLITE, test "x$HAVE_SQLITE" = "xyes")
>  
> +PKG_CHECK_MODULES(LIBSOUP, libsoup-2.4, HAVE_LIBSOUP=yes, HAVE_LIBSOUP=no)
> +AC_SUBST(LIBSOUP_CFLAGS)
> +AC_SUBST(LIBSOUP_LIBS)
> +AC_SUBST(HAVE_LIBSOUP)

AC_SUBST is not needed at all because PKG_CHECK_MODULES does it already:
http://linux.die.net/man/1/pkg-config

The same goes for the whole autoconfig.ac either in grilo and grilo-plugins

I'll cook a patch about it.

> +AM_CONDITIONAL(HAVE_LIBSOUP, test "x$HAVE_LIBSOUP" = "xyes")

This conditional is not needed in any Makefile.am:
http://www.delorie.com/gnu/docs/automake/automake_106.html

> +
> +AC_CHECK_HEADER([gcrypt.h], HAVE_GCRYPT=yes, HAVE_GCRYPT=no)
> +AM_CONDITIONAL(HAVE_GCRYPT, test "x$HAVE_GCRYPT" = "xyes")

ditto

> +
> +PKG_CHECK_MODULES(GTHREAD, gthread-2.0, HAVE_GTHREAD=yes, HAVE_GTHREAD=no)
> +AC_SUBST(GTHREAD_CFLAGS)
> +AC_SUBST(GTHREAD_LIBS)
> +AC_SUBST(HAVE_GTHREAD)

ditto

> +AM_CONDITIONAL(HAVE_GTHREAD, test "x$HAVE_GTHREAD" = "xyes")

ditto

Also, as a minor issue, there are several trailing white spaces in the
configure.ac and the Makefile.am

> +  if (!g_thread_supported ()) {
> +    g_thread_init (NULL);
> +  }

I'm still not convinced if this initialization is really needed here. IMHO
this is always carried out by the client application.

> +typedef struct { 

trailing white space

> +static gchar *
> +get_node_text (xmlXPathContextPtr context, gchar *xpath_expr)
> +{
> +  xmlNodePtr node;
> +  xmlXPathObjectPtr xpath_obj;

Unused structure. A strict compilation will fail.

> +  gchar *node_text = NULL;
> +  

Trailing white space


Cheers,

vmjl