Moved X, Y, Width, Height, Tooltip and IsDrawn to be actual properties in the metadata.

Added ability for metadata to return a finite list of valid values for a given member, such as enums and bool.

Bug: 938501
Change-Id: I1ca2d37b08122df832adff0c153c038a291f260d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2065428
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Wei Li <weili@chromium.org>
Auto-Submit: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842562}

chrome/browser/ui/views/tabs/tab_style_views.cc

@@ -971,6 +971,12 @@ base::Optional<TabStyle::TabColors> views::metadata::TypeConverter<
              : base::nullopt;
 }
 
+// static
+views::metadata::ValidStrings
+views::metadata::TypeConverter<TabStyle::TabColors>::GetValidStrings() {
+  return ValidStrings();
+}
+
 // TabStyle --------------------------------------------------------------------
 
 TabStyleViews::~TabStyleViews() = default;

chrome/browser/ui/views/tabs/tab_style_views.h

@@ -20,6 +20,7 @@ struct views::metadata::TypeConverter<TabStyle::TabColors> {
       views::metadata::ArgType<TabStyle::TabColors> source_value);
   static base::Optional<TabStyle::TabColors> FromString(
       const base::string16& source_value);
+  static views::metadata::ValidStrings GetValidStrings();
 };
 
 class Tab;

components/ui_devtools/css_agent.cc

@@ -51,7 +51,7 @@ std::unique_ptr<CSS::SourceRange> BuildDefaultSelectorSourceRange() {
 std::unique_ptr<Array<int>> BuildDefaultMatchingSelectors() {
   auto matching_selectors = std::make_unique<Array<int>>();
 
-  // Add index 0 to matching delectors array, so frontend uses the class mame
+  // Add index 0 to matching selectors array, so frontend uses the class name
   // from the selectors array as the header for the properties section
   matching_selectors->emplace_back(0);
   return matching_selectors;
@@ -272,9 +272,8 @@ Response CSSAgent::setStyleTexts(
     if (!ui_element)
       return Response::ServerError("Node id not found");
     // Handle setting properties from metadata for View.
-    if (ui_element->type() == VIEW)
-      ui_element->SetPropertiesFromString(edit->getText());
-
+    if (ui_element->type() != VIEW ||
+        !ui_element->SetPropertiesFromString(edit->getText())) {
       gfx::Rect updated_bounds;
       bool visible = false;
       if (!GetPropertiesForUIElement(ui_element, &updated_bounds, &visible))
@@ -287,6 +286,7 @@ Response CSSAgent::setStyleTexts(
 
       if (!SetPropertiesForUIElement(ui_element, updated_bounds, visible))
         return NodeNotFoundError(node_id);
+    }
 
     updated_styles->emplace_back(BuildCSSStyle(
         edit->getStyleSheetId(), GetClassPropertiesWithBounds(ui_element)

components/ui_devtools/views/view_element.cc

@@ -4,6 +4,8 @@
 
 #include "components/ui_devtools/views/view_element.h"
 
+#include <algorithm>
+
 #include "base/strings/string_split.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/ui_devtools/Protocol.h"
@@ -94,23 +96,6 @@ ViewElement::GetCustomPropertiesForMatchedStyle() const {
   std::vector<UIElement::UIProperty> class_properties;
   views::metadata::ClassMetaData* metadata = view_->GetClassMetaData();
   for (auto member = metadata->begin(); member != metadata->end(); member++) {
-    if (member.GetCurrentCollectionName() == "View" &&
-        class_properties.empty()) {
-      gfx::Rect bounds = view_->bounds();
-      class_properties.emplace_back("x", base::NumberToString(bounds.x()));
-      class_properties.emplace_back("y", base::NumberToString(bounds.y()));
-      class_properties.emplace_back("width",
-                                    base::NumberToString(bounds.width()));
-      class_properties.emplace_back("height",
-                                    base::NumberToString(bounds.height()));
-      class_properties.emplace_back("is-drawn",
-                                    view_->IsDrawn() ? "true" : "false");
-      base::string16 description = view_->GetTooltipText(gfx::Point());
-      if (!description.empty())
-        class_properties.emplace_back("tooltip",
-                                      base::UTF16ToUTF8(description));
-    }
-
     // Check if type is SkColor and add "--" to property name so that DevTools
     // frontend will interpret this field as a color. Also convert SkColor value
     // to rgba string.
@@ -182,13 +167,14 @@ bool ViewElement::SetPropertiesFromString(const std::string& text) {
     }
 
     // Since DevTools frontend doesn't check the value, we do a sanity check
-    // based on its type here.
-    if (member->member_type() == "bool") {
-      if (property_value != "true" && property_value != "false") {
+    // based on the allowed values specified in the metadata.
+    auto valid_values = member->GetValidValues();
+    if (!valid_values.empty() &&
+        std::find(valid_values.begin(), valid_values.end(),
+                  base::UTF8ToUTF16(property_value)) == valid_values.end()) {
       // Ignore the value.
       continue;
     }
-    }
 
     DCHECK(!!(member->GetPropertyFlags() &
               views::metadata::PropertyFlags::kSerializable));

components/ui_devtools/views/view_element_unittest.cc

@@ -203,7 +203,7 @@ TEST_F(ViewElementTest, GetCustomProperties) {
   // the vector.
   DCHECK_EQ(indices.first, 0U);
 
-  indices = GetPropertyIndices(element(), "tooltip");
+  indices = GetPropertyIndices(element(), "Tooltip");
   // The tooltip property should be in the second ClassProperties object in the
   // vector.
   DCHECK_EQ(indices.first, 1U);
@@ -211,7 +211,7 @@ TEST_F(ViewElementTest, GetCustomProperties) {
   std::vector<UIElement::UIProperty> ui_props =
       props[indices.first].properties_;
 
-  EXPECT_EQ(ui_props[indices.second].name_, "tooltip");
+  EXPECT_EQ(ui_props[indices.second].name_, "Tooltip");
   EXPECT_EQ(ui_props[indices.second].value_, "This is the tooltip");
 }
 

ui/views/metadata/metadata_impl_macros.h

@@ -29,20 +29,16 @@
 // This will fail to compile if the property accessors aren't in the form of
 // SetXXXX and GetXXXX.
 #define ADD_PROPERTY_METADATA(property_type, property_name)               \
-  std::unique_ptr<METADATA_PROPERTY_TYPE_INTERNAL(property_type,               \
-                                                  property_name)>              \
-      property_name##_prop = std::make_unique<METADATA_PROPERTY_TYPE_INTERNAL( \
+  auto property_name##_prop =                                             \
+      std::make_unique<METADATA_PROPERTY_TYPE_INTERNAL(                   \
           property_type, property_name)>(#property_name, #property_type); \
   AddMemberData(std::move(property_name##_prop));
 
 // This will fail to compile if the property accessor isn't in the form of
 // GetXXXX.
 #define ADD_READONLY_PROPERTY_METADATA(property_type, property_name)      \
-  std::unique_ptr<METADATA_READONLY_PROPERTY_TYPE_INTERNAL(property_type,     \
-                                                           property_name)>    \
-      property_name##_prop =                                                  \
+  auto property_name##_prop =                                             \
       std::make_unique<METADATA_READONLY_PROPERTY_TYPE_INTERNAL(          \
           property_type, property_name)>(#property_name, #property_type); \
   AddMemberData(std::move(property_name##_prop));
-
 #endif  // UI_VIEWS_METADATA_METADATA_IMPL_MACROS_H_

ui/views/metadata/metadata_types.cc

@@ -142,5 +142,9 @@ void MemberMetaDataBase::SetValueAsString(void* obj,
   NOTREACHED();
 }
 
+MemberMetaDataBase::ValueStrings MemberMetaDataBase::GetValidValues() const {
+  return ValueStrings();
+}
+
 }  // namespace metadata
 }  // namespace views

ui/views/metadata/metadata_types.h

@@ -140,6 +140,7 @@ class VIEWS_EXPORT ClassMetaData {
 // accessors to get/set the value of the member on an object.
 class VIEWS_EXPORT MemberMetaDataBase {
  public:
+  using ValueStrings = std::vector<base::string16>;
   MemberMetaDataBase(const std::string& member_name,
                      const std::string& member_type)
       : member_name_(member_name), member_type_(member_type) {}
@@ -160,6 +161,10 @@ class VIEWS_EXPORT MemberMetaDataBase {
   // Return various information flags about the property.
   virtual PropertyFlags GetPropertyFlags() const = 0;
 
+  // Return a list of valid property values as a vector of strings. An empty
+  // vector indicates that the natural limits of the underlying type applies.
+  virtual ValueStrings GetValidValues() const;
+
   const std::string& member_name() const { return member_name_; }
   const std::string& member_type() const { return member_type_; }
 

ui/views/metadata/property_metadata.h

@@ -78,6 +78,12 @@ class ClassPropertyMetaData
     }
   }
 
+  MemberMetaDataBase::ValueStrings GetValidValues() const override {
+    if (!kIsSerializable)
+      return {};
+    return TypeConverter<TValue>::GetValidStrings();
+  }
+
   PropertyFlags GetPropertyFlags() const override {
     return kIsSerializable
                ? (PropertyFlags::kEmpty | PropertyFlags::kSerializable)

ui/views/metadata/type_conversion.cc

@@ -65,6 +65,10 @@ base::string16 TypeConverter<bool>::ToString(bool source_value) {
   return base::ASCIIToUTF16(source_value ? "true" : "false");
 }
 
+ValidStrings TypeConverter<bool>::GetValidStrings() {
+  return {base::ASCIIToUTF16("false"), base::ASCIIToUTF16("true")};
+}
+
 base::string16 TypeConverter<const char*>::ToString(const char* source_value) {
   return base::UTF8ToUTF16(source_value);
 }

ui/views/metadata/type_conversion.h

@@ -29,6 +29,8 @@
 namespace views {
 namespace metadata {
 
+using ValidStrings = std::vector<base::string16>;
+
 // Various metadata methods pass types either by value or const ref depending on
 // whether the types are "small" (defined as "fundamental, enum, or pointer").
 // ArgType<T> gives the appropriate type to use as an argument in such cases.
@@ -55,6 +57,7 @@ template <typename T>
 struct TypeConverter : BaseTypeConverter<std::is_enum<T>::value> {
   static base::string16 ToString(ArgType<T> source_value);
   static base::Optional<T> FromString(const base::string16& source_value);
+  static ValidStrings GetValidStrings();
 };
 
 // Types and macros for generating enum converters ----------------------------
@@ -68,6 +71,13 @@ struct EnumStrings {
   explicit EnumStrings(std::vector<EnumString> init_val)
       : pairs(std::move(init_val)) {}
 
+  ValidStrings GetStringValues() const {
+    ValidStrings string_values;
+    for (const auto& pair : pairs)
+      string_values.push_back(pair.str_value);
+    return string_values;
+  }
+
   const std::vector<EnumString> pairs;
 };
 
@@ -107,6 +117,12 @@ static const EnumStrings<T>& GetEnumStringsInstance();
       }                                                            \
     }                                                              \
     return base::nullopt;                                          \
+  }                                                                \
+                                                                   \
+  template <>                                                      \
+  views::metadata::ValidStrings                                    \
+  views::metadata::TypeConverter<T>::GetValidStrings() {           \
+    return GetEnumStringsInstance<T>().GetStringValues();          \
   }
 
 // String Conversions ---------------------------------------------------------
@@ -123,6 +139,7 @@ VIEWS_EXPORT base::Optional<SkColor> RgbaPiecesToSkColor(
   struct VIEWS_EXPORT TypeConverter<T> : BaseTypeConverter<true> {           \
     static base::string16 ToString(ArgType<T> source_value);                 \
     static base::Optional<T> FromString(const base::string16& source_value); \
+    static ValidStrings GetValidStrings() { return {}; }                     \
   };
 
 DECLARE_CONVERSIONS(int8_t)
@@ -135,7 +152,6 @@ DECLARE_CONVERSIONS(uint32_t)
 DECLARE_CONVERSIONS(uint64_t)
 DECLARE_CONVERSIONS(float)
 DECLARE_CONVERSIONS(double)
-DECLARE_CONVERSIONS(bool)
 DECLARE_CONVERSIONS(const char*)
 DECLARE_CONVERSIONS(base::string16)
 DECLARE_CONVERSIONS(base::TimeDelta)
@@ -146,6 +162,15 @@ DECLARE_CONVERSIONS(gfx::Insets)
 
 #undef DECLARE_CONVERSIONS
 
+template <>
+struct VIEWS_EXPORT TypeConverter<bool> {
+  static constexpr bool is_serializable = true;
+  static bool IsSerializable() { return is_serializable; }
+  static base::string16 ToString(bool source_value);
+  static base::Optional<bool> FromString(const base::string16& source_value);
+  static ValidStrings GetValidStrings();
+};
+
 // Special Conversions for base::Optional<T> type ------------------------------
 
 VIEWS_EXPORT const base::string16& GetNullOptStr();
@@ -166,6 +191,7 @@ struct TypeConverter<base::Optional<T>>
     auto ret = TypeConverter<T>::FromString(source_value);
     return ret ? base::make_optional(ret) : base::nullopt;
   }
+  static ValidStrings GetValidStrings() { return {}; }
 };
 
 template <typename T>
@@ -173,6 +199,7 @@ struct TypeConverter<std::unique_ptr<T>> : BaseTypeConverter<false> {
   static base::string16 ToString(const std::unique_ptr<T>& source_value);
   static base::Optional<std::unique_ptr<T>> FromString(
       const base::string16& source_value);
+  static ValidStrings GetValidStrings() { return {}; }
 };
 
 }  // namespace metadata

ui/views/view.cc

@@ -96,6 +96,13 @@ constexpr int kDefaultHorizontalDragThreshold = 8;
 // Same as what gtk uses.
 constexpr int kDefaultVerticalDragThreshold = 8;
 
+// The following are used to offset the keys for the callbacks associated with
+// the bounds element callbacks.
+constexpr int kXChangedKey = sizeof(int) * 0;
+constexpr int kYChangedKey = sizeof(int) * 1;
+constexpr int kWidthChangedKey = sizeof(int) * 2;
+constexpr int kHeightChangedKey = sizeof(int) * 3;
+
 // Returns the top view in |view|'s hierarchy.
 const View* GetHierarchyRoot(const View* view) {
   const View* root = view;
@@ -394,6 +401,17 @@ void View::SetBoundsRect(const gfx::Rect& bounds) {
 
   for (ViewObserver& observer : observers_)
     observer.OnViewBoundsChanged(this);
+
+  // The property effects have already been taken into account above. No need to
+  // redo them here.
+  if (prev.x() != bounds_.x())
+    OnPropertyChanged(&bounds_ + kXChangedKey, kPropertyEffectsNone);
+  if (prev.y() != bounds_.y())
+    OnPropertyChanged(&bounds_ + kYChangedKey, kPropertyEffectsNone);
+  if (prev.width() != bounds_.width())
+    OnPropertyChanged(&bounds_ + kWidthChangedKey, kPropertyEffectsNone);
+  if (prev.height() != bounds_.height())
+    OnPropertyChanged(&bounds_ + kHeightChangedKey, kPropertyEffectsNone);
 }
 
 void View::SetSize(const gfx::Size& size) {
@@ -564,6 +582,10 @@ bool View::IsDrawn() const {
   return visible_ && parent_ ? parent_->IsDrawn() : false;
 }
 
+bool View::GetIsDrawn() const {
+  return IsDrawn();
+}
+
 bool View::GetEnabled() const {
   return enabled_;
 }
@@ -2243,6 +2265,34 @@ void View::OnPropertyChanged(PropertyKey property,
   property_changed_callbacks->Notify();
 }
 
+int View::GetX() const {
+  return x();
+}
+
+int View::GetY() const {
+  return y();
+}
+
+int View::GetWidth() const {
+  return width();
+}
+
+int View::GetHeight() const {
+  return height();
+}
+
+void View::SetWidth(int width) {
+  SetBounds(x(), y(), width, height());
+}
+
+void View::SetHeight(int height) {
+  SetBounds(x(), y(), width(), height);
+}
+
+base::string16 View::GetTooltip() const {
+  return GetTooltipText(gfx::Point());
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // View, private:
 
@@ -3171,14 +3221,20 @@ ADD_PROPERTY_METADATA(bool, Enabled)
 ADD_PROPERTY_METADATA(View::FocusBehavior, FocusBehavior)
 ADD_PROPERTY_METADATA(bool, FlipCanvasOnPaintForRTLUI)
 ADD_PROPERTY_METADATA(int, Group)
+ADD_PROPERTY_METADATA(int, Height)
 ADD_PROPERTY_METADATA(int, ID)
+ADD_READONLY_PROPERTY_METADATA(bool, IsDrawn);
 ADD_READONLY_PROPERTY_METADATA(gfx::Size, MaximumSize)
 ADD_READONLY_PROPERTY_METADATA(gfx::Size, MinimumSize)
 ADD_PROPERTY_METADATA(bool, Mirrored)
 ADD_PROPERTY_METADATA(bool, NotifyEnterExitOnChild)
+ADD_READONLY_PROPERTY_METADATA(base::string16, Tooltip)
 ADD_PROPERTY_METADATA(bool, Visible)
 ADD_PROPERTY_METADATA(bool, CanProcessEventsWithinSubtree)
 ADD_PROPERTY_METADATA(bool, UseDefaultFillLayout)
+ADD_PROPERTY_METADATA(int, Width)
+ADD_PROPERTY_METADATA(int, X)
+ADD_PROPERTY_METADATA(int, Y)
 END_METADATA
 
 }  // namespace views

ui/views/view.h

@@ -1902,6 +1902,20 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
   // function is NOT called if effects == kPropertyEffectsNone.
   void HandlePropertyChangeEffects(PropertyEffects effects);
 
+  // The following methods are used by the property access system described in
+  // the comments above. They follow the required naming convention in order to
+  // allow them to be visible via the metadata.
+  int GetX() const;
+  int GetY() const;
+  int GetWidth() const;
+  int GetHeight() const;
+  void SetWidth(int width);
+  void SetHeight(int height);
+  bool GetIsDrawn() const;
+
+  // Special property accessor used by metadata to get the ToolTip text.
+  base::string16 GetTooltip() const;
+
   //////////////////////////////////////////////////////////////////////////////
 
   // Creation and lifetime -----------------------------------------------------