Bug 27147 – JS Syntax Highlight is incorrect for keyword followed by string or regexp

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Keishi Hattori]

Created attachment 32553 [details]
testcase - inspect and see the syntax highlight for src.js

JS Syntax Highlight is wrong when a string or regexp immediately follows a keyword without a space. (ex. return"foo"; )

[Keishi Hattori]

Created attachment 32554 [details]
Patch

all-minified.js at slashdot.org looks much better with this patch.

[Timothy Hatcher]

Wait, why was this marked obsolete?

[WebKit Commit Bot]

Comment on attachment 32554 [details]
Patch

Rejecting patch 32554 from commit-queue.

This patch will require manual commit. Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None

[Eric Seidel (no email)]

Comment on attachment 32554 [details]
Patch

You were bitten by bug 28316.  Will have a fix this week, sorry.

[WebKit Commit Bot]

Comment on attachment 32554 [details]
Patch

Rejecting patch 32554 from commit-queue.

This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 32554 [details]
Patch

It would appear we have a flakey worker test. :(
fast/workers/dedicated-worker-lifecycle.html -> failed

Also, looks like you have tabs in your ChangeLog, so this would have failed anyway.  Leaving cq-.

[Eric Seidel (no email)]

A quick git log search says this hasn't been landed yet.  If you'd like to post a tab-less patch, we'd be happy to cq+ this again.

[Joseph Pecoraro]

Hmm, it looks like when checking for a String or a Number it first checks the previous character to make sure it was empty (thus scanning a new line) or a non-alpha character:

  ...
  else if (!prevChar || /^\W/.test(prevChar)) { 
    token = findNumber(codeFragment, code[i - 1]) || 
      findKeyword(codeFragment, code[i - 1]) || 
      findSingleLineString(codeFragment) || 
      findSingleLineRegExp(codeFragment);
  ...

This has been around since the beginning:
http://trac.webkit.org/changeset/34657/trunk/WebCore/page/inspector/SourceFrame.js

Tim, was there a reason for this check or is it just an optimization? Dropping that check fixes this edge case, but I don't know what types of affects it would have on the speed of the parsing.  Keishi's solution did just this, he moved the regex and string checks out of this constraint.

As for Keishi's patch which he said improves "all-minified.js" on slashdot. Syntax highlighting that file crashes Safari 4.0.3, WebKit r49073, and even TextMate my editor (which uses a similar syntax highlighting procedure).

[Keishi Hattori]

Created attachment 41019 [details]
New Javascript Synax Highlighter

I'm sorry for the bad patch. 

I have rewritten the syntax highlighter from scratch. 
It now uses similar logic as a proper ECMAScript tokenizer so it should not confuse DIV and REGEXP anymore.
It also can figure out IDENT tokens(even with Unicode characters too). It's turned off right now to cut down on the number of elements but maybe we could use it in the future for the debugger to show variable values.
I also changed it to process 100 tokens at a time. 
It was still beach balling with minified JS so I put a limit on the number of characters per line it will color.

[Eric Seidel (no email)]

Comment on attachment 32554 [details]
Patch

Clearing Tim's r+ on this obsolete patch.

[Keishi Hattori]

Created attachment 41833 [details]
proposed patch

Dropped unicode letter support. Uses [a-zA-Z] as unicode letter regexp.

Fixed syntax highlighting in ElementsTreeOutline.

I'll try to come up with some tests next.

[Timothy Hatcher]

Comment on attachment 41833 [details]
proposed patch


> +    this.LEXSTATE = {
> +        INITIAL: 1,
> +        DIV_ALLOWED: 2,
> +    };
> +    this.CONTINUESTATE = {
> +        NONE: 0,
> +        COMMENT: 1,
> +        SINGLEQUOTESTRING: 2,
> +        DOUBLEQUOTESTRING: 3,
> +        REGEXP: 4
> +    };

These should not be all caps. Make them CamelCase.

[Keishi Hattori]

Created attachment 41841 [details]
proposed patch with tests

> These should not be all caps. Make them CamelCase.
Fixed.

Added  some tests.

[Keishi Hattori]

Created attachment 41842 [details]
fixed some minor stuff

[Timothy Hatcher]

Comment on attachment 41842 [details]
fixed some minor stuff


> +        DivAllowed: 2,

I assume this means division allowed. Spell it out in the constant.


> +    this.state = this.LexState.Initial;

This be this.lexState to match the constant names. Or the constant name should just be this.State.


> +    }
> +    
> +    
> +    function singleQuoteStringStartAction(token)

Extra blank line here.


> +        var keywords = ["null", "true", "false", "break", "case", "catch", "const", "default", "finally", "for", "instanceof", "new", "var", "continue", "function", "return", "void", "delete", "if", "this", "do", "while", "else", "in", "switch", "throw", "try", "typeof", "with", "debugger", "class", "enum", "export", "extends", "import", "super", "get", "set"];

Use "const keywords" instead of "var keywords".


> +        if (keywords.indexOf(token) < 0) {

=== -1 is better here, since it is speced to return only -1. Or flip it to be !== -1 if you want.


> +            var i;
> +            for (i = 0; i < tokensPerChunk; i++) {

Put the var i in the for loop:

for (var i = 0; …


> +                var messageBubble = line.lastChild;
> +                if (messageBubble && messageBubble.nodeType === Node.ELEMENT_NODE && messageBubble.hasStyleClass("webkit-html-message-bubble"))
> +                    line.removeChild(messageBubble);
> +                else
> +                    messageBubble = null;

I think it would be better to start messageBubble out as null and assign it only after confirming the type and class on the lastChild.


> +                while (line.firstChild)
> +                    line.removeChild(line.firstChild);

We have a helper for this. Calling line.removeChildren() should do.


> +                this.lineFragment =null;

Missing a space after the "=".


> +    appendNonToken: function () {

Brace on the next line.


> +    syntaxHighlightNode: function(node) {

Ditto.


> +        while (1) {

Use "true".


> +        result.push(dumpSyntaxHighlight("return'foo';"));
> +        result.push(dumpSyntaxHighlight("/\\\//"));
> +        result.push(dumpSyntaxHighlight("//ig';"));
> +        result.push(dumpSyntaxHighlight("1 / 2 + /a/.test('a');"));
> +        result.push(dumpSyntaxHighlight("\"\\\"/\".length / 2"));
> +        result.push(dumpSyntaxHighlight("var foo = 1/****//2"));
> +        result.push(dumpSyntaxHighlight("/*comment*//.*/.test('a')"));

You should write some more tests that are multi-line and deal with things like message bubbles and nodes directly.

Can be another patch.

[Timothy Hatcher]

Nice work BTW! You should apply this to the CSS highlighter and make them share more code.

[Keishi Hattori]

Created attachment 41953 [details]
proposed patch

> You should write some more tests that are multi-line and deal with things like
> message bubbles and nodes directly.

I added some multiline tests. 
I tried to write a message bubble test like this
http://gist.github.com/219610
But I get the timeout error. The event doesn't seem to be getting dispatched but I couldn't figure out more.

[WebKit Commit Bot]

Comment on attachment 41953 [details]
proposed patch

Rejecting patch 41953 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Timothy Hatcher', '--force']" exit_code: 2
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/inspector/front-end/ElementsTreeOutline.js
patching file WebCore/inspector/front-end/SourceFrame.js
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/inspector/javascript-syntax-highlight-expected.txt
Died at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply line 438, <> line 638.

[Eric Seidel (no email)]

My apologies.  False rejection.  I'm working on a fix to svn-apply right now.  bug 30826

[Eric Seidel (no email)]

Comment on attachment 41953 [details]
proposed patch

svn-apply fixed.  Trying again.

[WebKit Commit Bot]

Comment on attachment 41953 [details]
proposed patch

Rejecting patch 41953 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
.html
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
	M	WebCore/inspector/front-end/SourceFrame.js
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/LayoutTests/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/libexec/git-core//git-svn line 469

[Timothy Hatcher]

Committed r50162: <http://trac.webkit.org/changeset/50162>

[Eric Seidel (no email)]

 5         Need a short description and bug URL (OOPS!)

in the LayoutTests ChangeLog was the problem. :(

Thank you for taking care of this Tim.