[webkit-reviews] review denied: [Bug 114319] PageConsole::addMessage should automatically determine column number alongside line number : [Attachment 216775] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 13 10:35:47 PST 2013


Joseph Pecoraro <joepeck at webkit.org> has denied László Langó
<lango at inf.u-szeged.hu>'s request for review:
Bug 114319: PageConsole::addMessage should automatically determine column
number alongside line number
https://bugs.webkit.org/show_bug.cgi?id=114319

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

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=216775&action=review


> Source/WebCore/page/PageConsole.cpp:156
>	       line = parser->lineNumber().oneBasedInt();
> +	       column = parser->textPosition().m_column.oneBasedInt();

Nice! You should remove the FIXME on line 145 that points to this bugzilla bug,
since you're fixing that now.

Nit: You might as well use textPosition() for both line / column. Under the
hood they are doing the same thing, this will ensure less work:

    TextPosition textPosition = parser->textPosition();
    line = textPosition.m_line.oneBasedInt();
    column = textPosition.m_column.oneBasedInt();

r- though, because we should really be adding some kind of a test for this to
make sure we don't regress it in the future! That will also help when the other
FIXME gets addressed to make sure we don't break this case.


More information about the webkit-reviews mailing list