[Bug 25766] Review Gabble Google Mail Notification

[bugzilla-daemon at freedesktop.org]

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





--- Comment #12 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk>  2010-02-24 16:21:24 PST ---
(In reply to comment #9)
> 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.)

ommit 3d68d43e9dbf72c3b1d2576327ac8b188c920415
    Replaced glib.h with glib-object.h in conn-mail-notif.h

> 
> Trivial changes
> ===============
> 
> conn-mail-notif.[ch] are copyright 2009-2010 Collabora Ltd., not just 2009.

I always got told that this was the right way to go (keep beginning year and
add - <Current year> when you modify the file). I would like some references
before doing anything.

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

Because I have not read the coding style carefully enough ;)

commit 2a20b335a18288f5ba778eede337a80701fe0948
    Fix conn-mail-notif.c headers ordering

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

Fixed in one of the commit in previous comment.

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

commit 0aeb8506a19a8d3c92c3595870abc04498803c64
    Fixed missing space before parenthesis in conn-mail-notif.c

> 
> 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.
"... I wonder why people are so frightened by the fact that 0 means false ..."

commit 0ed079f5001dc3b077c8d343892a728af0401ef0
    Add explicit comparison in conn-mail-notif.c

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

commit ef618d9ee7362ff62be8362de1ff6fc828aba37c
    conn-mail-notif.c: Add curly braces to "else", following coding style

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

commit 837da697eba5f18f9dec75cb8b4c75d27c29bbc4
    Add blank lines around each if/while in conn-mail-notif.c

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

commit 20a41e06b7fdffb4e139d56e31f22b034d215bab
    Removed intl/fr from link to Google Mail extension doc

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

commit 842f65512e18d4b319ea66b0d1ad8d3b9722f3ee
    Remove static empty_array in conn-mail-notif.c

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

My reflection didn't went that far. I call dbus_g_method_get_sender () which
gives me a sender. But in our case it's confusing with the sender being an
element of Mail. Let's uses subscriber with match well with mail_subscribers
hash table.

commit d99254cb214519df1dfd70f1e37b6dd82b56890e
    User subscriber instead of sender for var name in conn-mail-notif.c

> 
> query_unread_mails_cb shouldn't debug-output the whole node - Wocky will
> output all received nodes (if given appropriate debug flags) anyway.
> 
> +  DEBUG ("reached");
> I suspect/hope this is no longer useful :-)

commit fc7cd40516a001831987a5e2a840979737d1718e
    Removed unneeded debug output in conn-mail-notif.c

> 
> 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;
> 
> 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 *").

I agree that this name sucks, MailThreadCollector is correct, but not more.
I'll try to find something, otherwise I'll take that. I don't agree with
typdefing systematically all the structures. Unless it's a very special
structure (like a gobject) or an opaque structure. I think it just hides the
fact that it's a structure, and it's bad to hide things.

That fix will wait for tomorrow ... (along with moving TpDBusDaemon to a more
connection wide locattion).

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

That's explicit comparison, see previous comments.

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

commit daa971aef811fb222934ffcd5dc4f70809f41e96
    Use g_hash_table_steal () in conn-mail-notif.c

> 
> > +      handle_senders (node, mail, &dirty);
> 
> I think I'd prefer three occurrences of:
> 
>     if (handle_foo (node, mail))
>         dirty = TRUE;

commit 82d47b2374dc3222b3da9b11552f92acac3f4e4a
    Use return value instead of in/out param in conn-mail-notif.c

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

commit d4925f16609290059f00577420b6e10fbe1ecd0a
    Just use g_assert_not_reached() in conn-mail-notif.c property 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.)

The thing is that it is possible, the win32 client does it. It requires a bit
of reverse engineering, which I never completed. For this reason we should file
a but and refer it in the TODO comment, and maybe replace the TODO with a
FIXME.

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

We don't control the server side, so we must not make any assumption about the
the XML attributes presence. One thing is clear to me, we must not crash. 

Indeed, we could do more check (NULL, empty "" or not starting with
"http[s]://" and raise error in these cases, but that would mean the server is
bugged.

I won't change it because the server could also return an URL that is not
working, in all case the client will eventually discover that it's broken by
trying it, it's not CMs fault. 

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

The tid string is represented in decimal on XMPP side, but on the webmail side
they use hexadecimal (%" G_GINT64_MODIFIER "x").

commit 67a2007d767332f6c0603e852fea7c4c72698712
    Explain why we convert Mail id from decimal to hexadecimal

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

commit 72c97874236b07652d00039b8612afdf4da1c9a6
    Use g_error if we cannot allocate TpDBusDaemon

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

user at googlemail.com will have googlemail.com has a stream server. Turns out
that it's a DNS that points to the same pool of servers, not a redirection. The
TpHandle thing seems to be very obscure to me and it's not clearly stated what
it contains. I feel safer to keep what works atm, and it's not time critical.

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

The ?: is indeed a GNU extension, I'll fix that, whether it's "not clear" is an
opinion I don't share with you, sorry.

commit 8291e0c8e4e115ee03685d6a762d88b60f5f9a1e
    Don't use GNU extension operator ?: in conn-mail-notif.c

> I'd prefer the struct to be constructed with tp_value_array_build()
> (which probably requires a telepathy-glib 0.10 dependency).

I'm not sure if it's right to return a GPtrArray of generic GValueArray,
because a GPtrArray of GABBLE_STRUCT_TYPE_MAIL_ADDRESS is expected.

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

Has said before, CMs not responsible for server bugs, and google server always
gives an e-mail address. Faking name with e-mail address is a very bad idea. It
prevents the UI from detecting that Name is missing (empty string means that,
no choice since dbus-glib crash with NULL) and would lead to ugly display like:
    nicolas.dufresne at collabora.co.uk <nicolas.dufresne at collabora.co.uk>


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