Code review comment for lp:~charlesk/indicator-datetime/lp-1074999

Revision history for this message
Charles Kerr (charlesk) wrote :

> `geo_master` doesn't need to be a global variable: it's only ever used in geo_start.

The issue is that geoclue_master_create_client_async() populates its private GeoclueMasterAsyncData struct with a pointer to that master without reffing it. That's why datetime-service keeps its own ref.

> Why are you calling geo_stop unconditionally in on_use_geoclue_changed? I'd put it in the `else` below, in case the callback is called even though the value hasn't changed (which may happen according to the docs).

It's to ensure that even if the callback triggered multiple calls to geo_start(), we would have a call to geo_stop() in between to guarantee that geo_master, geo_client, and geo_address were cleaned up.

« Back to merge proposal