Ribbon: Dynamic CSSProperty objects.

Custom properties can currently not be represented by any subclass of
CSSProperty, because we have no strategy for creating any such instances
dynamically. (They all reside in static memory). This CL proposes a
solution to this problem---hopefully ushering in a new and glorious era of
code clean-ups and simplifications.

In short, to obtain a CSSProperty reference for some property (be it
standard property, custom property, or registered custom property),
create a CSSPropertyRef object. This object will either point to one
of the static instances (for standard properties), or will point to the
embedded CustomProperty object (for both kinds of custom properties).

For now, just IsInherited and the name functions have been implemented
in CustomProperty. The plan is to complete the rest incrementally.

Change-Id: I6905cbb23f67d0a3c1de0ecba03c9f0a18cb278c
Reviewed-on: https://chromium-review.googlesource.com/c/1262638
Commit-Queue: Anders Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605292}

third_party/blink/renderer/core/BUILD.gn

@@ -1767,6 +1767,8 @@ jumbo_source_set("unit_tests") {
     "css/parser/sizes_attribute_parser_test.cc",
     "css/parser/sizes_calc_parser_test.cc",
     "css/properties/css_parsing_utils_test.cc",
+    "css/properties/css_property_ref_test.cc",
+    "css/properties/longhands/custom_property_test.cc",
     "css/resolver/css_variable_data_test.cc",
     "css/resolver/css_variable_resolver_test.cc",
     "css/resolver/font_builder_test.cc",

third_party/blink/renderer/core/css/BUILD.gn

@@ -394,6 +394,8 @@ blink_core_sources("css") {
     "properties/css_parsing_utils.cc",
     "properties/css_parsing_utils.h",
     "properties/css_property_base_custom.cc",
+    "properties/css_property_ref.cc",
+    "properties/css_property_ref.h",
     "properties/longhand.h",
     "properties/longhands/align_content_custom.cc",
     "properties/longhands/align_items_custom.cc",
@@ -480,6 +482,8 @@ blink_core_sources("css") {
     "properties/longhands/counter_increment_custom.cc",
     "properties/longhands/counter_reset_custom.cc",
     "properties/longhands/cursor_custom.cc",
+    "properties/longhands/custom_property.cc",
+    "properties/longhands/custom_property.h",
     "properties/longhands/cx_custom.cc",
     "properties/longhands/cy_custom.cc",
     "properties/longhands/d_custom.cc",

third_party/blink/renderer/core/css/properties/css_property_ref.cc

+// Copyright 2018 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 "third_party/blink/renderer/core/css/properties/css_property_ref.h"
+
+#include "third_party/blink/renderer/core/dom/document.h"
+
+namespace blink {
+
+CSSPropertyRef::CSSPropertyRef(const String& name, const Document& document)
+    : property_id_(cssPropertyID(name)) {
+  if (property_id_ == CSSPropertyVariable)
+    custom_property_ = CustomProperty(AtomicString(name), document);
+}
+
+CSSPropertyRef::CSSPropertyRef(const CSSProperty& property)
+    : property_id_(property.PropertyID()) {
+  if (property.PropertyID() == CSSPropertyVariable) {
+    if (!Variable::IsStaticInstance(property))
+      custom_property_ = static_cast<const CustomProperty&>(property);
+    else
+      property_id_ = CSSPropertyInvalid;
+  }
+}
+
+}  // namespace blink

third_party/blink/renderer/core/css/properties/css_property_ref.h

+// Copyright 2018 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.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_PROPERTIES_CSS_PROPERTY_REF_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_PROPERTIES_CSS_PROPERTY_REF_H_
+
+#include "third_party/blink/renderer/core/css/properties/longhands/custom_property.h"
+
+namespace blink {
+
+class Document;
+
+// Use this class to acquire a reference to a CSSProperty instance. The
+// reference returned by GetProperty() may point to the embedded CustomProperty
+// object, hence this reference is only valid for the lifetime of the
+// CSSPropertyRef object.
+//
+// Usage:
+//
+//   CSSPropertyRef ref(some_string, document);
+//
+//   if (ref.IsValid()) {
+//     LOG(INFO) << ref.GetProperty().GetName();
+//   }
+//
+// Note that any CSSPropertyRef constructor may produce an invalid
+// CSSPropertyRef (e.g. if a non-existent property name is provided), so be
+// sure to always check IsValid() before calling GetProperty().
+class CORE_EXPORT CSSPropertyRef {
+  DISALLOW_NEW();
+
+ public:
+  // Look up (or create) a CSSProperty.
+  //
+  // If the incoming 'name' is not a CSS property, the CSSProperty is invalid.
+  CSSPropertyRef(const String& name, const Document&);
+
+  // If you already have a CSSProperty& object, you may use it to get
+  // a CSSPropertyRef again.
+  //
+  // Note that the CSSProperty& returned by GetProperty() may be different
+  // than the incoming CSSProperty&.
+  //
+  // Note also that this CSSPropertyRef is invalid if the incoming CSSProperty&
+  // is the static Variable instance.
+  CSSPropertyRef(const CSSProperty&);
+
+  bool IsValid() const { return property_id_ != CSSPropertyInvalid; }
+
+  const CSSProperty& GetProperty() const {
+    DCHECK(IsValid());
+    if (property_id_ == CSSPropertyVariable)
+      return custom_property_;
+    return CSSProperty::Get(property_id_);
+  }
+
+  void Trace(blink::Visitor* visitor) { visitor->Trace(custom_property_); }
+
+ private:
+  CSSPropertyID property_id_;
+  CustomProperty custom_property_;
+};
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_CSS_PROPERTIES_CSS_PROPERTY_REF_H_

third_party/blink/renderer/core/css/properties/css_property_ref_test.cc

+// Copyright 2018 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 "third_party/blink/renderer/core/css/properties/css_property_ref.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/core/css/property_descriptor.h"
+#include "third_party/blink/renderer/core/css/property_registration.h"
+#include "third_party/blink/renderer/core/css/property_registry.h"
+#include "third_party/blink/renderer/core/testing/page_test_base.h"
+
+namespace blink {
+
+namespace {
+
+class CSSPropertyRefTest : public PageTestBase {
+ public:
+  void RegisterProperty(const String& name,
+                        const String& syntax,
+                        const String& initial_value,
+                        bool is_inherited) {
+    DummyExceptionStateForTesting exception_state;
+    PropertyDescriptor* property_descriptor = PropertyDescriptor::Create();
+    property_descriptor->setName(name);
+    property_descriptor->setSyntax(syntax);
+    property_descriptor->setInitialValue(initial_value);
+    property_descriptor->setInherits(is_inherited);
+    PropertyRegistration::registerProperty(&GetDocument(), property_descriptor,
+                                           exception_state);
+    EXPECT_FALSE(exception_state.HadException());
+  }
+};
+
+}  // namespace
+
+TEST_F(CSSPropertyRefTest, LookupUnregistred) {
+  CSSPropertyRef ref("--x", GetDocument());
+  EXPECT_TRUE(ref.IsValid());
+  EXPECT_EQ(ref.GetProperty().PropertyID(), CSSPropertyVariable);
+}
+
+TEST_F(CSSPropertyRefTest, LookupRegistered) {
+  RegisterProperty("--x", "<length>", "42px", false);
+  CSSPropertyRef ref("--x", GetDocument());
+  EXPECT_TRUE(ref.IsValid());
+  EXPECT_EQ(ref.GetProperty().PropertyID(), CSSPropertyVariable);
+}
+
+TEST_F(CSSPropertyRefTest, LookupStandard) {
+  CSSPropertyRef ref("font-size", GetDocument());
+  EXPECT_TRUE(ref.IsValid());
+  EXPECT_EQ(ref.GetProperty().PropertyID(), CSSPropertyFontSize);
+}
+
+TEST_F(CSSPropertyRefTest, IsValid) {
+  CSSPropertyRef ref("nosuchproperty", GetDocument());
+  EXPECT_FALSE(ref.IsValid());
+}
+
+TEST_F(CSSPropertyRefTest, FromCustomProperty) {
+  CustomProperty custom(AtomicString("--x"), GetDocument());
+  CSSPropertyRef ref(custom);
+  EXPECT_TRUE(ref.IsValid());
+  EXPECT_EQ(ref.GetProperty().PropertyID(), CSSPropertyVariable);
+}
+
+TEST_F(CSSPropertyRefTest, FromStandardProperty) {
+  CSSPropertyRef ref(GetCSSPropertyFontSize());
+  EXPECT_TRUE(ref.IsValid());
+  EXPECT_EQ(ref.GetProperty().PropertyID(), CSSPropertyFontSize);
+}
+
+TEST_F(CSSPropertyRefTest, FromStaticVariableInstance) {
+  CSSPropertyRef ref(GetCSSPropertyVariable());
+  EXPECT_FALSE(ref.IsValid());
+}
+
+}  // namespace blink

third_party/blink/renderer/core/css/properties/longhands/custom_property.cc

+// Copyright 2018 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 "third_party/blink/renderer/core/css/properties/longhands/custom_property.h"
+
+#include "third_party/blink/renderer/core/css/property_registration.h"
+#include "third_party/blink/renderer/core/css/property_registry.h"
+
+namespace blink {
+
+CustomProperty::CustomProperty(const AtomicString& name,
+                               const Document& document)
+    : name_(name), registration_(PropertyRegistration::From(&document, name)) {}
+
+bool CustomProperty::IsInherited() const {
+  return !registration_ || registration_->Inherits();
+}
+
+const AtomicString& CustomProperty::GetPropertyNameAtomicString() const {
+  return name_;
+}
+
+}  // namespace blink

third_party/blink/renderer/core/css/properties/longhands/custom_property.h

+// Copyright 2018 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.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_PROPERTIES_LONGHANDS_CUSTOM_PROPERTY_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_PROPERTIES_LONGHANDS_CUSTOM_PROPERTY_H_
+
+#include "third_party/blink/renderer/core/css/properties/longhands/variable.h"
+
+#include "third_party/blink/renderer/core/css/property_registration.h"
+
+namespace blink {
+
+// Represents a custom property (both registered and unregistered).
+//
+// Unlike all other CSSProperty instances, instances of this class are
+// allocated dynamically on demand. (See CSSPropertyRef).
+//
+// TODO(andruud): Move functionality from Variable to here, and eventually
+// remove Variable.
+class CORE_EXPORT CustomProperty : public Variable {
+  DISALLOW_NEW();
+
+ public:
+  CustomProperty() = default;
+  CustomProperty(const AtomicString& name, const Document&);
+
+  bool IsInherited() const override;
+  const AtomicString& GetPropertyNameAtomicString() const override;
+
+  void Trace(blink::Visitor* visitor) { visitor->Trace(registration_); }
+
+ private:
+  AtomicString name_;
+  Member<const PropertyRegistration> registration_;
+};
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_CSS_PROPERTIES_LONGHANDS_CUSTOM_PROPERTY_H_

third_party/blink/renderer/core/css/properties/longhands/custom_property_test.cc

+// Copyright 2018 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 "third_party/blink/renderer/core/css/properties/longhands/custom_property.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/core/css/property_descriptor.h"
+#include "third_party/blink/renderer/core/css/property_registration.h"
+#include "third_party/blink/renderer/core/css/property_registry.h"
+#include "third_party/blink/renderer/core/testing/page_test_base.h"
+
+namespace blink {
+
+namespace {
+
+class CustomPropertyTest : public PageTestBase {
+ public:
+  void RegisterProperty(const String& name,
+                        const String& syntax,
+                        const String& initial_value,
+                        bool is_inherited) {
+    DummyExceptionStateForTesting exception_state;
+    PropertyDescriptor* property_descriptor = PropertyDescriptor::Create();
+    property_descriptor->setName(name);
+    property_descriptor->setSyntax(syntax);
+    property_descriptor->setInitialValue(initial_value);
+    property_descriptor->setInherits(is_inherited);
+    PropertyRegistration::registerProperty(&GetDocument(), property_descriptor,
+                                           exception_state);
+    EXPECT_FALSE(exception_state.HadException());
+  }
+};
+
+}  // namespace
+
+TEST_F(CustomPropertyTest, UnregisteredPropertyIsInherited) {
+  CustomProperty property("--x", GetDocument());
+  EXPECT_TRUE(property.IsInherited());
+}
+
+TEST_F(CustomPropertyTest, RegisteredNonInheritedPropertyIsNotInherited) {
+  RegisterProperty("--x", "<length>", "42px", false);
+  CustomProperty property("--x", GetDocument());
+  EXPECT_FALSE(property.IsInherited());
+}
+
+TEST_F(CustomPropertyTest, RegisteredInheritedPropertyIsInherited) {
+  RegisterProperty("--x", "<length>", "42px", true);
+  CustomProperty property("--x", GetDocument());
+  EXPECT_TRUE(property.IsInherited());
+}
+
+TEST_F(CustomPropertyTest, StaticVariableInstance) {
+  CustomProperty property("--x", GetDocument());
+  EXPECT_FALSE(Variable::IsStaticInstance(property));
+  EXPECT_TRUE(Variable::IsStaticInstance(GetCSSPropertyVariable()));
+}
+
+TEST_F(CustomPropertyTest, PropertyID) {
+  CustomProperty property("--x", GetDocument());
+  EXPECT_EQ(CSSPropertyVariable, property.PropertyID());
+}
+
+TEST_F(CustomPropertyTest, GetPropertyNameAtomicString) {
+  CustomProperty property("--x", GetDocument());
+  EXPECT_EQ(AtomicString("--x"), property.GetPropertyNameAtomicString());
+}
+
+}  // namespace blink

third_party/blink/renderer/core/css/properties/longhands/variable.cc

@@ -77,4 +77,8 @@ void Variable::ApplyValue(StyleResolverState& state,
   }
 }
 
+bool Variable::IsStaticInstance(const CSSProperty& property) {
+  return &property == &GetCSSPropertyVariable();
+}
+
 }  // namespace blink

third_party/blink/renderer/core/css/properties/longhands/variable.h

@@ -13,7 +13,9 @@
 
 namespace blink {
 
-class Variable final : public Longhand {
+// TODO(andruud): Remove this class when the static Variable instance
+// (as returned by GetCSSPropertyVariable()) has been removed.
+class CORE_EXPORT Variable : public Longhand {
  public:
   constexpr Variable() : Longhand() {}
 
@@ -37,6 +39,8 @@ class Variable final : public Longhand {
 
   void ApplyValue(StyleResolverState& state,
                   const CSSValue& value) const override;
+
+  static bool IsStaticInstance(const CSSProperty&);
 };
 
 }  // namespace blink