[webkit-reviews] review denied: [Bug 45271] HTML parser should provide script column position within HTML document to JavaScript engine : [Attachment 66664] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied anton muhin <antonm at chromium.org>'s
request for review:
Bug 45271: HTML parser should provide script column position within HTML
document to JavaScript engine
https://bugs.webkit.org/show_bug.cgi?id=45271

Attachment 66664: Patch
https://bugs.webkit.org/attachment.cgi?id=66664&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=66664&action=prettypatch

R- for lack of testing and for likely performance problems.  Check out how view
source figures out columns.  It's fast, works, and has some amount of testing.

> WebCore/ChangeLog:8
> +	   TODO(peter): fill in description.
Would be nice to have a description.

> WebCore/ChangeLog:10
> +	   No new tests. (OOPS!)
Tests?

> WebCore/bindings/v8/V8Proxy.cpp:402
> -	   v8::Handle<v8::Script> script = compileScript(code, source.url(),
source.startLine() - 1, scriptData.get());
> +	   v8::Handle<v8::Script> script = compileScript(code, source.url(),
source.startLine() - 1, source.startColumn() - 1, scriptData.get());
This whole +1/-1 thing is sadness.  We should leverage the C++ type system to
help us here.  V8LineNumber / ScriptLineNumber or some such.

> WebCore/html/parser/HTMLScriptRunner.cpp:312
> -void HTMLScriptRunner::runScript(Element* script, int startingLineNumber)
> +void HTMLScriptRunner::runScript(Element* script, int startingLineNumber,
int startingColumnNumber)
Does it make sense to put the line number and the column number together to a
struct?  ScriptPosition?  Seems like you're touching all these lines of code
anyway.

> WebCore/platform/text/SegmentedString.cpp:237
> +	     ++cursorPosition.column;
Did you run the parser benchmark?  I bet this costs a bunch of time.  If you
look at how the view source code figures out column numbers, you'll find a much
faster way.