[Webkit-unassigned] [Bug 70107] Custom written CSS lexer

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 24 13:04:12 PST 2012


https://bugs.webkit.org/show_bug.cgi?id=70107


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #123705|                            |commit-queue-
               Flag|                            |




--- Comment #57 from Alexey Proskuryakov <ap at webkit.org>  2012-01-24 13:04:10 PST ---
(From update of attachment 123705)
View in context: https://bugs.webkit.org/attachment.cgi?id=123705&action=review

> Source/WebCore/css/CSSParser.cpp:181
> +static inline UChar lowercaseCharacter(UChar chr)

"chr"? Please don't abbreviate like this.

I suggest putting this in ASCIICType.h as toASCIILowerUnchecked().

> Source/WebCore/css/CSSParser.cpp:190
> +static inline bool isCaselessEqual(UChar cssCharacter, UChar character)

Ditto.

> Source/WebCore/css/CSSParser.cpp:7489
> +static inline bool isCSSLetter(UChar chr)

Please don't abbreviate "character".

> Source/WebCore/css/CSSParser.cpp:7491
> +    return chr >= 128 || typesOfASCIICharacters[chr] <= CharacterDash;

Even U+FFFD and unassigned code points are CSS letters?

> Source/WebCore/css/CSSParser.cpp:7494
> +static inline bool isCSSEscape(UChar chr)

Repeated several more times in the patch.

> Source/WebCore/css/CSSParser.cpp:7510
> +static inline bool isCaselessEqual(UChar* cssString, const char* constantString)

What's important to say in function name is that it's ASCII case insensitive comparison (which is different from Unicode one even for ASCII characters!), and that this depends on CSS syntax.

Otherwise, call sites get really confusing.

> Source/WebCore/css/CSSParser.cpp:7527
> +static UChar* checkEscape(UChar* data)

A check function can return false or true. What does this function do?

> Source/WebCore/css/CSSParser.cpp:7543
> +        if (isHTMLSpace(*data))

Are HTML and CSS whitespace definitions the same?

> Source/WebCore/css/CSSParser.cpp:7550
> +static inline UChar* skipWhiteSpaces(UChar* data)

Should be named "skipWhiteSpace" (there is even a CSS keyword skip-white-space, not to mention that this is how the function is named elsewhere in WebCore).

> Source/WebCore/css/CSSParser.cpp:7552
> +    while (*data <= ' ' && (typesOfASCIICharacters[*data] == CharacterWhiteSpace))

This is a yet another definition of whitespace, with all lower ASCII code being treated as such. If this is right, please explain in a comment.

> Source/WebCore/css/CSSParser.cpp:7565
> +inline UChar* CSSParser::checkString(UChar* data, UChar quote)

Please use a name that implies returning a UChar*, not a boolean.

> Source/WebCore/css/CSSParser.cpp:7619
> +inline bool CSSParser::parseIdentifier(UChar*& result)

Some added "parse" functions are void, but this returns a boolean. This is super confusing. Is there a way to have uniform interface for these? Or names that explain the difference?

> Source/WebCore/css/CSSParser.cpp:7775
> +inline void CSSParser::detectFunctionToken(int length)

Does this actually detect the token? Looks like this function is called after a function has been detected, not to detect it.

> Source/WebCore/css/CSSParser.h:383
> +    OwnArrayPtr<UChar> m_dataStart;
> +    UChar* m_data;

What is m_data, is it actually m_nextCharacter?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list