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

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>


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

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
-*- */


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


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

â+ 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,
+ "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);

It wasn't :) now fixed.

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

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>


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.


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

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


32. What is src/libtracker-miner/ used for?

Can you remove this?

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

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.


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