[webkit-reviews] review granted: [Bug 210119] querySelector("#\u0000") should match an element with ID U+FFFD : [Attachment 395844] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 210119: querySelector("#\u0000") should match an element with ID U+FFFD
https://bugs.webkit.org/show_bug.cgi?id=210119

Attachment 395844: Patch

https://bugs.webkit.org/attachment.cgi?id=395844&action=review




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 395844
  --> https://bugs.webkit.org/attachment.cgi?id=395844
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395844&action=review

> Source/WebCore/css/parser/CSSTokenizer.cpp:44
> +static String& preprocessString(String& string)

Doesn’t seem like a great pattern to take a String& and also return it.

I suggest we take either String&& or const String& and return a String.

> Source/WebCore/css/parser/CSSTokenizer.cpp:48
> +    return string.replace('\0', 0xFFFD);

We have a named constant for this, replacementCharacter, in
<wtf/unicode/CharacterNames.h>. Let's use that.

> Source/WebCore/css/parser/CSSTokenizer.cpp:208
> +    if (m_input.peek(0) == '!'
> +	   && m_input.peek(1) == '-'
> +	   && m_input.peek(2) == '-') {

We could make this a one-liner now, easier to read.

> Source/WebCore/css/parser/CSSTokenizer.cpp:227
> +    if (m_input.peek(0) == '-'
> +	   && m_input.peek(1) == '>') {

We could make this a one-liner now, easier to read.

> Source/WebCore/css/parser/CSSTokenizer.cpp:330
> +    if (m_input.peek(0) == '+'
> +	   && (isASCIIHexDigit(m_input.peek(1)) || m_input.peek(1) == '?')) {

We could make this a one-liner now, easier to read.

> Source/WebCore/css/parser/CSSTokenizer.h:49
> -    explicit CSSTokenizer(const String&);
> -    CSSTokenizer(const String&, CSSParserObserverWrapper&); // For the
inspector
> +    explicit CSSTokenizer(String);
> +    CSSTokenizer(String, CSSParserObserverWrapper&); // For the inspector

We could have made this change without changing the argument type; it would
simple cost a little reference count churn.

If we want to change the interface to avoid the churn, then I suggest a
String&& argument instead of a String argument.

Or maybe you just like the simplicity of just String?

> Source/WebCore/css/parser/CSSTokenizerInputStream.h:50
>	       return '\0';

Messy that we have kEndOfFileMarker but don’t use it much.

> Source/WebCore/css/parser/CSSTokenizerInputStream.h:56
> +    UChar peek(unsigned lookaheadOffset) const

I almost wish this was just operator[] instead a named function.