[scroll-animations] Don't store a CSSPropertyValueSet on the style rule

Instead of storing a CSSPropertyValueSet, and then calling
GetPropertyCSSValue on-demand, dig out all the CSSValues during
construction of the StyleRuleScrollTimeline object.

The reason for this is somewhat dull: if a unit test disables the
CSSScrollTimeline runtime flag, and *then* tries to access some
descriptor using GetPropertyCSSValue, we hit a DCHECK because the
descriptor is (no longer) web-exposed.

It would have been possible to avoid accessing the StyleRuleScroll-
Timeline while the flag is disabled some other way (per test), but
it's easier to just allow it in general.

Added a bonus unit test for Copy(), since this CL changes how that
works.

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

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

@@ -687,6 +687,7 @@ blink_core_tests("unit_tests") {
     "style_element_test.cc",
     "style_engine_test.cc",
     "style_environment_variables_test.cc",
+    "style_rule_test.cc",
     "style_sheet_contents_test.cc",
     "style_traversal_root_test.cc",
     "threaded/css_parser_threaded_test.cc",

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

@@ -12,6 +12,7 @@
 #include "third_party/blink/renderer/core/css/css_style_sheet.h"
 #include "third_party/blink/renderer/core/css/css_syntax_string_parser.h"
 #include "third_party/blink/renderer/core/css/css_variable_data.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/parser/css_parser_local_context.h"
 #include "third_party/blink/renderer/core/css/parser/css_property_parser.h"
@@ -137,5 +138,13 @@ const CSSPropertyValueSet* ParseDeclarationBlock(const String& block_text,
   return set;
 }
 
+StyleRuleBase* ParseRule(Document& document, String text) {
+  TextPosition position;
+  auto* sheet = CSSStyleSheet::CreateInline(document, NullURL(), position,
+                                            UTF8Encoding());
+  const auto* context = MakeGarbageCollected<CSSParserContext>(document);
+  return CSSParser::ParseRule(context, sheet->Contents(), text);
+}
+
 }  // namespace css_test_helpers
 }  // namespace blink

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

@@ -66,6 +66,7 @@ const CSSValue* ParseLonghand(Document& document,
 const CSSPropertyValueSet* ParseDeclarationBlock(
     const String& block_text,
     CSSParserMode mode = kHTMLStandardMode);
+StyleRuleBase* ParseRule(Document& document, String text);
 
 }  // namespace css_test_helpers
 }  // namespace blink

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

@@ -380,38 +380,25 @@ void StyleRuleFontFace::TraceAfterDispatch(blink::Visitor* visitor) const {
 StyleRuleScrollTimeline::StyleRuleScrollTimeline(
     const String& name,
     const CSSPropertyValueSet* properties)
-    : StyleRuleBase(kScrollTimeline), name_(name), properties_(properties) {}
-
-StyleRuleScrollTimeline::StyleRuleScrollTimeline(
-    const StyleRuleScrollTimeline& scroll_timeline_rule)
-    : StyleRuleBase(scroll_timeline_rule),
-      properties_(scroll_timeline_rule.properties_->MutableCopy()) {}
+    : StyleRuleBase(kScrollTimeline),
+      name_(name),
+      source_(properties->GetPropertyCSSValue(CSSPropertyID::kSource)),
+      orientation_(
+          properties->GetPropertyCSSValue(CSSPropertyID::kOrientation)),
+      start_(properties->GetPropertyCSSValue(CSSPropertyID::kStart)),
+      end_(properties->GetPropertyCSSValue(CSSPropertyID::kEnd)),
+      time_range_(properties->GetPropertyCSSValue(CSSPropertyID::kTimeRange)) {}
 
 StyleRuleScrollTimeline::~StyleRuleScrollTimeline() = default;
 
-const CSSValue* StyleRuleScrollTimeline::GetSource() const {
-  return properties_->GetPropertyCSSValue(CSSPropertyID::kSource);
-}
-
-const CSSValue* StyleRuleScrollTimeline::GetOrientation() const {
-  return properties_->GetPropertyCSSValue(CSSPropertyID::kOrientation);
-}
-
-const CSSValue* StyleRuleScrollTimeline::GetStart() const {
-  return properties_->GetPropertyCSSValue(CSSPropertyID::kStart);
-}
-
-const CSSValue* StyleRuleScrollTimeline::GetEnd() const {
-  return properties_->GetPropertyCSSValue(CSSPropertyID::kEnd);
-}
-
-const CSSValue* StyleRuleScrollTimeline::GetTimeRange() const {
-  return properties_->GetPropertyCSSValue(CSSPropertyID::kTimeRange);
-}
-
 void StyleRuleScrollTimeline::TraceAfterDispatch(
     blink::Visitor* visitor) const {
-  visitor->Trace(properties_);
+  visitor->Trace(source_);
+  visitor->Trace(orientation_);
+  visitor->Trace(start_);
+  visitor->Trace(end_);
+  visitor->Trace(time_range_);
+
   StyleRuleBase::TraceAfterDispatch(visitor);
 }
 

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

@@ -212,10 +212,10 @@ class StyleRuleProperty : public StyleRuleBase {
   Member<CSSPropertyValueSet> properties_;
 };
 
-class StyleRuleScrollTimeline : public StyleRuleBase {
+class CORE_EXPORT StyleRuleScrollTimeline : public StyleRuleBase {
  public:
   StyleRuleScrollTimeline(const String& name, const CSSPropertyValueSet*);
-  StyleRuleScrollTimeline(const StyleRuleScrollTimeline&);
+  StyleRuleScrollTimeline(const StyleRuleScrollTimeline&) = default;
   ~StyleRuleScrollTimeline();
 
   StyleRuleScrollTimeline* Copy() const {
@@ -225,15 +225,19 @@ class StyleRuleScrollTimeline : public StyleRuleBase {
   void TraceAfterDispatch(blink::Visitor*) const;
 
   const String& GetName() const { return name_; }
-  const CSSValue* GetSource() const;
-  const CSSValue* GetOrientation() const;
-  const CSSValue* GetStart() const;
-  const CSSValue* GetEnd() const;
-  const CSSValue* GetTimeRange() const;
+  const CSSValue* GetSource() const { return source_; }
+  const CSSValue* GetOrientation() const { return orientation_; }
+  const CSSValue* GetStart() const { return start_; }
+  const CSSValue* GetEnd() const { return end_; }
+  const CSSValue* GetTimeRange() const { return time_range_; }
 
  private:
   String name_;
-  Member<const CSSPropertyValueSet> properties_;
+  Member<const CSSValue> source_;
+  Member<const CSSValue> orientation_;
+  Member<const CSSValue> start_;
+  Member<const CSSValue> end_;
+  Member<const CSSValue> time_range_;
 };
 
 class CORE_EXPORT StyleRuleGroup : public StyleRuleBase {

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

+// Copyright 2020 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/style_rule.h"
+
+#include "third_party/blink/renderer/core/css/css_test_helpers.h"
+#include "third_party/blink/renderer/core/testing/page_test_base.h"
+#include "third_party/blink/renderer/platform/testing/runtime_enabled_features_test_helpers.h"
+
+namespace blink {
+
+class StyleRuleTest : public PageTestBase {};
+
+// Verifies that a StyleRuleScrollTimeline can be accessed even if
+// the runtime flag CSSScrollTimeline is disabled.
+//
+// Note that this test can be removed when the CSSScrollTimeline flag is
+// removed.
+TEST_F(StyleRuleTest, StyleRuleScrollTimelineGettersWithoutFeature) {
+  ScopedCSSScrollTimelineForTest scoped_feature(false);
+
+  StyleRuleBase* base_rule = nullptr;
+
+  {
+    ScopedCSSScrollTimelineForTest scoped_feature(true);
+    base_rule = css_test_helpers::ParseRule(GetDocument(), R"CSS(
+        @scroll-timeline timeline {
+          source: selector(#foo);
+          start: 1px;
+          end: 2px;
+          time-range: 10s;
+        }
+      )CSS");
+  }
+
+  ASSERT_TRUE(base_rule);
+  const auto* rule = To<StyleRuleScrollTimeline>(base_rule);
+
+  // Don't crash:
+  EXPECT_FALSE(rule->GetName().IsEmpty());
+  EXPECT_TRUE(rule->GetSource());
+  EXPECT_TRUE(rule->GetStart());
+  EXPECT_TRUE(rule->GetEnd());
+  EXPECT_TRUE(rule->GetTimeRange());
+}
+
+TEST_F(StyleRuleTest, StyleRuleScrollTimelineCopy) {
+  ScopedCSSScrollTimelineForTest scoped_feature(true);
+
+  auto* base_rule = css_test_helpers::ParseRule(GetDocument(), R"CSS(
+      @scroll-timeline timeline {
+        source: selector(#foo);
+        start: 1px;
+        end: 2px;
+        time-range: 10s;
+      }
+    )CSS");
+
+  ASSERT_TRUE(base_rule);
+  auto* base_copy = base_rule->Copy();
+
+  EXPECT_NE(base_rule, base_copy);
+  EXPECT_EQ(base_rule->GetType(), base_copy->GetType());
+
+  auto* rule = DynamicTo<StyleRuleScrollTimeline>(base_rule);
+  auto* copy = DynamicTo<StyleRuleScrollTimeline>(base_copy);
+
+  ASSERT_TRUE(rule);
+  ASSERT_TRUE(copy);
+
+  EXPECT_EQ(rule->GetName(), copy->GetName());
+  EXPECT_EQ(rule->GetSource(), copy->GetSource());
+  EXPECT_EQ(rule->GetOrientation(), copy->GetOrientation());
+  EXPECT_EQ(rule->GetStart(), copy->GetStart());
+  EXPECT_EQ(rule->GetEnd(), copy->GetEnd());
+  EXPECT_EQ(rule->GetTimeRange(), copy->GetTimeRange());
+}
+
+}  // namespace blink