Code review comment for lp:~ycheng-twn/indicator-power/indicator-power_set-brightness-via-powerd

Thanks for the patch, Yuan-Chen! I like the idea of using powerd-via-dbus as the primary method and I appreciate the patch.

Some issues:

 * If we're able to get the powerd_proxy but then getBrightnessParams() fails, we don't fall back to the old control mechanism.

 * In powerd_get_proxy(), error->message is accessed after error's been freed

 * In _brightness_init(), why is ib_brightness_powerd_control_set_value(max brightness) being called? Do we want to unconditionally force max brightness whenever the power indicator loads?

 * In _brightness_init(), the call to ib_brightness_powerd_control_set_value(max brightness) is a no-op because it's being called before 'inited' is set to TRUE

 * In _control_set_value, wouldn't it be better to CLAMP(value,self->min,self->max) rather than returning silently if the value is out-of-range?

 * In -powerd-control.c, please put "return;" statements on their own line for readability.

 * In -powerd-control.c, need to use a cancellable when making dbus calls.

 * In service.c's get_brightness_range(), we need to test brightness_powerd_control for NULL before using it

 * In service.c's dispose(), no need for the NULL check before calling g_clear_pointer(). g_clear_pointer() contains its own NULL test.

 * In service.c's init(), no need to NULL out brightness_control and brightness_powerd_control; priv is already zeroed out

 * Not a blocker, but I'd be happier if this didn't introduce extra conditional logic into service.c. It would be nicer if there was a simpler wrapper that owned the two controls privately and gave service.c a unified API to call.

 * Not a blocker, but I'd be happier if avoided _sync calls. If you choose make them async, this will require a little extra plumbing that could be put into the wrapper class suggested above.