Fix MergeAndOverrideOnConflict for custom properties.

Utilizing CSSPropertyName, to make the code much nicer.

R=futhark@chromium.org

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

third_party/blink/renderer/core/BUILD.gn

@@ -1732,6 +1732,7 @@ jumbo_source_set("unit_tests") {
     "css/css_paint_value_test.cc",
     "css/css_primitive_value_test.cc",
     "css/css_property_name_test.cc",
+    "css/css_property_value_set_test.cc",
     "css/css_selector_test.cc",
     "css/css_selector_watch_test.cc",
     "css/css_style_declaration_test.cc",

third_party/blink/renderer/core/css/css_property_value.cc

@@ -20,6 +20,8 @@
 
 #include "third_party/blink/renderer/core/css/css_property_value.h"
 
+#include "third_party/blink/renderer/core/css/css_custom_property_declaration.h"
+#include "third_party/blink/renderer/core/css/css_property_name.h"
 #include "third_party/blink/renderer/core/style/computed_style_constants.h"
 #include "third_party/blink/renderer/core/style_property_shorthand.h"
 
@@ -46,6 +48,12 @@ CSSPropertyID CSSPropertyValueMetadata::ShorthandID() const {
   return shorthands.at(index_in_shorthands_vector_).id();
 }
 
+CSSPropertyName CSSPropertyValue::Name() const {
+  if (Id() != CSSPropertyVariable)
+    return CSSPropertyName(Id());
+  return CSSPropertyName(ToCSSCustomPropertyDeclaration(value_)->GetName());
+}
+
 bool CSSPropertyValue::operator==(const CSSPropertyValue& other) const {
   return DataEquivalent(value_, other.value_) &&
          IsImportant() == other.IsImportant();

third_party/blink/renderer/core/css/css_property_value.h

@@ -21,6 +21,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSS_PROPERTY_VALUE_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_CSS_PROPERTY_VALUE_H_
 
+#include "third_party/blink/renderer/core/css/css_property_name.h"
 #include "third_party/blink/renderer/core/css/css_value.h"
 #include "third_party/blink/renderer/core/css/properties/css_property.h"
 #include "third_party/blink/renderer/core/css_property_names.h"
@@ -86,6 +87,7 @@ class CSSPropertyValue {
   bool IsSetFromShorthand() const { return metadata_.is_set_from_shorthand_; }
   CSSPropertyID ShorthandID() const { return metadata_.ShorthandID(); }
   bool IsImportant() const { return metadata_.important_; }
+  CSSPropertyName Name() const;
 
   const CSSValue* Value() const { return value_.Get(); }
 

third_party/blink/renderer/core/css/css_property_value_set.cc

@@ -416,12 +416,8 @@ void MutableCSSPropertyValueSet::SetProperty(CSSPropertyID property_id,
 
 bool MutableCSSPropertyValueSet::SetProperty(const CSSPropertyValue& property,
                                              CSSPropertyValue* slot) {
-  const AtomicString& name =
-      (property.Id() == CSSPropertyVariable)
-          ? ToCSSCustomPropertyDeclaration(property.Value())->GetName()
-          : g_null_atom;
   CSSPropertyValue* to_replace =
-      slot ? slot : FindCSSPropertyWithID(property.Id(), name);
+      slot ? slot : FindCSSPropertyWithName(property.Name());
   if (to_replace && *to_replace == property)
     return false;
   if (to_replace) {
@@ -485,8 +481,7 @@ void MutableCSSPropertyValueSet::MergeAndOverrideOnConflict(
   unsigned size = other->PropertyCount();
   for (unsigned n = 0; n < size; ++n) {
     PropertyReference to_merge = other->PropertyAt(n);
-    // TODO(leviw): This probably doesn't work correctly with Custom Properties
-    CSSPropertyValue* old = FindCSSPropertyWithID(to_merge.Id());
+    CSSPropertyValue* old = FindCSSPropertyWithName(to_merge.Name());
     if (old) {
       SetProperty(
           CSSPropertyValue(to_merge.PropertyMetadata(), to_merge.Value()), old);
@@ -543,18 +538,11 @@ bool MutableCSSPropertyValueSet::RemovePropertiesInSet(const CSSProperty** set,
   return false;
 }
 
-CSSPropertyValue* MutableCSSPropertyValueSet::FindCSSPropertyWithID(
-    CSSPropertyID property_id,
-    const AtomicString& custom_property_name) {
-  int found_property_index = -1;
-  if (property_id == CSSPropertyVariable && !custom_property_name.IsNull()) {
-    // TODO(shanestephens): fix call sites so we always have a
-    // customPropertyName here.
-    found_property_index = FindPropertyIndex(custom_property_name);
-  } else {
-    DCHECK(custom_property_name.IsNull());
-    found_property_index = FindPropertyIndex(property_id);
-  }
+CSSPropertyValue* MutableCSSPropertyValueSet::FindCSSPropertyWithName(
+    const CSSPropertyName& name) {
+  int found_property_index = name.IsCustomProperty()
+                                 ? FindPropertyIndex(name.ToAtomicString())
+                                 : FindPropertyIndex(name.Id());
   if (found_property_index == -1)
     return nullptr;
   return &property_vector_.at(found_property_index);

third_party/blink/renderer/core/css/css_property_value_set.h

@@ -291,9 +291,7 @@ class CORE_EXPORT MutableCSSPropertyValueSet : public CSSPropertyValueSet {
   bool RemoveShorthandProperty(const AtomicString& custom_property_name) {
     return false;
   }
-  CSSPropertyValue* FindCSSPropertyWithID(
-      CSSPropertyID,
-      const AtomicString& custom_property_name = g_null_atom);
+  CSSPropertyValue* FindCSSPropertyWithName(const CSSPropertyName&);
   Member<PropertySetCSSStyleDeclaration> cssom_wrapper_;
 
   friend class CSSPropertyValueSet;

third_party/blink/renderer/core/css/css_property_value_set_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/css_property_value_set.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/core/css/parser/css_parser.h"
+#include "third_party/blink/renderer/core/css/parser/css_parser_context.h"
+#include "third_party/blink/renderer/core/css/style_rule.h"
+#include "third_party/blink/renderer/core/css/style_sheet_contents.h"
+#include "third_party/blink/renderer/core/testing/page_test_base.h"
+
+namespace blink {
+
+class CSSPropertyValueSetTest : public PageTestBase {
+ public:
+  StyleRule* RuleAt(StyleSheetContents* sheet, wtf_size_t index) {
+    return ToStyleRule(sheet->ChildRules()[index]);
+  }
+};
+
+TEST_F(CSSPropertyValueSetTest, MergeAndOverrideOnConflictCustomProperty) {
+  CSSParserContext* context = CSSParserContext::Create(GetDocument());
+  StyleSheetContents* style_sheet = StyleSheetContents::Create(context);
+
+  String sheet_text = R"CSS(
+    #first {
+      color: red;
+      --x:foo;
+      --y:foo;
+    }
+    #second {
+      color: green;
+      --x:bar;
+      --y:bar;
+    }
+  )CSS";
+
+  CSSParser::ParseSheet(context, style_sheet, sheet_text,
+                        CSSDeferPropertyParsing::kNo);
+  StyleRule* rule0 = RuleAt(style_sheet, 0);
+  StyleRule* rule1 = RuleAt(style_sheet, 1);
+  MutableCSSPropertyValueSet& set0 = rule0->MutableProperties();
+  MutableCSSPropertyValueSet& set1 = rule1->MutableProperties();
+
+  EXPECT_EQ(3u, set0.PropertyCount());
+  EXPECT_EQ("red", set0.GetPropertyValue(CSSPropertyColor));
+  EXPECT_EQ("foo", set0.GetPropertyValue(AtomicString("--x")));
+  EXPECT_EQ("foo", set0.GetPropertyValue(AtomicString("--y")));
+  EXPECT_EQ(3u, set1.PropertyCount());
+  EXPECT_EQ("green", set1.GetPropertyValue(CSSPropertyColor));
+  EXPECT_EQ("bar", set1.GetPropertyValue(AtomicString("--x")));
+  EXPECT_EQ("bar", set1.GetPropertyValue(AtomicString("--y")));
+
+  set0.MergeAndOverrideOnConflict(&set1);
+
+  EXPECT_EQ(3u, set0.PropertyCount());
+  EXPECT_EQ("green", set0.GetPropertyValue(CSSPropertyColor));
+  EXPECT_EQ("bar", set0.GetPropertyValue(AtomicString("--x")));
+  EXPECT_EQ("bar", set0.GetPropertyValue(AtomicString("--y")));
+  EXPECT_EQ(3u, set1.PropertyCount());
+  EXPECT_EQ("green", set1.GetPropertyValue(CSSPropertyColor));
+  EXPECT_EQ("bar", set1.GetPropertyValue(AtomicString("--x")));
+  EXPECT_EQ("bar", set1.GetPropertyValue(AtomicString("--y")));
+}
+
+}  // namespace blink