Lots of random cleanups, mostly for native_theme_win.cc:

Lots of random cleanups, mostly for native_theme_win.cc:

* Rename "max state" value to "num states" so it sounds less like it's the
  largest valid state
* Shorten/simplify code
* Reduce indenting
* No else after return
* Use scoping objects where possible
* Return directly out of switches where possible
* Don't handle DCHECK failure
* Omit NOTREACHED messages (not necessary in tracking down bugs)
* "const" goes on same line as last function arg
* Order "disabled/hovered/normal/pressed" cases the same way every time
* Fix spelling errors
* Eliminate unnecessary {}s

BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282533

[Peter Kasting, 2014-07-09 20:31:01]

https://codereview.chromium.org/377423003/diff/1/ui/native_theme/native_theme...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/377423003/diff/1/ui/native_theme/native_theme...
ui/native_theme/native_theme_win.cc:884: ((state == kDisabled) ?
MC_CHECKMARKDISABLED : MC_CHECKMARKNORMAL);
I usually try to avoid nested ternaries, but in this case it actually looked
clearer to me (and this file already was nesting them in one or two cases, so
there was some precedent)

[sky, 2014-07-10 16:05:39]

https://codereview.chromium.org/377423003/diff/50001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (left):

https://codereview.chromium.org/377423003/diff/50001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:139: case ui::NativeTheme::kMaxState:
NOTREACHED() << "Unknown state: " << state;
I like the old version better as if a new state is added without updating this
switch you get a compile error. That isn't the case with the new code.

https://codereview.chromium.org/377423003/diff/50001/ui/native_theme/native_t...
File ui/native_theme/native_theme_win.cc (left):

https://codereview.chromium.org/377423003/diff/50001/ui/native_theme/native_t...
ui/native_theme/native_theme_win.cc:783: default:
Isn't the point of code like this to make sure that if a new state is added we
consider if it needs special handling? Really there should be no default here so
that a compile error is generated.

https://codereview.chromium.org/377423003/diff/50001/ui/views/controls/button...
File ui/views/controls/button/button.cc (right):

https://codereview.chromium.org/377423003/diff/50001/ui/views/controls/button...
ui/views/controls/button/button.cc:22: default:                        
NOTREACHED(); return Button::STATE_NORMAL;
Same comment here about compile error.

[Peter Kasting, 2014-07-10 18:23:15]

Here are my counterarguments; see if you find them convincing or not :)

https://codereview.chromium.org/377423003/diff/50001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (left):

https://codereview.chromium.org/377423003/diff/50001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:139: case ui::NativeTheme::kMaxState:
NOTREACHED() << "Unknown state: " << state;
On 2014/07/10 16:05:39, sky wrote:
> I like the old version better as if a new state is added without updating this
> switch you get a compile error. That isn't the case with the new code.

That's correct, but you do still get a runtime error from the NOTREACHED(). 
Given that this function is called for every drawable object, there's no way
that will escape notice.  As long as that's the case, this doesn't seem worse to
me.

https://codereview.chromium.org/377423003/diff/50001/ui/native_theme/native_t...
File ui/native_theme/native_theme_win.cc (left):

https://codereview.chromium.org/377423003/diff/50001/ui/native_theme/native_t...
ui/native_theme/native_theme_win.cc:783: default:
On 2014/07/10 16:05:39, sky wrote:
> Isn't the point of code like this to make sure that if a new state is added we
> consider if it needs special handling?

I don't think that benefit is worth the extra code it costs.  Realistically, at
this point we're not going to be adding any more states, and if for some crazy
reason we did, I wouldn't think we'd need this particular reminder in order to
update this code -- buttons would just obviously paint wrong for that state.

https://codereview.chromium.org/377423003/diff/50001/ui/native_theme/native_t...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/377423003/diff/50001/ui/native_theme/native_t...
ui/native_theme/native_theme_win.cc:1133: classic_state |= DFCS_PUSHED;
Note that this code should also be resolved per your above comment.

https://codereview.chromium.org/377423003/diff/50001/ui/views/controls/button...
File ui/views/controls/button/button.cc (right):

https://codereview.chromium.org/377423003/diff/50001/ui/views/controls/button...
ui/views/controls/button/button.cc:22: default:                        
NOTREACHED(); return Button::STATE_NORMAL;
On 2014/07/10 16:05:39, sky wrote:
> Same comment here about compile error.

See reply on other comment.

[sky, 2014-07-10 19:33:10]

If you still disagree ask another owner for their opinion. I'll let them be the
arbitrator.

https://codereview.chromium.org/377423003/diff/50001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (left):

https://codereview.chromium.org/377423003/diff/50001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:139: case ui::NativeTheme::kMaxState:
NOTREACHED() << "Unknown state: " << state;
On 2014/07/10 18:23:15, Peter Kasting wrote:
> On 2014/07/10 16:05:39, sky wrote:
> > I like the old version better as if a new state is added without updating
this
> > switch you get a compile error. That isn't the case with the new code.
> 
> That's correct, but you do still get a runtime error from the NOTREACHED(). 
> Given that this function is called for every drawable object, there's no way
> that will escape notice.  As long as that's the case, this doesn't seem worse
to
> me.

I way prefer the compile time error than a runtime error here.

https://codereview.chromium.org/377423003/diff/50001/ui/native_theme/native_t...
File ui/native_theme/native_theme_win.cc (left):

https://codereview.chromium.org/377423003/diff/50001/ui/native_theme/native_t...
ui/native_theme/native_theme_win.cc:783: default:
On 2014/07/10 18:23:15, Peter Kasting wrote:
> On 2014/07/10 16:05:39, sky wrote:
> > Isn't the point of code like this to make sure that if a new state is added
we
> > consider if it needs special handling?
> 
> I don't think that benefit is worth the extra code it costs.

I don't see the reduction you're doing as significant, especially if you nuke
783-785 old.

> Realistically, at
> this point we're not going to be adding any more states, and if for some crazy
> reason we did, I wouldn't think we'd need this particular reminder in order to
> update this code -- buttons would just obviously paint wrong for that state.

You can't ignore errors from the compiler. I much prefer using compile errors
were possible.

[Peter Kasting, 2014-07-10 21:44:25]

PTAL.  Despite the comments below, I did my best to change all switch statements
in native_theme_win.cc so that they had all cases listed explicitly (and no
"default") whenever it made sense to do so.  I also reordered switches so that
in general, the cases were listed in the same order as the enums, except when
doing otherwise was necessary to group common handler code together.

https://codereview.chromium.org/377423003/diff/50001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_util.cc (left):

https://codereview.chromium.org/377423003/diff/50001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_util.cc:139: case ui::NativeTheme::kMaxState:
NOTREACHED() << "Unknown state: " << state;
On 2014/07/10 19:33:09, sky wrote:
> On 2014/07/10 18:23:15, Peter Kasting wrote:
> > On 2014/07/10 16:05:39, sky wrote:
> > > I like the old version better as if a new state is added without updating
> this
> > > switch you get a compile error. That isn't the case with the new code.
> > 
> > That's correct, but you do still get a runtime error from the NOTREACHED(). 
> > Given that this function is called for every drawable object, there's no way
> > that will escape notice.  As long as that's the case, this doesn't seem
worse
> to
> > me.
> 
> I way prefer the compile time error than a runtime error here.

Done.

https://codereview.chromium.org/377423003/diff/50001/ui/native_theme/native_t...
File ui/native_theme/native_theme_win.cc (left):

https://codereview.chromium.org/377423003/diff/50001/ui/native_theme/native_t...
ui/native_theme/native_theme_win.cc:783: default:
On 2014/07/10 19:33:10, sky wrote:
> On 2014/07/10 18:23:15, Peter Kasting wrote:
> > On 2014/07/10 16:05:39, sky wrote:
> > > Isn't the point of code like this to make sure that if a new state is
added
> we
> > > consider if it needs special handling?
> > 
> > I don't think that benefit is worth the extra code it costs.
> 
> I don't see the reduction you're doing as significant, especially if you nuke
> 783-785 old.

As noted in IM, we can't remove this -- only change it to "case kNumStates".

> > Realistically, at
> > this point we're not going to be adding any more states, and if for some
crazy
> > reason we did, I wouldn't think we'd need this particular reminder in order
to
> > update this code -- buttons would just obviously paint wrong for that state.
> 
> You can't ignore errors from the compiler. I much prefer using compile errors
> were possible.

As noted in IM, MSVC won't currently generate errors for "switch doesn't have
cases for all enum values".  There is an MSVC warning for this, but it's off by
default.  Perhaps I can look at turning that on.

https://codereview.chromium.org/377423003/diff/50001/ui/views/controls/button...
File ui/views/controls/button/button.cc (right):

https://codereview.chromium.org/377423003/diff/50001/ui/views/controls/button...
ui/views/controls/button/button.cc:22: default:                        
NOTREACHED(); return Button::STATE_NORMAL;
Changed this.

[sky, 2014-07-10 22:58:12]

LGTM

[Peter Kasting, 2014-07-10 23:05:05]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-07-10 23:06:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkasting@chromium.org/377423003/90001

[commit-bot: I haz the power, 2014-07-11 00:54:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-11 03:55:07]

Change committed as 282533