[Tracker] extractor-controller-thread branch review

[Martyn Russell <martyn lanedo com>]

Hi all,

Carlos recently worked on the extractor-controller-thread branch to add 
support for cancelling extraction during umount with files on that mount 
point being extracted at the time. This is the review of that branch:

  http://git.gnome.org/browse/tracker/log/?h=extractor-controller-thread

--

â Shouldn't tracker_controller_initable_init() return an GError? Perhaps 
chained up the g_thread_create() ?


â We don't disconnect "mount-point-removed" signal on finalize, are you 
hoping the storage will only ever have 1 ref count? As a precaution I 
would disconnect too.


â I would use separate new/free functions for GetMetadataData for 
clarity and to avoid missing members when freeing or setting (without 
g_slice_new0()).


â The tracker_controller_dbus_stop() function should set pointers to 
NULL in case _start() is ever called afterwards.


â The GError should be handled properly in tracker_controller_new() with 
propagation. We've recently changed the way error propagation is handled 
in Tracker. See the propagation example here:

  http://developer.gnome.org/glib/unstable/glib-Error-Reporting.html


â I saw this in the code:
  /* FIXME: notify success from the other thread */

  Shouldn't we fix that?


â I would use separate TrackerExtractTask new/free functions for the 
same reasons listed above.


â Is this a typo?
  +        if (!controller) {
  +               if (controller) {
  +                       g_object_unref (controller);
  +               }


â The config unref in main.c was moved from after log_shutdown to 
before. IIRC, it's needed there for verbosity (or was).


â There are a few whitespace issues which I can fix up if you don't get 
around to it. Philip noticed some of those too.

--

I need to review tracker-extract.c outside the diff, it's a bit munged 
up with all the changes, but the rest looks fine to me.

Great work Carlos.

--
Regards,
Martyn