[webkit-reviews] review denied: [Bug 53003] Web Inspector: [JSC] implement setting breakpoints by line:column : [Attachment 151028] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Peter Wang
<peter.wang at torchmobile.com.cn>'s request for review:
Bug 53003: Web Inspector: [JSC] implement setting breakpoints by line:column
https://bugs.webkit.org/show_bug.cgi?id=53003

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=151028&action=review


Overall approach in JSC looks good. One comment below.

> Source/JavaScriptCore/parser/Parser.cpp:1639
> +	   expr = context.makePostfixNode(m_lexer->lastLineNumber(),
tokenColumn(), expr, OpPlusPlus, subExprStart, lastTokenEnd(), tokenEnd());

It's awkward to get the line number from the lexer, but get the column number
from the current token. Let's get all the data from the current token instead.

The smallest change you can make to fix this is to pass a reference to the
current token instead of two separate numbers.

It would be even better to rename JSTokenInfo to TokenLocation, put line,
column, start, end, and divot all in TokenLocation, and pass just that one
object to all clients.