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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Dec 11 11:03:58 PST 2017

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

Attachment 328974: Rebased patch.


--- 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


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"),
> +	   else if (isPositiveInfinite())
> +	       value->setDouble(ASCIILiteral("value"),
> +	   else if (isNegativeInfinite())
> +	       value->setDouble(ASCIILiteral("value"),

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

    if (!std::isfinite(m_value.number)) {

> 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

More information about the webkit-reviews mailing list