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/+/1657288
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Dana Fried <dfried@chromium.org>
Cr-Commit-Position: refs/heads/master@{#670329}
diff --git a/ui/base/class_property.h b/ui/base/class_property.h
index 40bcdeb..d937c57 100644
--- a/ui/base/class_property.h
+++ b/ui/base/class_property.h
@@ -70,11 +70,26 @@
~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 @@
} // 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.
diff --git a/ui/base/class_property_unittest.cc b/ui/base/class_property_unittest.cc
index 870a40f..05c7137 100644
--- a/ui/base/class_property_unittest.cc
+++ b/ui/base/class_property_unittest.cc
@@ -35,17 +35,61 @@
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 @@
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
