Bug 180529 – Web Inspector: Optionally log WebKit log parameters as JSON

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Eric Carlson]

Allow WebKit logging to include JSON to make the log more readable.

[Radar WebKit Bug Importer]

<rdar://problem/35909462>

[Radar WebKit Bug Importer]

<rdar://problem/35909461>

[Eric Carlson]

Created attachment 328695 [details]
Proposed patch.

[Eric Carlson]

Created attachment 328697 [details]
Proposed patch.

[EWS Watchlist]

Attachment 328697 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/Logger.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Comment on attachment 328697 [details]
Proposed patch.

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

r- and deferring detailed review until this compiles.

> Source/WebCore/platform/graphics/iso/ISOVTTCue.h:56
> +    String toJSON() const;

The style throughout Web Inspector code is to either call this method toJSONValue() if returning JSON::Value, and otherwise toJSONString(). I recommend choosing either of those.

[Eric Carlson]

Created attachment 328974 [details]
Rebased patch.

[EWS Watchlist]

Attachment 328974 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Logger.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 328974 [details]
Rebased patch.

Attachment 328974 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5615592

New failing tests:
inspector/canvas/recording-2d.html
inspector/canvas/recording-webgl-snapshots.html
inspector/canvas/recording-webgl.html

[EWS Watchlist]

Created attachment 328984 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 328974 [details]
Rebased patch.

Attachment 328974 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5615618

New failing tests:
inspector/canvas/recording-2d.html
inspector/canvas/recording-webgl-snapshots.html
inspector/canvas/recording-webgl.html

[EWS Watchlist]

Created attachment 328986 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 328974 [details]
Rebased patch.

Attachment 328974 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5615648

New failing tests:
inspector/canvas/recording-2d.html
inspector/canvas/recording-webgl-snapshots.html
inspector/canvas/recording-webgl.html

[EWS Watchlist]

Created attachment 328987 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Comment on attachment 328974 [details]
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.

[Eric Carlson]

Comment on attachment 328974 [details]
Rebased patch.

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

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

Changed to m_jsonLogValues.

>> Source/JavaScriptCore/inspector/InjectedScript.cpp:275
>> +    auto r = callFunctionWithEvalEnabled(wrapFunction, hadException);
> 
> Style: Lets give `r` a better name. Maybe `evalResult` / `evalValue`?

Changed to evalResult.

>> Source/JavaScriptCore/inspector/InjectedScript.cpp:287
>> +    
> 
> Style: Unexpected empty line.

Removed.

>> Source/WTF/wtf/MediaTime.cpp:584
>> +            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;
>     }

Thanks, I changed to "NaN", "POSITIVE_INFINITY" and "NEGATIVE_INFINITY".

>> Source/WebCore/dom/Document.cpp:7608
>>          return;
> 
> page() uses m_frame, which I assume is what frame() is. So I think just having page() is enough to check both.

Opps, that is left over from an earlier incarnation. Fixed.

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

Changed them all to "object".

[Eric Carlson]

Created attachment 329020 [details]
Patch for landing.

[EWS Watchlist]

Attachment 329020 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Logger.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 329044 [details]
Patch for landing.

[EWS Watchlist]

Attachment 329044 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Logger.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 329047 [details]
Patch for landing.

[EWS Watchlist]

Attachment 329047 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Logger.h:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 329047 [details]
Patch for landing.

Clearing flags on attachment: 329047

Committed r225764: <https://trac.webkit.org/changeset/225764>