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

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,
+                            "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/ 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! :)


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