Fix enabled state of an input field in the browser settings.

Fix enabled state of an input field in the browser settings.

BUG=80200
TEST=Go to 'chrome://settings/browser'.  The input field for 'Add a new page' should be editable only if the 'On startup' option is set to 'Open the following pages'.

[Rick Byers, 2011-05-31 17:35:25]

Looks good Kevin, thanks for fixing this!  I've got one style nit, and one
suggestion that may or may not be worth doing - but worth a little research if
you haven't already.

Rick

http://codereview.chromium.org/7094006/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/7094006/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options.js:407: // explicitly set
disabled state for input text elements
This approach seems OK.

I'm not sure if Chrome supports a way of disabling input fields from CSS (eg.
see Mozilla docs here: https://developer.mozilla.org/en/CSS/-moz-user-input). 
If it does, then it would be cleaner in my opinion to just toggle a 'disabled'
class on the top-level element, and have a CSS rule that disables the input
elements underneath it when applied.  Not a big deal, but worth a quick look to
see if this is possible/simpler.

http://codereview.chromium.org/7094006/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options.js:409: for( i = 0; i <
inputs.length; i++ ) {
Style nit: space before the open paren, not after it (see below for other
examples)

[kevers1, 2011-05-31 20:19:01]

Rick, I have updated the code based on your review.

Eric, could you please have a look at the change.

http://codereview.chromium.org/7094006/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/7094006/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options.js:407: // explicitly set
disabled state for input text elements
On 2011/05/31 17:35:25, rbyers wrote:
> This approach seems OK.
> 
> I'm not sure if Chrome supports a way of disabling input fields from CSS (eg.
> see Mozilla docs here: https://developer.mozilla.org/en/CSS/-moz-user-input). 
> If it does, then it would be cleaner in my opinion to just toggle a 'disabled'
> class on the top-level element, and have a CSS rule that disables the input
> elements underneath it when applied.  Not a big deal, but worth a quick look
to
> see if this is possible/simpler.

Looks like no such option exists for chrome.

http://codereview.chromium.org/7094006/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options.js:409: for( i = 0; i <
inputs.length; i++ ) {
On 2011/05/31 17:35:25, rbyers wrote:
> Style nit: space before the open paren, not after it (see below for other
> examples)

fixed

[Rick Byers, 2011-05-31 20:57:38]

On 2011/05/31 20:19:01, kevers1 wrote:
> Rick, I have updated the code based on your review.
> 
> Eric, could you please have a look at the change.
> 
>
http://codereview.chromium.org/7094006/diff/1/chrome/browser/resources/option...
> File chrome/browser/resources/options/browser_options.js (right):
> 
>
http://codereview.chromium.org/7094006/diff/1/chrome/browser/resources/option...
> chrome/browser/resources/options/browser_options.js:407: // explicitly set
> disabled state for input text elements
> On 2011/05/31 17:35:25, rbyers wrote:
> > This approach seems OK.
> > 
> > I'm not sure if Chrome supports a way of disabling input fields from CSS
(eg.
> > see Mozilla docs here:
https://developer.mozilla.org/en/CSS/-moz-user-input). 
> > If it does, then it would be cleaner in my opinion to just toggle a
'disabled'
> > class on the top-level element, and have a CSS rule that disables the input
> > elements underneath it when applied.  Not a big deal, but worth a quick look
> to
> > see if this is possible/simpler.
> 
> Looks like no such option exists for chrome.
> 
>
http://codereview.chromium.org/7094006/diff/1/chrome/browser/resources/option...
> chrome/browser/resources/options/browser_options.js:409: for( i = 0; i <
> inputs.length; i++ ) {
> On 2011/05/31 17:35:25, rbyers wrote:
> > Style nit: space before the open paren, not after it (see below for other
> > examples)
> 
> fixed

LGTM

[James Hawkins, 2011-06-08 17:38:31]

http://codereview.chromium.org/7094006/diff/1003/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/7094006/diff/1003/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:407: // explicitly set
disabled state for input text elements
s/explicitly/Explicitly/

Add period at end of sentence.

http://codereview.chromium.org/7094006/diff/1003/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:408: var inputs =
startupPagesList.getElementsByTagName("input");
You can shorten this by searching on selector, "input[type='text']".

[kevers, 2011-06-08 20:08:35]

http://codereview.chromium.org/7094006/diff/1003/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/7094006/diff/1003/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:408: var inputs =
startupPagesList.getElementsByTagName("input");
On 2011/06/08 17:38:31, James Hawkins wrote:
> You can shorten this by searching on selector, "input[type='text']".

Made the requested changes.

[James Hawkins, 2011-06-08 23:16:16]

LGTM with nit.

http://codereview.chromium.org/7094006/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/7094006/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:409: for (var i = 0; i <
inputs.length; i++ ) {
nit: Remove braces.

[kevers, 2011-06-10 13:51:38]

http://codereview.chromium.org/7094006/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/7094006/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:409: for (var i = 0; i <
inputs.length; i++ ) {
On 2011/06/08 23:16:17, James Hawkins wrote:
> nit: Remove braces.

Done.

[commit-bot: I haz the power, 2011-06-10 14:45:14]

Change committed as 88662