Bug 173662 – Web Inspector: move Inspector::Protocol::Array<T> to JSON namespace

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

[Blaze Burg]

This is really the parametric version of JSON::Array. It should be alongside that so we can improve both to have more consistent API.

[Blaze Burg]

Created attachment 313550 [details]
Patch

Depends on earlier renaming, so it won't build. I'll attach a combined patch when I'm done with this series.

[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 313550 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:156:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 56 files


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

[Joseph Pecoraro]

Comment on attachment 313550 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        Move the parametric JSON array type to the JSON namespace, alongside JSON::Array.
> +        Rename it to ArrayOf<T> so that it reads well in most cases. This also avoids
> +        overloading the term "typed array" or similar concepts in the code base.

This patch seems out of date. It introduces JSON::TypedArray<T> but the changelog says it wants to introduce JSON::ArrayOf<T>.

Is this just an outdated patch?

[Blaze Burg]

Created attachment 328030 [details]
Patch

[Blaze Burg]

Created attachment 328058 [details]
Patch

[Blaze Burg]

Created attachment 328124 [details]
Fix Mac builds

[Joseph Pecoraro]

Comment on attachment 328124 [details]
Fix Mac builds

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

Looks good, but fix the test then I'll re-review.

> Source/WTF/wtf/JSONValues.h:435
> +    Array& openAccessors()
> +    {
> +        COMPILE_ASSERT(sizeof(Array) == sizeof(ArrayOf<T>), cannot_cast);
> +        return *static_cast<Array*>(static_cast<ArrayBase*>(this));
> +    }

I've never like the name `openAccessors()`. Maybe something like `asArray()`? Moot point though since it is only used right here.

> Tools/TestWebKitAPI/Tests/WTF/JSONValue.cpp:256
> +    array->addItem(1);
> +    EXPECT_EQ(array->length(), 3U);
> +    value = array->get(2);
> +    EXPECT_TRUE(value->type() == JSON::Value::Type::Integer);
> +    int integerValue;
> +    EXPECT_TRUE(value->asInteger(integerValue));
> +    EXPECT_EQ(integerValue, 1);

I'd expect the length to be 2 here, not 3. So the get should be `->get(1)`. The array only has the null added above. Maybe given the comment about booleans everything shifted a number.

> Tools/TestWebKitAPI/Tests/WTF/JSONValue.cpp:259
> +    EXPECT_EQ(array->length(), 4U);

3U and so on...

> Tools/TestWebKitAPI/Tests/WTF/JSONValue.cpp:299
> +    value = it->get();
> +    EXPECT_TRUE(value->type() == JSON::Value::Type::Boolean);
> +    ++it;
> +    EXPECT_FALSE(it == array->end());

Yeah, there is no boolean in the list.

[Blaze Burg]

Comment on attachment 328124 [details]
Fix Mac builds

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

>> Tools/TestWebKitAPI/Tests/WTF/JSONValue.cpp:256
>> +    EXPECT_EQ(integerValue, 1);
> 
> I'd expect the length to be 2 here, not 3. So the get should be `->get(1)`. The array only has the null added above. Maybe given the comment about booleans everything shifted a number.

Yep, forgot to double check.

>> Tools/TestWebKitAPI/Tests/WTF/JSONValue.cpp:259
>> +    EXPECT_EQ(array->length(), 4U);
> 
> 3U and so on...

Gah. Fixing it.

[Blaze Burg]

Created attachment 328151 [details]
Take 2

[Joseph Pecoraro]

Comment on attachment 328151 [details]
Take 2

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

r=me. Watch the bots closely!

> Tools/TestWebKitAPI/Tests/WTF/JSONValue.cpp:240
> +    Ref<JSON::ArrayOf<JSON::Value>> array = JSON::ArrayOf<JSON::Value>::create();

You normally auto these =)

This test actually brings to mind that JSON doesn't allow cyclic references but a programmer could create a cyclic reference.

It is certainly a programmer error to do so. But what happens in this case?

    auto array = JSON::ArrayOf<JSON::Value>::create();
    array->addItem(array);
    array->toJSONString();

It seems we would infinite loop. Since there is no cycle detection.

I wonder if we should have any kind of mitigations / detection with assertions. Certainly not part of this patch but something to think about. A straightforward approach would be:

    String toJSONString()
    {
    #ifndef NDEBUG
        assertValidity();
    #endif
        StringBuilder result;
        result.reserveCapacity(512);
        writeJSON(result);
        return result.toString();
    }

But catching issues would be even better if the moment an item is added and a cycle is detected that we catch it.

[WebKit Commit Bot]

Comment on attachment 328151 [details]
Take 2

Clearing flags on attachment: 328151

Committed r225425: <https://trac.webkit.org/changeset/225425>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35807927>