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"; )
Created attachment 32554 [details] Patch all-minified.js at slashdot.org looks much better with this patch.
Wait, why was this marked obsolete?
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
Comment on attachment 32554 [details] Patch You were bitten by bug 28316. Will have a fix this week, sorry.
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
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-.
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.
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).
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.
Comment on attachment 32554 [details] Patch Clearing Tim's r+ on this obsolete patch.
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.
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.
Created attachment 41841 [details] proposed patch with tests > These should not be all caps. Make them CamelCase. Fixed. Added some tests.
Created attachment 41842 [details] fixed some minor stuff
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.
Nice work BTW! You should apply this to the CSS highlighter and make them share more code.
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.
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.
My apologies. False rejection. I'm working on a fix to svn-apply right now. bug 30826
Comment on attachment 41953 [details] proposed patch svn-apply fixed. Trying again.
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
Committed r50162: <http://trac.webkit.org/changeset/50162>
5 Need a short description and bug URL (OOPS!) in the LayoutTests ChangeLog was the problem. :( Thank you for taking care of this Tim.