Use streaming tokenizer for CSS declarations

I think this is the lowest level where we can use the streaming
tokenizer directly without severe complications. A CSS value can contain
a custom property reference at any point, which necessitates switching
from parsing using the property's grammar, to parsing as an unresolved
value. That means either we have to buffer tokens at some point, or we
have to rewind the stream back to the start of the value and repeat
tokenization. I'm aiming towards the former because it's more compatible
with existing code and less likely to cause perf regression.

With that in mind, this CL also introduces CSSTokenizedValue to hold the
token range plus a view on the original string, for consumption by the
custom property handler in an upcoming CL. I've added a test to ensure
the string is captured correctly in the presence of !important.

Bug: 661854
Change-Id: Ic3bb882e0b766fc09d0c64e0dbee10690c475c75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2332374
Commit-Queue: Kevin Babbitt <kbabbitt@microsoft.com>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798337}

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

@@ -421,6 +421,7 @@ blink_core_sources("css") {
     "parser/css_selector_parser.h",
     "parser/css_supports_parser.cc",
     "parser/css_supports_parser.h",
+    "parser/css_tokenized_value.h",
     "parser/css_tokenizer.cc",
     "parser/css_tokenizer.h",
     "parser/css_tokenizer_input_stream.cc",

third_party/blink/renderer/core/css/parser/css_parser_impl.cc

@@ -343,14 +343,15 @@ std::unique_ptr<Vector<double>> CSSParserImpl::ParseKeyframeKeyList(
   return ConsumeKeyframeKeyList(CSSParserTokenRange(tokenizer.TokenizeToEOF()));
 }
 
-bool CSSParserImpl::SupportsDeclaration(CSSParserTokenRange& range) {
+bool CSSParserImpl::ConsumeSupportsDeclaration(CSSParserTokenStream& stream) {
   DCHECK(parsed_properties_.IsEmpty());
   // Even though we might use an observer here, this is just to test if we
-  // successfully parse the range, so we can pass RangeOffset::Ignore() here
-  // and temporarily remove the observer.
+  // successfully parse the range, so we can temporarily remove the observer.
   CSSParserObserver* observer_copy = observer_;
   observer_ = nullptr;
-  ConsumeDeclaration(range, RangeOffset::Ignore(), StyleRule::kStyle);
+  CSSParserTokenStream::RangeBoundary range_boundary(
+      stream, CSSParserTokenType::kRightParenthesisToken);
+  ConsumeDeclaration(stream, StyleRule::kStyle);
   observer_ = observer_copy;
 
   bool result = !parsed_properties_.IsEmpty();
@@ -1054,16 +1055,9 @@ void CSSParserImpl::ConsumeDeclarationList(CSSParserTokenStream& stream,
         stream.UncheckedConsume();
         break;
       case kIdentToken: {
-        // TODO(crbug.com/661854): Use streams instead of ranges
-        const wtf_size_t decl_offset_start = stream.Offset();
-        const CSSParserTokenRange decl =
-            stream.ConsumeUntilPeekedTypeIs<kSemicolonToken>();
-        // We want the offset of the kSemicolonToken, which is peeked but not
-        // consumed.
-        const RangeOffset decl_offset(decl_offset_start,
-                                      stream.LookAheadOffset());
-
-        ConsumeDeclaration(decl, decl_offset, rule_type);
+        CSSParserTokenStream::RangeBoundary range_boundary(stream,
+                                                           kSemicolonToken);
+        ConsumeDeclaration(stream, rule_type);
 
         if (!stream.AtEnd())
           stream.UncheckedConsume();  // kSemicolonToken
@@ -1087,29 +1081,23 @@ void CSSParserImpl::ConsumeDeclarationList(CSSParserTokenStream& stream,
     observer_->EndRuleBody(stream.LookAheadOffset());
 }
 
-void CSSParserImpl::ConsumeDeclaration(CSSParserTokenRange range,
-                                       const RangeOffset& decl_offset,
+void CSSParserImpl::ConsumeDeclaration(CSSParserTokenStream& stream,
                                        StyleRule::RuleType rule_type) {
-  DCHECK_EQ(range.Peek().GetType(), kIdentToken);
-  const CSSParserToken& lhs = range.ConsumeIncludingWhitespace();
-  if (range.Consume().GetType() != kColonToken)
-    return;  // Parse error
-
-  bool important = false;
-  const CSSParserToken* declaration_value_end = range.end();
-  const CSSParserToken* last = range.end() - 1;
-  while (last->GetType() == kWhitespaceToken)
-    --last;
-  if (last->GetType() == kIdentToken &&
-      EqualIgnoringASCIICase(last->Value(), "important")) {
-    --last;
-    while (last->GetType() == kWhitespaceToken)
-      --last;
-    if (last->GetType() == kDelimiterToken && last->Delimiter() == '!') {
-      important = true;
-      declaration_value_end = last;
-    }
+  const wtf_size_t decl_offset_start = stream.Offset();
+
+  DCHECK_EQ(stream.Peek().GetType(), kIdentToken);
+  const CSSParserToken& lhs = stream.ConsumeIncludingWhitespace();
+  if (stream.Peek().GetType() != kColonToken) {
+    // Parse error.
+    // Consume the remainder of the declaration for recovery before returning.
+    stream.ConsumeUntilPeekedBoundary();
+    return;
   }
+  stream.UncheckedConsume();  // kColonToken
+
+  CSSTokenizedValue tokenized_value = ConsumeValue(stream);
+
+  bool important = RemoveImportantAnnotationIfPresent(tokenized_value);
 
   size_t properties_count = parsed_properties_.size();
 
@@ -1120,7 +1108,8 @@ void CSSParserImpl::ConsumeDeclaration(CSSParserTokenRange range,
     if (important)  // Invalid
       return;
     atrule_id = lhs.ParseAsAtRuleDescriptorID();
-    AtRuleDescriptorParser::ParseAtRule(rule_type, atrule_id, range, *context_,
+    AtRuleDescriptorParser::ParseAtRule(rule_type, atrule_id,
+                                        tokenized_value.range, *context_,
                                         parsed_properties_);
   } else {
     unresolved_property = lhs.ParseAsUnresolvedCSSPropertyID(
@@ -1136,21 +1125,22 @@ void CSSParserImpl::ConsumeDeclaration(CSSParserTokenRange range,
       return;
     AtomicString variable_name = lhs.Value().ToAtomicString();
     bool is_animation_tainted = rule_type == StyleRule::kKeyframe;
-    ConsumeVariableValue(
-        range.MakeSubRange(&range.Peek(), declaration_value_end), variable_name,
-        important, is_animation_tainted);
+    ConsumeVariableValue(tokenized_value.range, variable_name, important,
+                         is_animation_tainted);
   } else if (unresolved_property != CSSPropertyID::kInvalid) {
     if (style_sheet_ && style_sheet_->SingleOwnerDocument())
       Deprecation::WarnOnDeprecatedProperties(
           style_sheet_->SingleOwnerDocument()->GetFrame(), unresolved_property);
-    ConsumeDeclarationValue(
-        range.MakeSubRange(&range.Peek(), declaration_value_end),
-        unresolved_property, important, rule_type);
+    ConsumeDeclarationValue(tokenized_value.range, unresolved_property,
+                            important, rule_type);
   }
 
   if (observer_ &&
       (rule_type == StyleRule::kStyle || rule_type == StyleRule::kKeyframe)) {
-    observer_->ObserveProperty(decl_offset.start, decl_offset.end, important,
+    // The end offset is the offset of the terminating token, which is peeked
+    // but not yet consumed.
+    observer_->ObserveProperty(decl_offset_start, stream.LookAheadOffset(),
+                               important,
                                parsed_properties_.size() != properties_count);
   }
 }
@@ -1176,6 +1166,46 @@ void CSSParserImpl::ConsumeDeclarationValue(CSSParserTokenRange range,
                                 parsed_properties_, rule_type);
 }
 
+CSSTokenizedValue CSSParserImpl::ConsumeValue(CSSParserTokenStream& stream) {
+  stream.EnsureLookAhead();
+  wtf_size_t value_start_offset = stream.LookAheadOffset();
+  CSSParserTokenRange range = stream.ConsumeUntilPeekedBoundary();
+  wtf_size_t value_end_offset = stream.LookAheadOffset();
+
+  return {range, stream.StringRangeAt(value_start_offset,
+                                      value_end_offset - value_start_offset)};
+}
+
+bool CSSParserImpl::RemoveImportantAnnotationIfPresent(
+    CSSTokenizedValue& tokenized_value) {
+  const CSSParserToken* first = tokenized_value.range.begin();
+  const CSSParserToken* last = tokenized_value.range.end() - 1;
+  while (last >= first && last->GetType() == kWhitespaceToken)
+    --last;
+  if (last >= first && last->GetType() == kIdentToken &&
+      EqualIgnoringASCIICase(last->Value(), "important")) {
+    --last;
+    while (last >= first && last->GetType() == kWhitespaceToken)
+      --last;
+    if (last >= first && last->GetType() == kDelimiterToken &&
+        last->Delimiter() == '!') {
+      tokenized_value.range = tokenized_value.range.MakeSubRange(first, last);
+
+      // Truncate the text to remove the delimiter and everything after it.
+      DCHECK_NE(tokenized_value.text.ToString().find('!'), kNotFound);
+      unsigned truncated_length = tokenized_value.text.length() - 1;
+      while (tokenized_value.text[truncated_length] != '!')
+        --truncated_length;
+      tokenized_value.text =
+          StringView(tokenized_value.text, 0, truncated_length);
+
+      return true;
+    }
+  }
+
+  return false;
+}
+
 std::unique_ptr<Vector<double>> CSSParserImpl::ConsumeKeyframeKeyList(
     CSSParserTokenRange range) {
   std::unique_ptr<Vector<double>> result = std::make_unique<Vector<double>>();

third_party/blink/renderer/core/css/parser/css_parser_impl.h

@@ -14,6 +14,7 @@
 #include "third_party/blink/renderer/core/css/css_property_value.h"
 #include "third_party/blink/renderer/core/css/css_property_value_set.h"
 #include "third_party/blink/renderer/core/css/parser/css_parser_token_range.h"
+#include "third_party/blink/renderer/core/css/parser/css_tokenized_value.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
 #include "third_party/blink/renderer/platform/wtf/vector.h"
@@ -115,7 +116,7 @@ class CORE_EXPORT CSSParserImpl {
 
   static std::unique_ptr<Vector<double>> ParseKeyframeKeyList(const String&);
 
-  bool SupportsDeclaration(CSSParserTokenRange&);
+  bool ConsumeSupportsDeclaration(CSSParserTokenStream&);
   const CSSParserContext* GetContext() const { return context_; }
 
   static void ParseDeclarationListForInspector(const String&,
@@ -131,6 +132,10 @@ class CORE_EXPORT CSSParserImpl {
       wtf_size_t offset,
       const CSSParserContext*);
 
+  static CSSTokenizedValue ConsumeValue(CSSParserTokenStream&);
+
+  static bool RemoveImportantAnnotationIfPresent(CSSTokenizedValue&);
+
  private:
   enum RuleListType {
     kTopLevelRuleList,
@@ -167,9 +172,7 @@ class CORE_EXPORT CSSParserImpl {
   StyleRule* ConsumeStyleRule(CSSParserTokenStream&);
 
   void ConsumeDeclarationList(CSSParserTokenStream&, StyleRule::RuleType);
-  void ConsumeDeclaration(CSSParserTokenRange,
-                          const RangeOffset& decl_offset,
-                          StyleRule::RuleType);
+  void ConsumeDeclaration(CSSParserTokenStream&, StyleRule::RuleType);
   void ConsumeDeclarationValue(CSSParserTokenRange,
                                CSSPropertyID,
                                bool important,

third_party/blink/renderer/core/css/parser/css_parser_impl_test.cc

@@ -7,6 +7,8 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 #include "third_party/blink/renderer/core/css/parser/css_parser_observer.h"
+#include "third_party/blink/renderer/core/css/parser/css_parser_token_stream.h"
+#include "third_party/blink/renderer/core/css/parser/css_tokenizer.h"
 #include "third_party/blink/renderer/core/css/style_sheet_contents.h"
 #include "third_party/blink/renderer/platform/testing/runtime_enabled_features_test_helpers.h"
 
@@ -191,4 +193,36 @@ TEST(CSSParserImplTest, AtScrollTimelineOffsets) {
   EXPECT_EQ(test_css_parser_observer.rule_body_end_, 24u);
 }
 
+TEST(CSSParserImplTest, RemoveImportantAnnotationIfPresent) {
+  struct TestCase {
+    String input;
+    String expected_text;
+    bool expected_is_important;
+  };
+  static const TestCase test_cases[] = {
+      {"", "", false},
+      {"!important", "", true},
+      {" !important", " ", true},
+      {"!", "!", false},
+      {"1px", "1px", false},
+      {"2px!important", "2px", true},
+      {"3px !important", "3px ", true},
+      {"4px ! important", "4px ", true},
+      {"5px !important ", "5px ", true},
+      {"6px !!important", "6px !", true},
+      {"7px !important !important", "7px !important ", true},
+      {"8px important", "8px important", false},
+  };
+  for (auto current_case : test_cases) {
+    CSSTokenizer tokenizer(current_case.input);
+    CSSParserTokenStream stream(tokenizer);
+    CSSTokenizedValue tokenized_value = CSSParserImpl::ConsumeValue(stream);
+    SCOPED_TRACE(current_case.input);
+    bool is_important =
+        CSSParserImpl::RemoveImportantAnnotationIfPresent(tokenized_value);
+    EXPECT_EQ(is_important, current_case.expected_is_important);
+    EXPECT_EQ(tokenized_value.text.ToString(), current_case.expected_text);
+  }
+}
+
 }  // namespace blink

third_party/blink/renderer/core/css/parser/css_parser_token_stream.h

@@ -5,6 +5,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_PARSER_CSS_PARSER_TOKEN_STREAM_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_PARSER_CSS_PARSER_TOKEN_STREAM_H_
 
+#include "base/auto_reset.h"
 #include "base/macros.h"
 #include "third_party/blink/renderer/core/css/parser/css_parser_token_range.h"
 #include "third_party/blink/renderer/core/css/parser/css_tokenizer.h"
@@ -67,6 +68,18 @@ class CORE_EXPORT CSSParserTokenStream {
     wtf_size_t initial_stack_depth_;
   };
 
+  // Instantiate this to set a short-term boundary for range extraction.
+  class RangeBoundary {
+   public:
+    RangeBoundary(CSSParserTokenStream& stream,
+                  CSSParserTokenType boundary_type)
+        : auto_reset_(&stream.boundary_type_, boundary_type) {}
+    ~RangeBoundary() = default;
+
+   private:
+    base::AutoReset<CSSParserTokenType> auto_reset_;
+  };
+
   // We found that this value works well empirically by printing out the
   // maximum buffer size for a few top alexa websites. It should be slightly
   // above the expected number of tokens in the prelude of an at rule and
@@ -155,7 +168,7 @@ class CORE_EXPORT CSSParserTokenStream {
   // token and return false.
   bool ConsumeCommentOrNothing();
 
-  // Invalidates any ranges created by previous calls to this function
+  // Invalidates any ranges created by previous calls to ConsumeUntil*()
   template <CSSParserTokenType... Types>
   CSSParserTokenRange ConsumeUntilPeekedTypeIs() {
     EnsureLookAhead();
@@ -163,24 +176,39 @@ class CORE_EXPORT CSSParserTokenStream {
     buffer_.Shrink(0);
     while (!UncheckedAtEnd() &&
            !detail::IsTokenTypeOneOf<Types...>(UncheckedPeek().GetType())) {
-      // Have to use internal consume/peek in here because they can read past
-      // start/end of blocks
-      unsigned nesting_level = 0;
-      do {
-        const CSSParserToken& token = UncheckedConsumeInternal();
-        buffer_.push_back(token);
-
-        if (token.GetBlockType() == CSSParserToken::kBlockStart)
-          nesting_level++;
-        else if (token.GetBlockType() == CSSParserToken::kBlockEnd)
-          nesting_level--;
-      } while (!PeekInternal().IsEOF() && nesting_level);
+      ConsumeTokenOrBlockAndAppendToBuffer();
     }
 
     return CSSParserTokenRange(buffer_);
   }
 
+  // Invalidates any ranges created by previous calls to ConsumeUntil*()
+  CSSParserTokenRange ConsumeUntilPeekedBoundary() {
+    EnsureLookAhead();
+
+    buffer_.Shrink(0);
+    while (!UncheckedAtEnd() && UncheckedPeek().GetType() != boundary_type_)
+      ConsumeTokenOrBlockAndAppendToBuffer();
+
+    return CSSParserTokenRange(buffer_);
+  }
+
  private:
+  inline void ConsumeTokenOrBlockAndAppendToBuffer() {
+    // Have to use internal consume/peek in here because they can read past
+    // start/end of blocks
+    unsigned nesting_level = 0;
+    do {
+      const CSSParserToken& token = UncheckedConsumeInternal();
+      buffer_.push_back(token);
+
+      if (token.GetBlockType() == CSSParserToken::kBlockStart)
+        nesting_level++;
+      else if (token.GetBlockType() == CSSParserToken::kBlockEnd)
+        nesting_level--;
+    } while (!PeekInternal().IsEOF() && nesting_level);
+  }
+
   const CSSParserToken& PeekInternal() {
     EnsureLookAhead();
     return UncheckedPeekInternal();
@@ -210,6 +238,7 @@ class CORE_EXPORT CSSParserTokenStream {
   CSSParserToken next_;
   wtf_size_t offset_ = 0;
   bool has_look_ahead_ = false;
+  CSSParserTokenType boundary_type_ = kEOFToken;
   DISALLOW_COPY_AND_ASSIGN(CSSParserTokenStream);
 };
 

third_party/blink/renderer/core/css/parser/css_supports_parser.cc

@@ -209,8 +209,7 @@ CSSSupportsParser::Result CSSSupportsParser::ConsumeSupportsDecl(
     CSSParserTokenStream& stream) {
   if (!IsSupportsDecl(first_token, stream.Peek()))
     return Result::kParseFailure;
-  auto block = stream.ConsumeUntilPeekedTypeIs<kRightParenthesisToken>();
-  if (parser_.SupportsDeclaration(block))
+  if (parser_.ConsumeSupportsDeclaration(stream))
     return Result::kSupported;
   return Result::kUnsupported;
 }

third_party/blink/renderer/core/css/parser/css_tokenized_value.h

+// 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.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_PARSER_CSS_TOKENIZED_VALUE_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_PARSER_CSS_TOKENIZED_VALUE_H_
+
+#include "third_party/blink/renderer/core/css/parser/css_parser_token_range.h"
+#include "third_party/blink/renderer/platform/wtf/text/string_view.h"
+
+namespace blink {
+
+struct CSSTokenizedValue {
+  STACK_ALLOCATED();
+
+ public:
+  CSSParserTokenRange range;
+  StringView text;
+};
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_CSS_PARSER_CSS_TOKENIZED_VALUE_H_