Bug 33592 - Use SerializedScriptValue for passing values between inspector front-end and the inspected page
Summary: Use SerializedScriptValue for passing values between inspector front-end and ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on: 32920
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-13 05:13 PST by Yury Semikhatsky
Modified: 2010-02-05 06:08 PST (History)
2 users (show)

See Also:


Attachments
patch (66.31 KB, patch)
2010-02-05 03:40 PST, Yury Semikhatsky
pfeldman: review-
Details | Formatted Diff | Diff
patch (66.59 KB, patch)
2010-02-05 05:32 PST, Yury Semikhatsky
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2010-01-13 05:13:18 PST
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.
Comment 1 Yury Semikhatsky 2010-02-05 03:40:05 PST
Created attachment 48221 [details]
patch
Comment 2 Pavel Feldman 2010-02-05 03:59:01 PST
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.
Comment 3 Yury Semikhatsky 2010-02-05 05:32:52 PST
Created attachment 48226 [details]
patch

Addressed all the comments.
Comment 4 Pavel Feldman 2010-02-05 05:36:55 PST
Comment on attachment 48226 [details]
patch

Nit: I'd probably do a constructor instead of ScriptValue::deserialize.
Comment 5 Yury Semikhatsky 2010-02-05 06:06:40 PST
(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.
Comment 6 Yury Semikhatsky 2010-02-05 06:08:56 PST
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