[webkit-reviews] review granted: [Bug 173793] Move JSONValues to WTF and convert uses of InspectorValues.h to JSONValues.h : [Attachment 327697] Fix GTK more

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 173793: Move JSONValues to WTF and convert uses of InspectorValues.h to
JSONValues.h
https://bugs.webkit.org/show_bug.cgi?id=173793

Attachment 327697: Fix GTK more

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




--- Comment #48 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 327697
  --> https://bugs.webkit.org/attachment.cgi?id=327697
Fix GTK more

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

r=me, Looks good to me.

For such a generic class and namespace we should probably have TestWTF tests
for this. I think the only tests we really have are for memoryCost. But there
are all kinds of things these classes do / need to do that should be tested
(property order, valid parsing, invalid parsing, valid stringification, invalid
stringification). I think I've commented at length about that elsewhere though.

> Source/JavaScriptCore/Sources.txt:530
>  inspector/InspectorAgentRegistry.cpp

I think DeprecatedInspectorValues should be in here somewhere and not in the
Sources section of the Xcode project. (Uncheck it from the JavaScriptCore
target, and just include it here in Sources). Unless we really want it to be
Mac only?!

> Source/WTF/wtf/JSONValues.cpp:102
> +    // According   to RFC4627, a valid number is: [minus] int [frac] [exp]

Style: Fix spacing.

> Source/WTF/wtf/JSONValues.cpp:118
> +    // Optional fraction part

Style: Fix style so this ends in a period.

> Source/WTF/wtf/JSONValues.cpp:131
> +    // Optional exponent part

Style: Fix style so this ends in a period.

> Source/WTF/wtf/JSONValues.cpp:515
> +bool Value::asValue(RefPtr<Value> & value)

Style: "RefPtr<Value> &" => "RefPtr<Value>&"

> Source/WTF/wtf/JSONValues.h:122
> +    Value()
> +	   : m_type(Type::Null) { }
> +
> +    explicit Value(Type type)
> +	   : m_type(type) { }

Style: We could probably convert all of these to use the newer brace instead of
parenthesis syntax: `m_type { Type::Null }`