Code review comment for lp:~unity-team/unity/unity.fix-731096

NULL-testing the LauncherIcon makes perfect sense and is the bulk of the fix for #731096 as Launcher::MouseIconIntersection can return NULL. Regarding the other change, I'd say that since the GSource's user_data is the Launcher instance and is queued in the GMainLoop from a Launcher method, NULL-testing the user_data pointer is useless because it's never going to be NULL, and if by any chance it's NULL it means something went really wrong and should have been catched earlier.

What do you think about removing that test?

That last point leads to a more general discussion. The common practice with GSources is to increase the refcount of the object associated to the GSource's callback before queueing it in a GMainLoop, and then decrease the refcount once consumed at the end of the callback. That prevents objects associated to a GSource queued in a GMainLoop to be disposed before the GSource's callback is dispatched. As far as I can see, that is not done in Unity. It makes sense for objects with controlled lifetime like the Launcher. My personal take on that is to always ref/unref otherwise it becomes a mess knowing when to do that or not.

Do you know if that's intended and what's the policy?