[Bug 25766] Review Gabble Google Mail Notification

[bugzilla-daemon at freedesktop.org]

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





--- Comment #3 from Danielle Madeley <danielle.madeley at collabora.co.uk>  2009-12-23 21:08:52 PST ---
(In reply to comment #2)
> (In reply to comment #1)
> > 586       g_value_set_boxed (value, mails);
> > 
> > You can use g_value_take_boxed() here instead. No need to free it then.
> 
> That would require to add a ref on every mail hash table. Also, I don't want to
> do that since mail hash table could be changed before the get operation
> completes. Note that the mail are copied since the boxed type is more then
> GPtrArray.

Ok. That's a valid point.

> >  - I'm concerned it's not very obvious how support for other mail protocols
> >    should be added
> 
> There is no XMPP spec for that, and that it's not clear what could be shared
> with those unkown protocol, I don't think it worth the effort. The result would
> have to be reworked anyway. (The Google specific part is update_unread_mails ()
> plus the new-mail signal. Which is everything, except data structure, getter
> and subscription.)

Ok. Perhaps we should document somewhere at the top that this currently only
supports the Google mail notification spec. Possibly with a link to how that
works?

Some more review:

125   if (g_hash_table_lookup_extended (conn->mail_subscribers, sender, NULL,
NULL))
126     {
127       DEBUG ("Sender '%s' is already subscribed!", sender);
128       goto done;
129     }

Looks like it leaks 'sender'

529 static gboolean
530 foreach_cancel_watch (gpointer key,
531     gpointer value,
532     gpointer user_data)
533 {
534   GabbleConnection *conn = user_data;
535   const gchar *sender_name = user_data;

How can these both be 'user_data' ?

Additionally, when casting GObjects, use the cast macro to ensure validity
(even if you don't have to because it's a void ptr), so it would be:

GabbleConnection *conn = GABBLE_CONNECTION (user_data);

There is a lot of implicit reference passing in this code, and it's very
subtle. I'm wondering if perhaps some comments might be useful for clearing up
who takes ownership of what references.

Some other style things:
 - Line 145, put the first parameter on the next line so that it fits
 - Line 183, comment wraps
 - Line 361, you're not meant to align arguments -- see style Example 5
 - Line 363, missing space
 - Line 372, misspelt "guarantee"
 - Line 510, missing return
 - Line 606, too much indenting
 - Line 646, "Unknown" is misspelt


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