[webkit-reviews] review denied: [Bug 30907] Web Inspector: Rewrite CSSSourceSyntaxHighlighter so it shares more code : [Attachment 42290] proposed patch 2

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Keishi Hattori
<casey.hattori at gmail.com>'s request for review:
Bug 30907: Web Inspector: Rewrite CSSSourceSyntaxHighlighter so it shares more
code
https://bugs.webkit.org/show_bug.cgi?id=30907

Attachment 42290: proposed patch 2
https://bugs.webkit.org/attachment.cgi?id=42290&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
> +    const urlPattern = /^url\(\s*(?:(?:"(?:[\t
!#\$%&\(-~]|\\n|\r\n|\r|\f|'|[\200-\377]|(?:\[0-9a-fA-F]{1,6}[ \n\r\t\f]?|\[
-~\200-\377\n]))*"|'(?:[\t
!#\$%&\(-~]|\\n|\r\n|\r|\f|'|[\200-\377]|(?:\[0-9a-fA-F]{1,6}[ \n\r\t\f]?|\[
-~\200-\377\n]))*')|(?:[!#$%&*-~]|[\200-\377]|(?:\[0-9a-fA-F]{1,6}[
\n\r\t\f]?|\[ -~\200-\377\n]))*)\s*\)/i;
> +    const stringPattern = /^(?:"(?:[\t
!#\$%&\(-~]|\\n|\r\n|\r|\f|'|[\200-\377]|(?:\[0-9a-fA-F]{1,6}[ \n\r\t\f]?|\[
-~\200-\377\n]))*"|'(?:[\t
!#\$%&\(-~]|\\n|\r\n|\r|\f|'|[\200-\377]|(?:\[0-9a-fA-F]{1,6}[ \n\r\t\f]?|\[
-~\200-\377\n]))*')/i;
> +    const identPattern = /^-?(?:[_a-zA-Z]|[\200-\377]|(?:\[0-9a-fA-F]{1,6}[
\n\r\t\f]?|\[
-~\200-\377\n]))(?:[_a-zA-Z0-9-]|[\200-\377]|(?:\[0-9a-fA-F]{1,6}[
\n\r\t\f]?|\[ -~\200-\377\n]))*/i;

The only thing I think needs fixed about this patch is the regexs. They are
super complex.

There are a number of places than can be simplified. Like: [ \n\r\t\f] should
just be \s.

There is also a mistake with \\n being used when it should be \n.

Also [\200-\377] is odd. I know it is fro mthe spec, but the spec also says:

> The "\377" represents the highest character number that current versions of
> Flex can deal with (decimal 255). It should be read as "\4177777" (decimal
> 1114111), which is the highest possible code point in Unicode/ISO-10646.

We can support full unicode.

The multiple character rnages ORed together should just be combined inot one
character range. Like the follow example.

> [_a-zA-Z0-9-]|[\200-\377]

So you would have:

[-_a-zA-Z0-9\200-\4177777]

I wonder if we should just use \w and call it a day?


This \ should be \\.

\[0-9a-fA-F]{1,6}

It is really hard to review this. These are just the things that stood out to
me. I recomend we just write simple regexs and not try to generate them from
the spec's FLEX tokenizer.