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

Revision history for this message
Lars Karlitski (larsu) 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.

That should be fixed in geoclue.

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

Sounds reasonable.

review: Approve

« Back to merge proposal