This is really the parametric version of JSON::Array. It should be alongside that so we can improve both to have more consistent API.
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.
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`)
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.
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?
Created attachment 328030 [details] Patch
Created attachment 328058 [details] Patch
Created attachment 328124 [details] Fix Mac builds
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.
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.
Created attachment 328151 [details] Take 2
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.
Comment on attachment 328151 [details] Take 2 Clearing flags on attachment: 328151 Committed r225425: <https://trac.webkit.org/changeset/225425>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35807927>