DOM UI options handler cleanup:

DOM UI options handler cleanup:

- improve StripColon
- remove superfluous parens (clang would catch these if they weren't inside ifdefs)
- move a couple more files to the abbreviated string definition style
- use SetBoolean rather than SetString("true")

BUG=none
TEST=trybots

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

[Mike Mammarella, 2011-02-11 18:38:48]

LGTM + a few unimportant comments

http://codereview.chromium.org/6490016/diff/1001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/advanced_options_handler.cc (left):

http://codereview.chromium.org/6490016/diff/1001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/advanced_options_handler.cc:379: bool enabled =
(checked_str == "true");
Personally I like these parens, because they make it clear that I'm not
assigning checked_str to enabled and then comparing the result to "true" and
throwing that away. In this case the types make that mostly clear, but you never
can tell what C++ might do with implicit conversions and such...

Does clang really not like them usually?

http://codereview.chromium.org/6490016/diff/1001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/dom_options_util.cc (right):

http://codereview.chromium.org/6490016/diff/1001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/dom_options_util.cc:12: const string16::value_type
kColon[] = { ':', 0 };
Doesn't the style guide say not to use spaces after { and before } here? (The
sawzall style guide requires them, but I think C++ says not to.)

[Evan Stade, 2011-02-11 19:23:32]

http://codereview.chromium.org/6490016/diff/1001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/advanced_options_handler.cc (left):

http://codereview.chromium.org/6490016/diff/1001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/advanced_options_handler.cc:379: bool enabled =
(checked_str == "true");
On 2011/02/11 18:38:48, Mike Mammarella wrote:
> Personally I like these parens, because they make it clear that I'm not
> assigning checked_str to enabled and then comparing the result to "true" and
> throwing that away. In this case the types make that mostly clear, but you
never
> can tell what C++ might do with implicit conversions and such...
> 
> Does clang really not like them usually?

well, I think clang would complain, but I guess I'm not positive. When an
assignment is used as a truth value, g++ warns unless you put extra parens
around it (to prove you really mean it). Transversely, extra parens around a
comparison could indicate that you meant to do an assignment, so clang warns.

What I'm not positive about is if assigning to a bool variable counts the same
as a truth value in the context of if().

p.s. what makes this clear for me is operator precedence, not the types

http://codereview.chromium.org/6490016/diff/1001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/dom_options_util.cc (right):

http://codereview.chromium.org/6490016/diff/1001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/dom_options_util.cc:12: const string16::value_type
kColon[] = { ':', 0 };
On 2011/02/11 18:38:48, Mike Mammarella wrote:
> Doesn't the style guide say not to use spaces after { and before } here? (The
> sawzall style guide requires them, but I think C++ says not to.)

optional, apparently:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo...

but I believe using them is more common than not within chrome's codebase

[Mike Mammarella, 2011-02-11 20:56:19]

http://codereview.chromium.org/6490016/diff/1001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/advanced_options_handler.cc (left):

http://codereview.chromium.org/6490016/diff/1001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/advanced_options_handler.cc:379: bool enabled =
(checked_str == "true");
On 2011/02/11 19:23:33, Evan Stade wrote:
> On 2011/02/11 18:38:48, Mike Mammarella wrote:
> > Personally I like these parens, because they make it clear that I'm not
> > assigning checked_str to enabled and then comparing the result to "true" and
> > throwing that away. In this case the types make that mostly clear, but you
> never
> > can tell what C++ might do with implicit conversions and such...
> > 
> > Does clang really not like them usually?
> 
> well, I think clang would complain, but I guess I'm not positive. When an
> assignment is used as a truth value, g++ warns unless you put extra parens
> around it (to prove you really mean it). Transversely, extra parens around a
> comparison could indicate that you meant to do an assignment, so clang warns.
> 
> What I'm not positive about is if assigning to a bool variable counts the same
> as a truth value in the context of if().
> 
> p.s. what makes this clear for me is operator precedence, not the types

Yeah, there are just some operators that I don't always remember the precedence
of, and that I've gotten wrong in the past, so I tend to try and avoid counting
on anything other than the simple ones that everybody remembers. Maybe I'm just
weird in not remembering = vs ==...

Overall LGTM in any event.

http://codereview.chromium.org/6490016/diff/1001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/dom_options_util.cc (right):

http://codereview.chromium.org/6490016/diff/1001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/dom_options_util.cc:12: const string16::value_type
kColon[] = { ':', 0 };
On 2011/02/11 19:23:33, Evan Stade wrote:
> On 2011/02/11 18:38:48, Mike Mammarella wrote:
> > Doesn't the style guide say not to use spaces after { and before } here?
(The
> > sawzall style guide requires them, but I think C++ says not to.)
> 
> optional, apparently:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo...
> 
> but I believe using them is more common than not within chrome's codebase

OK then.