Allow STATE_NORMAL text buttons to animate

ui/views/controls/button/text_button.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/controls/button/text_button.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "grit/ui_resources.h"
@@ skipping 101 matching lines @@
 TextButtonDefaultBorder::~TextButtonDefaultBorder() {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 //
 // TextButtonDefaultBorder - painting
 //
 ////////////////////////////////////////////////////////////////////////////////
 void TextButtonDefaultBorder::Paint(const View& view, gfx::Canvas* canvas) {
   const TextButton* button = static_cast<const TextButton*>(&view);
-  int state = button->state();
+  const int state = button->state();
+  const bool animating = button->GetAnimation()->is_animating();
 
   Painter* painter = normal_painter_.get();
   if (button->show_multiple_icon_states() &&
-      ((state == TextButton::STATE_HOVERED) ||
-       (state == TextButton::STATE_PRESSED))) {
-    painter = (state == TextButton::STATE_HOVERED) ?
-        hot_painter_.get() : pushed_painter_.get();
+      (state == TextButton::STATE_HOVERED ||

[Peter Kasting, 2013/05/13 22:40:35]

Nit: Please leave in place parens around all binary subexpressions (and add more
to new code).  Among other things this will make the && vs. || order explicit.

[ckocagil, 2013/05/14 11:28:27]

Done.

+       state == TextButton::STATE_PRESSED ||
+       state == TextButton::STATE_NORMAL && animating)) {

[Peter Kasting, 2013/05/13 22:40:35]

Nit: Add a comment about why the animating flag means we should switch from the
normal painter to the hot painter.

[ckocagil, 2013/05/14 11:28:27]

Done.

+    painter = (state == TextButton::STATE_PRESSED) ?
+        pushed_painter_.get() : hot_painter_.get();
   }
-  if (painter) {
-    if (button->GetAnimation()->is_animating()) {
-      // TODO(pkasting): Really this should crossfade between states so it could
-      // handle the case of having a non-NULL |normal_set_|.
-      canvas->SaveLayerAlpha(static_cast<uint8>(
-          button->GetAnimation()->CurrentValueBetween(0, 255)));
-      painter->Paint(canvas, view.size());
-      canvas->Restore();
-    } else {
-      painter->Paint(canvas, view.size());
-    }
+  if (painter && animating) {

[Peter Kasting, 2013/05/13 22:40:35]

Nit: Don't flatten the old nested conditional this way.

[ckocagil, 2013/05/14 11:28:27]

Done.

+    // TODO(pkasting): Really this should crossfade between states so it could
+    // handle the case of having a non-NULL |normal_set_|.

[ckocagil, 2013/05/12 16:50:46]

There doesn't seem to be a |normal_set_| anymore, should I remove this TODO?

[msw, 2013/05/12 21:40:36]

The TODO is still valid, just rename |normal_set_| to |normal_painter_|.

[ckocagil, 2013/05/14 11:28:27]

Done.

+    canvas->SaveLayerAlpha(static_cast<uint8>(
+        button->GetAnimation()->CurrentValueBetween(0, 255)));
+    painter->Paint(canvas, view.size());
+    canvas->Restore();
+  } else if (painter) {
+    painter->Paint(canvas, view.size());
   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 //
 // TextButtonNativeThemeBorder
 //
 ////////////////////////////////////////////////////////////////////////////////
 
 TextButtonNativeThemeBorder::TextButtonNativeThemeBorder(
@@ skipping 660 matching lines @@
   // when not using NativeThemeWin.
 #if defined(OS_WIN)
   if (GetNativeTheme() == ui::NativeThemeWin::instance())
     return;
 #endif
   params->button.is_focused = HasFocus() &&
       (focusable() || IsAccessibilityFocusable());
 }
 
 }  // namespace views