Bug 30907 - Web Inspector: Rewrite CSSSourceSyntaxHighlighter so it shares more code
Summary: Web Inspector: Rewrite CSSSourceSyntaxHighlighter so it shares more code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-29 08:32 PDT by Keishi Hattori
Modified: 2009-11-02 09:28 PST (History)
7 users (show)

See Also:


Attachments
proposed patch (59.56 KB, patch)
2009-10-29 12:30 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
proposed patch 2 (57.26 KB, patch)
2009-11-01 18:53 PST, Keishi Hattori
timothy: review-
Details | Formatted Diff | Diff
proposed patch 3 (61.33 KB, patch)
2009-11-01 20:33 PST, Keishi Hattori
timothy: review-
Details | Formatted Diff | Diff
proposed patch 4 (61.33 KB, patch)
2009-11-01 21:17 PST, Keishi Hattori
timothy: review-
Details | Formatted Diff | Diff
Sorry, I was editing the wrong file (69.70 KB, patch)
2009-11-01 23:00 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2009-10-29 08:32:39 PDT
We should rewrite CSSSourceSyntaxHighlighter to share code with JavaScriptSourceSyntaxHighlighter.
Comment 1 Keishi Hattori 2009-10-29 12:30:24 PDT
Created attachment 42126 [details]
proposed patch

Code became uglier than I hoped. This version features full list of keywords so users can spot their typos.
Comment 2 Keishi Hattori 2009-11-01 18:53:42 PST
Created attachment 42290 [details]
proposed patch 2

I think I was able to make it much more organized. Patch also fixes few issues with JS syntax highlighter.
Comment 3 Timothy Hatcher 2009-11-01 19:27:31 PST
Comment on attachment 42290 [details]
proposed patch 2

> +    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.
Comment 4 Keishi Hattori 2009-11-01 20:33:22 PST
Created attachment 42293 [details]
proposed patch 3

Simplified the regexps.
Comment 5 Timothy Hatcher 2009-11-01 21:04:50 PST
Comment on attachment 42293 [details]
proposed patch 3

propertyKeywords, valueKeywords, mediaTypes and keywords should be objects with properties not arrays. That way it is fast to look up.

So:

const mediaTypes = {"all": true, "aural": true, "braille": true, "embossed": true, "handheld": true, "print": true, "projection": true, "screen": true, "tty": true, "tv": true};

And test like:

token in mediaTypes
Comment 6 Keishi Hattori 2009-11-01 21:17:48 PST
Created attachment 42298 [details]
proposed patch 4
Comment 7 Timothy Hatcher 2009-11-01 22:45:15 PST
I don't see a difference between 3 and 4.
Comment 8 Keishi Hattori 2009-11-01 23:00:58 PST
Created attachment 42303 [details]
Sorry, I was editing the wrong file
Comment 9 WebKit Commit Bot 2009-11-02 09:27:59 PST
Comment on attachment 42303 [details]
Sorry, I was editing the wrong file

Clearing flags on attachment: 42303

Committed r50411: <http://trac.webkit.org/changeset/50411>
Comment 10 WebKit Commit Bot 2009-11-02 09:28:04 PST
All reviewed patches have been landed.  Closing bug.