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

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"


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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]