Code review comment for lp:~mhr3/unity/fix-861144

Revision history for this message
Michal Hruby (mhr3) wrote :

> Ah, however to fix this crash (bug #861144) I guess that we only need (at the
> current state) to add a
>
> g_return_if_fail (object);
>

Yes that should also suffice, but we *really* don't want unchecked pointer casting.

> In the panel_service_show_entry function. I forgot to add it previously, and
> I'd suggest to use this check always, after that the entry_id has been parsed,
> leaving the old get_entry_by_id implementation.
> Otherwise we'll duplicate the number of hashtable lookups.
>
> Otherwise using something like
> void panel_service_get_indicator_entry_by_id(PanelService *self,
> IndicatorObject **io, IndicatorObjectEntry **entry);
>
> To get also the IndicatorObject and to avoid further lookups.
>

Ok, I'll fix it to do that to save a couple of cpu cycles.

> A simple way to optimize all these operations, by the way, would be to link an
> entry to its own parent indicator (also if this could lead to other issues,
> though).

Feel free to do so, there are too many scary bits in libindicator... Although that'd be most likely an ABI break, so P stuff.

« Back to merge proposal