On Wed, 17 Mar 2010 11:30:48 +0000, Martyn Russell <[email protected]> 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 _______________________________________________ tracker-list mailing list [email protected] http://mail.gnome.org/mailman/listinfo/tracker-list
