Bug 137190 – Web Inspector: InspectorValues should use references for out parameters

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

[Brian Burg]

We never check for null, so...

[Radar WebKit Bug Importer]

<rdar://problem/18480915>

[Brian Burg]

Created attachment 238822 [details]
Patch

[WebKit Commit 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`)

[Joseph Pecoraro]

Comment on attachment 238822 [details]
Patch

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.

[Brian Burg]

Comment on attachment 238822 [details]
Patch

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

>> Source/JavaScriptCore/inspector/InspectorValues.cpp:334
>> +        output = "";
> 
> Nit: We should use String() or emptyString() instead of "".

OK. (My next plan was to make ErrorString everywhere a reference, but this would of course completely clash with this patch)

>> 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.

had never thought of it, but sure!

>> 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?

I prefer the slightly more cumbersome outparam approach, because it forces callers to handle parse failure (or look suspicious not doing so).  This patch standardizes on that pattern (for object and array casts/gets, too).

>> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:340
>> +        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.

yuck, i'll rename the literals.

[Brian Burg]

Committed r174094: <http://trac.webkit.org/changeset/174094>