Refactor and push window properties up to class properties.

ui/base/class_property_unittest.cc

Index: ui/base/class_property_unittest.cc
diff --git a/ui/base/class_property_unittest.cc b/ui/base/class_property_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..cb420f793db67aa071f2081c95546dfd2a1ed330
--- /dev/null
+++ b/ui/base/class_property_unittest.cc
@@ -0,0 +1,119 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/base/class_property.h"
+
+#include <limits.h>
+
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "base/compiler_specific.h"
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "build/build_config.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+DECLARE_UI_CLASS_PROPERTY_TYPE(const char*)
+DECLARE_UI_CLASS_PROPERTY_TYPE(int)
+
+namespace {
+
+class TestProperty {
+ public:
+  TestProperty() {}
+  ~TestProperty() {
+    last_deleted_ = this;
+  }
+  static TestProperty* last_deleted() { return last_deleted_; }

[sky, 2017/01/24 18:45:38]

Returning a pointer to an object that was deleted seems to encourage mis-use. As
the value is only used for comparison could you return a void* to make it clear
consumers shouldn't try to use the returned value? Alternative assign an ever
increasing int value to each TestProperty and compare that.

[kylix_rd, 2017/01/24 21:20:00]

These tests were lifted, nearly verbatim from window_unittest.cc. Some cleanup
of the tests over there is probably also warranted.

Given that, I agree that these tests would probably be written a little
differently if done more recently. I'll work on that.

+
+ private:
+  static TestProperty* last_deleted_;
+  DISALLOW_COPY_AND_ASSIGN(TestProperty);
+};
+
+TestProperty* TestProperty::last_deleted_ = nullptr;
+
+DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(TestProperty, kOwnedKey, NULL);
+
+}  // namespace
+
+DECLARE_UI_CLASS_PROPERTY_TYPE(TestProperty*);
+
+namespace ui {
+namespace test {
+
+class PropertyTest : public testing::Test {

[sky, 2017/01/24 18:45:38]

To write gtests you don't have to subclass Test. Only subclass Test if you need
to do interesting Setup or to share code. As you aren't doing either you
shouldn't need the subclass. Instead use TEST(PropertyTest, ...).

[kylix_rd, 2017/01/24 21:20:00]

Done.

+ public:
+  PropertyTest() : testing::Test() {}
+  ~PropertyTest() override {}
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PropertyTest);
+};
+
+namespace {
+DEFINE_UI_CLASS_PROPERTY_KEY(int, kIntKey, -2);
+DEFINE_UI_CLASS_PROPERTY_KEY(const char*, kStringKey, "squeamish");
+}
+
+TEST_F(PropertyTest, Property) {
+  std::unique_ptr<PropertyHandler> h = base::MakeUnique<PropertyHandler>();

[sky, 2017/01/24 18:45:38]

As you never explicitly delete 'h'. It seems like you could define
PropertyHandler on the stack here and not use the unique_ptr.

[kylix_rd, 2017/01/24 21:20:00]

Done.

+
+  // Non-existent properties should return the default values.
+  EXPECT_EQ(-2, h->GetProperty(kIntKey));

[sky, 2017/01/24 18:45:38]

These assertions would be less mysterious if you used constants for the default
values.

[kylix_rd, 2017/01/24 21:20:00]

Done.

+  EXPECT_EQ(std::string("squeamish"), h->GetProperty(kStringKey));
+
+  // A set property value should be returned again (even if it's the default
+  // value).
+  h->SetProperty(kIntKey, INT_MAX);
+  EXPECT_EQ(INT_MAX, h->GetProperty(kIntKey));
+  h->SetProperty(kIntKey, -2);
+  EXPECT_EQ(-2, h->GetProperty(kIntKey));
+  h->SetProperty(kIntKey, INT_MIN);
+  EXPECT_EQ(INT_MIN, h->GetProperty(kIntKey));
+
+  h->SetProperty(kStringKey, static_cast<const char*>(NULL));

[sky, 2017/01/24 18:45:38]

NULL -> nullptr. Maybe that means you don't need the static_cast either?

[kylix_rd, 2017/01/24 21:20:00]

NULL->nullptr - Done.

Either the static_cast is needed or the template method must be instantiated
with the explicit type.

+  EXPECT_EQ(NULL, h->GetProperty(kStringKey));
+  h->SetProperty(kStringKey, "squeamish");
+  EXPECT_EQ(std::string("squeamish"), h->GetProperty(kStringKey));
+  h->SetProperty(kStringKey, "ossifrage");
+  EXPECT_EQ(std::string("ossifrage"), h->GetProperty(kStringKey));
+
+  // ClearProperty should restore the default value.
+  h->ClearProperty(kIntKey);
+  EXPECT_EQ(-2, h->GetProperty(kIntKey));
+  h->ClearProperty(kStringKey);
+  EXPECT_EQ(std::string("squeamish"), h->GetProperty(kStringKey));
+}
+
+TEST_F(PropertyTest, OwnedProperty) {
+  std::unique_ptr<PropertyHandler> h = base::MakeUnique<PropertyHandler>();
+
+  EXPECT_EQ(NULL, h->GetProperty(kOwnedKey));
+  TestProperty* last_deleted = TestProperty::last_deleted();
+  TestProperty* p1 = new TestProperty();
+  h->SetProperty(kOwnedKey, p1);
+  EXPECT_EQ(p1, h->GetProperty(kOwnedKey));
+  EXPECT_EQ(last_deleted, TestProperty::last_deleted());
+
+  TestProperty* p2 = new TestProperty();
+  h->SetProperty(kOwnedKey, p2);
+  EXPECT_EQ(p2, h->GetProperty(kOwnedKey));
+  EXPECT_EQ(p1, TestProperty::last_deleted());
+
+  h->ClearProperty(kOwnedKey);
+  EXPECT_EQ(NULL, h->GetProperty(kOwnedKey));
+  EXPECT_EQ(p2, TestProperty::last_deleted());
+
+  TestProperty* p3 = new TestProperty();
+  h->SetProperty(kOwnedKey, p3);
+  EXPECT_EQ(p3, h->GetProperty(kOwnedKey));
+  EXPECT_EQ(p2, TestProperty::last_deleted());
+  h.reset();
+  EXPECT_EQ(p3, TestProperty::last_deleted());
+}
+
+} // namespace test
+} // namespace ui