Allow setting class properties by value for assignable types.

Most of our Views class properties are of type ClassProperty<T*> where
T is a value type, but we're forced to use pointers because T can be of
any type and must therefore be allocated on the heap.

This leads to a lot of code like:
my_view->SetProperty(kMarginsKey, new Insets(kMyViewDefaultInsets));
...
*my_view->GetProperty(kMarginsKey) = new_insets;

(The latter pattern is to prevent a second heap allocation - but only
works if the initial allocation happens; otherwise it crashes.)

This CL shortcuts this behavior so that it behaves in the way we
actually want to use the system 90% of the time:

// Allocates a copy of kMyViewDefaultInsets for the property.
my_view->SetProperty(kMarginsKey, kMyViewDefaultInsets);

// Updates the value of the existing property.
my_view->SetProperty(kMarginsKey, new_insets);

// De-allocates the existing property value.
my_view->ClearProperty(kMarginsKey);

In order to use this new functionality:
 - The property must be an owned property of pointer type.
 - The property type behind the pointer must be copy- or
   move-assignable.

Change-Id: Idec1d5b9c104814f234270214b615774fa5a7084
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1657288Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Dana Fried <dfried@chromium.org>
Cr-Commit-Position: refs/heads/master@{#670329}

ui/base/class_property.h

@@ -70,11 +70,26 @@ class UI_BASE_EXPORT PropertyHandler {
   ~PropertyHandler();
 
   // Sets the |value| of the given class |property|. Setting to the default
-  // value (e.g., NULL) removes the property. The caller is responsible for the
-  // lifetime of any object set as a property on the class.
+  // value (e.g., NULL) removes the property. The lifetime of objects set as
+  // values of unowned properties is managed by the caller (owned properties are
+  // freed when they are overwritten or cleared).
   template<typename T>
   void SetProperty(const ClassProperty<T>* property, T value);
 
+  // Sets the |value| of the given class |property|, which must be an owned
+  // property of pointer type. The property will be assigned a copy of |value|;
+  // if no property object exists one will be allocated. T must support copy
+  // construction and assignment.
+  template <typename T>
+  void SetProperty(const ClassProperty<T*>* property, const T& value);
+
+  // Sets the |value| of the given class |property|, which must be an owned
+  // property and of pointer type. The property will be move-assigned or move-
+  // constructed from |value|; if no property object exists one will be
+  // allocated. T must support at least copy (and ideally move) semantics.
+  template <typename T>
+  void SetProperty(const ClassProperty<T*>* property, T&& value);
+
   // Returns the value of the given class |property|.  Returns the
   // property-specific default value if the property was not previously set.
   template<typename T>
@@ -182,6 +197,40 @@ class UI_BASE_EXPORT PropertyHelper {
 
 }  // namespace subtle
 
+// Template implementation is necessary in the .h file unless we want to break
+// [DECLARE|DEFINE]_EXPORTED_UI_CLASS_PROPERTY_TYPE() below into different
+// macros for owned and unowned properties; implementing them as pure templates
+// makes them nearly impossible to implement or use incorrectly at the cost of a
+// small amount of code duplication across libraries.
+
+template <typename T>
+void PropertyHandler::SetProperty(const ClassProperty<T*>* property,
+                                  const T& value) {
+  // Prevent additional heap allocation if possible.
+  T* const old = GetProperty(property);
+  if (old) {
+    T temp(*old);
+    *old = value;
+    AfterPropertyChange(property, reinterpret_cast<int64_t>(&temp));
+  } else {
+    SetProperty(property, new T(value));
+  }
+}
+
+template <typename T>
+void PropertyHandler::SetProperty(const ClassProperty<T*>* property,
+                                  T&& value) {
+  // Prevent additional heap allocation if possible.
+  T* const old = GetProperty(property);
+  if (old) {
+    T temp(std::move(*old));
+    *old = std::forward<T>(value);
+    AfterPropertyChange(property, reinterpret_cast<int64_t>(&temp));
+  } else {
+    SetProperty(property, new T(std::forward<T>(value)));
+  }
+}
+
 }  // namespace ui
 
 // Macros to declare the property getter/setter template functions.

ui/base/class_property_unittest.cc

@@ -35,17 +35,61 @@ class TestProperty {
 
 void* TestProperty::last_deleted_ = nullptr;
 
-DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(TestProperty, kOwnedKey, NULL)
+class AssignableTestProperty {
+ public:
+  AssignableTestProperty() {}
+  AssignableTestProperty(int value) : value_(value) {}
+  AssignableTestProperty(const AssignableTestProperty& other)
+      : value_(other.value_) {}
+  AssignableTestProperty(AssignableTestProperty&& other)
+      : value_(std::move(other.value_)), was_move_assigned_(true) {}
+  AssignableTestProperty& operator=(const AssignableTestProperty& other) {
+    value_ = other.value_;
+    was_move_assigned_ = false;
+    return *this;
+  }
+  AssignableTestProperty& operator=(AssignableTestProperty&& other) {
+    value_ = std::move(other.value_);
+    was_move_assigned_ = true;
+    return *this;
+  }
+
+  int value() const { return value_; }
+  bool was_move_assigned() const { return was_move_assigned_; }
+
+ private:
+  int value_ = 0;
+  bool was_move_assigned_ = false;
+};
+
+DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(TestProperty, kOwnedKey, nullptr)
+DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(AssignableTestProperty,
+                                   kAssignableKey,
+                                   nullptr)
 
 }  // namespace
 
 DEFINE_UI_CLASS_PROPERTY_TYPE(TestProperty*)
+DEFINE_UI_CLASS_PROPERTY_TYPE(AssignableTestProperty*)
 
 namespace ui {
 namespace test {
 
 namespace {
 
+class TestPropertyHandler : public PropertyHandler {
+ public:
+  int num_events() const { return num_events_; }
+
+ protected:
+  void AfterPropertyChange(const void* key, int64_t old_value) override {
+    ++num_events_;
+  }
+
+ private:
+  int num_events_ = 0;
+};
+
 const int kDefaultIntValue = -2;
 const char* kDefaultStringValue = "squeamish";
 const char* kTestStringValue = "ossifrage";
@@ -113,5 +157,110 @@ TEST(PropertyTest, OwnedProperty) {
   EXPECT_EQ(p3, TestProperty::last_deleted());
 }
 
+TEST(PropertyTest, AssignableProperty) {
+  PropertyHandler h;
+
+  // Verify that assigning a property by value allocates a value.
+  EXPECT_EQ(nullptr, h.GetProperty(kAssignableKey));
+  const AssignableTestProperty kTestProperty{1};
+  h.SetProperty(kAssignableKey, kTestProperty);
+  AssignableTestProperty* p = h.GetProperty(kAssignableKey);
+  EXPECT_NE(nullptr, p);
+  EXPECT_EQ(1, p->value());
+
+  // Verify that assigning by value updates the existing value without
+  // additional allocation.
+  h.SetProperty(kAssignableKey, AssignableTestProperty{2});
+  EXPECT_EQ(p, h.GetProperty(kAssignableKey));
+  EXPECT_EQ(2, p->value());
+
+  // Same as the above case, but with a const reference instead of a move.
+  const AssignableTestProperty kTestProperty2{3};
+  h.SetProperty(kAssignableKey, kTestProperty2);
+  EXPECT_EQ(p, h.GetProperty(kAssignableKey));
+  EXPECT_EQ(3, p->value());
+
+  // Verify that clearing the property deallocates the value.
+  h.ClearProperty(kAssignableKey);
+  EXPECT_EQ(nullptr, h.GetProperty(kAssignableKey));
+
+  // Verify that setting by value after clearing allocates a new value.
+  h.SetProperty(kAssignableKey, AssignableTestProperty{4});
+  EXPECT_EQ(4, h.GetProperty(kAssignableKey)->value());
+}
+
+TEST(PropertyTest, SetProperty_ForwardsParametersCorrectly) {
+  PropertyHandler h;
+
+  // New property from a const ref.
+  const AssignableTestProperty kTestProperty{1};
+  h.SetProperty(kAssignableKey, kTestProperty);
+  EXPECT_FALSE(h.GetProperty(kAssignableKey)->was_move_assigned());
+
+  // Set property from inline rvalue ref.
+  h.SetProperty(kAssignableKey, AssignableTestProperty{2});
+  EXPECT_TRUE(h.GetProperty(kAssignableKey)->was_move_assigned());
+
+  // Set property from lvalue.
+  AssignableTestProperty test_property{3};
+  h.SetProperty(kAssignableKey, test_property);
+  EXPECT_FALSE(h.GetProperty(kAssignableKey)->was_move_assigned());
+
+  // Set property from lvalue ref.
+  AssignableTestProperty& ref = test_property;
+  h.SetProperty(kAssignableKey, ref);
+  EXPECT_FALSE(h.GetProperty(kAssignableKey)->was_move_assigned());
+
+  // Set property from moved rvalue ref.
+  h.SetProperty(kAssignableKey, std::move(test_property));
+  EXPECT_TRUE(h.GetProperty(kAssignableKey)->was_move_assigned());
+
+  // Set property from const ref.
+  const AssignableTestProperty& const_ref = kTestProperty;
+  h.SetProperty(kAssignableKey, const_ref);
+  EXPECT_FALSE(h.GetProperty(kAssignableKey)->was_move_assigned());
+
+  // New property from rvalue ref.
+  h.ClearProperty(kAssignableKey);
+  h.SetProperty(kAssignableKey, AssignableTestProperty{4});
+  EXPECT_TRUE(h.GetProperty(kAssignableKey)->was_move_assigned());
+
+  // New property from lvalue.
+  h.ClearProperty(kAssignableKey);
+  test_property = AssignableTestProperty{5};
+  h.SetProperty(kAssignableKey, test_property);
+  EXPECT_FALSE(h.GetProperty(kAssignableKey)->was_move_assigned());
+}
+
+TEST(PropertyTest, PropertyChangedEvent) {
+  TestPropertyHandler h;
+
+  // Verify that initially setting the value creates an event.
+  const AssignableTestProperty kTestProperty{1};
+  h.SetProperty(kAssignableKey, kTestProperty);
+  EXPECT_EQ(1, h.num_events());
+
+  // Verify that assigning by value sends an event.
+  h.SetProperty(kAssignableKey, AssignableTestProperty{2});
+  EXPECT_EQ(2, h.num_events());
+
+  // Same as the above case, but with a const reference instead of a move.
+  const AssignableTestProperty kTestProperty2{3};
+  h.SetProperty(kAssignableKey, kTestProperty2);
+  EXPECT_EQ(3, h.num_events());
+
+  // Verify that clearing the property creates an event.
+  h.ClearProperty(kAssignableKey);
+  EXPECT_EQ(4, h.num_events());
+
+  // Verify that setting a heap-allocated value also ticks the event counter.
+  h.SetProperty(kAssignableKey, new AssignableTestProperty{4});
+  EXPECT_EQ(5, h.num_events());
+
+  // Verify that overwriting a heap-allocated value ticks the event counter.
+  h.SetProperty(kAssignableKey, new AssignableTestProperty{5});
+  EXPECT_EQ(6, h.num_events());
+}
+
 } // namespace test
 } // namespace ui