Add builder and metadata to ToolbarButton

Allow ToolbarButton to have metadata and builder support. Also add
support for gfx::Insets in type conversions.

Bug: 1130078
Change-Id: Ib98aede08866dc0bf88ddab1a3c0a70de3959685
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436514
Commit-Queue: Wei Li <weili@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813458}

chrome/browser/ui/views/toolbar/toolbar_button.cc

@@ -28,6 +28,7 @@
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
 #include "ui/gfx/color_utils.h"
+#include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/text_utils.h"
 #include "ui/views/animation/ink_drop.h"
@@ -69,6 +70,7 @@ ToolbarButton::ToolbarButton(PressedCallback callback,
       model_(std::move(model)),
       tab_strip_model_(tab_strip_model),
       trigger_menu_on_long_press_(trigger_menu_on_long_press),
+      layout_insets_(::GetLayoutInsets(TOOLBAR_BUTTON)),
       highlight_color_animation_(this) {
   SetHasInkDropActionOnClick(true);
   set_context_menu_controller(this);
@@ -147,9 +149,8 @@ void ToolbarButton::UpdateColorsAndInsets() {
     SetBackground(nullptr);
   }
 
-  gfx::Insets target_insets =
-      layout_insets_.value_or(GetLayoutInsets(TOOLBAR_BUTTON)) +
-      layout_inset_delta_ + *GetProperty(views::kInternalPaddingKey);
+  gfx::Insets target_insets = layout_insets_ + layout_inset_delta_ +
+                              *GetProperty(views::kInternalPaddingKey);
   base::Optional<SkColor> border_color =
       highlight_color_animation_.GetBorderColor();
   if (!border() || target_insets != border()->GetInsets() ||
@@ -266,6 +267,10 @@ bool ToolbarButton::IsMenuShowing() const {
   return menu_showing_;
 }
 
+gfx::Insets ToolbarButton::GetLayoutInsets() const {
+  return layout_insets_;
+}
+
 void ToolbarButton::SetLayoutInsets(const gfx::Insets& insets) {
   if (layout_insets_ == insets)
     return;
@@ -524,10 +529,6 @@ void ToolbarButton::OnMenuClosed() {
   menu_model_adapter_.reset();
 }
 
-const char* ToolbarButton::GetClassName() const {
-  return "ToolbarButton";
-}
-
 namespace {
 
 // The default duration does not work well for dark mode where the animation has
@@ -650,3 +651,7 @@ void ToolbarButton::HighlightColorAnimation::ClearHighlightColor() {
   highlight_color_.reset();
   parent_->UpdateColorsAndInsets();
 }
+
+BEGIN_METADATA(ToolbarButton, views::LabelButton)
+ADD_PROPERTY_METADATA(gfx::Insets, LayoutInsets)
+END_METADATA

chrome/browser/ui/views/toolbar/toolbar_button.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/optional.h"
+#include "chrome/browser/ui/views/chrome_views_export.h"
 #include "ui/base/pointer/touch_ui_controller.h"
 #include "ui/base/theme_provider.h"
 #include "ui/gfx/animation/animation_delegate.h"
@@ -15,6 +16,8 @@
 #include "ui/gfx/geometry/point.h"
 #include "ui/views/context_menu_controller.h"
 #include "ui/views/controls/button/label_button.h"
+#include "ui/views/metadata/metadata_header_macros.h"
+#include "ui/views/metadata/view_factory.h"
 
 class TabStripModel;
 
@@ -37,6 +40,8 @@ class MenuRunner;
 class ToolbarButton : public views::LabelButton,
                       public views::ContextMenuController {
  public:
+  METADATA_HEADER(ToolbarButton);
+
   // More convenient form of the ctor below, when |model| and |tab_strip_model|
   // are both nullptr.
   explicit ToolbarButton(PressedCallback callback);
@@ -82,7 +87,8 @@ class ToolbarButton : public views::LabelButton,
   // icon state changes, e.g. in response to theme or touch mode changes.
   virtual void UpdateIcon() {}
 
-  // Sets |layout_insets_|, see comment there.
+  // Gets/Sets |layout_insets_|, see comment there.
+  gfx::Insets GetLayoutInsets() const;
   void SetLayoutInsets(const gfx::Insets& insets);
 
   // views::LabelButton:
@@ -222,9 +228,6 @@ class ToolbarButton : public views::LabelButton,
   // Callback for MenuModelAdapter.
   void OnMenuClosed();
 
-  // views::ImageButton:
-  const char* GetClassName() const override;
-
   // views::LabelButton:
   // This is private to avoid a foot-shooter. Callers should use SetHighlight()
   // instead which sets an optional color as well.
@@ -251,11 +254,11 @@ class ToolbarButton : public views::LabelButton,
   // Menu runner to display drop down menu.
   std::unique_ptr<views::MenuRunner> menu_runner_;
 
-  // Layout insets to use. This is used when the ToolbarButton is not actually
-  // hosted inside the toolbar. If not supplied,
-  // |GetLayoutInsets(TOOLBAR_BUTTON)| is used instead which is not appropriate
-  // outside the toolbar.
-  base::Optional<gfx::Insets> layout_insets_;
+  // Layout insets to use.
+  // The default value is GetLayoutInsets(TOOLBAR_BUTTON) which is only
+  // appropriate inside the toolbar. When it is hosted outside the toolbar, use
+  // SetLayoutInsets() to supply the proper value.
+  gfx::Insets layout_insets_;
 
   // Delta from regular toolbar-button insets. This is necessary for buttons
   // that use smaller or larger icons than regular ToolbarButton instances.
@@ -286,4 +289,8 @@ class ToolbarButton : public views::LabelButton,
   base::WeakPtrFactory<ToolbarButton> show_menu_factory_{this};
 };
 
+BEGIN_VIEW_BUILDER(CHROME_VIEWS_EXPORT, ToolbarButton, views::LabelButton)
+VIEW_BUILDER_PROPERTY(gfx::Insets, LayoutInsets)
+END_VIEW_BUILDER(CHROME_VIEWS_EXPORT, ToolbarButton)
+
 #endif  // CHROME_BROWSER_UI_VIEWS_TOOLBAR_TOOLBAR_BUTTON_H_

chrome/browser/ui/views/toolbar/toolbar_button_unittest.cc

@@ -15,6 +15,7 @@
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/models/simple_menu_model.h"
+#include "ui/views/controls/button/button.h"
 #include "ui/views/controls/menu/menu_runner.h"
 
 namespace test {
@@ -28,6 +29,7 @@ class ToolbarButtonTestApi {
 
   views::MenuRunner* menu_runner() { return button_->menu_runner_.get(); }
   bool menu_showing() const { return button_->menu_showing_; }
+
   const gfx::Insets last_paint_insets() const {
     return button_->last_paint_insets_;
   }
@@ -93,6 +95,21 @@ class TestToolbarButton : public ToolbarButton {
 
 using ToolbarButtonViewsTest = ChromeViewsTestBase;
 
+TEST_F(ToolbarButtonViewsTest, DefaultLayoutInsets) {
+  ToolbarButton button{views::Button::PressedCallback()};
+  gfx::Insets default_insets = ::GetLayoutInsets(TOOLBAR_BUTTON);
+  EXPECT_EQ(default_insets, button.GetLayoutInsets());
+  EXPECT_EQ(default_insets, button.GetInsets());
+}
+
+TEST_F(ToolbarButtonViewsTest, SetLayoutInsets) {
+  ToolbarButton button{views::Button::PressedCallback()};
+  gfx::Insets new_insets(2, 3, 4, 5);
+  button.SetLayoutInsets(new_insets);
+  EXPECT_EQ(new_insets, button.GetLayoutInsets());
+  EXPECT_EQ(new_insets, button.GetInsets());
+}
+
 TEST_F(ToolbarButtonViewsTest, MenuDoesNotShowWhenTabStripIsEmpty) {
   TestTabStripModelDelegate delegate;
   TestingProfile profile;

ui/views/metadata/type_conversion.cc

@@ -85,6 +85,13 @@ base::string16 TypeConverter<gfx::Range>::ToString(
       "{%i, %i}", source_value.GetMin(), source_value.GetMax()));
 }
 
+base::string16 TypeConverter<gfx::Insets>::ToString(
+    const gfx::Insets& source_value) {
+  return base::ASCIIToUTF16(base::StringPrintf(
+      "{%d, %d, %d, %d}", source_value.top(), source_value.left(),
+      source_value.bottom(), source_value.right()));
+}
+
 base::Optional<int8_t> TypeConverter<int8_t>::FromString(
     const base::string16& source_value) {
   int32_t ret = 0;
@@ -247,6 +254,21 @@ base::Optional<gfx::Range> TypeConverter<gfx::Range>::FromString(
   return base::nullopt;
 }
 
+base::Optional<gfx::Insets> TypeConverter<gfx::Insets>::FromString(
+    const base::string16& source_value) {
+  const auto values =
+      base::SplitStringPiece(source_value, base::ASCIIToUTF16("{,,,}"),
+                             base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+  int top, left, bottom, right;
+  if ((values.size() == 4) && base::StringToInt(values[0], &top) &&
+      base::StringToInt(values[1], &left) &&
+      base::StringToInt(values[2], &bottom) &&
+      base::StringToInt(values[3], &right)) {
+    return gfx::Insets(top, left, bottom, right);
+  }
+  return base::nullopt;
+}
+
 }  // namespace metadata
 }  // namespace views
 

ui/views/metadata/type_conversion.h

@@ -18,6 +18,7 @@
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
+#include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/range/range.h"
 #include "ui/gfx/shadow_value.h"
@@ -127,6 +128,7 @@ DECLARE_CONVERSIONS(base::TimeDelta)
 DECLARE_CONVERSIONS(gfx::ShadowValues)
 DECLARE_CONVERSIONS(gfx::Size)
 DECLARE_CONVERSIONS(gfx::Range)
+DECLARE_CONVERSIONS(gfx::Insets)
 
 #undef DECLARE_CONVERSIONS
 

ui/views/metadata/type_conversion_unittest.cc

@@ -7,6 +7,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
+#include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/geometry/rect.h"
 
 namespace MD = views::metadata;
@@ -101,3 +102,16 @@ TEST_F(TypeConversionTest, TestConversion_StringToShadowValues) {
   EXPECT_EQ(result[0].blur(), 0.53);
   EXPECT_EQ(result[1].blur(), 4.33);
 }
+
+TEST_F(TypeConversionTest, TestConversion_InsetsToString) {
+  base::string16 to_string =
+      MD::TypeConverter<gfx::Insets>::ToString(gfx::Insets(3, 5, 7, 9));
+
+  EXPECT_EQ(to_string, base::ASCIIToUTF16("{3, 5, 7, 9}"));
+}
+
+TEST_F(TypeConversionTest, TestConversion_StringToInsets) {
+  base::string16 from_string = base::ASCIIToUTF16("{2, 3, 4, 5}");
+  EXPECT_EQ(MD::TypeConverter<gfx::Insets>::FromString(from_string),
+            gfx::Insets(2, 3, 4, 5));
+}

ui/views/metadata/view_factory.h

@@ -188,7 +188,7 @@ class BaseViewBuilderT : public internal::ViewBuilderCore {
   };                                         \
                                              \
   template <>                                \
-  class export Builder<view_class>           \
+  class export views::Builder<view_class>           \
       : public view_class##BuilderT<views::Builder<view_class>>{};
 
 // clang-format on