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

views/controls/link.cc

Index: views/controls/link.cc
===================================================================
--- views/controls/link.cc	(revision 27132)
+++ views/controls/link.cc	(working copy)
@@ -1,4 +1,4 @@
-// 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.
 
@@ -61,25 +61,6 @@
   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;
@@ -117,18 +98,18 @@
   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) {
@@ -141,40 +122,6 @@
 #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();
@@ -190,16 +137,55 @@
 
 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
+}
+
+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 {
-    return NULL;
+    if ((font.style() & gfx::Font::UNDERLINED) != 0)
+      Label::SetFont(font.DeriveFont(0, font.style() & ~gfx::Font::UNDERLINED));
+    Label::SetColor(disabled_color_);
   }
 }