This updates the StaticIP configuration UI to match new mocks.

This updates the StaticIP configuration UI to match new mocks.

Splits out the IP routing parameters (address, netmask, gateway) from
the name server parameter, so that we can override just the name
servers or the routing parameters, and get the other one from DHCP.

Also adds a new widget class: EditableTextField, that allows a static
display value when not editable, and an edit field when editable, with
the ability to do validation and has a model that can be reverted.

This new class is used for the basis of an IP Address input field that
does correct IP address validation.

Also, the new StaticIP dialog now sets/gets the shill properties for
static IP instead of setting IPConfig objects (which isn't supported by
shill), which should fix manual static IP configuration.

BUG=chromium:129052
TEST=Ran on device and tested StaticIP configuration, ran some unit tests.


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

[Greg Spencer (Chromium), 2012-08-10 23:23:56]

Sorry for the large CL...

Steven, I need you to review the C++ changes.
Dan, I'd like a review of the JS code for options2 changes if you're game.

The UI mock I'm implementing is attached to the bug.

Thanks!

[Dan Beam, 2012-08-11 01:38:02]

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/internet_detail.css (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.css:59: border-top:
1px solid #EEE;
nit: I think we tend to prefer lowercase (e.g. #eee) instead

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/internet_detail.html (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.html:411:
type="checkbox">
nit: I think we tend to encourage non-ASCII indent here (i.e. 4 \s, not lined
up)

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.html:422: <td><div
id="ip-address"/></td>
can you close these tags? <div></div>

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.html:428: <td><div
id="ip-netmask"></td>
it seem otherwise people might forget to put a /, ;)

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.html:464: <td><div
id="ipconfig-dns1" i18n-placeholder-text="userNameServer1"
nit: whenever a tag's more than one line, it should have a \n in front of the
start tag (according to style guide), so this would be:

  <td>
    <div id="ipconfig-dns1" i18n-placeholder-text="userNameServer1"
        allow-empty>
    </div>
  </td>

(or something along these lines)

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/internet_detail.js (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:307:
$('ip-gateway')];
nit: you can probably fit all these $('id')s on the same line

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:590: var
ipFromDHCP = $('ip-automatic-configuration-checkbox').checked;
nit: ipFromDhcp (is probably the preferred capitalization, this prevents things
like CSSHTTPURL from being hard to read, :D)

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:605: // Clean out
any empty values.
nit: this is fine, but it may be less error prone in the future to start with an
empty [] and simply check each value is valid before inserting rather than
writing code that deletes from an iterator (essentially) :)

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:614:
$('ip-address').model.value ? $('ip-address').model.value : '';
how I'd write this:

  chrome.send('setIPConfig', [servicePath,
                              !!ipFromDhcp,
                              $('ip-address') || '',
                              $('ip-netmask').model.value || '',
                              $('ip-gateway').model.value || '',
                              nameServerType,
                              userNameServers]);

also, why are you sending 'true'/'false' rather than just a boolean? 
additionally, why cast String(servicePath)? it seems it'd already be this, no?

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:631: var editable
= (type == 'user');
nit: you don't really need these (), and the style guide says don't use
unnecessary (), so you probably want to remove

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:728: };
nit: could probably fit these all on the same line if you want

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:773: }
};

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:792: value:
nameServersUser[i] ? nameServersUser[i] : ''
nit: same trick here:

  value: nameServersUser[i] || ''

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js
(right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:4:

nit: one less \n here, IMO

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:13:
* @private
don't really need @private if this isn't a member of a class

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:16:
var singleIp_ = new RegExp('^\\s*([0-9]+)\\s*\\.\\s*([0-9]+)\\s*\\.' +
I'd highly recommend writing this in literal form and condensing:

  var singleIp_ = /^\s*(?:(\d{1,3})\s*\.\s*){3}(\d{1,3})\s*$/;

or stripping [^0-9.] characters and testing against a simpler RegExp:

  function prepIPForCheck(str) {
    return str.replace(/[^0-9.]/g, '');  // Implicitly trims.
  }
  var singleIp_ = /^(?:(\d{1,3})\.){3}(\d{1,3})$/;

(if that's possible)

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:27:
};
nit: no ; when using function declarations() { }

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:42:
/** @inheritDoc */
@inheritDoc is deprecated now, use @override instead

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:63:
var range_correct = true;
var rangeCorrent (camelCase in JavaScript)

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:68:
range_correct = false;
why not break; here?

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:71:
return (this.editField.validity.valid && matches != null &&
nit: less ()

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:81:
* Mutates the input during a successful commit.  For the purposes of
nit: 1 \s between sentences in comments (you have 2 \s here)

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:85:
* @param {Object} value Input IP address to be mutated.
shouldn't @param and @return be a {string}?

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:90:
return value;
nit: \n after this if ()

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:92:
var matches = singleIp_.exec(value);
seems like you could simplify this to:
  
  return (value.match(/(\d+)/) || []).map(function() {
    return parseInt(a, 10);
  }).join('.');

ability to deal with hackiness may be required, though... (to like this, :P)

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:94:
if (matches != null) {  // should never be null if we got this far
make a full sentence if this loop sticks around

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:97:
result.push(octet);
inline octect if this loop sticks around

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/editable_text_field.js (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:58: var localizedText =
loadTimeData.getString(identifier);
do you also need to check if loadTimeData.valueExists(indentifier) or will the
value always exist but just possibly be ''?

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:63: var self = this;
I think .bind() is preferred, i.e.

  this.editField_.addEventListener('focus',
                                   this.handleFocus_.bind(this));

rather than a var self or var that variation.

I'm also not sure about the indent, but I don't really care

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:89: if (text) {
nit: no curlies since all the bodys and conditionals are 1 line

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:173:
this.checkForEmpty_();  // Also updates the editField value
nit: . at end (even if it's a sentence fragment, IMO)

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:179: * if a specific
element wasn't clicked.
nit: you can leave this \n here between sentences if you want, but it's not
common

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:182: * @type
{HTMLElement}
@type {?HTMLElement} is probably more appropriate here (as querySelector() can
return null)

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:210: * Mutates the
input during a successful commit.  Can be overridden to
nit: 1 \s between sentences

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:234:
this.appendChild(textEl);
do you ever only call this method once?  might be worth checking to see if
there's a current |this.staticText_| and removing it from the DOM if it exists
so you don't leak DOM nodes

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:260: if
(editField.tagName == 'INPUT')
nit: if (editField instanceof HTMLInputElement) IMO

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:373:
handleWindowBlurs();
why are you making this a function then calling it? why not just call it and
strip the function(){} part?

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/inline_editable_list.js (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/inline_editable_list.js:299: * Called a key is
pressed. Handles committing and canceling edits.
called *when* a key is pressed

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/options_page.css (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/options_page.css:163: padding-top: 2px;
padding: 2px 3px;

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/options_page.css:169: padding: 4px;
padding: 3px 4px;

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/options_page.css:185: color: #CCC;
nit: #ccc

[stevenjb, 2012-08-13 17:43:44]

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/cros_network_functions.cc (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cros_network_functions.cc:794: }
Nit: this check might make a little more sense where count is incremented.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library_impl_cros.cc (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library_impl_cros.cc:1182: VLOG(2) <<
shill::kStaticIPAddressProperty << "=" << current_address;
I think these could all be VLOG_IF(2, foo_exists) << ...

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library_impl_cros.cc:1198: << "=" <<
current_name_servers;
nit: align << with << here and elsewhere.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library_impl_cros.cc:1273: 
nit: 'if (!something_changed) return;' early exit here.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library_impl_cros.cc:1281: }
nit: blank line

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library_impl_cros.h (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library_impl_cros.h:139: const
base::DictionaryValue* properties);
Does this need to be part of the public API? If so, then I suggest having
SetIPParameters also use IPParemeterInfo. Otherwise I would make this and
IPParemeterInfo private (and forward declare IPParemeterInfo, defining it in the
C++ file).

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:294:
std::string value;
nit: I think this would be more readable using separate specifically named
variables instead of reusing 'value'

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:297:
ip_info_dict->SetString("address", value);
"address" and other JS (I assume?) keys should be defined as constants at the
top of this file.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:915: if
(name_server_type == "automatic") {
nit: kNameServerTypeAutomatic

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:918: }
else if (name_server_type == "google") {
nit: kNameServerTypeGoogle

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:935: //
before we continue.
nit: 'before we continue' implies waiting to me; 'and continue in the callback'
might be better.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:979:
ipconfig_dhcp->SetString("address", ipconfig.address);
Use consts

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:998:
dictionary.SetBoolean("ipAutoConfig", automatic_ip_config == 0);
Use const here and below

[Greg Spencer (Chromium), 2012-08-13 19:20:03]

Sorry for all the style nits: this is the first time I've made significant webui
changes, so I'm still getting the style rules down.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/internet_detail.css (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.css:59: border-top:
1px solid #EEE;
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: I think we tend to prefer lowercase (e.g. #eee) instead

Fixed here and below.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/internet_detail.html (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.html:411:
type="checkbox">
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: I think we tend to encourage non-ASCII indent here (i.e. 4 \s, not lined
> up)

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.html:422: <td><div
id="ip-address"/></td>
On 2012/08/11 01:38:02, Dan Beam wrote:
> can you close these tags? <div></div>

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.html:428: <td><div
id="ip-netmask"></td>
On 2012/08/11 01:38:02, Dan Beam wrote:
> it seem otherwise people might forget to put a /, ;)

Haha! I see your point. :)  Fixed, here and below.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.html:464: <td><div
id="ipconfig-dns1" i18n-placeholder-text="userNameServer1"
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: whenever a tag's more than one line, it should have a \n in front of the
> start tag (according to style guide), so this would be:
> 
>   <td>
>     <div id="ipconfig-dns1" i18n-placeholder-text="userNameServer1"
>         allow-empty>
>     </div>
>   </td>
> 
> (or something along these lines)

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/internet_detail.js (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:307:
$('ip-gateway')];
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: you can probably fit all these $('id')s on the same line

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:590: var
ipFromDHCP = $('ip-automatic-configuration-checkbox').checked;
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: ipFromDhcp (is probably the preferred capitalization, this prevents
things
> like CSSHTTPURL from being hard to read, :D)

Yeah, I actually prefer the CamelCase capitalization, for that reason, but
wasn't sure what the convention was here (we waffle on that quite a bit on the
C++ side).  Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:605: // Clean out
any empty values.
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: this is fine, but it may be less error prone in the future to start with
an
> empty [] and simply check each value is valid before inserting rather than
> writing code that deletes from an iterator (essentially) :)

Hmm. Good point. I've rearranged it that way.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:614:
$('ip-address').model.value ? $('ip-address').model.value : '';
On 2012/08/11 01:38:02, Dan Beam wrote:
> how I'd write this:
> 
>   chrome.send('setIPConfig', [servicePath,
>                               !!ipFromDhcp,
>                               $('ip-address') || '',
>                               $('ip-netmask').model.value || '',
>                               $('ip-gateway').model.value || '',
>                               nameServerType,
>                               userNameServers]);
> 
> also, why are you sending 'true'/'false' rather than just a boolean? 
> additionally, why cast String(servicePath)? it seems it'd already be this, no?

Both good points.  I was just inheriting how it was written before, but I've now
updated the C++ side to expect a boolean, and removed the String().

I don't like the "!!" notation: it's easy to miss, or to interpret as a typo, so
I chose to use ipFromDhcp || false.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:631: var editable
= (type == 'user');
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: you don't really need these (), and the style guide says don't use
> unnecessary (), so you probably want to remove

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:728: };
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: could probably fit these all on the same line if you want

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:773: }
On 2012/08/11 01:38:02, Dan Beam wrote:
> };

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:792: value:
nameServersUser[i] ? nameServersUser[i] : ''
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: same trick here:
> 
>   value: nameServersUser[i] || ''

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js
(right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:16:
var singleIp_ = new RegExp('^\\s*([0-9]+)\\s*\\.\\s*([0-9]+)\\s*\\.' +
On 2012/08/11 01:38:02, Dan Beam wrote:
> I'd highly recommend writing this in literal form and condensing:
> 
>   var singleIp_ = /^\s*(?:(\d{1,3})\s*\.\s*){3}(\d{1,3})\s*$/;
> 
> or stripping [^0-9.] characters and testing against a simpler RegExp:
> 
>   function prepIPForCheck(str) {
>     return str.replace(/[^0-9.]/g, '');  // Implicitly trims.
>   }
>   var singleIp_ = /^(?:(\d{1,3})\.){3}(\d{1,3})$/;
> 
> (if that's possible)

Yeah, I tried that already, but the problem is that then the match object only
returns the last of the first three octets instead of all three (yes, even with
the (?:), sadly).  And I went with the "new RegExp" form instead of the literal
form because I couldn't figure out how to split the literal form across lines.

I'll try splitting out the whitespace removal (I don't like having non-literal
RegExps either!).  Your way won't work though: I can't remove non-whitespace
characters when doing a validity check.  I'm just removing whitespace instead,
because I want that to be largely irrelevant.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:27:
};
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: no ; when using function declarations() { }

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:42:
/** @inheritDoc */
On 2012/08/11 01:38:02, Dan Beam wrote:
> @inheritDoc is deprecated now, use @override instead

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:63:
var range_correct = true;
On 2012/08/11 01:38:02, Dan Beam wrote:
> var rangeCorrent (camelCase in JavaScript)

aaagh!  Switching back and forth between C++ and JavaScript can drive one mad.
Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:68:
range_correct = false;
On 2012/08/11 01:38:02, Dan Beam wrote:
> why not break; here?

Why indeed.  Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:71:
return (this.editField.validity.valid && matches != null &&
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: less ()

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:81:
* Mutates the input during a successful commit.  For the purposes of
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: 1 \s between sentences in comments (you have 2 \s here)

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:85:
* @param {Object} value Input IP address to be mutated.
On 2012/08/11 01:38:02, Dan Beam wrote:
> shouldn't @param and @return be a {string}?

Yeah, I guess so.  I was originally going to mutate the model, but now just the
input string.  Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:90:
return value;
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: \n after this if ()

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:92:
var matches = singleIp_.exec(value);
On 2012/08/11 01:38:02, Dan Beam wrote:
> seems like you could simplify this to:
>   
>   return (value.match(/(\d+)/) || []).map(function() {
>     return parseInt(a, 10);
>   }).join('.');
> 
> ability to deal with hackiness may be required, though... (to like this, :P)

Well, and the fact that it doesn't quite work.. :)

The regexp in the match would have to be my regexp, since the one you have only
returns the first octet (twice, since match() returns the whole match first
too).

In any case, I think I prefer the verbose version: it's easy to follow what's
happening, and it's not really any slower.  I'll simplify it as you suggest
below.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:94:
if (matches != null) {  // should never be null if we got this far
On 2012/08/11 01:38:02, Dan Beam wrote:
> make a full sentence if this loop sticks around

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:97:
result.push(octet);
On 2012/08/11 01:38:02, Dan Beam wrote:
> inline octect if this loop sticks around

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/editable_text_field.js (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:58: var localizedText =
loadTimeData.getString(identifier);
On 2012/08/11 01:38:02, Dan Beam wrote:
> do you also need to check if loadTimeData.valueExists(indentifier) or will the
> value always exist but just possibly be ''?

No, you're right, I should probably check that too.  Do you think I should log a
console message if I can't find the i18n string?

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:63: var self = this;
On 2012/08/11 01:38:02, Dan Beam wrote:
> I think .bind() is preferred, i.e.
> 
>   this.editField_.addEventListener('focus',
>                                    this.handleFocus_.bind(this));
> 
> rather than a var self or var that variation.
> 
> I'm also not sure about the indent, but I don't really care

OK, thanks. Wasn't sure about bind here.  Indent isn't an issue because it fits
on one line then.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:89: if (text) {
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: no curlies since all the bodys and conditionals are 1 line

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:173:
this.checkForEmpty_();  // Also updates the editField value
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: . at end (even if it's a sentence fragment, IMO)

Sorry, that was sloppy: I usually like that too.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:179: * if a specific
element wasn't clicked.
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: you can leave this \n here between sentences if you want, but it's not
> common

Done, here and below.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:182: * @type
{HTMLElement}
On 2012/08/11 01:38:02, Dan Beam wrote:
> @type {?HTMLElement} is probably more appropriate here (as querySelector() can
> return null)

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:210: * Mutates the
input during a successful commit.  Can be overridden to
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: 1 \s between sentences

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:234:
this.appendChild(textEl);
On 2012/08/11 01:38:02, Dan Beam wrote:
> do you ever only call this method once?  might be worth checking to see if
> there's a current |this.staticText_| and removing it from the DOM if it exists
> so you don't leak DOM nodes

Yes, it's only called once (well, it's called only if this.editField_ is empty
when decoration occurs).

It shouldn't leak nodes in that case, but I'm enforcing that it's only called
once here now, instead of in decorate().

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:260: if
(editField.tagName == 'INPUT')
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: if (editField instanceof HTMLInputElement) IMO

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:373:
handleWindowBlurs();
On 2012/08/11 01:38:02, Dan Beam wrote:
> why are you making this a function then calling it? why not just call it and
> strip the function(){} part?

Yeah, good point.  It was a pattern I saw in some other code, and didn't really
think much about it.  Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/inline_editable_list.js (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/inline_editable_list.js:299: * Called a key is
pressed. Handles committing and canceling edits.
On 2012/08/11 01:38:02, Dan Beam wrote:
> called *when* a key is pressed

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/options_page.css (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/options_page.css:163: padding-top: 2px;
On 2012/08/11 01:38:02, Dan Beam wrote:
> padding: 2px 3px;

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/options_page.css:169: padding: 4px;
On 2012/08/11 01:38:02, Dan Beam wrote:
> padding: 3px 4px;

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/options_page.css:185: color: #CCC;
On 2012/08/11 01:38:02, Dan Beam wrote:
> nit: #ccc

Done.

[Dan Beam, 2012-08-13 19:48:55]

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/internet_detail.js (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:614:
$('ip-address').model.value ? $('ip-address').model.value : '';
On 2012/08/13 19:20:04, Greg Spencer (Chromium) wrote:
> On 2012/08/11 01:38:02, Dan Beam wrote:
> > how I'd write this:
> > 
> >   chrome.send('setIPConfig', [servicePath,
> >                               !!ipFromDhcp,
> >                               $('ip-address') || '',
> >                               $('ip-netmask').model.value || '',
> >                               $('ip-gateway').model.value || '',
> >                               nameServerType,
> >                               userNameServers]);
> > 
> > also, why are you sending 'true'/'false' rather than just a boolean? 
> > additionally, why cast String(servicePath)? it seems it'd already be this,
no?
> 
> Both good points.  I was just inheriting how it was written before, but I've
now
> updated the C++ side to expect a boolean, and removed the String().
> 
> I don't like the "!!" notation: it's easy to miss, or to interpret as a typo,
so
> I chose to use ipFromDhcp || false.

OK, that's fine.  You can also say Boolean(someTruthyOrFalseyValue), btw, and it
casts to a Boolean() the same way !! does.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js
(right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:16:
var singleIp_ = new RegExp('^\\s*([0-9]+)\\s*\\.\\s*([0-9]+)\\s*\\.' +
On 2012/08/13 19:20:04, Greg Spencer (Chromium) wrote:
> On 2012/08/11 01:38:02, Dan Beam wrote:
> > I'd highly recommend writing this in literal form and condensing:
> > 
> >   var singleIp_ = /^\s*(?:(\d{1,3})\s*\.\s*){3}(\d{1,3})\s*$/;
> > 
> > or stripping [^0-9.] characters and testing against a simpler RegExp:
> > 
> >   function prepIPForCheck(str) {
> >     return str.replace(/[^0-9.]/g, '');  // Implicitly trims.
> >   }
> >   var singleIp_ = /^(?:(\d{1,3})\.){3}(\d{1,3})$/;
> > 
> > (if that's possible)
> 
> Yeah, I tried that already, but the problem is that then the match object only
> returns the last of the first three octets instead of all three (yes, even
with
> the (?:), sadly).

were you passing the /g (global) flag to the RegExp and/or making it non-greedy
with "?"?

"?" is the non-greedy modifier to "+" operator, means the match will have to
occur more than once but don't keep consuming greedily, .e.g. 'aaa'.match(/a+/g)
vs. 'aaa'.match(/a+?/g))?

> And I went with the "new RegExp" form instead of the literal
> form because I couldn't figure out how to split the literal form across lines.
> 
> I'll try splitting out the whitespace removal (I don't like having non-literal
> RegExps either!).  Your way won't work though: I can't remove non-whitespace
> characters when doing a validity check.  I'm just removing whitespace instead,
> because I want that to be largely irrelevant.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/editable_text_field.js (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/editable_text_field.js:58: var localizedText =
loadTimeData.getString(identifier);
On 2012/08/13 19:20:04, Greg Spencer (Chromium) wrote:
> On 2012/08/11 01:38:02, Dan Beam wrote:
> > do you also need to check if loadTimeData.valueExists(indentifier) or will
the
> > value always exist but just possibly be ''?
> 
> No, you're right, I should probably check that too.  Do you think I should log
a
> console message if I can't find the i18n string?

If you assume it's there, just use the value.  loadTimeData itself will squawk
about the value not being there on the console if it doesn't exist.

[Dan Beam, 2012-08-13 19:49:27]

will give this another round of review soon

[Greg Spencer (Chromium), 2012-08-13 22:17:44]

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/internet_detail.js (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail.js:614:
$('ip-address').model.value ? $('ip-address').model.value : '';
On 2012/08/13 19:48:55, Dan Beam wrote:
> OK, that's fine.  You can also say Boolean(someTruthyOrFalseyValue), btw, and
it
> casts to a Boolean() the same way !! does.

Ooh, I like that better.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js
(right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:16:
var singleIp_ = new RegExp('^\\s*([0-9]+)\\s*\\.\\s*([0-9]+)\\s*\\.' +
On 2012/08/13 19:48:55, Dan Beam wrote:
> were you passing the /g (global) flag to the RegExp and/or making it
non-greedy
> with "?"?
> 
> "?" is the non-greedy modifier to "+" operator, means the match will have to
> occur more than once but don't keep consuming greedily, .e.g.
'aaa'.match(/a+/g)
> vs. 'aaa'.match(/a+?/g))?

Yeah, I'm pretty familiar with regexps, and no, I wasn't using ? or /g. The code
you gave above returns the last one only:

-> var singleIp_ = /^(?:(\d{1,3})\.){3}(\d{1,3})$/;
-> singleIp_.exec("111.222.333.444")
["111.222.333.444", "333", "444"]

And variations on that don't change things:

-> var singleIp_ = /^(?:(\d{1,3})\.){3}?(\d{1,3})$/;
-> singleIp_.exec("111.222.333.444")
["111.222.333.444", "333", "444"]
-> var singleIp_ = /^(?:(\d{1,3})\.)+?(\d{1,3})$/;
-> singleIp_.exec("111.222.333.444")
["111.222.333.444", "333", "444"]
-> var singleIp_ = /^((\d{1,3})\.)+?(\d{1,3})$/;
-> singleIp_.exec("111.222.333.444")
["111.222.333.444", "333.", "333", "444"]
-> var singleIp_ = /^(?:(\d{1,3})\.)+(\d{1,3})$/g;
-> singleIp_.exec("111.222.333.444")
["111.222.333.444", "333", "444"]

The result I need is:
["111.222.333.444", "111", "222", "333", "444"]

The problem is that the regexp engine will only return one result in the match
array for each opening "(", regardless of how many times it matches.

[Dan Beam, 2012-08-13 22:56:17]

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
File
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js
(right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:16:
var singleIp_ = new RegExp('^\\s*([0-9]+)\\s*\\.\\s*([0-9]+)\\s*\\.' +
On 2012/08/13 22:17:44, Greg Spencer (Chromium) wrote:
> On 2012/08/13 19:48:55, Dan Beam wrote:
> > were you passing the /g (global) flag to the RegExp and/or making it
> non-greedy
> > with "?"?
> > 
> > "?" is the non-greedy modifier to "+" operator, means the match will have to
> > occur more than once but don't keep consuming greedily, .e.g.
> 'aaa'.match(/a+/g)
> > vs. 'aaa'.match(/a+?/g))?
> 
> Yeah, I'm pretty familiar with regexps, and no, I wasn't using ? or /g. The
code
> you gave above returns the last one only:
> 
> -> var singleIp_ = /^(?:(\d{1,3})\.){3}(\d{1,3})$/;
> -> singleIp_.exec("111.222.333.444")
> ["111.222.333.444", "333", "444"]
> 
> And variations on that don't change things:
> 
> -> var singleIp_ = /^(?:(\d{1,3})\.){3}?(\d{1,3})$/;
> -> singleIp_.exec("111.222.333.444")
> ["111.222.333.444", "333", "444"]
> -> var singleIp_ = /^(?:(\d{1,3})\.)+?(\d{1,3})$/;
> -> singleIp_.exec("111.222.333.444")
> ["111.222.333.444", "333", "444"]
> -> var singleIp_ = /^((\d{1,3})\.)+?(\d{1,3})$/;
> -> singleIp_.exec("111.222.333.444")
> ["111.222.333.444", "333.", "333", "444"]
> -> var singleIp_ = /^(?:(\d{1,3})\.)+(\d{1,3})$/g;
> -> singleIp_.exec("111.222.333.444")
> ["111.222.333.444", "333", "444"]
> 
> The result I need is:
> ["111.222.333.444", "111", "222", "333", "444"]
> 
> The problem is that the regexp engine will only return one result in the match
> array for each opening "(", regardless of how many times it matches.

Yeah, OK.

[Greg Spencer (Chromium), 2012-08-13 23:53:43]

Addressing Steven's comments.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/cros_network_functions.cc (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cros_network_functions.cc:794: }
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> Nit: this check might make a little more sense where count is incremented.

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library_impl_cros.cc (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library_impl_cros.cc:1182: VLOG(2) <<
shill::kStaticIPAddressProperty << "=" << current_address;
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> I think these could all be VLOG_IF(2, foo_exists) << ...

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library_impl_cros.cc:1198: << "=" <<
current_name_servers;
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> nit: align << with << here and elsewhere.

I blame Eclipse. :-)  Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library_impl_cros.cc:1273: 
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> nit: 'if (!something_changed) return;' early exit here.

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library_impl_cros.cc:1281: }
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> nit: blank line

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library_impl_cros.h (right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library_impl_cros.h:139: const
base::DictionaryValue* properties);
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> Does this need to be part of the public API? If so, then I suggest having
> SetIPParameters also use IPParemeterInfo. Otherwise I would make this and
> IPParemeterInfo private (and forward declare IPParemeterInfo, defining it in
the
> C++ file).

No, I guess it doesn't.  I had thought that it needed to be public for Bind to
work, but I guess not.  Fixed.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:294:
std::string value;
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> nit: I think this would be more readable using separate specifically named
> variables instead of reusing 'value'

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:297:
ip_info_dict->SetString("address", value);
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> "address" and other JS (I assume?) keys should be defined as constants at the
> top of this file.

Done, but I'd like to point out that there are several hundred other instances
of this problem in this file.  I could fix them all, but I'm not sure it's worth
it.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:915: if
(name_server_type == "automatic") {
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> nit: kNameServerTypeAutomatic

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:918: }
else if (name_server_type == "google") {
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> nit: kNameServerTypeGoogle

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:935: //
before we continue.
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> nit: 'before we continue' implies waiting to me; 'and continue in the
callback'
> might be better.

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:979:
ipconfig_dhcp->SetString("address", ipconfig.address);
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> Use consts

Done.

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:998:
dictionary.SetBoolean("ipAutoConfig", automatic_ip_config == 0);
On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> Use const here and below

Done in my changes at least.  Do you want me to fix the entire file (I can, if
you want, but there's a heck of a lot of one-time-use strings in here)?

[stevenjb, 2012-08-14 01:06:38]

Thanks for the changes.
LGTM (w/ a minor nit)

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:297:
ip_info_dict->SetString("address", value);
On 2012/08/13 23:53:44, Greg Spencer (Chromium) wrote:
> On 2012/08/13 17:43:44, stevenjb (chromium) wrote:
> > "address" and other JS (I assume?) keys should be defined as constants at
the
> > top of this file.
> 
> Done, but I'd like to point out that there are several hundred other instances
> of this problem in this file.  I could fix them all, but I'm not sure it's
worth
> it.
> 

Thanks for doing these. Anything that is used as a key between C++ and Web UI
really ought to (especially) be defined as a const. It makes finding the shared
keys easier and they tend to be easier to search for within the file. 

If there are other such occurrences in this file, we don't need to fix them all,
but we should definitely use the new consts anywhere that is appropriate.

http://codereview.chromium.org/10827283/diff/12004/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/cros_network_functions.cc (right):

http://codereview.chromium.org/10827283/diff/12004/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/cros_network_functions.cc:822: if (count == 4) {
nit: if (++count == 4), no {}

[Greg Spencer (Chromium), 2012-08-14 01:19:26]

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/10827283/diff/1025/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:297:
ip_info_dict->SetString("address", value);
On 2012/08/14 01:06:38, stevenjb (chromium) wrote:
> Thanks for doing these. Anything that is used as a key between C++ and Web UI
> really ought to (especially) be defined as a const. It makes finding the
shared
> keys easier and they tend to be easier to search for within the file. 
> 
> If there are other such occurrences in this file, we don't need to fix them
all,
> but we should definitely use the new consts anywhere that is appropriate.

OK, I do agree that they *should* be.  I'll take a stab at fixing everything in
the file, since if it should be that way, we should be consistent.

http://codereview.chromium.org/10827283/diff/12004/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/cros_network_functions.cc (right):

http://codereview.chromium.org/10827283/diff/12004/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/cros_network_functions.cc:822: if (count == 4) {
On 2012/08/14 01:06:38, stevenjb (chromium) wrote:
> nit: if (++count == 4), no {}

Done, plus the one up above too.

[Dan Beam, 2012-08-14 03:59:56]

chrome/browser/resources/* lgtm w/nits

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/internet_detail.js (right):

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.js:639: if (type ==
'google') {
nit: these should probably be else if () or switch (type) eh?

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.js:711: var
inetAddress = { autoConfig: ipAutoConfig };
nit: {autoConfig: ipAutoConfig} (no spaces)

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.js:757:
field.editable = (model.autoConfig == 'user');
nit: less ()s here

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.js:765:
$('automatic-dns-display').textContent = data.ipconfig.value.nameServers;
opt nit: \n after each if () here

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.js:776:
nameServerModels.push({ value: nameServersUser[i] || '' });
nit: {value: nameServersUser[i] || ''} (no spaces again)

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
File
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js
(right):

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:4:

nit: remove 1 \n

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:100:
for (var i = 1; i < matches.length; ++i)
nit: might want to comment that you're starting at 1 because the whole match
text will be in matches[0] (otherwise looks like potential off-by-one)

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
File chrome/browser/resources/options2/editable_text_field.js (right):

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:22: * @type
{HTMLElement}
nit: I think these are all technically supposed to have a ? in front of the
@type, e.g. {?HTMLElement}, but I'm not totally sure. Annotating @types for (at
least for closure) are confusing in this sense as they say complex type are
"nullable" but this actually means @type {HTMLElement} creates a type list of
(HTMLElement|undefined) but doesn't include null.

tl;dr - {?HTMLElement}, also we don't even compile this code, so it's just for
the future folks that try to do this.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:210: * @param {String}
value Input text to be mutated.
nit: I think technically these should be {string} vs {String} as they're not a
new String() (i.e. instanceof String, the wrapper class)

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:277: if
(editField.tagName == 'INPUT') {
nit: same nit as before - editField instanceof HTMLInputElement

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:296: editField.value =
this.placeholderText ? this.placeholderText : '';
nit: this.placeHolderText || ''

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:310: return;
nit: \n after if ()

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:323: return;
nit: \n after if ()

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:336: var endEdit =
false;
nit: you don't really need to initialize this to false, but I guess it doesn't
hurt... (ah the typeless world!)

[Greg Spencer (Chromium), 2012-08-14 16:54:53]

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/internet_detail.js (right):

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.js:639: if (type ==
'google') {
On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: these should probably be else if () or switch (type) eh?

Done.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.js:711: var
inetAddress = { autoConfig: ipAutoConfig };
On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: {autoConfig: ipAutoConfig} (no spaces)

Done, and elsewhere.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.js:757:
field.editable = (model.autoConfig == 'user');
On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: less ()s here

Done.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.js:765:
$('automatic-dns-display').textContent = data.ipconfig.value.nameServers;
On 2012/08/14 03:59:56, Dan Beam wrote:
> opt nit: \n after each if () here

Done.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.js:776:
nameServerModels.push({ value: nameServersUser[i] || '' });
On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: {value: nameServersUser[i] || ''} (no spaces again)

Done.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
File
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js
(right):

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:4:

On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: remove 1 \n

Done.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail_ip_address_field.js:100:
for (var i = 1; i < matches.length; ++i)
On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: might want to comment that you're starting at 1 because the whole match
> text will be in matches[0] (otherwise looks like potential off-by-one)

Done.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
File chrome/browser/resources/options2/editable_text_field.js (right):

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:22: * @type
{HTMLElement}
On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: I think these are all technically supposed to have a ? in front of the
> @type, e.g. {?HTMLElement}, but I'm not totally sure. Annotating @types for
(at
> least for closure) are confusing in this sense as they say complex type are
> "nullable" but this actually means @type {HTMLElement} creates a type list of
> (HTMLElement|undefined) but doesn't include null.
> 
> tl;dr - {?HTMLElement}, also we don't even compile this code, so it's just for
> the future folks that try to do this.

OK, understood.  Done.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:210: * @param {String}
value Input text to be mutated.
On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: I think technically these should be {string} vs {String} as they're not a
> new String() (i.e. instanceof String, the wrapper class)

Done.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:277: if
(editField.tagName == 'INPUT') {
On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: same nit as before - editField instanceof HTMLInputElement

Done.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:296: editField.value =
this.placeholderText ? this.placeholderText : '';
On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: this.placeHolderText || ''

Done.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:310: return;
On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: \n after if ()

Done.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:323: return;
On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: \n after if ()

Done.

http://codereview.chromium.org/10827283/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/editable_text_field.js:336: var endEdit =
false;
On 2012/08/14 03:59:56, Dan Beam wrote:
> nit: you don't really need to initialize this to false, but I guess it doesn't
> hurt... (ah the typeless world!)

Hmm.  Good point. :)  It seems wrong somehow to leave it as "undefined" to
indicate "false" though.  It works just fine, of course.  I guess I'll embrace
typeless coding and leave it undefined.

[Greg Spencer (Chromium), 2012-08-14 17:26:03]

On 2012/08/14 01:19:26, Greg Spencer (Chromium) wrote:
> OK, I do agree that they *should* be.  I'll take a stab at fixing everything
in
> the file, since if it should be that way, we should be consistent.

OK, I've converted all the literals in this file to constants.  I scripted it a
bit, so check it and see if you like the result (mainly the names).  I split out
the i18n strings and the function names from the rest of the tags.

[stevenjb, 2012-08-14 17:50:00]

Thanks for doing this, generally I think this is much better. I do however think
the kStr* constants are unnecessary, but otherwise this lgtm. (fwiw, if there
were errors I don't think it likely that I caught them, but if it compiles and
generally looks OK...)

http://codereview.chromium.org/10827283/diff/6007/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/10827283/diff/6007/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:232: const
char kStrWimaxTitle[] = "wimaxTitle";
Since these are all just used once in a table, I'm not sure there is any benefit
to extracting them out.

http://codereview.chromium.org/10827283/diff/6007/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler.cc:860:
base::Unretained(this)));
Same with these tags - presumably they are used just once here for registration
(although, since it's not really a table, they're fine as constants).

[Greg Spencer (Chromium), 2012-08-14 18:01:47]

On 2012/08/14 17:50:00, stevenjb (chromium) wrote:
> Thanks for doing this, generally I think this is much better. I do however
think
> the kStr* constants are unnecessary, but otherwise this lgtm. (fwiw, if there
> were errors I don't think it likely that I caught them, but if it compiles and
> generally looks OK...)

Yes, it compiles and generally looks OK. :)

I reverted the i18n strings and put them back in the table.
I split out the message handler strings into a separate section and renamed them
(kFooMessage), since that seemed to make more sense.

[commit-bot: I haz the power, 2012-08-14 18:04:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/10827283/3011

[stevenjb, 2012-08-14 18:05:28]

Thanks! lgtm

[commit-bot: I haz the power, 2012-08-14 20:40:12]

Try job failure for 10827283-3011 (retry) on linux_chromeos for step
"unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-14 21:03:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/10827283/1033

[commit-bot: I haz the power, 2012-08-14 23:30:15]

Change committed as 151600