[webkit-reviews] review denied: [Bug 78503] Web Inspector: [InspectorIndexedDB] Pass data entries from object stores and indexes to front-end. : [Attachment 126790] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 78503: Web Inspector: [InspectorIndexedDB] Pass data entries from object
stores and indexes to front-end.
https://bugs.webkit.org/show_bug.cgi?id=78503

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

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126790&action=review


> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1456
> +ScriptValue SerializedScriptValue::deserializeForInspector(ScriptState*
scriptState)

We may guard it with #if ENABLE(INSPECTOR)

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1458
> +    JSValue value = deserialize(scriptState,
scriptState->dynamicGlobalObject(), 0);

It might be safer to use lexicalGlobalObject() since it would work even if
there is no js running in the script state.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2263
> +    V8LocalContext localContext;

This will create a new context. You should take one from the ScriptState
instead and enter it here instead. r- for this.

> Source/WebCore/inspector/Inspector.json:980
> +		       { "name": "type", "type": "number", "description": "Key
type." },

This one should be enum.

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:333
> +	   break;

We should report an error here if the type is invalid.

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:340
> +    RefPtr<InspectorObject> lower = keyRange->getObject("lower");

Could we reuse generated classes for parsing protocol messages? They seem to be
already capable of validating InspectorValues and performing runtimeCast on
them.