Bug 27459

This might be a little bit subtle, but shouldn't be too hard to do.

It wasn't totally easy! Have a look at my branch (URL).

Namely, when developing that I stumbled upon
 * A race condition in contact building, uncovered by the awesome performance of GetAll introspection
 * Several tests relying on the individual slow-path introspection calls being made with a specific timing
 * TpConnection being a maze of inter-connected callbacks with a lot of corner cases (to be expected, really)

lcov-check indicates that both the fast and slow paths are hit for both the pre-connected and connected phases of introspection, and also as a response to StatusChanged.

As a side note, this actually produces a measurable performance benefit!

with master:

make -j check  5.60s user 0.93s system 63% cpu 10.359 total

with oggis/conn-introspection

make -j check  5.44s user 0.92s system 62% cpu 10.189 total    

So the CPU time taken for running the test suite decreases by about 2.9% and the wallclock time by about 1.6%, although a few tests still deliberately hit the old slow-path to test corner-cases for it. The timings also include a lot of Makefile crunching, so the relative tp-glib speedup is actually greater.

http://git.collabora.co.uk/?p=user/oggis/telepathy-glib.git;a=commitdiff;h=f4514c86c30c2cfa5a18de84918af5c8079a96c6

Doesn't TpDBusPropertiesMixin crash (or at least critical) if getting the GObject property fails?

http://git.collabora.co.uk/?p=user/oggis/telepathy-glib.git;a=commitdiff;h=222bfe4ee068d08c633457906610299f4c2d8eba

I think I'd split the grabbing-things-from-properties block into its own function, so that you can just return FALSE; when things go awry; this gets rid of the goto and would make it easier to skim-read.

Test-wise, I guess that the existing tests implicitly exercise the new code paths, and the tests where you've deliberately broken the properties test the fallback?

(In reply to comment #4)
> http://git.collabora.co.uk/?p=user/oggis/telepathy-glib.git;a=commitdiff;h=f4514c86c30c2cfa5a18de84918af5c8079a96c6
> 
> Doesn't TpDBusPropertiesMixin crash (or at least critical) if getting the
> GObject property fails?
> 

No :>

> http://git.collabora.co.uk/?p=user/oggis/telepathy-glib.git;a=commitdiff;h=222bfe4ee068d08c633457906610299f4c2d8eba
> 
> I think I'd split the grabbing-things-from-properties block into its own
> function, so that you can just return FALSE; when things go awry; this gets rid
> of the goto and would make it easier to skim-read.

I did.

I also split out the bit of the code which adds the interfaces to the TpProxy as tp_proxy_add_interfaces. One more bit of API which we'll have to add to 0.14 :P

> 
> Test-wise, I guess that the existing tests implicitly exercise the new code
> paths, and the tests where you've deliberately broken the properties test the
> fallback?

Yes.

The branch conn-introspection-0.14 is da same thing backported to 0.14.

+ * Declare that this proxy supports the given interfaces. Equivalent to calling
+ * g_quark_from_string () followed by tp_proxy_add_interface () for each of the
+ * interface names.

tp_proxy_add_interface() doesn't exist; you mean tp_proxy_add_interface_by_id().

This branch is going to fail make check with gtk-doc enabled, because you didn't add tp_proxy_add_interfaces to telepathy-glib-sections.txt.

I'd kind of expect tp_proxy_add_interfaces to early-return if interfaces == NULL.

Fix the above, and then merge the -0.14 variant to 0.14 please!

(In reply to comment #7)
> + * Declare that this proxy supports the given interfaces. Equivalent to
> calling
> + * g_quark_from_string () followed by tp_proxy_add_interface () for each of
> the
> + * interface names.
> 
> tp_proxy_add_interface() doesn't exist; you mean
> tp_proxy_add_interface_by_id().
> 

True! Fixed.

> This branch is going to fail make check with gtk-doc enabled, because you
> didn't add tp_proxy_add_interfaces to telepathy-glib-sections.txt.

Oh. I knew I had to have noobed something with gtk-doc. Fixed, and make check with --enable-gtk-doc verified to pass.

> 
> I'd kind of expect tp_proxy_add_interfaces to early-return if interfaces ==
> NULL.

Fair enough. It does now.

> 
> Fix the above, and then merge the -0.14 variant to 0.14 please!

Thanks! Done. I leave the forward-porting runes to get it into master to you, though.