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

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted 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 218580: Patch
https://bugs.webkit.org/attachment.cgi?id=218580&action=review

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


r=me, though there are a few small nits that would be nice to cleanup.

> Source/WebCore/page/PageConsole.cpp:71
> +	       printf("%s:%d:%d: ", sourceURL.utf8().data(), lineNumber,
columnNumber);

Nit: This code, pre-existing your patch, is really weird. If you could clean
that up at the same time that would be nice. Seems like we just need your new
line and you could remove the other else blocks.

    * lineNumber and columnNumber are unsigned. So their values will always be
greater than 0. These if checks don't make sense.
    * printf format specifier for unsigned should be %u.

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

Nit: This still calls textPosition twice. Rather then think about what the
compiler would do, I'd rather see a local variable for the result of
textPosition().

> Source/WebCore/page/PageConsole.h:51
> +    static void printSourceURLAndPosition(const String& sourceURL, unsigned
lineNumber, unsigned columnNumber = 0);

Nit: I've been bitten too many times by optional parameters. It looks like
you've covered all of the call sites of this function. I'd like to see this
parameter be made non-optional. We always want to push towards having column
number everywhere anyways.

> LayoutTests/inspector-protocol/page/deny-X-FrameOption.html:16
> +	   // FIXME The line and column values will be zeros until this fix:
> +	   // https://bugs.webkit.org/show_bug.cgi?id=125340
> +	   // After this probably we should update the expected.txt.
> +	   InspectorTest.log(simplifiedMessage.location);

Excellent! Thanks