Refactor the UpdateColorsForBrave into UpdateBackgroundColor & UpdateTextColor

Author: fallaciousreasoning

chromium_src/ui/views/controls/button/md_text_button.cc

@@ -25,9 +25,7 @@
 #include "ui/views/view_class_properties.h"
 
 // To be called from MdTextButtonBase::UpdateColors().
-#define BRAVE_MD_TEXT_BUTTON_UPDATE_COLORS \
-  UpdateColorsForBrave();                  \
-  UpdateIconForBrave();
+#define BRAVE_MD_TEXT_BUTTON_UPDATE_COLORS UpdateIconForBrave();
 
 #define MdTextButton MdTextButtonBase
 #include "src/ui/views/controls/button/md_text_button.cc"
@@ -207,10 +205,59 @@ void MdTextButton::SetLoading(bool loading) {
   UpdateColors();
 }
 
+void MdTextButton::UpdateTextColor() {
+  MdTextButtonBase::UpdateTextColor();
+
+  // Once we update the buttons across Brave to use the new style, we can remove
+  // this branch.
+  if (kind_ == kOld) {
+    if (GetProminent())
+      return;
+    const ui::NativeTheme* theme = GetNativeTheme();
+    // Override different text hover color
+    if (theme->GetPlatformHighContrastColorScheme() !=
+        ui::NativeTheme::PlatformHighContrastColorScheme::kDark) {
+      SetTextColor(ButtonState::STATE_HOVERED, kBraveBrandColor);
+      SetTextColor(ButtonState::STATE_PRESSED, kBraveBrandColor);
+    }
+    return;
+  }
+
+  auto colors = GetButtonColors();
+  SetTextColor(GetVisualState(), colors.text_color);
+}
+
 void MdTextButton::UpdateBackgroundColor() {
-  // Handled via |UpdateColorsForBrave|.
+  // Once we update the buttons across Brave to use the new style, we can remove
+  // this branch.
   if (kind_ == kOld) {
     MdTextButtonBase::UpdateBackgroundColor();
+
+    // We don't modify the Prominent button at all.
+    if (GetProminent()) {
+      return;
+    }
+
+    // Override border color for hover on non-prominent
+    if (GetState() == ButtonState::STATE_PRESSED ||
+        GetState() == ButtonState::STATE_HOVERED) {
+      // First, get the same background fill color that MdTextButtonBase does.
+      // It is unfortunate to copy these lines almost as-is. Consider otherwise
+      // patching it in via a #define.
+      SkColor bg_color =
+          GetColorProvider()->GetColor(ui::kColorDialogBackground);
+      if (GetBgColorOverride()) {
+        bg_color = *GetBgColorOverride();
+      }
+      if (GetState() == STATE_PRESSED) {
+        bg_color = GetNativeTheme()->GetSystemButtonPressedColor(bg_color);
+      }
+      // The only thing that differs for Brave is the stroke color
+      SkColor stroke_color = kBraveBrandColor;
+      SetBackground(CreateBackgroundFromPainter(
+          Painter::CreateRoundRectWith1PxBorderPainter(bg_color, stroke_color,
+                                                       GetCornerRadius())));
+    }
     return;
   }
 
@@ -224,49 +271,6 @@ void MdTextButton::UpdateBackgroundColor() {
           colors.background_color, colors.stroke_color, GetCornerRadius())));
 }
 
-void MdTextButton::UpdateOldColorsForBrave() {
-  if (GetProminent()) {
-    return;
-  }
-
-  const ui::NativeTheme* theme = GetNativeTheme();
-  // Override different text hover color
-  if (theme->GetPlatformHighContrastColorScheme() !=
-      ui::NativeTheme::PlatformHighContrastColorScheme::kDark) {
-    SetTextColor(ButtonState::STATE_HOVERED, kBraveBrandColor);
-    SetTextColor(ButtonState::STATE_PRESSED, kBraveBrandColor);
-  }
-  // Override border color for hover on non-prominent
-  if (GetState() == ButtonState::STATE_PRESSED ||
-      GetState() == ButtonState::STATE_HOVERED) {
-    // First, get the same background fill color that MdTextButtonBase does.
-    // It is undfortunate to copy these lines almost as-is. Consider otherwise
-    // patching it in via a #define.
-    SkColor bg_color = GetColorProvider()->GetColor(ui::kColorDialogBackground);
-    if (GetBgColorOverride()) {
-      bg_color = *GetBgColorOverride();
-    }
-    if (GetState() == STATE_PRESSED) {
-      bg_color = GetNativeTheme()->GetSystemButtonPressedColor(bg_color);
-    }
-    // The only thing that differs for Brave is the stroke color
-    SkColor stroke_color = kBraveBrandColor;
-    SetBackground(CreateBackgroundFromPainter(
-        Painter::CreateRoundRectWith1PxBorderPainter(bg_color, stroke_color,
-                                                     GetCornerRadius())));
-  }
-}
-
-// To be called from MdTextButtonBase::UpdateColors().
-void MdTextButton::UpdateColorsForBrave() {
-  if (GetKind() == Kind::kOld) {
-    UpdateOldColorsForBrave();
-    return;
-  }
-
-  SetTextColor(GetVisualState(), );
-}
-
 void MdTextButton::UpdateIconForBrave() {
   if (icon_) {
     SetImageModel(

chromium_src/ui/views/controls/button/md_text_button.h

@@ -6,31 +6,25 @@
 #ifndef BRAVE_CHROMIUM_SRC_UI_VIEWS_CONTROLS_BUTTON_MD_TEXT_BUTTON_H_
 #define BRAVE_CHROMIUM_SRC_UI_VIEWS_CONTROLS_BUTTON_MD_TEXT_BUTTON_H_
 
-#include "include/core/SkColor.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
+#include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/vector_icon_types.h"
 #include "ui/views/controls/button/label_button.h"
 
 // Rename MdTextButton to MdTextButtonBase
 #define MdTextButton MdTextButtonBase
 
-// Define a Brave-specific method to be called from UpdateColors() to extend its
-// functionality instead of defining UpdateColors() "virtual" and overriding it
-// in our version of the MdTextButton class because there are some subclasses
-// that define their own UpdateColors() method (OmniboxChipButton) now, which
-// would not work with the virtual + override approach.
-// Note: We redefine UpdateBackgroundColor because we want it to be protected.
-#define UpdateBackgroundColor              \
-  UpdateBackgroundColor_Unused();          \
-                                           \
- protected:                                \
-  virtual void UpdateColorsForBrave() = 0; \
-  virtual void UpdateIconForBrave() = 0;   \
-  void UpdateBackgroundColor
+// Redefine UpdateTextColor as protected virtual so we can override it.
+#define UpdateTextColor                  \
+  UpdateTextColor_Unused();              \
+                                         \
+ protected:                              \
+  virtual void UpdateIconForBrave() = 0; \
+  virtual void UpdateTextColor
 
 #include "src/ui/views/controls/button/md_text_button.h"
 
-#undef UpdateBackgroundColor
+#undef UpdateTextColor
 #undef MdTextButton
 
 namespace views {
@@ -66,13 +60,9 @@ class VIEWS_EXPORT MdTextButton : public MdTextButtonBase {
   bool GetLoading() const;
   void SetLoading(bool loading);
 
-  // Until we decide to update the whole UI to use the new Leo colors, we
-  // need to keep this logic around. Currently the new colors are opt-in only.
-  void UpdateOldColorsForBrave();
-
   // MdTextButtonBase:
+  void UpdateTextColor() override;
   void UpdateBackgroundColor() override;
-  void UpdateColorsForBrave() override;
   void UpdateIconForBrave() override;
 
  protected: