[webkit-reviews] review granted: [Bug 137190] Web Inspector: InspectorValues should use references for out parameters : [Attachment 238822] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg
<burg at cs.washington.edu>'s request for review:
Bug 137190: Web Inspector: InspectorValues should use references for out
parameters
https://bugs.webkit.org/show_bug.cgi?id=137190

Attachment 238822: Patch
https://bugs.webkit.org/attachment.cgi?id=238822&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=238822&action=review


r=me

> Source/JavaScriptCore/inspector/InspectorBackendDispatcher.cpp:229
> +    static bool asInteger(InspectorValue& value, int& output) { return
value.asInteger(output); }
> +    static bool asDouble(InspectorValue& value, double& output) { return
value.asDouble(output); }
> +    static bool asString(InspectorValue& value, String& output) { return
value.asString(output); }
> +    static bool asBoolean(InspectorValue& value, bool& output) { return
value.asBoolean(output); }
> +    static bool asObject(InspectorValue& value, RefPtr<InspectorObject>&
output) { return value.asObject(output); }
> +    static bool asArray(InspectorValue& value, RefPtr<InspectorArray>&
output) { return value.asArray(output); }

Nit: "const" InspectorValue& value

> Source/JavaScriptCore/inspector/InspectorValues.cpp:334
> +	   output = "";

Nit: We should use String() or emptyString() instead of "".

> Source/JavaScriptCore/inspector/InspectorValues.cpp:684
> +	       output.append(trueString, 4);

I think we can change all of these append(const char*, unsigned) into
appendLiteral(const char*). Which will have the compiler computer the length
and call append(const char*, unsigned) for us. But that could be done
separately.

> Source/JavaScriptCore/inspector/InspectorValues.cpp:765
> +    return result;

Nit: .release()

> Source/JavaScriptCore/inspector/InspectorValues.cpp:776
> +    return result;

Nit: .release()

> Source/JavaScriptCore/inspector/InspectorValues.h:87
> +    static bool parseJSON(const String& jsonInput, RefPtr<InspectorValue>&
output);

I liked the original parseJSON just fine. This seems fine though. Do you see an
advantage?

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:340
>      if (type == number) {
>	   double number;
> -	   if (!key->getDouble("number", &number))
> +	   if (!key->getDouble("number", number))

I don't like the shadowing of variables.

outer scope: NeverDestroyed<const String> number(ASCIILiteral("number"));
inner scope: double number;

But you are not changing this.