Enable brightness controls for all ChromeOS builds. Make brightness controls auto-repeat.

Enable brightness controls for all ChromeOS builds.  Make brightness controls auto-repeat.

BUG=chromium:100732
TEST=Launch ChromeOS and go to the System settings page.  Tap or press and hold the brightness buttons to adjust the screen brightness.


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

[Rick Byers, 2011-10-26 20:48:35]

Why do you want to enable brightness buttons in the options screen for all
ChromeOS builds?  On any non-TouchUI build there will always be a keyboard with
brightness buttons on it.  I'm not sure we want to expose these less-than-ideal
buttons to all ChromeOS users.

[kevers1, 2011-10-26 20:55:26]

Removed the touch only check because of chromiumos:21855.

-- Kevin

On Wed, Oct 26, 2011 at 4:48 PM, <rbyers@chromium.org> wrote:

> Why do you want to enable brightness buttons in the options screen for all
> ChromeOS builds?  On any non-TouchUI build there will always be a keyboard
> with
> brightness buttons on it.  I'm not sure we want to expose these
> less-than-ideal
> buttons to all ChromeOS users.
>
>
http://codereview.chromium.**org/8340002/<http://codereview.chromium.org/8340...
>

[Daniel Erat, 2011-10-26 20:57:34]

(Thanks for doing this!)

On 2011/10/26 20:55:26, kevers1 wrote:
> Removed the touch only check because of chromiumos:21855.
> 
> -- Kevin
> 
> On Wed, Oct 26, 2011 at 4:48 PM, <mailto:rbyers@chromium.org> wrote:
> 
> > Why do you want to enable brightness buttons in the options screen for all
> > ChromeOS builds?  On any non-TouchUI build there will always be a keyboard
> > with
> > brightness buttons on it.  I'm not sure we want to expose these
> > less-than-ideal
> > buttons to all ChromeOS users.
> >
> >
>
http://codereview.chromium.**org/8340002/%3Chttp://codereview.chromium.org/83...>
> >

[cwolfe, 2011-10-26 21:03:54]

Thanks a lot for picking this up.

http://codereview.chromium.org/8340002/diff/3001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/repeating_button.js (right):

http://codereview.chromium.org/8340002/diff/3001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:101: * @param
{!Event} e The triggeed event.
nit typo

[kevers, 2011-10-26 21:28:33]

http://codereview.chromium.org/8340002/diff/3001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/repeating_button.js (right):

http://codereview.chromium.org/8340002/diff/3001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:101: * @param
{!Event} e The triggeed event.
On 2011/10/26 21:03:55, cwolfe wrote:
> nit typo

Fixed.

[James Hawkins, 2011-10-26 21:49:46]

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/repeating_button.js (right):

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:6: 
nit: Remove blank line.

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:8: * Creates a new
button element.  The repeating button behaves like a
nit: Single space between comments.

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:24: holdDelayTime:
500,
These vars are private, right?

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:74: * Called when
the user presses the button.
To remove ambiguity when referring to "this" button, s/the button/this button/

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:77: buttonDown:
function(e) {
It seems a lot of these methods should be private.

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:81: // in agreement
with the defaults for the ChromeOS keyboard and virtual
re: in agreement

Should we pull/set these values from some common location in the backend?

[kevers, 2011-10-27 15:06:34]

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/repeating_button.js (right):

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:6: 
On 2011/10/26 21:49:46, James Hawkins wrote:
> nit: Remove blank line.

Done.

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:8: * Creates a new
button element.  The repeating button behaves like a
On 2011/10/26 21:49:46, James Hawkins wrote:
> nit: Single space between comments.

Updated all comments in this file to use single spacing between sentences.

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:24: holdDelayTime:
500,
On 2011/10/26 21:49:46, James Hawkins wrote:
> These vars are private, right?

Initially, I thought that it would be useful to keep these variables public;
however, in retrospect it is probably best to keep private until there is a need
to override the defaults.  Easy enough to add get/set methods later as required.

Fixed.

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:74: * Called when
the user presses the button.
On 2011/10/26 21:49:46, James Hawkins wrote:
> To remove ambiguity when referring to "this" button, s/the button/this button/

Updated docs for buttonDown_, buttonUp_ and buttonHeld_.

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:77: buttonDown:
function(e) {
On 2011/10/26 21:49:46, James Hawkins wrote:
> It seems a lot of these methods should be private.

Fixed.

http://codereview.chromium.org/8340002/diff/7003/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:81: // in agreement
with the defaults for the ChromeOS keyboard and virtual
On 2011/10/26 21:49:46, James Hawkins wrote:
> re: in agreement
> 
> Should we pull/set these values from some common location in the backend?

Added a TODO comment.

[James Hawkins, 2011-10-29 22:07:45]

LGTM with nit.

http://codereview.chromium.org/8340002/diff/8001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/repeating_button.js (right):

http://codereview.chromium.org/8340002/diff/8001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:99:
self.intervalCallback_ = setInterval(self.buttonHeld_.bind(self),
nit: Start of params need to line up on the same column, here and elsewhere.

[Rick Byers, 2011-10-31 15:35:55]

Channeling Arv, I have one suggestion for the cr.ui piece.

Also, you're adding a unit test for the repeating button, right?  Ideally that
would be in the same CL (but if there's someone blocked on getting the feature,
the test can be checked in separately afterwards).

Thanks!
   Rick

http://codereview.chromium.org/8340002/diff/12001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/repeating_button.js (right):

http://codereview.chromium.org/8340002/diff/12001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:138:
this.onButtonHeldAction();
To be consistent with other cr.ui components (and for additional flexibility), I
think you should use DOM events rather than have clients write to a callback
property.  You can use cr.dispatchSimpleEvent(this, 'buttonHeld').
See
http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/browser/resources/shared...
for an example.

[kevers, 2011-10-31 19:07:28]

http://codereview.chromium.org/8340002/diff/8001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/repeating_button.js (right):

http://codereview.chromium.org/8340002/diff/8001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:99:
self.intervalCallback_ = setInterval(self.buttonHeld_.bind(self),
On 2011/10/29 22:07:45, James Hawkins wrote:
> nit: Start of params need to line up on the same column, here and elsewhere.

Done.

http://codereview.chromium.org/8340002/diff/12001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/repeating_button.js (right):

http://codereview.chromium.org/8340002/diff/12001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/repeating_button.js:138:
this.onButtonHeldAction();
On 2011/10/31 15:35:56, Rick Byers wrote:
> To be consistent with other cr.ui components (and for additional flexibility),
I
> think you should use DOM events rather than have clients write to a callback
> property.  You can use cr.dispatchSimpleEvent(this, 'buttonHeld').
> See
>
http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/browser/resources/shared...
> for an example.

Done.

[Rick Byers, 2011-10-31 19:22:57]

On 2011/10/31 19:07:28, kevers wrote:
http://codereview.chromium.org/8340002/diff/12001/chrome/browser/resources/sh...
> chrome/browser/resources/shared/js/cr/ui/repeating_button.js:138:
> this.onButtonHeldAction();
> On 2011/10/31 15:35:56, Rick Byers wrote:
> > To be consistent with other cr.ui components (and for additional
flexibility),
> I
> > think you should use DOM events rather than have clients write to a callback
> > property.  You can use cr.dispatchSimpleEvent(this, 'buttonHeld').
> > See
> >
>
http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/browser/resources/shared...
> > for an example.
> 
> Done.

Thanks for switching to the event.  The change LGTM (with minor style nit).

[Rick Byers, 2011-10-31 19:23:20]

Whoops - meant to include this, here's the nit.
LGTM otherwise.

http://codereview.chromium.org/8340002/diff/19002/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/system_options.js (right):

http://codereview.chromium.org/8340002/diff/19002/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/system_options.js:86:
button.addEventListener(RepeatingButton.Event.BUTTON_HELD, handler);
style nit: please just put the anonymous function in-line, no need to introduce
a 'handler' variable here.

[commit-bot: I haz the power, 2011-10-31 21:00:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/8340002/24001

[commit-bot: I haz the power, 2011-10-31 22:17:53]

Change committed as 108022