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

On Wed, 17 Mar 2010 11:30:48 +0000, Martyn Russell <martyn lanedo com>
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/ 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 :)


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 ?



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