Re: [Tracker] Review request: miner-web-review branch

[Adrien Bustany <abustany gnome org>]

On Wed, 17 Mar 2010 11:30:48 +0000, Martyn Russell <martyn lanedo com>
wrote:
> On 01/03/10 11:31, Adrien Bustany wrote:
> > Hello everyone,
>
> Hi, I finally managed to get around to reviewing this branch.
Woohoo :)


> 9. Shouldn't tracker-miner-web-full.xml be generated in data/dbus/ ?
Not sure... It's a generated file that is only needed for compilation.
As stated in the Makefile, it wouldn't be necessary if dbus-binding-tool
properly handled several files on the command line...
So the answer-not-so-answer is "it's a build file, so I kept it in the
build dir"

> 19. Is this cast needed?â
>
> +       found = (GnomeKeyringFound *)(found_items->data);
Well found_items->data is a gpointer, and found a GnomeKeyringFound*. So
I guess it's necessary ?


> 22. Why do we need a static mutex in the password provider?
the password provider is a singleton. If a non threaded context, we don't
need locking, but people might want to write threaded miners...

> 32. What is src/libtracker-miner/tracker-miner-web.deps.in used for?
Wooops that's an artifact of when the VAPI file for the web miners was a
separate file. You can safely ignore it.

> 38. Why don't we put the #define boiler plate for the password providers

> (for GNOME and GKeyFile) in a header file?
They're not supposed to be used anywhere else... This is internal stuff,
so
I consider a .h file wasn't necessary. I don't mind doing it though :)

> --
>
> I will get started on fixing these after lunch today, so you don't need 
> to make any further commits.
>
> I also need to test the work and I will be doing that later today too.
>
> Thanks again for doing the initial work here! :)
Thanks for the (long ;) ) review.

Here's a side question:
When we're going to merge some web miners (which I hope we'll do soon),
should we put them in $(datadir)/tracker/miners, or
$(datadir)/tracker/miners-web ?

Cheers

Adrien