Add methods for controlling keyboard brightness.

Add methods for controlling keyboard brightness.

BUG=135482
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146229

[Simon Que, 2012-07-10 23:37:21]

LGTM

[Daniel Erat, 2012-07-10 23:42:34]

Do you plan to later expose UI (e.g. a slider) to change the keyboard
brightness?  If so, you'll probably need methods for getting and setting the
brightness, like what we ended up adding for screen brightness.  It might be
easier to just do that from the very beginning instead of relying on
increase/decrease methods; that way you can e.g. make changes to the amount by
which the keys change the brightness with a Chrome change instead of a power
manager change.

[Yusuke Sato, 2012-07-11 00:32:49]

On 2012/07/10 23:42:34, Daniel Erat wrote:
> Do you plan to later expose UI (e.g. a slider) to change the keyboard
> brightness?  

It seems that we haven't decided it yet:
http://code.google.com/p/chromium/issues/detail?id=135482#c6

> If so, you'll probably need methods for getting and setting the
> brightness, like what we ended up adding for screen brightness.  It might be
> easier to just do that from the very beginning instead of relying on
> increase/decrease methods; that way you can e.g. make changes to the amount by
> which the keys change the brightness with a Chrome change instead of a power
> manager change.

But either way, I guess that Increase/DecreaseKeyboardBrightness() methods are
necessary for Alt+F6/F7 because chromeos/dbus/power_manager_client.cc has the
following comment and the same should be true for keyboard brightness:

    // Monitor the D-Bus signal for brightness changes. Only the power          
                                                           
    // manager knows the actual brightness level. We don't cache the            
                                                           
    // brightness level in Chrome as it'll make things less reliable.    

What do you think?

[Daniel Erat, 2012-07-11 00:56:24]

On 2012/07/11 00:32:49, Yusuke Sato wrote:
> On 2012/07/10 23:42:34, Daniel Erat wrote:
> > Do you plan to later expose UI (e.g. a slider) to change the keyboard
> > brightness?  
> 
> It seems that we haven't decided it yet:
> http://code.google.com/p/chromium/issues/detail?id=135482#c6
> 
> > If so, you'll probably need methods for getting and setting the
> > brightness, like what we ended up adding for screen brightness.  It might be
> > easier to just do that from the very beginning instead of relying on
> > increase/decrease methods; that way you can e.g. make changes to the amount
by
> > which the keys change the brightness with a Chrome change instead of a power
> > manager change.
> 
> But either way, I guess that Increase/DecreaseKeyboardBrightness() methods are
> necessary for Alt+F6/F7 because chromeos/dbus/power_manager_client.cc has the
> following comment and the same should be true for keyboard brightness:
> 
>     // Monitor the D-Bus signal for brightness changes. Only the power        
 
>                                                            
>     // manager knows the actual brightness level. We don't cache the          
 
>                                                            
>     // brightness level in Chrome as it'll make things less reliable.    
> 
> What do you think?

That comment seems misleading to me.  We cache the brightness level in Chrome in
at least one place (the slider in the system tray popup), and possibly others;
we additionally need to listen for brightness changes over D-Bus since powerd
can adjust the level itself due to e.g. user inactivity.  The same situation
probably applies to the keyboard backlight.

Just having increase/decrease methods for now seems okay to me if we're not sure
that we'll need any UI for this, though.

[Daniel Erat, 2012-07-11 00:56:48]

lgtm

[Yusuke Sato, 2012-07-11 21:14:48]

Thanks for the detailed comment; I think I now understand what you mean. I'm
going to land the CL as-is for now but if we decide to implement a UI, I'll
revert this and move to the way you suggested.

[commit-bot: I haz the power, 2012-07-11 21:14:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/10700150/4001

[commit-bot: I haz the power, 2012-07-11 22:44:01]

Change committed as 146229