New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 661854 link

Starred by 7 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 754739

Blocking:
issue 512705
issue 827521
issue 661007



Sign in to add a comment

Custom properties must preserve their input exactly for serialization

Project Member Reported by timloh@chromium.org, Nov 3 2016

Issue description

The CSSWG recently resolved that "custom properties must preserve their input exactly for serialization". Our implementation currently serializes the parser tokens, although doesn't yet handle many of the edge cases like comment insertion. Serializing tokens leads to some potentially unexpected behaviour if custom properties are just used to store text, rather than as CSS.

Firefox already preserves the user input (adding closing brackets as required), while Safari currently serializes a parsed representation.

Example test case: https://jsfiddle.net/6ogth0dj/

https://logs.csswg.org/irc.w3.org/css/2016-09-19/#e722375
https://drafts.csswg.org/css-syntax/#serialization

[forked from  bug 617694 ]
 
Labels: Hotlist-Interop
Blocking: 661007
Cc: csharrison@chromium.org
Cc: nyerramilli@chromium.org timloh@chromium.org
 Issue 661007  has been merged into this issue.

Comment 6 by timloh@chromium.org, Jan 24 2017

Owner: meade@chromium.org
I'm not working on this any more, assigning to the style team TL, meade@, to triage. I also updated the doc since I posted the link above with some more details a couple weeks ago.

Comment 7 by timloh@chromium.org, Jan 25 2017

So it doesn't get lost, I was previously working on the first part of this (streaming parser): https://codereview.chromium.org/2503683003/
Labels: Update-Quarterly

Comment 9 by meade@chromium.org, Mar 6 2017

Cc: sashab@chromium.org meade@chromium.org
 Issue 697663  has been merged into this issue.
Owner: ----
Status: Available (was: Assigned)
Cc: -sashab@chromium.org
Cc: -timloh@chromium.org nainar@chromium.org
Owner: shend@chromium.org
Status: Assigned (was: Available)
Taking this over from timloh@.
The plan going forward is that timloh's WIP patch will be split into smaller pieces that are easier to test and review. Ideally we won't hit any perf regression along the way.
Project Member

Comment 14 by bugdroid1@chromium.org, Aug 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7aaef89230f0d9bce34530df37b11ad9b34227b7

commit 7aaef89230f0d9bce34530df37b11ad9b34227b7
Author: Darren Shen <shend@chromium.org>
Date: Fri Aug 04 06:12:58 2017

[CSSParser] Do not tokenize in CSSTokenizer constructor.

Currently, when a CSSTokenizer is created from a String, the string is
completely tokenized in the constructor. In order to make the css parser
tokenize on demand, we do not tokenize everything in the constructor.
Instead, we introduce a new class called CSSParserTokenStream that acts
a streaming interface to the tokenizer. Eventually, this will have
methods such as "Next" and "Peek" that tokenize on demand. For now,
it just has a method to fully tokenize the input and returns the range.
Hence, this patch simply moves full string tokenization from
CSSTokenizer constructor to CSSParserTokenStream::MakeRangeToEOF().

The plan is to eventually replace (most) CSSParserTokenRanges with
CSSParserTokenStreams. This patch replaces some of the top level parsing
functions to use a CSSParserTokenStream. To call lower level parsing
functions that use ranges, we convert these streams to ranges, which
triggers a full tokenization. This is a temporary fallback to the
current range system. Future patches will change those level parsing
functions to use streams as well, layer by layer, so we won't need to
trigger full tokenizations.

Note that only one of the two constructors are affected by this patch.
The other constructor uses observers and is more difficult to change.

This patch does not change observable behaviour, and should not have
any performance impacts (since we're just delaying when the string
gets fully tokenized).

Bug: 661854
Change-Id: I6f20269835d806106e95740f8dcee6744d0de944
Reviewed-on: https://chromium-review.googlesource.com/595025
Reviewed-by: nainar <nainar@chromium.org>
Reviewed-by: meade_UTC10 <meade@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491950}
[modify] https://crrev.com/7aaef89230f0d9bce34530df37b11ad9b34227b7/third_party/WebKit/Source/core/css/BUILD.gn
[modify] https://crrev.com/7aaef89230f0d9bce34530df37b11ad9b34227b7/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/7aaef89230f0d9bce34530df37b11ad9b34227b7/third_party/WebKit/Source/core/css/parser/CSSParserToken.h
[add] https://crrev.com/7aaef89230f0d9bce34530df37b11ad9b34227b7/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[modify] https://crrev.com/7aaef89230f0d9bce34530df37b11ad9b34227b7/third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp
[modify] https://crrev.com/7aaef89230f0d9bce34530df37b11ad9b34227b7/third_party/WebKit/Source/core/css/parser/CSSTokenizer.h

Project Member

Comment 15 by bugdroid1@chromium.org, Aug 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/726787e05f41c9103250fad8b85881544365f77f

commit 726787e05f41c9103250fad8b85881544365f77f
Author: Darren Shen <shend@chromium.org>
Date: Fri Aug 11 01:47:27 2017

[CSSParser] Add Next/Peek for stream parsing of ConsumeDeclarationList.

This patch adds Next/Peek methods on CSSParserTokenStream, which
tokenizes on demand. It also offers fast path UncheckedNext/Peek methods
that can be used when we've already called Peek().

Using these new features, we can begin to convert CSSParserImpl::
ConsumeDeclarationList to use streams (we add an overload because there
are still other callers that use ranges). Unfortunately, using
CSSParserObservers with streaming requires some refactoring, so for now,
we only call ConsumeDeclarationList when we don't have an observer.

Since ConsumeDeclarationList calls lower level parsing functions that
require ranges, we convert streams to ranges using MakeRangeToEOF().
When those lower level parsing functions modify the ranges that they
take, we need to sync those changes with the stream using
UpdatePositionFromRange.

Future patches will continue to streamify those lower level parsing
functions, layer by layer.

This patch does not change observable parsing behaviour, and should
have minimal performance impact: ranges are inherently faster than
streams, but the unchecked consume/peeks should close the gap.

For reviewers:
- CSSParserTokenStream::ConsumeComponentValue is copied from
  CSSParserTokenRange::ConsumeComponentValue.
- The streaming CSSParserTokenStream::ConsumeDeclarationList is adapted
  from the ranges version.

Bug: 661854
Change-Id: Id2732f7030d012daf213f963ed0e0506d6eed145
Reviewed-on: https://chromium-review.googlesource.com/597607
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: meade_UTC10 <meade@chromium.org>
Reviewed-by: Bugs Nash <bugsnash@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493640}
[modify] https://crrev.com/726787e05f41c9103250fad8b85881544365f77f/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/726787e05f41c9103250fad8b85881544365f77f/third_party/WebKit/Source/core/css/BUILD.gn
[modify] https://crrev.com/726787e05f41c9103250fad8b85881544365f77f/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/726787e05f41c9103250fad8b85881544365f77f/third_party/WebKit/Source/core/css/parser/CSSParserImpl.h
[add] https://crrev.com/726787e05f41c9103250fad8b85881544365f77f/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.cpp
[modify] https://crrev.com/726787e05f41c9103250fad8b85881544365f77f/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[add] https://crrev.com/726787e05f41c9103250fad8b85881544365f77f/third_party/WebKit/Source/core/css/parser/CSSParserTokenStreamTest.cpp

Comment 16 by kbr@chromium.org, Aug 11 2017

Blockedon: 754739
Project Member

Comment 17 by bugdroid1@chromium.org, Aug 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d39de8fbe45814b9b4c75b41f85e472c103f68e8

commit d39de8fbe45814b9b4c75b41f85e472c103f68e8
Author: Kenneth Russell <kbr@chromium.org>
Date: Fri Aug 11 18:22:58 2017

Revert "[CSSParser] Add Next/Peek for stream parsing of ConsumeDeclarationList."

This reverts commit 726787e05f41c9103250fad8b85881544365f77f.

Reason for revert: caused assertion failures in CSSParserImpl.cpp. See  Issue 754739 .

Original change's description:
> [CSSParser] Add Next/Peek for stream parsing of ConsumeDeclarationList.
> 
> This patch adds Next/Peek methods on CSSParserTokenStream, which
> tokenizes on demand. It also offers fast path UncheckedNext/Peek methods
> that can be used when we've already called Peek().
> 
> Using these new features, we can begin to convert CSSParserImpl::
> ConsumeDeclarationList to use streams (we add an overload because there
> are still other callers that use ranges). Unfortunately, using
> CSSParserObservers with streaming requires some refactoring, so for now,
> we only call ConsumeDeclarationList when we don't have an observer.
> 
> Since ConsumeDeclarationList calls lower level parsing functions that
> require ranges, we convert streams to ranges using MakeRangeToEOF().
> When those lower level parsing functions modify the ranges that they
> take, we need to sync those changes with the stream using
> UpdatePositionFromRange.
> 
> Future patches will continue to streamify those lower level parsing
> functions, layer by layer.
> 
> This patch does not change observable parsing behaviour, and should
> have minimal performance impact: ranges are inherently faster than
> streams, but the unchecked consume/peeks should close the gap.
> 
> For reviewers:
> - CSSParserTokenStream::ConsumeComponentValue is copied from
>   CSSParserTokenRange::ConsumeComponentValue.
> - The streaming CSSParserTokenStream::ConsumeDeclarationList is adapted
>   from the ranges version.
> 
> Bug: 661854
> Change-Id: Id2732f7030d012daf213f963ed0e0506d6eed145
> Reviewed-on: https://chromium-review.googlesource.com/597607
> Commit-Queue: Darren Shen <shend@chromium.org>
> Reviewed-by: meade_UTC10 <meade@chromium.org>
> Reviewed-by: Bugs Nash <bugsnash@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#493640}

TBR=meade@chromium.org,shend@chromium.org,bugsnash@chromium.org

Change-Id: If0244556e6746f0cba43148575f0512a190998f5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 661854
Reviewed-on: https://chromium-review.googlesource.com/612371
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Kenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493819}
[modify] https://crrev.com/d39de8fbe45814b9b4c75b41f85e472c103f68e8/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/d39de8fbe45814b9b4c75b41f85e472c103f68e8/third_party/WebKit/Source/core/css/BUILD.gn
[modify] https://crrev.com/d39de8fbe45814b9b4c75b41f85e472c103f68e8/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/d39de8fbe45814b9b4c75b41f85e472c103f68e8/third_party/WebKit/Source/core/css/parser/CSSParserImpl.h
[delete] https://crrev.com/0eb2231add3b155a475d9dcd77f939e89484c226/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.cpp
[modify] https://crrev.com/d39de8fbe45814b9b4c75b41f85e472c103f68e8/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[delete] https://crrev.com/0eb2231add3b155a475d9dcd77f939e89484c226/third_party/WebKit/Source/core/css/parser/CSSParserTokenStreamTest.cpp

Comment 18 by kbr@chromium.org, Aug 11 2017

Cc: thakis@chromium.org
Sorry, but 726787e05f41c9103250fad8b85881544365f77f caused assertion failures inside the CSS parser per  Issue 754739  and I've reverted it in https://chromium-review.googlesource.com/612371 .

The chromium.gpu.fyi bots are currently compiling with MSVC rather than Clang, and that may be the reason for the difference compared to the normal CQ bots. You can prevent this regression from happening again by adding the following line to your CL description when relanding:

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel

+thakis because this is another MSVC/Clang difference in the CSS code.

Project Member

Comment 19 by bugdroid1@chromium.org, Aug 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/655f24fcfa2938dcbf8f774be7c8db88f031e935

commit 655f24fcfa2938dcbf8f774be7c8db88f031e935
Author: Darren Shen <shend@chromium.org>
Date: Wed Aug 16 01:00:23 2017

Revert "Revert "[CSSParser] Add Next/Peek for stream parsing of ConsumeDeclarationList.""

This reverts commit d39de8fbe45814b9b4c75b41f85e472c103f68e8.

Reason for revert: Relanding

Change: Fixed undefined behaviour due to unspecified order of operations with
side effects (compare Patch 5 with Patch 1).

Original change's description:
> Revert "[CSSParser] Add Next/Peek for stream parsing of ConsumeDeclarationList."
> 
> This reverts commit 726787e05f41c9103250fad8b85881544365f77f.
> 
> Reason for revert: caused assertion failures in CSSParserImpl.cpp. See  Issue 754739 .
> 
> Original change's description:
> > [CSSParser] Add Next/Peek for stream parsing of ConsumeDeclarationList.
> > 
> > This patch adds Next/Peek methods on CSSParserTokenStream, which
> > tokenizes on demand. It also offers fast path UncheckedNext/Peek methods
> > that can be used when we've already called Peek().
> > 
> > Using these new features, we can begin to convert CSSParserImpl::
> > ConsumeDeclarationList to use streams (we add an overload because there
> > are still other callers that use ranges). Unfortunately, using
> > CSSParserObservers with streaming requires some refactoring, so for now,
> > we only call ConsumeDeclarationList when we don't have an observer.
> > 
> > Since ConsumeDeclarationList calls lower level parsing functions that
> > require ranges, we convert streams to ranges using MakeRangeToEOF().
> > When those lower level parsing functions modify the ranges that they
> > take, we need to sync those changes with the stream using
> > UpdatePositionFromRange.
> > 
> > Future patches will continue to streamify those lower level parsing
> > functions, layer by layer.
> > 
> > This patch does not change observable parsing behaviour, and should
> > have minimal performance impact: ranges are inherently faster than
> > streams, but the unchecked consume/peeks should close the gap.
> > 
> > For reviewers:
> > - CSSParserTokenStream::ConsumeComponentValue is copied from
> >   CSSParserTokenRange::ConsumeComponentValue.
> > - The streaming CSSParserTokenStream::ConsumeDeclarationList is adapted
> >   from the ranges version.
> > 
> > Bug: 661854
> > Change-Id: Id2732f7030d012daf213f963ed0e0506d6eed145
> > Reviewed-on: https://chromium-review.googlesource.com/597607
> > Commit-Queue: Darren Shen <shend@chromium.org>
> > Reviewed-by: meade_UTC10 <meade@chromium.org>
> > Reviewed-by: Bugs Nash <bugsnash@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#493640}
> 
> TBR=meade@chromium.org,shend@chromium.org,bugsnash@chromium.org
> 
> Change-Id: If0244556e6746f0cba43148575f0512a190998f5
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 661854
> Reviewed-on: https://chromium-review.googlesource.com/612371
> Reviewed-by: Kenneth Russell <kbr@chromium.org>
> Commit-Queue: Kenneth Russell <kbr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#493819}

TBR=meade@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel

Change-Id: I497adee4374828b1e8040a2e0c11745b64a5c4d0
Bug: 661854
Reviewed-on: https://chromium-review.googlesource.com/611884
Reviewed-by: Darren Shen <shend@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494646}
[modify] https://crrev.com/655f24fcfa2938dcbf8f774be7c8db88f031e935/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/655f24fcfa2938dcbf8f774be7c8db88f031e935/third_party/WebKit/Source/core/css/BUILD.gn
[modify] https://crrev.com/655f24fcfa2938dcbf8f774be7c8db88f031e935/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/655f24fcfa2938dcbf8f774be7c8db88f031e935/third_party/WebKit/Source/core/css/parser/CSSParserImpl.h
[add] https://crrev.com/655f24fcfa2938dcbf8f774be7c8db88f031e935/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.cpp
[modify] https://crrev.com/655f24fcfa2938dcbf8f774be7c8db88f031e935/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[add] https://crrev.com/655f24fcfa2938dcbf8f774be7c8db88f031e935/third_party/WebKit/Source/core/css/parser/CSSParserTokenStreamTest.cpp

Project Member

Comment 21 by bugdroid1@chromium.org, Aug 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7220fb9e1b829a9275b878b6fa499e55c4999cb9

commit 7220fb9e1b829a9275b878b6fa499e55c4999cb9
Author: Darren Shen <shend@chromium.org>
Date: Wed Aug 16 23:07:59 2017

[CSSParser] Allow parsers with observers to use streams.

Previously, we have CSSParserObservers which receive information about
the parsing process (e.g. where a particular rule starts or ends). The
way it worked for stylesheet parsing was that we first tokenized the
entirety of the input, storing token information in a vector. Then,
as we parsed, we queried the tokens for their location in the input
string (their offset) using the vector.

To get the same behaviour with streams, it's more difficult. Before,
offset information were available prior to parsing. Now, offset
information is only available as we parse. This requires some
refactoring to work.

In the meantime, we would like parsers with observers to still use
streams. Hence, this patch implements a workaround: when we have
observers, we tokenize everything before parsing as we did before.
We then create a stream over that tokenizer (technically no longer
a stream since everything's already tokenized) and use the same
parsing code path. Any time we call into the observer with a range,
we just convert the stream back into a range.

This means that until we implement proper streaming with observers,
parsing with observers will tokenize up front, whereas parsing
without observers will tokenize on demand (mostly).

For this to work, we also have to relax the requirement that
streams must use 'fresh' tokenizers with no consumed tokens,
since streams with observers requires using tokenizers that have
already consumed the entirety of the input. Turns out this doesn't
require any changes other than removing a DCHECK.

For reviewers:
- We add back the observer code that was in ConsumeDeclarationList
  but was deleted because observers didn't accept streams before.

Bug: 661854
Change-Id: Ifa53b2e7f2622edb371c74260fd24287d0fd4f89
Reviewed-on: https://chromium-review.googlesource.com/599467
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: meade_UTC10 <meade@chromium.org>
Reviewed-by: Bugs Nash <bugsnash@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494989}
[modify] https://crrev.com/7220fb9e1b829a9275b878b6fa499e55c4999cb9/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/7220fb9e1b829a9275b878b6fa499e55c4999cb9/third_party/WebKit/Source/core/css/parser/CSSParserObserverWrapper.h
[modify] https://crrev.com/7220fb9e1b829a9275b878b6fa499e55c4999cb9/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[modify] https://crrev.com/7220fb9e1b829a9275b878b6fa499e55c4999cb9/third_party/WebKit/Source/core/css/parser/CSSParserTokenStreamTest.cpp
[modify] https://crrev.com/7220fb9e1b829a9275b878b6fa499e55c4999cb9/third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp

Project Member

Comment 22 by bugdroid1@chromium.org, Aug 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/08801673b73f9a7bb098fc0988e821f02b036587

commit 08801673b73f9a7bb098fc0988e821f02b036587
Author: Darren Shen <shend@chromium.org>
Date: Thu Aug 17 08:02:37 2017

[CSSParser] Add iterators to CSSParserTokenStream.

We often want to get a CSSParserTokenRange to a particular subsequence
of tokens. For example, in ConsumeDeclarationList, we want to get
a range to the "declaration start" section and pass that down to
ConsumeDeclaration. The canonical way of doing this is to expose
iterators: we record where the declaration starts in an iterator,
consume some values, record where the declaration ends, and get a range
between those two iterators.

Currently, we use pointers to CSSParserToken as iterators. This was fine
before because we prefilled the tokens vector, so all iterators are
guaranteed to remain valid while parsing. However, when we use streams,
we incrementally push tokens to the tokens vector, which may trigger a
resize, thus invalidating all the iterators. Hence, we create our own
stable iterator in CSSParserTokenStream that does not get invalidated
because it's based on indices and not pointers.

Bug: 661854
Change-Id: I8aaf002de398101f20feda1e5a685707b76e3a35
Reviewed-on: https://chromium-review.googlesource.com/599267
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: meade_UTC10 <meade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495102}
[modify] https://crrev.com/08801673b73f9a7bb098fc0988e821f02b036587/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/08801673b73f9a7bb098fc0988e821f02b036587/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[modify] https://crrev.com/08801673b73f9a7bb098fc0988e821f02b036587/third_party/WebKit/Source/core/css/parser/CSSParserTokenStreamTest.cpp
[modify] https://crrev.com/08801673b73f9a7bb098fc0988e821f02b036587/third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp
[modify] https://crrev.com/08801673b73f9a7bb098fc0988e821f02b036587/third_party/WebKit/Source/core/css/parser/CSSTokenizer.h

Project Member

Comment 23 by bugdroid1@chromium.org, Aug 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/006fe05062cd9696cd11f2cb607c92f577471b7d

commit 006fe05062cd9696cd11f2cb607c92f577471b7d
Author: Darren Shen <shend@chromium.org>
Date: Mon Aug 21 00:04:28 2017

[CSSParser] Allow stream parsing for ConsumeAtRule.

This patch adds an overload to CSSParserImpl::ConsumeAtRule which takes
streams instead of ranges. The reason that we're implementing streams
for ConsumeAtRule first is that it provides a use case for streaming
block parsing. Once we implemented streaming block parsing for
ConsumeAtRule, we will be in a position to convert the remaining
parsing functions to streams in one go.

For reviewers:
- The new ConsumeAtRule is copied from the old one that takes ranges,
  with some changes to make it work with streams.
- ConsumeWhitespace and ConsumeIncludingWhitespace are copied from
  CSSParserTokenRange.

Bug: 661854
Change-Id: I87b3e2aec9854224eb8503a7e4fcebc58cd3feb5
Reviewed-on: https://chromium-review.googlesource.com/601131
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: meade_UTC10 <meade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495856}
[modify] https://crrev.com/006fe05062cd9696cd11f2cb607c92f577471b7d/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/006fe05062cd9696cd11f2cb607c92f577471b7d/third_party/WebKit/Source/core/css/parser/CSSParserImpl.h
[modify] https://crrev.com/006fe05062cd9696cd11f2cb607c92f577471b7d/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.cpp
[modify] https://crrev.com/006fe05062cd9696cd11f2cb607c92f577471b7d/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[modify] https://crrev.com/006fe05062cd9696cd11f2cb607c92f577471b7d/third_party/WebKit/Source/core/css/parser/CSSParserTokenStreamTest.cpp

Project Member

Comment 24 by bugdroid1@chromium.org, Aug 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5819e64ac263e30c62fe1e2b3ebb565665ad3035

commit 5819e64ac263e30c62fe1e2b3ebb565665ad3035
Author: Darren Shen <shend@chromium.org>
Date: Mon Aug 21 22:32:11 2017

[CSSParser] Fix CSSParserObserverWrapper workaround.

In [1], we introduced a temporary workaround that allowed
CSSParserObserverWrapper to take streams as well as ranges. However,
there was a bug in that patch. We used MakeRangeToEOF() to convert
the stream to a range, but that means the converted range would end
at EOF. While this was fine for CSSParserObserverWrapper::StartOffset,
which only used the the starting point of the range, it was broken for
CSSParserObserverWrapper::EndOffset, which used the ending point.

It's quite tricky to get the "end point" of a stream, since it's not
well defined. We looked at usages of CSSParserObserverWrapper::EndOffset
and saw that they were only called when parsing a range is successful
(i.e. the range was fully consumed). In that scenario, the end point of
the range is equal to the start point of the range. Hence, the correct
end point is really just the position of the stream when we call
EndOffset.

Since the observer wrapper only looks at either the starting or ending
point of the range, we can just pass it an empty range with starting
and ending points set to the correct value (the current position of
the stream).

[1] https://chromium-review.googlesource.com/c/599467

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel

Bug: 661854
Change-Id: I191dbf6946dc7b3b1a6e81b11d2cf95ef3b72b33
Reviewed-on: https://chromium-review.googlesource.com/622331
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496089}
[modify] https://crrev.com/5819e64ac263e30c62fe1e2b3ebb565665ad3035/third_party/WebKit/Source/core/css/parser/CSSParserObserverWrapper.h
[modify] https://crrev.com/5819e64ac263e30c62fe1e2b3ebb565665ad3035/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h

Project Member

Comment 25 by bugdroid1@chromium.org, Aug 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26d5c0d9218faabcdf678e29c867ee90a971efe2

commit 26d5c0d9218faabcdf678e29c867ee90a971efe2
Author: Darren Shen <shend@chromium.org>
Date: Thu Aug 24 04:38:42 2017

[CSSParser] Do not invalidate ranges when consuming from streams.

This patch makes ranges stable: they do not get invalidated when the
backing vector resizes. This is done by keeping a reference to the
backing vector.

To make this work, we had to change CSSTokenizer to not use an inline
buffer size. This may have a small performance cost for short strings.

We add a unit test for this (which fails without these changes).

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel

Bug: 661854
Change-Id: Idaa2478abe77f13b1fcd17ad11c8879f84795480
Reviewed-on: https://chromium-review.googlesource.com/625552
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: meade_UTC10 <meade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496951}
[modify] https://crrev.com/26d5c0d9218faabcdf678e29c867ee90a971efe2/third_party/WebKit/Source/core/css/parser/CSSParserTokenRange.cpp
[modify] https://crrev.com/26d5c0d9218faabcdf678e29c867ee90a971efe2/third_party/WebKit/Source/core/css/parser/CSSParserTokenRange.h
[modify] https://crrev.com/26d5c0d9218faabcdf678e29c867ee90a971efe2/third_party/WebKit/Source/core/css/parser/CSSParserTokenStreamTest.cpp
[modify] https://crrev.com/26d5c0d9218faabcdf678e29c867ee90a971efe2/third_party/WebKit/Source/core/css/parser/CSSTokenizer.h

Project Member

Comment 26 by bugdroid1@chromium.org, Sep 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/16c95656c2e3d266d10fd2af768b87c9b6574ce2

commit 16c95656c2e3d266d10fd2af768b87c9b6574ce2
Author: Darren Shen <shend@chromium.org>
Date: Mon Sep 04 00:42:39 2017

[CSSparser] Implement stream block parsing for ConsumeFontFaceRule.

This patch implements the stream parsing of CSS blocks (tokens wrapped
by '(' or '{' or '['). Stream block parsing solves the following issue:

When we parse with ranges, we consume a block in its entirety and then
pass that block to another function which will do a second pass over
that range. This means that even if the block contained invalid syntax,
the whole block will still be consumed. This allows us to continue
parsing the rest of the stylesheet without issues (error recovery).

If we simply substituted ranges with streams in this case, we would
get a different result. When we encounter an error, the rest of the
block would not be consumed yet.

To fix this, we conceptually change these streams to be 'block streams',
meaning that they can only parse the inner contents of a single block.
This means it is a runtime error to consume a token representing
an open block, and the stream ends at EOF or if the next token closes
the block. We also need to add internal peek/consume methods that do
not have these checks (i.e. that have the old behaviour).

To parse nested blocks, you must create a BlockGuard. Upon creation,
the BlockGuard would advance the stream past the open block token.
When the BlockGuard goes out of scope, error recovery kicks in and
we ensure that the rest of the block is consumed using the BlockGuard
destructor.

We test this on ConsumeFontFaceRule (because it is one of the simpler
functions that use blocks). Unfortunately, we need to keep both the
stream and range code paths temporarily to keep the CL small.
We also had to overload ConsumeFontFaceRule for streams, but the range
overload will be removed in the future.

Bug: 661854
Change-Id: I4dcc7e20c8eb62c23b589277556960dcb85eca70
Reviewed-on: https://chromium-review.googlesource.com/601133
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: meade_UTC10 <meade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499422}
[modify] https://crrev.com/16c95656c2e3d266d10fd2af768b87c9b6574ce2/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/16c95656c2e3d266d10fd2af768b87c9b6574ce2/third_party/WebKit/Source/core/css/parser/CSSParserImpl.h
[modify] https://crrev.com/16c95656c2e3d266d10fd2af768b87c9b6574ce2/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.cpp
[modify] https://crrev.com/16c95656c2e3d266d10fd2af768b87c9b6574ce2/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[modify] https://crrev.com/16c95656c2e3d266d10fd2af768b87c9b6574ce2/third_party/WebKit/Source/core/css/parser/CSSParserTokenStreamTest.cpp

Project Member

Comment 27 by bugdroid1@chromium.org, Sep 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1717b6b46acfdddc31580854f3be47171ef004c0

commit 1717b6b46acfdddc31580854f3be47171ef004c0
Author: Darren Shen <shend@chromium.org>
Date: Mon Sep 04 04:17:55 2017

[CSSParser] Add offset information to CSSParserTokenStream.

One of the benefits of streaming parser is that we can get token offset
information (where a token starts in the input string) on the fly.
This patch adds CSSParserTokenStream::Offset() which returns the index
in the input string of the next token to be consumed. Naively, we would
implement this by querying CSSTokenizerParserInputStream::Offset().
However, CSSTokenizerInputStream::Offset() might be ahead of
CSSParserTokenStream::Offset() because peeking a token would advance
CSSTokenizerInputStream but not CSSParserTokenStream, so some extra
logic is required to handle this.

Note that this method is not used (except for tests) until later,
but we put it in this patch to avoid large patches.

Bug: 661854
Change-Id: I67ceaeb4cb16345454b1ccf92c103401996fd298
Reviewed-on: https://chromium-review.googlesource.com/615857
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Bugs Nash <bugsnash@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499434}
[modify] https://crrev.com/1717b6b46acfdddc31580854f3be47171ef004c0/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[modify] https://crrev.com/1717b6b46acfdddc31580854f3be47171ef004c0/third_party/WebKit/Source/core/css/parser/CSSParserTokenStreamTest.cpp

Project Member

Comment 28 by bugdroid1@chromium.org, Sep 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25f72cb089f2be7f89c2dfedf2e8693ab2e2b5be

commit 25f72cb089f2be7f89c2dfedf2e8693ab2e2b5be
Author: Darren Shen <shend@chromium.org>
Date: Tue Sep 05 09:07:09 2017

[CSSParser] Remove tokenization tracing metrics for stylesheet parsing.

When we eventually switch to a streaming parser, tokenization will be
part of parsing, so it is difficult to actually track how much time
tokenization takes. We could wrap CSSTokenizer::TokenizeSingle() with
tracing, but it is unclear if it's bad practice to put tracing calls
into a frequently called function.

For the lack of a better solution, we are just going to remove the
tokenization metrics from stylesheet parsing. We do this in its own
patch for clarity.

Bug: 661854
Change-Id: Iabef67f4a34ac7d8568a0e1b3d827644c0885f28
Reviewed-on: https://chromium-review.googlesource.com/619812
Reviewed-by: dstockwell <dstockwell@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499588}
[modify] https://crrev.com/25f72cb089f2be7f89c2dfedf2e8693ab2e2b5be/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp

Project Member

Comment 29 by bugdroid1@chromium.org, Sep 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ca7c1bf8eb877f562f5a38e745aea68874876d81

commit ca7c1bf8eb877f562f5a38e745aea68874876d81
Author: Darren Shen <shend@chromium.org>
Date: Tue Sep 05 23:47:45 2017

[CSSParser] Switch some parsing functions to use streams.

This patch switches the majority of functions in CSSParserImpl to use
streams instead of ranges. The functions we picked correspond to the
original patch in https://codereview.chromium.org/2503683003.

To note:

- We delete some duplicate parsing functions that use ranges since they
  are no longer used.

- For lazy parsing to work with streams, we store the offset of where
  the block (to be lazily parsed) starts. When we decide to parse the
  block, we create a new stream starting at that offset. Note that we
  are still eagerly tokenizing the block. In the future, we will skip
  the block entirely without tokenizing.

- We add ConsumeDeclarationListForAtApply, which is like a special case of
  ConsumeDeclarationList that works on ranges and only for @apply rules.
  The code is from https://codereview.chromium.org/2503683003

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel

Bug: 661854
Change-Id: I994985459afbde7a992760094d9849f952477e72
Reviewed-on: https://chromium-review.googlesource.com/620290
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: meade_UTC10 <meade@chromium.org>
Reviewed-by: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499803}
[modify] https://crrev.com/ca7c1bf8eb877f562f5a38e745aea68874876d81/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp
[modify] https://crrev.com/ca7c1bf8eb877f562f5a38e745aea68874876d81/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h
[modify] https://crrev.com/ca7c1bf8eb877f562f5a38e745aea68874876d81/third_party/WebKit/Source/core/css/parser/CSSLazyParsingTest.cpp
[modify] https://crrev.com/ca7c1bf8eb877f562f5a38e745aea68874876d81/third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.cpp
[modify] https://crrev.com/ca7c1bf8eb877f562f5a38e745aea68874876d81/third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.h
[modify] https://crrev.com/ca7c1bf8eb877f562f5a38e745aea68874876d81/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/ca7c1bf8eb877f562f5a38e745aea68874876d81/third_party/WebKit/Source/core/css/parser/CSSParserImpl.h
[modify] https://crrev.com/ca7c1bf8eb877f562f5a38e745aea68874876d81/third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp
[modify] https://crrev.com/ca7c1bf8eb877f562f5a38e745aea68874876d81/third_party/WebKit/Source/core/css/parser/CSSTokenizer.h

Project Member

Comment 30 by bugdroid1@chromium.org, Sep 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f181c11ae2db3aac9abadb2fce12682677c23430

commit f181c11ae2db3aac9abadb2fce12682677c23430
Author: Darren Shen <shend@chromium.org>
Date: Thu Sep 07 09:44:10 2017

Revert "[CSSParser] Switch some parsing functions to use streams."

This reverts commit ca7c1bf8eb877f562f5a38e745aea68874876d81.

Reason for revert: Performance regression  crbug.com/762209 

Original change's description:
> [CSSParser] Switch some parsing functions to use streams.
> 
> This patch switches the majority of functions in CSSParserImpl to use
> streams instead of ranges. The functions we picked correspond to the
> original patch in https://codereview.chromium.org/2503683003.
> 
> To note:
> 
> - We delete some duplicate parsing functions that use ranges since they
>   are no longer used.
> 
> - For lazy parsing to work with streams, we store the offset of where
>   the block (to be lazily parsed) starts. When we decide to parse the
>   block, we create a new stream starting at that offset. Note that we
>   are still eagerly tokenizing the block. In the future, we will skip
>   the block entirely without tokenizing.
> 
> - We add ConsumeDeclarationListForAtApply, which is like a special case of
>   ConsumeDeclarationList that works on ranges and only for @apply rules.
>   The code is from https://codereview.chromium.org/2503683003
> 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel
> 
> Bug: 661854
> Change-Id: I994985459afbde7a992760094d9849f952477e72
> Reviewed-on: https://chromium-review.googlesource.com/620290
> Commit-Queue: Darren Shen <shend@chromium.org>
> Reviewed-by: meade_UTC10 <meade@chromium.org>
> Reviewed-by: nainar <nainar@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#499803}

TBR=nainar@chromium.org,meade@chromium.org,shend@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 661854
Change-Id: I254115563a146c8d6e2f48f8e111e85baaa3f8ba
Cq-Include-Trybots: master.tryserver.chromium.win:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/654538
Reviewed-by: Darren Shen <shend@chromium.org>
Reviewed-by: meade_UTC10 <meade@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500259}
[modify] https://crrev.com/f181c11ae2db3aac9abadb2fce12682677c23430/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp
[modify] https://crrev.com/f181c11ae2db3aac9abadb2fce12682677c23430/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h
[modify] https://crrev.com/f181c11ae2db3aac9abadb2fce12682677c23430/third_party/WebKit/Source/core/css/parser/CSSLazyParsingTest.cpp
[modify] https://crrev.com/f181c11ae2db3aac9abadb2fce12682677c23430/third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.cpp
[modify] https://crrev.com/f181c11ae2db3aac9abadb2fce12682677c23430/third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.h
[modify] https://crrev.com/f181c11ae2db3aac9abadb2fce12682677c23430/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/f181c11ae2db3aac9abadb2fce12682677c23430/third_party/WebKit/Source/core/css/parser/CSSParserImpl.h
[modify] https://crrev.com/f181c11ae2db3aac9abadb2fce12682677c23430/third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp
[modify] https://crrev.com/f181c11ae2db3aac9abadb2fce12682677c23430/third_party/WebKit/Source/core/css/parser/CSSTokenizer.h

Project Member

Comment 31 by bugdroid1@chromium.org, Sep 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/34de07a7da1921205306d2f6299187dcddd42d2d

commit 34de07a7da1921205306d2f6299187dcddd42d2d
Author: Darren Shen <shend@chromium.org>
Date: Thu Sep 28 13:43:01 2017

[CSSParser] Use streaming parser for CSS parsing.

This is a squashed commit of the following five CLs:
https://chromium-review.googlesource.com/c/656637/
https://chromium-review.googlesource.com/c/642666/
https://chromium-review.googlesource.com/c/656657/
https://chromium-review.googlesource.com/c/663102/
https://chromium-review.googlesource.com/c/665778/

We did this to make it easier to revert if anything
goes wrong. All five CLs have been LGTM'd. No
significant performance regression is expected from
the patch.

Bug: 661854
Change-Id: I5f08b4bebcb687488fb43183a490b607512f3910
Reviewed-on: https://chromium-review.googlesource.com/676366
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: nainar <nainar@chromium.org>
Reviewed-by: dstockwell <dstockwell@chromium.org>
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505005}
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/BUILD.gn
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/PropertyRegistration.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/cssom/CSSUnparsedValue.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSLazyParsingTest.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.h
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSParser.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSParserImpl.h
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSParserObserverWrapper.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSParserObserverWrapper.h
[add] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSParserScopedTokenBuffer.h
[add] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSParserTokenBuffer.h
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSParserTokenRange.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSParserTokenRange.h
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSParserTokenStreamTest.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSSelectorParserTest.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSTokenizer.h
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/CSSTokenizerTest.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/MediaConditionTest.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/MediaQueryParser.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/SizesAttributeParser.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/css/parser/SizesCalcParserTest.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp
[modify] https://crrev.com/34de07a7da1921205306d2f6299187dcddd42d2d/third_party/WebKit/Source/core/intersection_observer/IntersectionObserver.cpp

Comment 32 by shend@chromium.org, Sep 29 2017

Blocking: 512705
Still waiting for any perf and clusterfuzz reports to come in. Will consider the patch safely landed if no more reports at the end of this week (fingers crossed!)
Project Member

Comment 34 by bugdroid1@chromium.org, Oct 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ab1291fb2b682683463ca51ed3e616af39c0926f

commit ab1291fb2b682683463ca51ed3e616af39c0926f
Author: Darren Shen <shend@chromium.org>
Date: Tue Oct 17 06:19:00 2017

[CSSParser] Use streaming parser for selector parsing.

This patch change selector parsing to use streams. The advantage is two
fold:

- We no longer have to allocate memory for long selector preludes, e.g.
  .a, .b, .c, .d, .e .... { }. This is useful for websites that minify
  their CSS, as the minified CSS tend to form very long selector lists.

- This is the last use of CSSParserObserverWrapper, so we can remove it
  in a future patch.

To use streaming, we have to be careful with error recovery. If an error
occurs in the prelude, we still have to skip the rest of the block.

For reviewers:
- Most of this code is from the original patch in [1].
- The 'new' code in CSSSelectorParser is pretty much just the streaming
  version of the Range based version of ConsumeComplexSelectorList.

[1] https://codereview.chromium.org/2503683003/

Bug: 661854
Change-Id: Ied369ffba695f15baf347591e833aaec40080662
Reviewed-on: https://chromium-review.googlesource.com/676888
Reviewed-by: meade_UTC10 <meade@chromium.org>
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509297}
[modify] https://crrev.com/ab1291fb2b682683463ca51ed3e616af39c0926f/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/ab1291fb2b682683463ca51ed3e616af39c0926f/third_party/WebKit/Source/core/css/parser/CSSParserImpl.h
[modify] https://crrev.com/ab1291fb2b682683463ca51ed3e616af39c0926f/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[modify] https://crrev.com/ab1291fb2b682683463ca51ed3e616af39c0926f/third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp
[modify] https://crrev.com/ab1291fb2b682683463ca51ed3e616af39c0926f/third_party/WebKit/Source/core/css/parser/CSSSelectorParser.h
[modify] https://crrev.com/ab1291fb2b682683463ca51ed3e616af39c0926f/third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp

Project Member

Comment 35 by bugdroid1@chromium.org, Oct 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9048063d3ef0e7635882d37a9df8916a91fbbac3

commit 9048063d3ef0e7635882d37a9df8916a91fbbac3
Author: Darren Shen <shend@chromium.org>
Date: Mon Oct 30 01:29:13 2017

[CSSParser] Delete CSSParserObserverWrapper.

CSSParserObserverWrapper was used as an interface between
CSSParserObserver and CSSParserTokenRanges for inspector parsing. Since
we no longer use CSSParserTokenRanges for inspector parsing, we can
delete this class and use CSSParserObserver directly.

Bug: 661854
Change-Id: I099e2db34437ce666274508396f1e6eb9915382c
Reviewed-on: https://chromium-review.googlesource.com/688974
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Renée Wright <rjwright@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512430}
[modify] https://crrev.com/9048063d3ef0e7635882d37a9df8916a91fbbac3/third_party/WebKit/Source/core/css/BUILD.gn
[modify] https://crrev.com/9048063d3ef0e7635882d37a9df8916a91fbbac3/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/9048063d3ef0e7635882d37a9df8916a91fbbac3/third_party/WebKit/Source/core/css/parser/CSSParserImpl.h
[delete] https://crrev.com/9c310d7826c99786affa4c0704c6071a90b09b4c/third_party/WebKit/Source/core/css/parser/CSSParserObserverWrapper.cpp
[delete] https://crrev.com/9c310d7826c99786affa4c0704c6071a90b09b4c/third_party/WebKit/Source/core/css/parser/CSSParserObserverWrapper.h
[modify] https://crrev.com/9048063d3ef0e7635882d37a9df8916a91fbbac3/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.cpp
[modify] https://crrev.com/9048063d3ef0e7635882d37a9df8916a91fbbac3/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[modify] https://crrev.com/9048063d3ef0e7635882d37a9df8916a91fbbac3/third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp
[modify] https://crrev.com/9048063d3ef0e7635882d37a9df8916a91fbbac3/third_party/WebKit/Source/core/css/parser/CSSSelectorParser.h
[modify] https://crrev.com/9048063d3ef0e7635882d37a9df8916a91fbbac3/third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp

Project Member

Comment 36 by bugdroid1@chromium.org, Nov 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/93b5990b8beca44d533e66b959d4baafdc0b7dad

commit 93b5990b8beca44d533e66b959d4baafdc0b7dad
Author: Darren Shen <shend@chromium.org>
Date: Tue Nov 07 04:57:50 2017

[CSSParser] Simplify token lifetime management.

Most of this patch is from the original patch:
https://codereview.chromium.org/2503683003/

The way we currently manage token lifetimes in the parser is a bit
complicated. This patch tries to simplify this by taking advantage of
two assumptions which currently hold:

- The tokens for only one prelude or declaration needs to be stored
  at any point.
- We never need to 'append' tokens to a Range.

With these assumptions, we can introduce a new method called
ConsumeUntilPeekedIs, which consumes tokens until the peeked
token type is one of the specified ones. These tokens are stored
in a vector and kept alive until the next call to ConsumeUntilPeekedIs.
This dramatically simplifies token lifetime management and allows us
to delete CSSParserScopedTokenRange and move CSSParserTokenRange to
be a nested class in CSSParserTokenStream.

Bug: 661854
Change-Id: I33bb8956a1333c280a8cc35609be800d41645d61
Reviewed-on: https://chromium-review.googlesource.com/688258
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514397}
[modify] https://crrev.com/93b5990b8beca44d533e66b959d4baafdc0b7dad/third_party/WebKit/Source/core/css/BUILD.gn
[modify] https://crrev.com/93b5990b8beca44d533e66b959d4baafdc0b7dad/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
[modify] https://crrev.com/93b5990b8beca44d533e66b959d4baafdc0b7dad/third_party/WebKit/Source/core/css/parser/CSSParserImpl.h
[delete] https://crrev.com/a6e04d9a38b99858c7247da58edc43867b312447/third_party/WebKit/Source/core/css/parser/CSSParserScopedTokenBuffer.h
[delete] https://crrev.com/a6e04d9a38b99858c7247da58edc43867b312447/third_party/WebKit/Source/core/css/parser/CSSParserTokenBuffer.h
[modify] https://crrev.com/93b5990b8beca44d533e66b959d4baafdc0b7dad/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.cpp
[modify] https://crrev.com/93b5990b8beca44d533e66b959d4baafdc0b7dad/third_party/WebKit/Source/core/css/parser/CSSParserTokenStream.h
[modify] https://crrev.com/93b5990b8beca44d533e66b959d4baafdc0b7dad/third_party/WebKit/Source/core/css/parser/CSSParserTokenStreamTest.cpp
[modify] https://crrev.com/93b5990b8beca44d533e66b959d4baafdc0b7dad/third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp

Comment 37 by shend@chromium.org, Nov 20 2017

Owner: ----
Status: Available (was: Assigned)
Unassigning myself for now, will probably continue this work in 2018Q1. In the meantime, anyone can feel free to take this over.
Labels: ApproachableBug
Labels: -Update-Quarterly
Blocking: 827521

Sign in to add a comment