Tidy up <select>s in tabbed options.

Tidy up <select>s in tabbed options.

Use pref ui for <select> where possible. This simplifies the zoom select and fixes the sync select.

Also add more error checking.

BUG=none
TEST=manual

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

[stuartmorgan, 2011-01-12 22:59:41]

http://codereview.chromium.org/6174009/diff/1/chrome/browser/dom_ui/options/c...
File chrome/browser/dom_ui/options/core_options_handler.cc (right):

http://codereview.chromium.org/6174009/diff/1/chrome/browser/dom_ui/options/c...
chrome/browser/dom_ui/options/core_options_handler.cc:189:
CHECK(base::StringToInt(value_string, &int_value));
We're going to crash the whole browser for end users if a value from JS is ever
not what we expect? That seems like a really bad failure mode.

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

http://codereview.chromium.org/6174009/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options.js:361: setDefaultSearchEngine:
function() {
Why is this still here?

Or, alternately, why is it better to have this be an anonymous function instead
of binding onchange to this method? Having a named and documented function seems
like a win to me for readability, so I don't understand the point of this
change.

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

http://codereview.chromium.org/6174009/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/pref_ui.js:449: */
The code I've seen doesn't have separate docs for getters and setters, just one
doc explaining what the property means, and with a @type line.

[Evan Stade, 2011-01-12 23:19:20]

http://codereview.chromium.org/6174009/diff/1/chrome/browser/dom_ui/options/c...
File chrome/browser/dom_ui/options/core_options_handler.cc (right):

http://codereview.chromium.org/6174009/diff/1/chrome/browser/dom_ui/options/c...
chrome/browser/dom_ui/options/core_options_handler.cc:189:
CHECK(base::StringToInt(value_string, &int_value));
On 2011/01/12 22:59:41, stuartmorgan wrote:
> We're going to crash the whole browser for end users if a value from JS is
ever
> not what we expect? That seems like a really bad failure mode.

yes. We have complete control over the options page. If this check fails, there
is a bug we need to fix. Making the failure recoverable will cause us to not
find and fix it. From the chrome style guide:

"""
it is preferable to crash instead of handling a DCHECK failure. Attempting to
handle a DCHECK failure in a graceful manner is a statement that the DCHECK can
fail, which contradicts the point of writing the DCHECK [...] In some cases the
consequences of a failed DCHECK may result in a crash. This is not necessarily a
terrible thing. The crash will be logged, and that will result in visibility to
the fact that the DCHECK fails in the wild. It is far better to receive detailed
crash reports than vague anecdotes about unreproducible hangs, dead clicks, or
other misbehaving non-crash conditions.
"""

(In my mind, as long as we are not recovering, failing fast via a CHECK is
preferable to DCHECK)

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

http://codereview.chromium.org/6174009/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options.js:361: setDefaultSearchEngine:
function() {
On 2011/01/12 22:59:41, stuartmorgan wrote:
> Why is this still here?
> 
> Or, alternately, why is it better to have this be an anonymous function
instead
> of binding onchange to this method? Having a named and documented function
seems
> like a win to me for readability, so I don't understand the point of this
> change.

well, it's not supposed to be... i will remove the stub.

I wanted to remove the onchange handler from the .html because it seems to me
that javascript doesn't belong there. In doing so I matched the style of the
other handlers above, which used anonymous functions. An additional win is that
the code is more self-documenting now and doesn't need other comments.

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

http://codereview.chromium.org/6174009/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/pref_ui.js:449: */
On 2011/01/12 22:59:41, stuartmorgan wrote:
> The code I've seen doesn't have separate docs for getters and setters, just
one
> doc explaining what the property means, and with a @type line.

i copied this style from within this same file.

[stuartmorgan, 2011-01-12 23:30:09]

Okay, LGTM with stub removed.

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

http://codereview.chromium.org/6174009/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options.js:361: setDefaultSearchEngine:
function() {
On 2011/01/12 23:19:20, Evan Stade wrote:
> In doing so I matched the style of the other handlers above,
> which used anonymous functions.

Well, I wouldn't have made the other non-trivial function there inline either.

> An additional win is that the code is more self-documenting now

If you bound to this.setDefaultSearchEngine in initializePage (which I agree is
better than having it in the HTML), then there's a function name that describes
the behavior. Inlining it removes the function name; I don't see how it can
possibly be more self-documenting when the only change is having less
human-readable information.

I guess if you feel strongly about this I'm okay with it, but it feels like
unnecessary thrashing on personal style.

[Evan Stade, 2011-01-12 23:38:47]

ok. restored the function. Changed the getter/setter to the better style (also
updated getter/setter for pref).

[arv (Not doing code reviews), 2011-01-13 20:51:10]

LGTM

http://codereview.chromium.org/6174009/diff/11001/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/6174009/diff/11001/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:65:
$('defaultSearchEngine').onchange = this.setDefaultSearchEngine;
This looks a bit scary since we lose the 'this' pointer here. I see that
setDefaultSearchEngine does not depend on 'this' so it will work but things
might break after some refactoring.

http://codereview.chromium.org/6174009/diff/11001/chrome/browser/resources/op...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/6174009/diff/11001/chrome/browser/resources/op...
chrome/browser/resources/options/pref_ui.js:442: get dataType() {
There is a macro for this.

cr.defineProperty(PrefSelect, 'dataType', cr.PropertyKind.ATTR);

http://codereview.chromium.org/6174009/diff/11001/chrome/browser/resources/op...
File chrome/browser/resources/options/preferences.js (right):

http://codereview.chromium.org/6174009/diff/11001/chrome/browser/resources/op...
chrome/browser/resources/options/preferences.js:73: var arguments = [name,
String(value)];
rename arguments (not forwards compatible with strict mode)