RESOLVED FIXED 33592
Use SerializedScriptValue for passing values between inspector front-end and the inspected page
https://bugs.webkit.org/show_bug.cgi?id=33592
Summary Use SerializedScriptValue for passing values between inspector front-end and ...
Yury Semikhatsky
Reported 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.
Attachments
patch (66.31 KB, patch)
2010-02-05 03:40 PST, Yury Semikhatsky
pfeldman: review-
patch (66.59 KB, patch)
2010-02-05 05:32 PST, Yury Semikhatsky
pfeldman: review+
Yury Semikhatsky
Comment 1 2010-02-05 03:40:05 PST
Pavel Feldman
Comment 2 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.
Yury Semikhatsky
Comment 3 2010-02-05 05:32:52 PST
Created attachment 48226 [details] patch Addressed all the comments.
Pavel Feldman
Comment 4 2010-02-05 05:36:55 PST
Comment on attachment 48226 [details] patch Nit: I'd probably do a constructor instead of ScriptValue::deserialize.
Yury Semikhatsky
Comment 5 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.
Yury Semikhatsky
Comment 6 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
Note You need to log in before you can comment on or make changes to this bug.