[cascade] Simplify initial, inherited and invalid variables.

The current approach is quite confusing and inconsistent. The value
nullptr means too many different things, depending on the situation.

This CL introduces a new, hopefully clearer model, where nullptr always
means invalid at computed-value-time, and base::nullopt means "no entry
found". This means that, if a ComputedStyle has a present entry of
'nullptr' for a given custom property, we do not e.g. fall back to
values from StyleInitialData.

However, if there is no entry (base::nullopt), we look up values from the
root-bucket (see StyleInheritedVariables), and/or StyleInitialData.

A consequence of this approach, is that explicitly applied initial and
inherited values must actually be present on the ComputedStyle; we can not
return to a state of base::nullopt. This is because of the root-bucket
optimization of StyleInheritedVariables, which makes it impossible to
actually erase a value in all cases. (We actually don't have the same
restriction for StyleNonInheritedVariables, but the behavior is the same,
for simplicity).

Another change in this CL, is that StyleInitialData now has the initial
"data" as well as the initial "values". This is again to avoid that
nullptr has different meanings. If we didn't store the initial data,
we could end up with a nullptr for registered properties with an
implicitly initial value. This is incompatible with the "nullptr means
nullptr" idea.

Note that CSSInvalidVariableValue isn't technically needed anymore after
this CL, but since it's going to be useful for the Cascade project (during
the application phase only, not as a persistently stored object), I'd like
to keep it. It has a unit tests, so it's not completely unused.

BUG=947004
R=futhark@chromium.org

Change-Id: I4922f0d36f3d3d4fde6a25e56ef5c14c0c9890f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1601135Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657681}

third_party/blink/renderer/core/BUILD.gn

@@ -2153,6 +2153,7 @@ jumbo_source_set("unit_tests") {
     "style/computed_style_test.cc",
     "style/filter_operations_test.cc",
     "style/style_difference_test.cc",
+    "style/style_variables_test.cc",
     "style/svg_computed_style_test.cc",
     "svg/graphics/svg_image_test.cc",
     "svg/svg_element_test.cc",

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

@@ -36,27 +36,31 @@ CSSPropertyName CustomProperty::GetCSSPropertyName() const {
 }
 
 void CustomProperty::ApplyInitial(StyleResolverState& state) const {
-  state.Style()->RemoveVariable(name_, IsInherited());
+  bool is_inherited_property = IsInherited();
+
+  if (!registration_) {
+    state.Style()->SetVariable(name_, nullptr, is_inherited_property);
+    return;
+  }
+
+  state.Style()->SetVariable(name_, registration_->InitialVariableData(),
+                             is_inherited_property);
+  state.Style()->SetRegisteredVariable(name_, registration_->Initial(),
+                                       is_inherited_property);
 }
 
 void CustomProperty::ApplyInherit(StyleResolverState& state) const {
   bool is_inherited_property = IsInherited();
-  state.Style()->RemoveVariable(name_, is_inherited_property);
 
   CSSVariableData* parent_value =
       state.ParentStyle()->GetVariable(name_, is_inherited_property);
 
-  if (!parent_value)
-    return;
-
   state.Style()->SetVariable(name_, parent_value, is_inherited_property);
 
   if (registration_) {
-    const CSSValue* parent_css_value =
-        parent_value ? state.ParentStyle()->GetNonInitialRegisteredVariable(
-                           name_, is_inherited_property)
-                     : nullptr;
-    state.Style()->SetRegisteredVariable(name_, parent_css_value,
+    const CSSValue* parent_value =
+        state.ParentStyle()->GetRegisteredVariable(name_);
+    state.Style()->SetRegisteredVariable(name_, parent_value,
                                          is_inherited_property);
   }
 }

third_party/blink/renderer/core/css/resolver/css_variable_resolver.cc

@@ -73,21 +73,8 @@ scoped_refptr<CSSVariableData> CSSVariableResolver::ValueForCustomProperty(
 
   CSSVariableData* variable_data = GetVariable(name, registration);
 
-  if (!variable_data) {
-    // For unregistered properties, not having a CSSVariableData here means
-    // that it either never existed, or we have resolved it earlier, but
-    // resolution failed. Either way, we return nullptr to signify that this is
-    // an invalid variable.
-    if (!registration)
-      return nullptr;
-    // For registered properties, it's more complicated. Here too, it can mean
-    // that it never existed, or that resolution failed earlier, but now we need
-    // to know which; in the former case we must provide the initial value, and
-    // in the latter case the variable is invalid.
-    return IsRegisteredVariableInvalid(name, *registration)
-               ? nullptr
-               : registration->InitialVariableData();
-  }
+  if (!variable_data)
+    return nullptr;
 
   bool cycle_detected = false;
   scoped_refptr<CSSVariableData> resolved_data = ResolveCustomPropertyIfNeeded(
@@ -126,16 +113,17 @@ scoped_refptr<CSSVariableData> CSSVariableResolver::ValueForCustomProperty(
     }
   }
 
-  // If either parsing or resolution failed, and this property inherits,
-  // take inherited values instead of falling back on initial.
-  if (registration->Inherits() && !resolved_data) {
-    resolved_data = state_.ParentStyle()->GetVariable(name, true);
-    resolved_value =
-        state_.ParentStyle()->GetNonInitialRegisteredVariable(name, true);
+  // If either parsing or resolution failed, fall back on "unset".
+  if (!resolved_data) {
+    if (registration->Inherits()) {
+      resolved_data = state_.ParentStyle()->GetVariable(name, true);
+      resolved_value = state_.ParentStyle()->GetRegisteredVariable(name, true);
+    } else {
+      resolved_data = registration->InitialVariableData();
+      resolved_value = registration->Initial();
+    }
   }
 
-  DCHECK(!!resolved_data == !!resolved_value);
-
   // Registered custom properties substitute as token sequences equivalent to
   // their computed values. CSSVariableData instances which represent such token
   // sequences are called "absolutized".
@@ -162,10 +150,6 @@ scoped_refptr<CSSVariableData> CSSVariableResolver::ValueForCustomProperty(
   if (value != resolved_value)
     SetRegisteredVariable(name, *registration, resolved_value);
 
-  if (!resolved_data) {
-    return registration->InitialVariableData();
-  }
-
   return resolved_data;
 }
 
@@ -229,24 +213,14 @@ bool CSSVariableResolver::IsVariableDisallowed(
 CSSVariableData* CSSVariableResolver::GetVariable(
     const AtomicString& name,
     const PropertyRegistration* registration) {
-  if (!registration || registration->Inherits()) {
-    return inherited_variables_ ? inherited_variables_->GetVariable(name)
-                                : nullptr;
-  }
-  return non_inherited_variables_ ? non_inherited_variables_->GetVariable(name)
-                                  : nullptr;
+  return state_.Style()->GetVariable(name,
+                                     !registration || registration->Inherits());
 }
 
 const CSSValue* CSSVariableResolver::GetRegisteredVariable(
     const AtomicString& name,
     const PropertyRegistration& registration) {
-  if (registration.Inherits()) {
-    return inherited_variables_ ? inherited_variables_->RegisteredVariable(name)
-                                : nullptr;
-  }
-  return non_inherited_variables_
-             ? non_inherited_variables_->RegisteredVariable(name)
-             : nullptr;
+  return state_.Style()->GetRegisteredVariable(name, registration.Inherits());
 }
 
 void CSSVariableResolver::SetVariable(
@@ -278,20 +252,9 @@ void CSSVariableResolver::SetRegisteredVariable(
 void CSSVariableResolver::SetInvalidVariable(
     const AtomicString& name,
     const PropertyRegistration* registration) {
-  // TODO(andruud): Use RemoveVariable instead, but currently it also does
-  // a lookup in the registered map, which seems wasteful.
   SetVariable(name, registration, nullptr);
-  if (registration) {
-    const CSSValue* value = CSSInvalidVariableValue::Create();
-    SetRegisteredVariable(name, *registration, value);
-  }
-}
-
-bool CSSVariableResolver::IsRegisteredVariableInvalid(
-    const AtomicString& name,
-    const PropertyRegistration& registration) {
-  const CSSValue* value = GetRegisteredVariable(name, registration);
-  return value && value->IsInvalidVariableValue();
+  if (registration)
+    SetRegisteredVariable(name, *registration, nullptr);
 }
 
 bool CSSVariableResolver::ResolveVariableReference(CSSParserTokenRange range,
@@ -498,16 +461,16 @@ void CSSVariableResolver::ResolveVariableDefinitions() {
 
   int variable_count = 0;
   if (inherited_variables_ && inherited_variables_->NeedsResolution()) {
-    for (auto& variable : inherited_variables_->data_)
+    for (auto& variable : inherited_variables_->Data())
       ValueForCustomProperty(variable.key, options);
     inherited_variables_->ClearNeedsResolution();
-    variable_count += inherited_variables_->data_.size();
+    variable_count += inherited_variables_->Data().size();
   }
   if (non_inherited_variables_ && non_inherited_variables_->NeedsResolution()) {
-    for (auto& variable : non_inherited_variables_->data_)
+    for (auto& variable : non_inherited_variables_->Data())
       ValueForCustomProperty(variable.key, options);
     non_inherited_variables_->ClearNeedsResolution();
-    variable_count += non_inherited_variables_->data_.size();
+    variable_count += non_inherited_variables_->Data().size();
   }
   INCREMENT_STYLE_STATS_COUNTER(state_.GetDocument().GetStyleEngine(),
                                 custom_properties_applied, variable_count);
@@ -517,11 +480,11 @@ void CSSVariableResolver::ComputeRegisteredVariables() {
   Options options;
 
   if (inherited_variables_) {
-    for (auto& variable : *inherited_variables_->registered_data_)
+    for (auto& variable : inherited_variables_->Values())
       ValueForCustomProperty(variable.key, options);
   }
   if (non_inherited_variables_) {
-    for (auto& variable : *non_inherited_variables_->registered_data_)
+    for (auto& variable : non_inherited_variables_->Values())
       ValueForCustomProperty(variable.key, options);
   }
 }

third_party/blink/renderer/core/css/resolver/css_variable_resolver.h

@@ -207,9 +207,6 @@ class CORE_EXPORT CSSVariableResolver {
                              const CSSValue*);
   void SetInvalidVariable(const AtomicString& name,
                           const PropertyRegistration*);
-  bool IsRegisteredVariableInvalid(const AtomicString& name,
-                                   const PropertyRegistration&);
-
   const StyleResolverState& state_;
   StyleInheritedVariables* inherited_variables_;
   StyleNonInheritedVariables* non_inherited_variables_;

third_party/blink/renderer/core/css/resolver/css_variable_resolver_test.cc

@@ -315,86 +315,6 @@ TEST_F(CSSVariableResolverTest, NeedsResolutionClearedByResolver) {
   EXPECT_FALSE(state.Style()->NonInheritedVariables()->NeedsResolution());
 }
 
-TEST_F(CSSVariableResolverTest, RemoveInheritedVariableAtRoot) {
-  scoped_refptr<StyleInheritedVariables> inherited_variables_root =
-      StyleInheritedVariables::Create();
-
-  AtomicString name("--prop");
-  const auto* prop = CreateCustomProperty("test");
-  const CSSValue* value = CreatePxValue(10.0);
-  inherited_variables_root->SetVariable(name, prop->Value());
-  inherited_variables_root->SetRegisteredVariable(name, value);
-
-  EXPECT_TRUE(inherited_variables_root->GetVariable(name));
-  EXPECT_TRUE(inherited_variables_root->RegisteredVariable(name));
-
-  inherited_variables_root->RemoveVariable(name);
-
-  EXPECT_FALSE(inherited_variables_root->GetVariable(name));
-  EXPECT_FALSE(inherited_variables_root->RegisteredVariable(name));
-}
-
-TEST_F(CSSVariableResolverTest, RemoveInheritedVariableAtNonRoot) {
-  scoped_refptr<StyleInheritedVariables> inherited_variables_root =
-      StyleInheritedVariables::Create();
-  scoped_refptr<StyleInheritedVariables> inherited_variables =
-      inherited_variables_root->Copy();
-
-  AtomicString name("--prop");
-  const auto* prop = CreateCustomProperty("test");
-  const CSSValue* value = CreatePxValue(10.0);
-  inherited_variables->SetVariable(name, prop->Value());
-  inherited_variables->SetRegisteredVariable(name, value);
-
-  EXPECT_TRUE(inherited_variables->GetVariable(name));
-  EXPECT_TRUE(inherited_variables->RegisteredVariable(name));
-
-  inherited_variables->RemoveVariable(name);
-
-  EXPECT_FALSE(inherited_variables->GetVariable(name));
-  EXPECT_FALSE(inherited_variables->RegisteredVariable(name));
-}
-
-TEST_F(CSSVariableResolverTest, RemoveVariableInheritedViaRoot) {
-  scoped_refptr<StyleInheritedVariables> inherited_variables_root =
-      StyleInheritedVariables::Create();
-
-  AtomicString name("--prop");
-  const auto* prop = CreateCustomProperty("test");
-  const CSSValue* value = CreatePxValue(10.0);
-  inherited_variables_root->SetVariable(name, prop->Value());
-  inherited_variables_root->SetRegisteredVariable(name, value);
-
-  scoped_refptr<StyleInheritedVariables> inherited_variables =
-      inherited_variables_root->Copy();
-
-  EXPECT_TRUE(inherited_variables->GetVariable(name));
-  EXPECT_TRUE(inherited_variables->RegisteredVariable(name));
-
-  inherited_variables->RemoveVariable(name);
-
-  EXPECT_FALSE(inherited_variables->GetVariable(name));
-  EXPECT_FALSE(inherited_variables->RegisteredVariable(name));
-}
-
-TEST_F(CSSVariableResolverTest, RemoveNonInheritedVariable) {
-  auto non_inherited_variables = std::make_unique<StyleNonInheritedVariables>();
-
-  AtomicString name("--prop");
-  const auto* prop = CreateCustomProperty("test");
-  const CSSValue* value = CreatePxValue(10.0);
-  non_inherited_variables->SetVariable(name, prop->Value());
-  non_inherited_variables->SetRegisteredVariable(name, value);
-
-  EXPECT_TRUE(non_inherited_variables->GetVariable(name));
-  EXPECT_TRUE(non_inherited_variables->RegisteredVariable(name));
-
-  non_inherited_variables->RemoveVariable(name);
-
-  EXPECT_FALSE(non_inherited_variables->GetVariable(name));
-  EXPECT_FALSE(non_inherited_variables->RegisteredVariable(name));
-}
-
 TEST_F(CSSVariableResolverTest, DontCrashWhenSettingInheritedNullVariable) {
   scoped_refptr<StyleInheritedVariables> inherited_variables =
       StyleInheritedVariables::Create();

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

@@ -91,6 +91,8 @@ blink_core_sources("rendering") {
     "style_ray.h",
     "style_reflection.h",
     "style_self_alignment_data.h",
+    "style_variables.cc",
+    "style_variables.h",
     "text_size_adjust.h",
   ]
 }

third_party/blink/renderer/core/style/computed_style.cc

@@ -1743,31 +1743,40 @@ void ComputedStyle::SetRegisteredVariable(const AtomicString& name,
     MutableNonInheritedVariables().SetRegisteredVariable(name, value);
 }
 
-void ComputedStyle::RemoveVariable(const AtomicString& name,
-                                   bool is_inherited_property) {
-  if (is_inherited_property) {
-    MutableInheritedVariables().RemoveVariable(name);
-  } else {
-    MutableNonInheritedVariables().RemoveVariable(name);
-  }
+static CSSVariableData* GetInitialVariableData(
+    const AtomicString& name,
+    const StyleInitialData* initial_data) {
+  if (!initial_data)
+    return nullptr;
+  return initial_data->GetVariableData(name);
 }
 
 CSSVariableData* ComputedStyle::GetVariable(const AtomicString& name) const {
-  CSSVariableData* variable = GetVariable(name, true);
-  if (variable) {
-    return variable;
+  if (InheritedVariables()) {
+    if (auto data = InheritedVariables()->GetData(name))
+      return *data;
   }
-  return GetVariable(name, false);
+  if (NonInheritedVariables()) {
+    if (auto data = NonInheritedVariables()->GetData(name))
+      return *data;
+  }
+  return GetInitialVariableData(name, InitialDataInternal().get());
 }
 
 CSSVariableData* ComputedStyle::GetVariable(const AtomicString& name,
                                             bool is_inherited_property) const {
   if (is_inherited_property) {
-    return InheritedVariables() ? InheritedVariables()->GetVariable(name)
-                                : nullptr;
+    if (InheritedVariables()) {
+      if (auto data = InheritedVariables()->GetData(name))
+        return *data;
+    }
+  } else {
+    if (NonInheritedVariables()) {
+      if (auto data = NonInheritedVariables()->GetData(name))
+        return *data;
+    }
   }
-  return NonInheritedVariables() ? NonInheritedVariables()->GetVariable(name)
-                                 : nullptr;
+  return GetInitialVariableData(name, InitialDataInternal().get());
 }
 
 static const CSSValue* GetInitialRegisteredVariable(
@@ -1775,40 +1784,37 @@ static const CSSValue* GetInitialRegisteredVariable(
     const StyleInitialData* initial_data) {
   if (!initial_data)
     return nullptr;
-  return initial_data->GetInitialVariable(name);
+  return initial_data->GetVariableValue(name);
 }
 
 const CSSValue* ComputedStyle::GetRegisteredVariable(
     const AtomicString& name,
     bool is_inherited_property) const {
-  const CSSValue* result =
-      GetNonInitialRegisteredVariable(name, is_inherited_property);
-  if (result)
-    return result;
+  if (is_inherited_property) {
+    if (InheritedVariables()) {
+      if (auto value = InheritedVariables()->GetValue(name))
+        return *value;
+    }
+  } else {
+    if (NonInheritedVariables()) {
+      if (auto value = NonInheritedVariables()->GetValue(name))
+        return *value;
+    }
+  }
   return GetInitialRegisteredVariable(name, InitialDataInternal().get());
 }
 
 const CSSValue* ComputedStyle::GetRegisteredVariable(
     const AtomicString& name) const {
-  const CSSValue* result = GetNonInitialRegisteredVariable(name, false);
-  if (result)
-    return result;
-  result = GetNonInitialRegisteredVariable(name, true);
-  if (result)
-    return result;
-  return GetInitialRegisteredVariable(name, InitialDataInternal().get());
-}
-
-const CSSValue* ComputedStyle::GetNonInitialRegisteredVariable(
-    const AtomicString& name,
-    bool is_inherited_property) const {
-  if (is_inherited_property) {
-    return InheritedVariables() ? InheritedVariables()->RegisteredVariable(name)
-                                : nullptr;
+  if (InheritedVariables()) {
+    if (auto value = InheritedVariables()->GetValue(name))
+      return *value;
   }
-  return NonInheritedVariables()
-             ? NonInheritedVariables()->RegisteredVariable(name)
-             : nullptr;
+  if (NonInheritedVariables()) {
+    if (auto value = NonInheritedVariables()->GetValue(name))
+      return *value;
+  }
+  return GetInitialRegisteredVariable(name, InitialDataInternal().get());
 }
 
 bool ComputedStyle::SetFontDescription(const FontDescription& v) {

third_party/blink/renderer/core/style/computed_style.h

@@ -1146,8 +1146,6 @@ class ComputedStyle : public ComputedStyleBase,
                              const CSSValue*,
                              bool is_inherited_property);
 
-  void RemoveVariable(const AtomicString&, bool is_inherited_property);
-
   // Handles both inherited and non-inherited variables
   CORE_EXPORT CSSVariableData* GetVariable(const AtomicString&) const;
 
@@ -1159,12 +1157,6 @@ class ComputedStyle : public ComputedStyleBase,
 
   const CSSValue* GetRegisteredVariable(const AtomicString&) const;
 
-  // Like GetRegisteredVariable, but returns nullptr if the computed value
-  // for the specified variable is the initial value.
-  const CSSValue* GetNonInitialRegisteredVariable(
-      const AtomicString&,
-      bool is_inherited_property) const;
-
   // Animations.
   CSSAnimationData& AccessAnimations();
   const CSSAnimationData* Animations() const {

third_party/blink/renderer/core/style/style_inherited_variables.cc

@@ -10,92 +10,64 @@ namespace blink {
 
 bool StyleInheritedVariables::operator==(
     const StyleInheritedVariables& other) const {
-  // It's technically possible for divergent roots to be value-equal,
-  // but unlikely. This equality operator is used for optimization purposes
-  // so it's OK to be occasionally wrong.
-  // TODO(shanestephens): Rename this to something that indicates it may not
-  // always return equality.
-  if (root_ != other.root_)
-    return false;
-
-  if (data_.size() != other.data_.size())
-    return false;
-
-  for (const auto& iter : data_) {
-    scoped_refptr<CSSVariableData> other_data = other.data_.at(iter.key);
-    if (!DataEquivalent(iter.value, other_data))
-      return false;
-  }
-
-  return true;
+  return DataEquivalent(root_, other.root_) && variables_ == other.variables_;
 }
 
 StyleInheritedVariables::StyleInheritedVariables()
-    : registered_data_(
-          MakeGarbageCollected<HeapHashMap<AtomicString, Member<CSSValue>>>()),
-      root_(nullptr),
-      needs_resolution_(false) {}
+    : root_(nullptr), needs_resolution_(false) {}
 
-StyleInheritedVariables::StyleInheritedVariables(
-    StyleInheritedVariables& other) {
+StyleInheritedVariables::StyleInheritedVariables(StyleInheritedVariables& other)
+    : needs_resolution_(other.needs_resolution_) {
   if (!other.root_) {
-    registered_data_ =
-        MakeGarbageCollected<HeapHashMap<AtomicString, Member<CSSValue>>>();
     root_ = &other;
   } else {
-    data_ = other.data_;
-    registered_data_ =
-        MakeGarbageCollected<HeapHashMap<AtomicString, Member<CSSValue>>>(
-            *other.registered_data_);
+    variables_ = other.variables_;
     root_ = other.root_;
   }
-  needs_resolution_ = other.needs_resolution_;
 }
 
 CSSVariableData* StyleInheritedVariables::GetVariable(
     const AtomicString& name) const {
-  auto result = data_.find(name);
-  if (result == data_.end() && root_)
-    return root_->GetVariable(name);
-  if (result == data_.end())
-    return nullptr;
-  return result->value.get();
+  return GetData(name).value_or(nullptr);
+}
+
+StyleVariables::OptionalData StyleInheritedVariables::GetData(
+    const AtomicString& name) const {
+  if (auto data = variables_.GetData(name))
+    return *data;
+  if (root_)
+    return root_->variables_.GetData(name);
+  return base::nullopt;
 }
 
 void StyleInheritedVariables::SetRegisteredVariable(
     const AtomicString& name,
     const CSSValue* parsed_value) {
   needs_resolution_ = true;
-  registered_data_->Set(name, const_cast<CSSValue*>(parsed_value));
+  variables_.SetValue(name, parsed_value);
 }
 
 const CSSValue* StyleInheritedVariables::RegisteredVariable(
     const AtomicString& name) const {
-  auto result = registered_data_->find(name);
-  if (result != registered_data_->end())
-    return result->value.Get();
-  if (root_)
-    return root_->RegisteredVariable(name);
-  return nullptr;
+  return GetValue(name).value_or(nullptr);
 }
 
-void StyleInheritedVariables::RemoveVariable(const AtomicString& name) {
-  data_.Set(name, nullptr);
-  auto iterator = registered_data_->find(name);
-  if (iterator != registered_data_->end()) {
-    iterator->value = nullptr;
-  } else if (root_ && root_->RegisteredVariable(name)) {
-    SetRegisteredVariable(name, nullptr);
-  }
+StyleVariables::OptionalValue StyleInheritedVariables::GetValue(
+    const AtomicString& name) const {
+  if (auto data = variables_.GetValue(name))
+    return *data;
+  if (root_)
+    return root_->variables_.GetValue(name);
+  return base::nullopt;
 }
 
 HashSet<AtomicString> StyleInheritedVariables::GetCustomPropertyNames() const {
   HashSet<AtomicString> names;
   if (root_) {
-    for (const auto& pair : root_->data_)
+    for (const auto& pair : root_->Data())
       names.insert(pair.key);
   }
-  for (const auto& pair : data_)
+  for (const auto& pair : Data())
     names.insert(pair.key);
   return names;
 }

third_party/blink/renderer/core/style/style_inherited_variables.h

@@ -7,6 +7,7 @@
 
 #include "third_party/blink/renderer/core/css/css_value.h"
 #include "third_party/blink/renderer/core/css/css_variable_data.h"
+#include "third_party/blink/renderer/core/style/style_variables.h"
 #include "third_party/blink/renderer/platform/heap/persistent.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"
 #include "third_party/blink/renderer/platform/wtf/hash_map.h"
@@ -35,19 +36,23 @@ class CORE_EXPORT StyleInheritedVariables
                    scoped_refptr<CSSVariableData> value) {
     needs_resolution_ =
         needs_resolution_ || (value && value->NeedsVariableResolution());
-    data_.Set(name, std::move(value));
+    variables_.SetData(name, std::move(value));
   }
   CSSVariableData* GetVariable(const AtomicString& name) const;
-  void RemoveVariable(const AtomicString&);
+  StyleVariables::OptionalData GetData(const AtomicString&) const;
 
   void SetRegisteredVariable(const AtomicString&, const CSSValue*);
   const CSSValue* RegisteredVariable(const AtomicString&) const;
+  StyleVariables::OptionalValue GetValue(const AtomicString&) const;
 
   // Note that not all custom property names returned here necessarily have
   // valid values, due to cycles or references to invalid variables without
   // using a fallback.
   HashSet<AtomicString> GetCustomPropertyNames() const;
 
+  const StyleVariables::DataMap& Data() const { return variables_.Data(); }
+  const StyleVariables::ValueMap& Values() const { return variables_.Values(); }
+
   bool NeedsResolution() const { return needs_resolution_; }
   void ClearNeedsResolution() { needs_resolution_ = false; }
 
@@ -55,10 +60,7 @@ class CORE_EXPORT StyleInheritedVariables
   StyleInheritedVariables();
   StyleInheritedVariables(StyleInheritedVariables& other);
 
-  friend class CSSVariableResolver;
-
-  HashMap<AtomicString, scoped_refptr<CSSVariableData>> data_;
-  Persistent<HeapHashMap<AtomicString, Member<CSSValue>>> registered_data_;
+  StyleVariables variables_;
   scoped_refptr<StyleInheritedVariables> root_;
   bool needs_resolution_;
 };

third_party/blink/renderer/core/style/style_initial_data.cc

@@ -9,33 +9,19 @@
 
 namespace blink {
 
-StyleInitialData::StyleInitialData(const PropertyRegistry& registry)
-    : variables_(MakeGarbageCollected<
-                 HeapHashMap<AtomicString, Member<const CSSValue>>>()) {
+StyleInitialData::StyleInitialData(const PropertyRegistry& registry) {
   for (const auto& entry : registry) {
-    const CSSValue* initial = entry.value->Initial();
-    if (!initial)
+    CSSVariableData* data = entry.value->InitialVariableData();
+    if (!data)
       continue;
-    variables_->Set(entry.key, initial);
+    variables_.SetData(entry.key, data);
+    if (const CSSValue* initial = entry.value->Initial())
+      variables_.SetValue(entry.key, initial);
   }
 }
 
-StyleInitialData::StyleInitialData(StyleInitialData& other)
-    : variables_(MakeGarbageCollected<
-                 HeapHashMap<AtomicString, Member<const CSSValue>>>(
-          *other.variables_)) {}
-
 bool StyleInitialData::operator==(const StyleInitialData& other) const {
-  if (variables_->size() != other.variables_->size())
-    return false;
-
-  for (const auto& iter : *variables_) {
-    const CSSValue* other_value = other.variables_->at(iter.key);
-    if (iter.value != other_value)
-      return false;
-  }
-
-  return true;
+  return variables_ == other.variables_;
 }
 
 }  // namespace blink

third_party/blink/renderer/core/style/style_initial_data.h

@@ -7,6 +7,7 @@
 
 #include "base/memory/ptr_util.h"
 #include "third_party/blink/renderer/core/css/css_value.h"
+#include "third_party/blink/renderer/core/style/style_variables.h"
 #include "third_party/blink/renderer/platform/heap/persistent.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"
 #include "third_party/blink/renderer/platform/wtf/hash_map.h"
@@ -32,20 +33,23 @@ class CORE_EXPORT StyleInitialData : public RefCounted<StyleInitialData> {
     return !(*this == other);
   }
 
-  bool HasInitialVariables() const { return variables_->size(); }
+  bool HasInitialVariables() const { return !variables_.IsEmpty(); }
 
-  const CSSValue* GetInitialVariable(const AtomicString& name) const {
-    return variables_->at(name);
+  CSSVariableData* GetVariableData(const AtomicString& name) const {
+    return variables_.GetData(name).value_or(nullptr);
+  }
+
+  const CSSValue* GetVariableValue(const AtomicString& name) const {
+    return variables_.GetValue(name).value_or(nullptr);
   }
 
  private:
   StyleInitialData(const PropertyRegistry&);
-  StyleInitialData(StyleInitialData&);
 
   // Initial values for all registered properties. This is set on
   // the initial style, and then shared with all other styles that directly or
   // indirectly inherit from that.
-  Persistent<HeapHashMap<AtomicString, Member<const CSSValue>>> variables_;
+  StyleVariables variables_;
 };
 
 }  // namespace blink

third_party/blink/renderer/core/style/style_non_inherited_variables.cc

@@ -10,55 +10,34 @@ namespace blink {
 
 bool StyleNonInheritedVariables::operator==(
     const StyleNonInheritedVariables& other) const {
-  if (data_.size() != other.data_.size())
-    return false;
-
-  for (const auto& iter : data_) {
-    scoped_refptr<CSSVariableData> other_data = other.data_.at(iter.key);
-    if (!DataEquivalent(iter.value, other_data))
-      return false;
-  }
-
-  return true;
+  return variables_ == other.variables_;
 }
 
 CSSVariableData* StyleNonInheritedVariables::GetVariable(
     const AtomicString& name) const {
-  return data_.at(name);
+  return variables_.GetData(name).value_or(nullptr);
+}
+
+StyleVariables::OptionalData StyleNonInheritedVariables::GetData(
+    const AtomicString& name) const {
+  return variables_.GetData(name);
 }
 
 void StyleNonInheritedVariables::SetRegisteredVariable(
     const AtomicString& name,
     const CSSValue* parsed_value) {
   needs_resolution_ = true;
-  registered_data_->Set(name, const_cast<CSSValue*>(parsed_value));
+  variables_.SetValue(name, parsed_value);
 }
 
-void StyleNonInheritedVariables::RemoveVariable(const AtomicString& name) {
-  data_.Set(name, nullptr);
-  registered_data_->Set(name, nullptr);
-}
-
-StyleNonInheritedVariables::StyleNonInheritedVariables()
-    : registered_data_(
-          MakeGarbageCollected<HeapHashMap<AtomicString, Member<CSSValue>>>()),
-      needs_resolution_(false) {}
-
-StyleNonInheritedVariables::StyleNonInheritedVariables(
-    StyleNonInheritedVariables& other) {
-  data_ = other.data_;
-  registered_data_ =
-      MakeGarbageCollected<HeapHashMap<AtomicString, Member<CSSValue>>>(
-          *other.registered_data_);
-  needs_resolution_ = other.needs_resolution_;
+StyleVariables::OptionalValue StyleNonInheritedVariables::GetValue(
+    const AtomicString& name) const {
+  return variables_.GetValue(name);
 }
 
 HashSet<AtomicString> StyleNonInheritedVariables::GetCustomPropertyNames()
     const {
-  HashSet<AtomicString> names;
-  for (const auto& pair : data_)
-    names.insert(pair.key);
-  return names;
+  return variables_.GetNames();
 }
 
 }  // namespace blink

third_party/blink/renderer/core/style/style_non_inherited_variables.h

@@ -11,6 +11,7 @@
 #include "base/memory/ptr_util.h"
 #include "third_party/blink/renderer/core/css/css_value.h"
 #include "third_party/blink/renderer/core/css/css_variable_data.h"
+#include "third_party/blink/renderer/core/style/style_variables.h"
 #include "third_party/blink/renderer/platform/heap/persistent.h"
 #include "third_party/blink/renderer/platform/wtf/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"
@@ -27,9 +28,6 @@ class CORE_EXPORT StyleNonInheritedVariables {
     return base::WrapUnique(new StyleNonInheritedVariables(*this));
   }
 
-  StyleNonInheritedVariables();
-  explicit StyleNonInheritedVariables(StyleNonInheritedVariables&);
-
   bool operator==(const StyleNonInheritedVariables& other) const;
   bool operator!=(const StyleNonInheritedVariables& other) const {
     return !(*this == other);
@@ -39,27 +37,28 @@ class CORE_EXPORT StyleNonInheritedVariables {
                    scoped_refptr<CSSVariableData> value) {
     needs_resolution_ =
         needs_resolution_ || (value && value->NeedsVariableResolution());
-    data_.Set(name, std::move(value));
+    variables_.SetData(name, std::move(value));
   }
   CSSVariableData* GetVariable(const AtomicString& name) const;
-  void RemoveVariable(const AtomicString&);
+  StyleVariables::OptionalData GetData(const AtomicString&) const;
 
   void SetRegisteredVariable(const AtomicString&, const CSSValue*);
   const CSSValue* RegisteredVariable(const AtomicString& name) const {
-    return registered_data_->at(name);
+    return variables_.GetValue(name).value_or(nullptr);
   }
+  StyleVariables::OptionalValue GetValue(const AtomicString&) const;
 
   HashSet<AtomicString> GetCustomPropertyNames() const;
 
+  const StyleVariables::DataMap& Data() const { return variables_.Data(); }
+  const StyleVariables::ValueMap& Values() const { return variables_.Values(); }
+
   bool NeedsResolution() const { return needs_resolution_; }
   void ClearNeedsResolution() { needs_resolution_ = false; }
 
  private:
-  friend class CSSVariableResolver;
-
-  HashMap<AtomicString, scoped_refptr<CSSVariableData>> data_;
-  Persistent<HeapHashMap<AtomicString, Member<CSSValue>>> registered_data_;
-  bool needs_resolution_;
+  StyleVariables variables_;
+  bool needs_resolution_ = false;
 };
 
 }  // namespace blink

third_party/blink/renderer/core/style/style_variables.cc

+// Copyright 2019 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/style/style_variables.h"
+
+#include "third_party/blink/renderer/core/style/data_equivalency.h"
+
+namespace blink {
+
+namespace {
+
+using OptionalData = StyleVariables::OptionalData;
+using OptionalValue = StyleVariables::OptionalValue;
+
+bool IsEqual(const OptionalData& a, const OptionalData& b) {
+  if (a.has_value() != b.has_value())
+    return false;
+  if (!a.has_value())
+    return true;
+  return DataEquivalent(a.value(), b.value());
+}
+
+bool IsEqual(const OptionalValue& a, const OptionalValue& b) {
+  if (a.has_value() != b.has_value())
+    return false;
+  if (!a.has_value())
+    return true;
+  return DataEquivalent(a.value(), b.value());
+}
+
+}  // namespace
+
+StyleVariables::StyleVariables() : values_(MakeGarbageCollected<ValueMap>()) {}
+
+StyleVariables::StyleVariables(const StyleVariables& other)
+    : data_(other.data_),
+      values_(MakeGarbageCollected<ValueMap>(*other.values_)) {}
+
+bool StyleVariables::operator==(const StyleVariables& other) const {
+  if (data_.size() != other.data_.size())
+    return false;
+
+  for (const auto& pair : data_) {
+    if (!IsEqual(GetData(pair.key), other.GetData(pair.key)))
+      return false;
+  }
+
+  if (values_->size() != other.values_->size())
+    return false;
+
+  for (const auto& pair : *values_) {
+    if (!IsEqual(GetValue(pair.key), other.GetValue(pair.key)))
+      return false;
+  }
+
+  return true;
+}
+
+StyleVariables::OptionalData StyleVariables::GetData(
+    const AtomicString& name) const {
+  auto i = data_.find(name);
+  if (i != data_.end())
+    return i->value.get();
+  return base::nullopt;
+}
+
+StyleVariables::OptionalValue StyleVariables::GetValue(
+    const AtomicString& name) const {
+  auto i = values_->find(name);
+  if (i != values_->end())
+    return i->value;
+  return base::nullopt;
+}
+
+void StyleVariables::SetData(const AtomicString& name,
+                             scoped_refptr<CSSVariableData> data) {
+  data_.Set(name, std::move(data));
+}
+
+void StyleVariables::SetValue(const AtomicString& name, const CSSValue* value) {
+  values_->Set(name, value);
+}
+
+bool StyleVariables::IsEmpty() const {
+  return data_.IsEmpty() && values_->IsEmpty();
+}
+
+HashSet<AtomicString> StyleVariables::GetNames() const {
+  HashSet<AtomicString> names;
+  for (const auto& pair : data_)
+    names.insert(pair.key);
+  return names;
+}
+
+}  // namespace blink

third_party/blink/renderer/core/style/style_variables.h

+// Copyright 2019 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_STYLE_STYLE_VARIABLES_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_STYLE_STYLE_VARIABLES_H_
+
+#include "base/optional.h"
+#include "third_party/blink/renderer/core/css/css_value.h"
+#include "third_party/blink/renderer/core/css/css_variable_data.h"
+#include "third_party/blink/renderer/platform/heap/persistent.h"
+#include "third_party/blink/renderer/platform/wtf/forward.h"
+#include "third_party/blink/renderer/platform/wtf/hash_map.h"
+#include "third_party/blink/renderer/platform/wtf/text/atomic_string_hash.h"
+
+namespace blink {
+
+// Contains values for custom properties.
+//
+// Each custom property has "variable data" and optionally a "variable value".
+//
+// * Data:  A CSSVariableData that contains the tokens used for substitution.
+// * Value: An optional CSSValue that may be present if the custom property
+//          is registered with a non-universal syntax descriptor.
+//
+// Note that StyleVariables may explicitly contain a nullptr value for a given
+// custom property. This is necessary to be able to mark variables that become
+// invalid at computed-value time [1] as such.
+//
+// If StyleVariables does not contain an entry at all for a given property,
+// base::nullopt is returned. This allows us to differentiate between the case
+// where we want to try to find the variable elsewhere (e.g. StyleInitialData,
+// in the case of base::nullopt), or return nullptr without looking further.
+//
+// Due to the subtleties introduced by the root-bucket optimization in
+// StyleInheritedVariables, there is deliberately no way to erase an entry
+// from StyleVariables. This means that non-implicit initial/inherited values
+// must be explicitly stored.
+//
+// [1] https://drafts.csswg.org/css-variables/#invalid-at-computed-value-time
+class CORE_EXPORT StyleVariables {
+  USING_FAST_MALLOC(StyleVariables);
+
+ public:
+  using DataMap = HashMap<AtomicString, scoped_refptr<CSSVariableData>>;
+  using ValueMap = HeapHashMap<AtomicString, Member<const CSSValue>>;
+
+  StyleVariables();
+  StyleVariables(const StyleVariables&);
+
+  bool operator==(const StyleVariables& other) const;
+  bool operator!=(const StyleVariables& other) const {
+    return !(*this == other);
+  }
+
+  using OptionalData = base::Optional<CSSVariableData*>;
+  using OptionalValue = base::Optional<Member<const CSSValue>>;
+
+  OptionalValue GetValue(const AtomicString&) const;
+  OptionalData GetData(const AtomicString&) const;
+  void SetData(const AtomicString&, scoped_refptr<CSSVariableData>);
+  void SetValue(const AtomicString&, const CSSValue*);
+
+  bool IsEmpty() const;
+  HashSet<AtomicString> GetNames() const;
+
+  const DataMap& Data() const { return data_; }
+  const ValueMap& Values() const { return *values_; }
+
+ private:
+  DataMap data_;
+  Persistent<ValueMap> values_;
+};
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_STYLE_STYLE_VARIABLES_H_

third_party/blink/renderer/core/style/style_variables_test.cc

+// Copyright 2019 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/style/style_variables.h"
+
+#include "third_party/blink/renderer/core/css/css_custom_ident_value.h"
+#include "third_party/blink/renderer/core/css/css_test_helpers.h"
+#include "third_party/blink/renderer/core/css/css_variable_data.h"
+#include "third_party/blink/renderer/core/css/parser/css_tokenizer.h"
+#include "third_party/blink/renderer/core/testing/page_test_base.h"
+
+namespace blink {
+namespace {
+
+using namespace css_test_helpers;
+
+class StyleVariablesTest : public PageTestBase {
+ public:
+  scoped_refptr<CSSVariableData> CreateData(String s) {
+    auto tokens = CSSTokenizer(s).TokenizeToEOF();
+    CSSParserTokenRange range(tokens);
+    bool is_animation_tainted = false;
+    bool needs_variable_resolution = false;
+    return CSSVariableData::Create(range, is_animation_tainted,
+                                   needs_variable_resolution, KURL(),
+                                   WTF::TextEncoding());
+  }
+
+  const CSSValue* CreateValue(AtomicString s) {
+    return MakeGarbageCollected<CSSCustomIdentValue>(s);
+  }
+};
+
+TEST_F(StyleVariablesTest, EmptyEqual) {
+  StyleVariables vars1;
+  StyleVariables vars2;
+  EXPECT_EQ(vars1, vars1);
+  EXPECT_EQ(vars2, vars2);
+  EXPECT_EQ(vars1, vars2);
+}
+
+TEST_F(StyleVariablesTest, Copy) {
+  auto foo_data = CreateData("foo");
+  const CSSValue* foo_value = CreateValue("foo");
+
+  StyleVariables vars1;
+  vars1.SetData("--x", foo_data);
+  vars1.SetValue("--x", foo_value);
+
+  StyleVariables vars2(vars1);
+  EXPECT_EQ(foo_data, vars2.GetData("--x").value_or(nullptr));
+  EXPECT_EQ(foo_value, vars2.GetValue("--x").value_or(nullptr));
+  EXPECT_EQ(vars1, vars2);
+}
+
+TEST_F(StyleVariablesTest, GetNames) {
+  StyleVariables vars;
+  vars.SetData("--x", CreateData("foo"));
+  vars.SetData("--y", CreateData("bar"));
+
+  HashSet<AtomicString> names = vars.GetNames();
+  EXPECT_EQ(2u, names.size());
+  EXPECT_TRUE(names.Contains("--x"));
+  EXPECT_TRUE(names.Contains("--y"));
+}
+
+// CSSVariableData
+
+TEST_F(StyleVariablesTest, IsEmptyData) {
+  StyleVariables vars;
+  EXPECT_TRUE(vars.IsEmpty());
+  vars.SetData("--x", CreateData("foo"));
+  EXPECT_FALSE(vars.IsEmpty());
+}
+
+TEST_F(StyleVariablesTest, SetData) {
+  StyleVariables vars;
+
+  auto foo = CreateData("foo");
+  auto bar = CreateData("bar");
+
+  EXPECT_FALSE(vars.GetData("--x").has_value());
+
+  vars.SetData("--x", foo);
+  EXPECT_EQ(foo, vars.GetData("--x").value_or(nullptr));
+
+  vars.SetData("--x", bar);
+  EXPECT_EQ(bar, vars.GetData("--x").value_or(nullptr));
+}
+
+TEST_F(StyleVariablesTest, SetNullData) {
+  StyleVariables vars;
+  EXPECT_FALSE(vars.GetData("--x").has_value());
+  vars.SetData("--x", nullptr);
+  auto data = vars.GetData("--x");
+  ASSERT_TRUE(data.has_value());
+  EXPECT_EQ(nullptr, data.value());
+}
+
+TEST_F(StyleVariablesTest, SingleDataSamePointer) {
+  auto data = CreateData("foo");
+  StyleVariables vars1;
+  StyleVariables vars2;
+  vars1.SetData("--x", data);
+  vars2.SetData("--x", data);
+  EXPECT_EQ(vars1, vars2);
+}
+
+TEST_F(StyleVariablesTest, SingleDataSameContent) {
+  StyleVariables vars1;
+  StyleVariables vars2;
+  vars1.SetData("--x", CreateData("foo"));
+  vars2.SetData("--x", CreateData("foo"));
+  EXPECT_EQ(vars1, vars2);
+}
+
+TEST_F(StyleVariablesTest, SingleDataContentNotEqual) {
+  StyleVariables vars1;
+  StyleVariables vars2;
+  vars1.SetData("--x", CreateData("foo"));
+  vars2.SetData("--x", CreateData("bar"));
+  EXPECT_NE(vars1, vars2);
+}
+
+TEST_F(StyleVariablesTest, DifferentDataSize) {
+  StyleVariables vars1;
+  StyleVariables vars2;
+  vars1.SetData("--x", CreateData("foo"));
+  vars2.SetData("--x", CreateData("bar"));
+  vars2.SetData("--y", CreateData("foz"));
+  EXPECT_NE(vars1, vars2);
+}
+
+// CSSValue
+
+TEST_F(StyleVariablesTest, IsEmptyValue) {
+  StyleVariables vars;
+  EXPECT_TRUE(vars.IsEmpty());
+  vars.SetValue("--x", CreateValue("foo"));
+  EXPECT_FALSE(vars.IsEmpty());
+}
+
+TEST_F(StyleVariablesTest, SetValue) {
+  StyleVariables vars;
+
+  const CSSValue* foo = CreateValue("foo");
+  const CSSValue* bar = CreateValue("bar");
+
+  EXPECT_FALSE(vars.GetValue("--x").has_value());
+
+  vars.SetValue("--x", foo);
+  EXPECT_EQ(foo, vars.GetValue("--x").value_or(nullptr));
+
+  vars.SetValue("--x", bar);
+  EXPECT_EQ(bar, vars.GetValue("--x").value_or(nullptr));
+}
+
+TEST_F(StyleVariablesTest, SetNullValue) {
+  StyleVariables vars;
+  EXPECT_FALSE(vars.GetValue("--x").has_value());
+  vars.SetValue("--x", nullptr);
+  auto value = vars.GetValue("--x");
+  ASSERT_TRUE(value.has_value());
+  EXPECT_EQ(nullptr, value.value());
+}
+
+TEST_F(StyleVariablesTest, SingleValueSamePointer) {
+  const CSSValue* foo = CreateValue("foo");
+  StyleVariables vars1;
+  StyleVariables vars2;
+  vars1.SetValue("--x", foo);
+  vars2.SetValue("--x", foo);
+  EXPECT_EQ(vars1, vars2);
+}
+
+TEST_F(StyleVariablesTest, SingleValueSameContent) {
+  StyleVariables vars1;
+  StyleVariables vars2;
+  vars1.SetValue("--x", CreateValue("foo"));
+  vars2.SetValue("--x", CreateValue("foo"));
+  EXPECT_EQ(vars1, vars2);
+}
+
+TEST_F(StyleVariablesTest, SingleValueContentNotEqual) {
+  StyleVariables vars1;
+  StyleVariables vars2;
+  vars1.SetValue("--x", CreateValue("foo"));
+  vars2.SetValue("--x", CreateValue("bar"));
+  EXPECT_NE(vars1, vars2);
+}
+
+TEST_F(StyleVariablesTest, DifferentValueSize) {
+  StyleVariables vars1;
+  StyleVariables vars2;
+  vars1.SetValue("--x", CreateValue("foo"));
+  vars2.SetValue("--x", CreateValue("bar"));
+  vars2.SetValue("--y", CreateValue("foz"));
+  EXPECT_NE(vars1, vars2);
+}
+
+}  // namespace
+}  // namespace blink