[webkit-reviews] review granted: [Bug 107157] Make Console::shouldPrintExceptions work in WebKit2 : [Attachment 183244] Proposed Change

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 107157: Make Console::shouldPrintExceptions work in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=107157

Attachment 183244: Proposed Change
https://bugs.webkit.org/attachment.cgi?id=183244&action=review

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


r=me with the switch to Settings.in

> Source/WebCore/page/Console.cpp:218
> +    for (unsigned i = 0; i < arguments->argumentCount(); ++i) {

NiT: You might as well update `i` to size_t here.
ScriptArguments::argumentCount returns a size_t.

> Source/WebCore/page/Console.cpp:227
>	   for (unsigned i = 0; i < callStack->size(); ++i) {

Ditto for ScriptCallstack::size.

> Source/WebCore/page/Settings.h:372
> +	   bool m_logsPageMessagesToSystemConsoleEnabled : 1;

For simple boolean settings like this you should use probably declare it in
"Source/WebCore/page/Settings.in". A single line:

    logsPageMessagesToSystemConsoleEnabled initial=false

That would also take care of initialization this in the constructor, which
appears to be missing from this patch. And getters should be const.