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

[Martyn Russell <martyn lanedo com>]

On 17/03/10 12:00, Adrien Bustany wrote:
> On Wed, 17 Mar 2010 11:30:48 +0000, Martyn Russell<martyn lanedo com>

> > 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.

I will check, but I would prefer it to be kept with the other xml files 
if possible.

> 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"

OK.

> > 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 ?

I don't think it is needed, I will check this too. A gpointer is a void* 
IIRC and setting a GnomeKeyringFound* from a void* shouldn't be 
problematic. It doesn't warn here either :)

> > 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...

I see.

> > 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.

OK.

> > 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 :)

Well, we also have other header files which are not exported to the 
include directory so we can still have headers here if that makes sense.

> Thanks for the (long ;) ) review.

No problem :)

> 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 ?

I suppose that needs thinking about. I will get back to you on that.

--
Regards,
Martyn