[Bug 25766] Review Gabble Google Mail Notification

Wed Feb 24 15:45:06 CET 2010

http://bugs.freedesktop.org/show_bug.cgi?id=25766

Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|danielle.madeley at collabora.c|nicolas.dufresne at collabora.c
                   |o.uk                        |o.uk
           Keywords|patch                       |
          QAContact|                            |telepathy-
                   |                            |bugs at lists.freedesktop.org

--- Comment #9 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2010-02-24 06:45:05 PST ---
OK, I'm done with reviewing; please put the patch keyword back when this is
ready for further review. There's no need to reassign to me, I receive
telepathy-bugs mail.

Review of conn-mail-notif.[ch]:

Notes for the future
====================

Pedantically, conn-mail-notif.h should include <glib-object.h> instead of or
in addition to <glib.h>, since it references GObject. (This isn't very
important, since connection.h necessarily includes <glib-object.h> anyway,
but it would become more important in library code.)

Trivial changes
===============

Why is namespaces.h included right at the beginning of conn-mail-notif.c?
I'd have put it with connection/debug/util. Likewise for
extensions/extensions.h, which is part of Gabble.

(You were right to include conn-mail-notif.h immediately after config.h
though, since this is an implicit assertion that it's self-contained.)

conn-mail-notif.c contains trailing whitespace; it shouldn't.

Coding style: many occurrences of [a-zA-Z]( should have a space before the
parenthesis (although #define IMPLEMENT(x) must not have a space there,
due to cpp syntax).

Coding style: we compare pointers to NULL with "pointer != NULL", not with
"!pointer", and compare integers to zero explicitly too. The only case
where there's no explicit operator should be gboolean.

Coding style: the outer "if" in unsubscribe() has {}, therefore the
corresponding "else" should have them too (even though it doesn't strictly
need them).

Coding style: we prefer blank lines around each if{}, if{}else{}, while{},
etc., like this:

>    node = wocky_xmpp_node_get_child (parent_node, "snippet");
> +
>    if (node)

conn-mail-notif.c should reference the API by its English URL :-) - remove
/intl/fr.

I don't like the assumption that a zero-filled GPtrArray is valid. Does GLib
explicitly say so? If not, then we shouldn't assume this. Since empty_array
is only used in dbus-glib method calls, which use deep-copying and a lot of
g_malloc'ing already, I'd just allocate a new empty GPtrArray in each function
that needs one and free it afterwards.

sender_name_owner_changed() should refer to the unique name as "client",
"subscriber" or "unique_name", not as "sender" (the message being processed
here wasn't sent by the client). It's up to you whether you rename the variable
"sender" in gabble_mail_notification_[un]subscribe() to be consistent with that
or not (in these contexts, the client *is* the sender).

In struct MailsHelperData, the hash tables and array should have a brief
comment explaining what's in them and the ownership (owned / weak ref /
borrowed / etc., and if it's borrowed, mention who owns the reference and
ensures that it remains valid). A hypothetical example:

  /* name (string) borrowed from the mushroom => GMushroom ref */
  GHashTable *mushrooms;

query_unread_mails_cb shouldn't debug-output the whole node - Wocky will
output all received nodes (if given appropriate debug flags) anyway.

I don't like the name of struct MailsHelperData. It appears to be a closure
for the call to mail_thread_info_each in store_unread_emails? If that's the
case, then it should be declared just above mail_thread_info_each,
and should be typedef'd so you don't have to keep saying "struct". something
like this:

    typedef struct {
    ...
    } MailThreadCollector;

(then replace all "struct MailsHelperData *" with "MailThreadCollector *").

> +  if (data.mails_added->len || mails_removed->len)

Coding style: this should be of the form if (x > 0 || y > 0) (i.e. compare
against 0 explicitly).

> +  DEBUG ("reached");

I suspect/hope this is no longer useful :-)

> +          g_hash_table_ref (mail);
> +          g_hash_table_remove (data->old_mails, tid);

You can use g_hash_table_steal to avoid it being freed prematurely?

> +      handle_senders (node, mail, &dirty);

I think I'd prefer three occurrences of:

    if (handle_foo (node, mail))
        dirty = TRUE;

The final "else" clause of conn_mail_notif_properties_getter should be in
{}, and should just be a g_assert_not_reached (it can only be reached if
someone adds a property to the GabbleConnection and fails to add it to the
getter).

Logic
=====

In gabble_mail_notification_request_inbox_url (also request_mail_url):
> +  /* TODO Make sure we don't have to authenticate again */

Either fix that TODO, declare it to be unimportant, or file a bug.

(One of my spec changes was to relax the pre-authenticated URL from
"pre-authenticated" to "pre-authenticated if possible", so I think it would
be OK to just delete these.)

In gabble_mail_notification_request_inbox_url (also r_m_u), how could
conn->inbox_url be NULL? Shouldn't you raise NotAvailable if that's the case?

In gabble_mail_notification_request_mail_url, why do you convert in_id
to an int64 and then back to a string? Can't you just incorporate it into
the URL with %s?

In conn_mail_notif_init, if conn->daemon is NULL, all future methods are
basically going to assert. I think it would be reasonable to use g_error
here, since Gabble connects to the session bus as the first thing it does.

In conn_mail_notif_properties_getter, is username at stream-server definitely
correct for all Google servers? In particular I'm thinking of: SRV records,
gmail.com vs. googlemail.com, Google Apps For Your Domain.

If stream-server is the "logical" name of the server (gmail.com/googlemail.com
rather than talk.google.com), you don't need to encode the JID, you could just
use the self-handle (tp_base_connection_get_self_handle()) and stringify it
with the appropriate TpHandleRepo.

If it's the "physical" name of the server (talk.google.com) then I think the
logic's wrong.

In sender_each:

> +          0, wocky_xmpp_node_get_attribute (node, "name") ?: "",

I'm pretty sure ?: isn't standard C, and it's certainly not clear.
I'd prefer the struct to be constructed with tp_value_array_build()
(which probably requires a telepathy-glib 0.10 dependency). However,
I don't think the sender should be added at all if they don't have an
email address, and the name should probably default to the email address
rather than the empty string?

-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.