[webkit-reviews] review granted: [Bug 213631] Convert ContentSecurityPolicy related parsers over to using StringParsingBuffer : [Attachment 402874] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 213631: Convert ContentSecurityPolicy related parsers over to using
StringParsingBuffer
https://bugs.webkit.org/show_bug.cgi?id=213631

Attachment 402874: Patch

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




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

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

> Source/WebCore/html/parser/ParsingUtilities.h:38
> +template<typename CharType>
> +inline bool isNotASCIISpace(CharType c)

CharacterType, single line, constexpr?

> Source/WebCore/html/parser/ParsingUtilities.h:44
> +template<typename CharType, typename DelimiterType>
> +bool skipExactly(const CharType*& position, const CharType* end,
DelimiterType delimiter)

CharacterType, single line?

> Source/WebCore/html/parser/ParsingUtilities.h:54
> +template<typename CharType, typename DelimiterType>
> +bool skipExactly(StringParsingBuffer<CharType>& parsingBuffer, DelimiterType
delimiter)

CharacterType, single line?

I suggest naming the local variable just "buffer". Single words seem to work
well in short functions.

> Source/WebCore/html/parser/ParsingUtilities.h:74
> +template<typename CharType, bool characterPredicate(CharType)>
> +bool skipExactly(StringParsingBuffer<CharType>& parsingBuffer)

CharacterType, single line?

I suggest naming the local variable just "buffer". Single words seem to work
well in short functions.

> Source/WebCore/html/parser/ParsingUtilities.h:84
> +template<typename CharType, typename DelimiterType>
> +void skipUntil(const CharType*& position, const CharType* end, DelimiterType
delimiter)

CharacterType, single line?

> Source/WebCore/html/parser/ParsingUtilities.h:91
> +template<typename CharType, typename DelimiterType>
> +void skipUntil(StringParsingBuffer<CharType>& parsingBuffer, DelimiterType
delimiter)

CharacterType, single line?

> Source/WebCore/html/parser/ParsingUtilities.h:105
>  template<typename CharType, bool characterPredicate(CharType)>
> +void skipUntil(StringParsingBuffer<CharType>& parsingBuffer)

CharacterType, single line, "buffer"?

> Source/WebCore/html/parser/ParsingUtilities.h:119
>  template<typename CharType, bool characterPredicate(CharType)>
> +void skipWhile(StringParsingBuffer<CharType>& parsingBuffer)

CharacterType, single line, "buffer"?

> Source/WebCore/html/parser/ParsingUtilities.h:143
> +template<typename CharacterType, unsigned lowercaseLettersLength>

I think that "length" is not a great name for the size of an array that
contains a null-terminated string. We use it elsewhere, but it seems like a
mistake.

> Source/WebCore/html/parser/ParsingUtilities.h:146
> +    if (parsingBuffer.lengthRemaining() < lowercaseLettersLength - 1)

Seems like the whole function would be better if it said:

    constexpr auto lowercaseLettersLength = lowercaseLettersArraySize - 1;
    ASSERT(!lowercaseLetters[lowercaseLettersLength]);

Maybe with some kind of comment, and might need to be the right kind of assert,
maybe not ASSERT.

> Source/WebCore/html/parser/ParsingUtilities.h:150
> +    parsingBuffer += (lowercaseLettersLength - 1);

Not sure the parentheses make this clearer.

> Source/WebCore/loader/ResourceCryptographicDigest.cpp:37
> +template<typename CharacterType> static
Optional<ResourceCryptographicDigest::Algorithm>
parseHashAlgorithmAdvancingPosition(StringParsingBuffer<CharacterType>&
parsingBuffer)

"buffer"

> Source/WebCore/loader/ResourceCryptographicDigest.cpp:52
> +template<typename CharacterType> static
Optional<ResourceCryptographicDigest>
parseCryptographicDigestImpl(StringParsingBuffer<CharacterType>& parsingBuffer)

"buffer"

> Source/WebCore/loader/ResourceCryptographicDigest.cpp:67
> +    skipExactly<CharacterType>(parsingBuffer, '=');
> +    skipExactly<CharacterType>(parsingBuffer, '=');

Compiler can’t deduce CharacterType?

> Source/WebCore/loader/ResourceCryptographicDigest.cpp:82
> +Optional<ResourceCryptographicDigest>
parseCryptographicDigest(StringParsingBuffer<UChar>& parsingBuffer)

"buffer" (I’ll stop commenting this each time)

> Source/WebCore/page/csp/ContentSecurityPolicyMediaListDirective.cpp:39
> +template<typename CharacterType>
> +static bool isMediaTypeCharacter(CharacterType c)

Single-line?