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

[Martyn Russell <martyn lanedo com>]

On 17/03/10 11:30, Martyn Russell wrote:
>

Just so you know, I have rebased the branch against origin/master too. 
So you will need to check it out again.

Before I forget, Do we need a g_hash_table_unref() in the web-miner.c 
where we send the results off using the async d-bus apis in 
tracker_miner_web_dbus_get_association_data()? not sure if we need to 
make sure TRACKER_MINER_WEB_GET_CLASS (miner)->get_association_data() 
creates or references hash tables. I think it is const.

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

If you could explain this perhaps clearer in the documentation that 
would be great. Seems like the API has multiple purposes?

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

I will fix this tomorrow.

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

Fixed. Actually, I moved this to tests/libtracker-miner so we now test 
that API before each release.

> 4. Space between include groupings please:
>
> â+#include "config.h"
> â+#include <libtracker-miner/tracker-password-provider.h>

Fixed.

> 5. Coding style:â
>
> +int main (int argc, char **argv)

Fixed.

> 6. Use EXIT_FAILURE or EXIT_SUCCESS instead of:
>
> â+ return 1;

Fixed.

> 7. Don't use C++ commentsâ
>
> + // Also test without getting the username

Fixed.

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

Fixed.

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

Will tackle this tomorrow if at all.

> 10. We no longer use this:â
>
> +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8
> -*- */

Fixed.

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

Fixed.

> 12. No need for extra spacing here:
>
> â+ gchar *name;

Fixed.

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

Fixed.

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

Fixed.

> 15. Looks like you forget to call g_object_notify() on setting the name
> here:
>
> â+ priv->name = g_value_dup_string (value);

Fixed.

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

Fixed.

> 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);

Fixed.

> 18. Coding style:â
>
> + static gchar*

Fixed.

> 19. Is this cast needed?â
>
> + found = (GnomeKeyringFound *)(found_items->data);

It wasn't :) now fixed.

> 20. Coding style:
>
> â+ for (i = 0 ; i < found->attributes->len ; ++i) {

Fixed.

> 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")) {

Fixed. I moved the if (username) part to before the for() loop because 
the whole thing can be avoided if username is NULL.

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

Removed this.

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

Fixed.

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

This still needs fixing at some point. Not sure how to do this nicely, 
but we either have 2 functions a new/free or we keep it internally. Not 
sure yet.

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

Fixed.

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

Fixed.

> 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().

Fixed.

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

Fixed.

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

Will do this tomorrow. Unless you can do it later this afternoon/tonight?

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

Fixed.

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

Can you remove this?

> 33. Header is inconsistent with other defines:â
>
> +#ifndef __TRACKER_MINER_WEB_H__â
> +#define __TRACKER_MINER_WEB_H__

Fixed.

> 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;

Fixed.

> 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?

We can do this later, it is internal anyway.

--
Regards,
Martyn