Bug 173619 – Web Inspector: use JSON::{Array,Object,Value} instead of Inspector{Array,Object,Value}

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

This object system is used outside of Inspector code so let's start moving it out.

[Blaze Burg]

Created attachment 313434 [details]
Patch

[Build Bot]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Build Bot]

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


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:43:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 1 in 18 files


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

[Blaze Burg]

Created attachment 313529 [details]
Patch v1.1

[Build Bot]

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


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:43:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 1 in 23 files


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

[Alex Christensen]

Comment on attachment 313529 [details]
Patch v1.1

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

> Source/JavaScriptCore/inspector/InspectorValues.h:52
> +namespace JSON {

Is this namespace used anywhere else?  How are you making JSON objects?  Probably completely unrelated to this patch, but we've been meaning to re-organize the JSON parser to use callbacks like the YARR parser so we can use it for other things and remove json.hpp.  Would that help this project, too?

[Blaze Burg]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 313529 [details]
> Patch v1.1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313529&action=review
> 
> > Source/JavaScriptCore/inspector/InspectorValues.h:52
> > +namespace JSON {
> 
> Is this namespace used anywhere else?

Unknown. I have never seen it, though.

> How are you making JSON objects? 

This code is ancient, and has its own JSON parser and stringifier. I believe ScriptValue is able to bridge between our C++ object representation of a JSON object and JS object representation.

Usually, though, this code parses strings from the remote debugger and send back out strings with payloads / command results.

> Probably completely unrelated to this patch, but we've been meaning to
> re-organize the JSON parser to use callbacks like the YARR parser so we can
> use it for other things and remove json.hpp.  Would that help this project,
> too?

Unless there's some reason I am unaware of, this code's parser (and maybe stringifier) could be replaced if JSC's JSON parser used callbacks (so we could create C++ objects for values instead of JS objects).

[Alex Christensen]

Comment on attachment 313529 [details]
Patch v1.1

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

r=me

>>> Source/JavaScriptCore/inspector/InspectorValues.h:52
>>> +namespace JSON {
>> 
>> Is this namespace used anywhere else?  How are you making JSON objects?  Probably completely unrelated to this patch, but we've been meaning to re-organize the JSON parser to use callbacks like the YARR parser so we can use it for other things and remove json.hpp.  Would that help this project, too?
> 
> Unknown. I have never seen it, though.

I think I'd prefer this namespace be called something like "InspectorProtocol" or "InspectorJSON" unless we want to make it purely for JSON and not have any inspector anything inside it.

[Joseph Pecoraro]

Comment on attachment 313529 [details]
Patch v1.1

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

>>>> Source/JavaScriptCore/inspector/InspectorValues.h:52
>>>> +namespace JSON {
>>> 
>>> Is this namespace used anywhere else?  How are you making JSON objects?  Probably completely unrelated to this patch, but we've been meaning to re-organize the JSON parser to use callbacks like the YARR parser so we can use it for other things and remove json.hpp.  Would that help this project, too?
>> 
>> Unknown. I have never seen it, though.
> 
> I think I'd prefer this namespace be called something like "InspectorProtocol" or "InspectorJSON" unless we want to make it purely for JSON and not have any inspector anything inside it.

I tend to agree. I was under the impression that InspectorValue was not totally compatible with JSON. But I did a quick scan and didn't find any issues. In any case, I wouldn't want it named JSON unless it really did handle all of JSON. Moving it to such a generic namespace I'd also expect to see unit tests. To a lesser degree this is somewhat well tested via Inspector tests right now.

[Blaze Burg]

(In reply to Joseph Pecoraro from comment #9)
> Comment on attachment 313529 [details]
> Patch v1.1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313529&action=review
> 
> >>>> Source/JavaScriptCore/inspector/InspectorValues.h:52
> >>>> +namespace JSON {
> >>> 
> >>> Is this namespace used anywhere else?  How are you making JSON objects?  Probably completely unrelated to this patch, but we've been meaning to re-organize the JSON parser to use callbacks like the YARR parser so we can use it for other things and remove json.hpp.  Would that help this project, too?
> >> 
> >> Unknown. I have never seen it, though.
> > 
> > I think I'd prefer this namespace be called something like "InspectorProtocol" or "InspectorJSON" unless we want to make it purely for JSON and not have any inspector anything inside it.
> 
> I tend to agree. I was under the impression that InspectorValue was not
> totally compatible with JSON. But I did a quick scan and didn't find any
> issues. In any case, I wouldn't want it named JSON unless it really did
> handle all of JSON. Moving it to such a generic namespace I'd also expect to
> see unit tests. To a lesser degree this is somewhat well tested via
> Inspector tests right now.

I'm willing to pick this patch back up and write tests. However, we need to first do the renaming so that further attempts on this bug stop bit-rotting. Just since this patch, there are dozens of new uses especially in Canvas and Source/WebDriver.

[Blaze Burg]

(In reply to Brian Burg from comment #10)
> (In reply to Joseph Pecoraro from comment #9)
> > Comment on attachment 313529 [details]
> > Patch v1.1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=313529&action=review
> > 
> > >>>> Source/JavaScriptCore/inspector/InspectorValues.h:52
> > >>>> +namespace JSON {
> > >>> 
> > >>> Is this namespace used anywhere else?  How are you making JSON objects?  Probably completely unrelated to this patch, but we've been meaning to re-organize the JSON parser to use callbacks like the YARR parser so we can use it for other things and remove json.hpp.  Would that help this project, too?
> > >> 
> > >> Unknown. I have never seen it, though.
> > > 
> > > I think I'd prefer this namespace be called something like "InspectorProtocol" or "InspectorJSON" unless we want to make it purely for JSON and not have any inspector anything inside it.
> > 
> > I tend to agree. I was under the impression that InspectorValue was not
> > totally compatible with JSON. But I did a quick scan and didn't find any
> > issues. In any case, I wouldn't want it named JSON unless it really did
> > handle all of JSON. Moving it to such a generic namespace I'd also expect to
> > see unit tests. To a lesser degree this is somewhat well tested via
> > Inspector tests right now.
> 
> I'm willing to pick this patch back up and write tests. However, we need to
> first do the renaming so that further attempts on this bug stop bit-rotting.
> Just since this patch, there are dozens of new uses especially in Canvas and
> Source/WebDriver.

In other words, let's land the renamings and then I'll work on the tests. Unit tests are a prerequisite to adopting the shared JSON parser via callbacks, to ensure we don't regress anything.

Joe, did you have any particular ideas for JSON unit tests? It seems like a big area and a priori deciding what tests to write seems kind of weird.

Alex, how did you go about writing tests for the new URL parser? It seems like a similarly large space to test from scratch.

Maybe we can start by writing tests that deserialize various JSON strings, both valid and invalid. We can write the equivalent tests that create valid and invalid JSON::Value trees in memory and see what happens when we try to serialize.

[Joseph Pecoraro]

> Joe, did you have any particular ideas for JSON unit tests? It seems like a
> big area and a priori deciding what tests to write seems kind of weird.

You'd should update the InspectorValue TestWebKitAPI test that was recently added:

    Tools/TestWebKitAPI/Tests/JavaScriptCore/InspectorValue.cpp

I'd expect at least basic tests for:

    Basic Construction of the different JSON Value types:
        Null,
        Boolean,
        Double,
        Integer,
        String,
        Object,
        Array,
    
    InspectorValue::toJSONString
        - validate the output of the above types, non-finite numbers, nested objects / arrays

    InspectorValue::parseString
        - all JSON types, empty string, numbers >2^53, non-finite numbers, NaN, nested objects / arrays, some invalid strings

    InspectorObject
        - size()
        - get/set/remove

    InspectorArray:
        - length()
        - get/push

These would be trivial to write but validate our expectations in edge cases to see if this really is JSON or not.

---

I see already there is one place this can produce invalid JSON, since NaN is not valid (maybe it should be "null"):

>            // Not enough room for decimal. Use exponential format.
>            if (decimal.bufferLengthForStringExponential() > WTF::NumberToStringBufferLength) {
>                // Fallback for an abnormal case if it's too little even for exponential.
>                output.appendLiteral("NaN");
>                return;
>            }

[Carlos Garcia Campos]

Created attachment 325891 [details]
Rebased patch + unit tests

Rebased patch and included basic unit tests.

[Build Bot]

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


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:43:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 1 in 24 files


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

[Carlos Garcia Campos]

Created attachment 326105 [details]
Try to fix mac builds

[Build Bot]

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


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:43:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 1 in 24 files


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

[Carlos Garcia Campos]

Brian, could you review the unit test changes?

[Blaze Burg]

Comment on attachment 326105 [details]
Try to fix mac builds

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

r=me, very thorough tests!

> Source/JavaScriptCore/ChangeLog:6
> +        Reviewed by Alex Christensen.

Please fix this so that you are the top line author, the original patch is by me, and the patch is reviewed by me and Alex.

[Carlos Garcia Campos]

Committed r224576: <https://trac.webkit.org/changeset/224576>

[Radar WebKit Bug Importer]

<rdar://problem/35569025>