[Bug 25766] Review Gabble Google Mail Notification

[bugzilla-daemon at freedesktop.org]

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





--- Comment #13 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2010-02-25 04:15:08 PST ---
(In reply to comment #12)
> > 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).

Sorry, yes, I phrased that ambiguously. What I meant is that their real
copyright status is 2009-2010, but you wrote 2009 and haven't updated it since.

(So, your understanding of copyright as expressed here is right, but the branch
isn't :-)

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

I disagree: GPtrArray, GValue, ..., and basically every other opaque type are
necessarily structs too (what else could they be?), so everyone's GLib coding
style is full of structs already. Structs are the closest C has to classes, so
everything that would be a (simple) class in C++/Java/... is a struct here;
there's no point in repeating that every time you define a struct.

> > > +  /* TODO Make sure we don't have to authenticate again */
[...]
> 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.

Yeah, sounds like a job for an enhancement/low bug.

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

Sure, we mustn't crash; this is why D-Bus has recoverable exceptions. Raising
NotAvailable seems preferable to returning a known-wrong inbox_url, but if you
insist on shifting complexity into the client, it seems mostly harmless to
leave it as-is.

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

For XMPP, the indirection is done with SRV records, FWIW:

% host -t srv _xmpp-client._tcp.gmail.com
_xmpp-client._tcp.gmail.com has SRV record 5 0 5222 talk.l.google.com.
[... and some alternatives ...]
% host -t srv _xmpp-client._tcp.googlemail.com
_xmpp-client._tcp.googlemail.com has SRV record 5 0 5222 talk.l.google.com.
[... and some alternatives ...]

> The
> TpHandle thing seems to be very obscure to me and it's not clearly stated what
> it contains.

tp_base_connection_get_self_handle() returns the result of the GetSelfHandle()
D-Bus method, which is (an integer that maps to) the user's own unique
identifier in the IM protocol.

If you're not comfortable with the handle mechanism, please consult
telepathy-spec for details (you'll need to be aware of it for most CM work).
Handles are basically integer tokens representing normalized strings, much like
a GQuark, but reference-counted rather than being "immortal" like a GQuark.

In the case of XMPP, users are identified by JIDs, so the self-handle is (an
integer that maps to) the user's own JID.

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

It is right: the GType-generating macros are just a way to generate boxed types
for GLib representations of D-Bus types, and get those types registered with
dbus-glib. There's no subclassing or anything like that going on (you can't
subclass GValueArray).

The versions of these macros that appear in telepathy-glib do (somewhat)
document what GLib types they represent, but since this particular one is a
Gabble extension, it doesn't go into any gtkdoc.

> Has said before, CMs not responsible for server bugs, and google server always
> gives an e-mail address.

I still think omitting senders who don't have an email address, and making an
effort to supply some sort of non-empty display name, is a better handling for
this, to be nicer to UIs (UIs should be programmed defensively too, of course,
but there are likely to be more of them than there are CMs and they won't all
get the subtleties right).


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