[webkit-reviews] review granted: [Bug 180529] Web Inspector: Optionally log WebKit log parameters as JSON : [Attachment 328974] Rebased patch.

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 180529: Web Inspector: Optionally log WebKit log parameters as JSON
https://bugs.webkit.org/show_bug.cgi?id=180529

Attachment 328974: Rebased patch.

https://bugs.webkit.org/attachment.cgi?id=328974&action=review




--- Comment #15 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 328974
  --> https://bugs.webkit.org/attachment.cgi?id=328974
Rebased patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=328974&action=review

r=me

Looks like the canvas tests just need rebaselined results because
InjectedScriptSource changed.

> Source/JavaScriptCore/inspector/ConsoleMessage.h:92
> +    Vector<JSONLogValue> m_messagesJSON;

I don't like the name `messagesJSON` but I do like the name `JSONLogValue`. May
a better name would be `m_jsonLogValues` or just `m_loggerValues`.

> Source/JavaScriptCore/inspector/InjectedScript.cpp:275
> +    auto r = callFunctionWithEvalEnabled(wrapFunction, hadException);

Style: Lets give `r` a better name. Maybe `evalResult` / `evalValue`?

> Source/JavaScriptCore/inspector/InjectedScript.cpp:287
> +    

Style: Unexpected empty line.

> Source/WTF/wtf/MediaTime.cpp:584
> +	   if (isInvalid() || isIndefinite())
> +	       value->setDouble(ASCIILiteral("value"),
std::numeric_limits<float>::quiet_NaN());
> +	   else if (isPositiveInfinite())
> +	       value->setDouble(ASCIILiteral("value"),
std::numeric_limits<double>::infinity());
> +	   else if (isNegativeInfinite())
> +	       value->setDouble(ASCIILiteral("value"),
-std::numeric_limits<double>::infinity());

Be aware that JSON doesn't support NaN / Infinity values. So these will be
serialized as null:

    if (!std::isfinite(m_value.number)) {
	output.appendLiteral("null");
	return;
    }

> Source/WebCore/dom/Document.cpp:7608
> -    if (!this->page())
> +    if (!page() || !frame())
>	   return;

page() uses m_frame, which I assume is what frame() is. So I think just having
page() is enough to check both.

> Source/WebCore/html/track/TextTrackCueGeneric.cpp:271
> +    auto value = JSON::Object::create();

Nit: It might be better to name this `object` instead of `value` because
objects have properties and values may not (primitives). But I'm fine either
way.