Bug 33592 – Use SerializedScriptValue for passing values between inspector front-end and the inspected page

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Yury Semikhatsky]

Currently we serialize the values using custom JSON implementation to serialize the values(see bug 32554 for more details on why we need that custom implementation). Using SerializedScriptValue would allow to get rid of that custom implementation and would work for both in-process and out-of-process front-end since SerializedScriptValue can be converted into a String or new JS object in specified ExecState.

[Yury Semikhatsky]

Created attachment 48221 [details]
patch

[Pavel Feldman]

Comment on attachment 48221 [details]
patch

I like this a lot!

> +PassRefPtr<SerializedScriptValue> serialize(ScriptState* scriptState, const ScriptValue& value)
> +{
> +    return SerializedScriptValue::create(scriptState, value.jsValue());
> +}

Should this be defined in ScriptValue instead?

> +
> +ScriptValue deserialize(ScriptState* scriptState, SerializedScriptValue* value)
> +{
> +    return ScriptValue(value->deserialize(scriptState, scriptState->lexicalGlobalObject()));
> +}
> +

ditto.

> -    var argsArray = InjectedScript.JSON.parse(args);
> +    var argsArray = JSON.parse(args);

Overriding JSON would bring harm us here. I realize that parse is much more standard than stringify, but still. I'd suggest that you either inline copy of parse method here or simply use eval. This method is being called from the trusted source, it has only valid symbols, etc. r- for this.

[Yury Semikhatsky]

Created attachment 48226 [details]
patch

Addressed all the comments.

[Pavel Feldman]

Comment on attachment 48226 [details]
patch

Nit: I'd probably do a constructor instead of ScriptValue::deserialize.

[Yury Semikhatsky]

(In reply to comment #4)
> (From update of attachment 48226 [details])
> Nit: I'd probably do a constructor instead of ScriptValue::deserialize.

Adding a constructor would still require a static method which would still require a static method that would enter the script state scope before deserializing value so I'd rather stick with this approach.

[Yury Semikhatsky]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/JSInjectedScriptHostCustom.cpp
	M	WebCore/bindings/js/ScriptArray.cpp
	M	WebCore/bindings/js/ScriptFunctionCall.cpp
	M	WebCore/bindings/js/ScriptFunctionCall.h
	M	WebCore/bindings/js/ScriptObject.cpp
	M	WebCore/bindings/js/ScriptValue.cpp
	M	WebCore/bindings/js/ScriptValue.h
	M	WebCore/bindings/v8/ScriptArray.cpp
	M	WebCore/bindings/v8/ScriptFunctionCall.cpp
	M	WebCore/bindings/v8/ScriptFunctionCall.h
	M	WebCore/bindings/v8/ScriptObject.cpp
	M	WebCore/bindings/v8/ScriptState.h
	M	WebCore/bindings/v8/ScriptValue.cpp
	M	WebCore/bindings/v8/ScriptValue.h
	M	WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp
	M	WebCore/inspector/InjectedScript.cpp
	M	WebCore/inspector/InjectedScript.h
	M	WebCore/inspector/InjectedScriptHost.cpp
	M	WebCore/inspector/InjectedScriptHost.h
	M	WebCore/inspector/InjectedScriptHost.idl
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/front-end/InjectedScript.js
	M	WebCore/inspector/front-end/InjectedScriptAccess.js
	M	WebCore/inspector/front-end/inspector.js
	M	WebKit/qt/Api/qwebelement.cpp
	M	WebKit/qt/ChangeLog
Committed r54421