Clean up a few bits of files that I'm about to modify. The only visible chan...

views/controls/link.cc

-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 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 "views/controls/link.h"
 
 #if defined(OS_LINUX)
 #include <gdk/gdk.h>
 #endif
 
 #include "app/gfx/font.h"
@@ skipping 42 matching lines @@
 }
 
 void Link::SetController(LinkController* controller) {
   controller_ = controller;
 }
 
 const LinkController* Link::GetController() {
   return controller_;
 }
 
-std::string Link::GetClassName() const {
-  return kViewClassName;
-}
-
-void Link::SetHighlightedColor(const SkColor& color) {
-  normal_color_ = color;
-  ValidateStyle();
-}
-
-void Link::SetDisabledColor(const SkColor& color) {
-  disabled_color_ = color;
-  ValidateStyle();
-}
-
-void Link::SetNormalColor(const SkColor& color) {
-  normal_color_ = color;
-  ValidateStyle();
-}
-
 bool Link::OnMousePressed(const MouseEvent& e) {
   if (!enabled_ || (!e.IsLeftMouseButton() && !e.IsMiddleMouseButton()))
     return false;
   SetHighlighted(true);
   return true;
 }
 
 bool Link::OnMouseDragged(const MouseEvent& e) {
   SetHighlighted(enabled_ &&
                  (e.IsLeftMouseButton() || e.IsMiddleMouseButton()) &&
@@ skipping 17 matching lines @@
 }
 
 bool Link::OnKeyPressed(const KeyEvent& e) {
 #if defined(OS_WIN)
   bool activate = ((e.GetCharacter() == VK_SPACE) ||
                    (e.GetCharacter() == VK_RETURN));
 #else
   bool activate = false;
   NOTIMPLEMENTED();
 #endif
-  if (activate) {
-    SetHighlighted(false);
+  if (!activate)
+    return false;
 
-    // Focus the link on key pressed.
-    RequestFocus();
+  SetHighlighted(false);
 
-    if (controller_)
-      controller_->LinkActivated(this, e.GetFlags());
+  // Focus the link on key pressed.
+  RequestFocus();
 
-    return true;
-  }
-  return false;
+  if (controller_)
+    controller_->LinkActivated(this, e.GetFlags());
+
+  return true;
 }
 
 bool Link::SkipDefaultKeyEventProcessing(const KeyEvent& e) {
 #if defined(OS_WIN)
   // Make sure we don't process space or enter as accelerators.
   return (e.GetCharacter() == VK_SPACE) || (e.GetCharacter() == VK_RETURN);
 #else
   NOTIMPLEMENTED();
   return false;
 #endif
 }
 
-void Link::SetHighlighted(bool f) {
-  if (f != highlighted_) {
-    highlighted_ = f;
-    ValidateStyle();
-    SchedulePaint();
-  }
-}
-
-void Link::ValidateStyle() {
-  gfx::Font font = GetFont();
-
-  if (enabled_) {
-    if ((font.style() & gfx::Font::UNDERLINED) == 0) {
-      Label::SetFont(font.DeriveFont(0, font.style() |
-                                     gfx::Font::UNDERLINED));
-    }
-  } else {
-    if ((font.style() & gfx::Font::UNDERLINED) != 0) {
-      Label::SetFont(font.DeriveFont(0, font.style() &
-                                     ~gfx::Font::UNDERLINED));
-    }
-  }
-
-  if (enabled_) {
-    if (highlighted_) {
-      Label::SetColor(highlighted_color_);
-    } else {
-      Label::SetColor(normal_color_);
-    }
-  } else {
-    Label::SetColor(disabled_color_);
-  }
-}
-
 void Link::SetFont(const gfx::Font& font) {
   Label::SetFont(font);
   ValidateStyle();
 }
 
 void Link::SetEnabled(bool f) {
   if (f != enabled_) {
     enabled_ = f;
     ValidateStyle();
     SchedulePaint();
   }
 }
 
 gfx::NativeCursor Link::GetCursorForPoint(Event::EventType event_type, int x,
                                           int y) {
-  if (enabled_) {
+  if (!enabled_)
+    return NULL;
 #if defined(OS_WIN)
-    if (!g_hand_cursor)
-      g_hand_cursor = LoadCursor(NULL, IDC_HAND);
-    return g_hand_cursor;
+  if (!g_hand_cursor)
+    g_hand_cursor = LoadCursor(NULL, IDC_HAND);
+  return g_hand_cursor;
 #elif defined(OS_LINUX)
-    return gdk_cursor_new(GDK_HAND2);
+  return gdk_cursor_new(GDK_HAND2);
 #endif
-  } else {
-    return NULL;
+}
+
+std::string Link::GetClassName() const {
+  return kViewClassName;
+}
+
+void Link::SetHighlightedColor(const SkColor& color) {
+  normal_color_ = color;

[whywhat, 2010/10/27 12:28:12]

Is this a typo? Shouldn't be highlighted_color_ = color?
Related to http://codereview.chromium.org/4179001

[Peter Kasting, 2010/10/27 18:23:32]

If you look more closely you'll notice that I was merely moving functions from
one place in the file to another.  This is indeed wrong, but the bug didn't
originate in this review.

Since this is fixed anyway by the change you linked, I'm not sure why it
matters.

[whywhat, 2010/10/28 11:35:23]

I wasn't about to blame anyone :) I just found someone who touched this code and
who might know if this typo was intentional somehow (you never know, right?).
It's too easy to think you're fixing something obvious while you're actually
breaking something not commented well-enough.

+  ValidateStyle();
+}
+
+void Link::SetDisabledColor(const SkColor& color) {
+  disabled_color_ = color;
+  ValidateStyle();
+}
+
+void Link::SetNormalColor(const SkColor& color) {
+  normal_color_ = color;
+  ValidateStyle();
+}
+
+void Link::SetHighlighted(bool f) {
+  if (f != highlighted_) {
+    highlighted_ = f;
+    ValidateStyle();
+    SchedulePaint();
   }
 }
 
+void Link::ValidateStyle() {
+  gfx::Font font = GetFont();
+
+  if (enabled_) {
+    if ((font.style() & gfx::Font::UNDERLINED) == 0)
+      Label::SetFont(font.DeriveFont(0, font.style() | gfx::Font::UNDERLINED));
+    Label::SetColor(highlighted_ ? highlighted_color_ : normal_color_);
+  } else {
+    if ((font.style() & gfx::Font::UNDERLINED) != 0)
+      Label::SetFont(font.DeriveFont(0, font.style() & ~gfx::Font::UNDERLINED));
+    Label::SetColor(disabled_color_);
+  }
+}
+
 }  // namespace views