[webkit-reviews] review denied: [Bug 173662] Web Inspector: move Inspector::Protocol::Array<T> to JSON namespace : [Attachment 328124] Fix Mac builds

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Brian Burg <bburg at apple.com>'s
request for review:
Bug 173662: Web Inspector: move Inspector::Protocol::Array<T> to JSON namespace
https://bugs.webkit.org/show_bug.cgi?id=173662

Attachment 328124: Fix Mac builds

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




--- Comment #8 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 328124
  --> https://bugs.webkit.org/attachment.cgi?id=328124
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.