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

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

> The miner-web-review branch contains a patch that adds a new class in
> libtracker-miner, called TrackerMinerWeb. This is basically an abstract
> subclass of TrackerMiner which adds a few signals and conveniences
> useful when writing miners fetching data from web services.
>
> This is only the first step of the review, in the pipe are:
>
> - Various web miners. But I want the API to be freezed before porting them.
> - A miner manager used to bind miners to their web services. This one is
> not fully cooked yet, and Rob Taylor suggested it should be a gnome
> control panel applet more than a tracker-preferences pane. Any feedback
> is welcome.

First of all, let me say, the work is pretty good and thank you for 
writing the patch/branch in the first place.

Note, some pedantic comments follow :) also some of these issues occur 
in more than one place too.

--

1. Need more information to understand D-Bus method: GetAssociationData


2. This method AssociationStatus should probably be 
GetAssociationStatus, more consistent.


3. The password_provider_test probably doesn't need all those LDADDs


4. Space between include groupings please:

â+#include "config.h"
â+#include <libtracker-miner/tracker-password-provider.h>


5. Coding style:â

+int main (int argc, char **argv)


6. Use EXIT_FAILURE or EXIT_SUCCESS instead of:

â+               return 1;


7. Don't use C++ commentsâ

+       // Also test without getting the username


8. I would rather have implementations as a suffix file name so instead 
ofâ tracker-gnome-password-provider.câ use 
tracker-password-provider-gnome.c - that way it is easy to see all the 
providers listed in the directory together.


9. Shouldn't tracker-miner-web-full.xml be generated in data/dbus/ ?


10. We no longer use this:â

+/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */


11. Don't forget to include "config.h" in all built files.


12. No need for extra spacing here:

â+       gchar    *name;


13. Looks like there is no need to set the 
password_provider_gnome_finalize() function, nothing is done in it.


14. Please don't use GET_PRIV macros, instead use 
TRACKER_PASSWORD_PROVIDER_GNOME_GET_PRIVATE, for consistency.


15. Looks like you forget to call g_object_notify() on setting the name 
here:

â+               priv->name = g_value_dup_string (value);


16. I always try to make errors less ambiguous, so I would change this:

â+               g_set_error (error,
+                            TRACKER_PASSWORD_PROVIDER_ERROR,
+                            TRACKER_PASSWORD_PROVIDER_ERROR_SERVICE,
+                            "Cannot store password: %s",
+                            gnome_keyring_result_to_message (r));

To use something like "Could not store password in GNOME Keyring", that 
way you know which backend was tried and which password we are talking 
about here.


17. Try to avoid putting parameters inconsistently on lines, i.e. either 
all on one line or all on separate lines makes more sense for clarity

â+ gnome_keyring_attribute_list_append_string (search_attributes,
â+                                             service", service);


18. Coding style:â

+ static gchar*


19. Is this cast needed?â

+       found = (GnomeKeyringFound *)(found_items->data);


20. Coding style:

â+       for (i = 0 ; i < found->attributes->len ; ++i) {


21. No need to check arguments for NULL before using g_strcmp0(), that's 
the point of using it:

â+               if (username && !g_strcmp0 (attr->name, "username")) {


22. Why do we need a static mutex in the password provider?


23. Not sure how I feel about using the g_assert() there in the 
provider_get() API.


24. System includes should be before higher level includes like 
glib:â+#include <sys/mman.h>


25. We should bzero() and munlock() the memory locked with mlock() 
before freeing it.


26. Don't use g_print() unless printing output before logging is 
initialised which should never be the case in a library. Always use 
g_message(), g_debug(), etc.


27. Try to avoid using single letters for variable names, it is clearer 
to use real names like "error".


28. It doesn't make sense to use g_file_test() before using 
g_key_file_load_from_file(), since it will return the appropriate error 
for us anyway, it just adds an extra stat().


29. There is no need to have local_error and to propagate it up, why not 
just use error itself? functions like g_file_set_contents() et al, 
should all handle NULL error conditions.


30. I would prefer it if PROP_ASSOCIATION_STATUS was a proper G_TYPE_ENUM


31. Don't use g_assert() for public functions, use g_return_{val_}if_fail()


32. What is src/libtracker-miner/tracker-miner-web.deps.in used for?


33. Header is inconsistent with other defines:â

+#ifndef __TRACKER_MINER_WEB_H__â
+#define __TRACKER_MINER_WEB_H__


34. TrackerMinerWebError needs properly documenting


35. TrackerMinerWebAssociationStatus needs properly documenting


36. Please define variables at the top of the code block:â

+       g_assert (TRACKER_IS_PASSWORD_PROVIDER (provider));â
+
+       gchar *name;


37. TrackerPasswordProviderError and TrackerPasswordProvider need 
documenting.


38. Why don't we put the #define boiler plate for the password providers 
(for GNOME and GKeyFile) in a header file?

--

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

--
Regards,
Martyn