Bug 107829

Summary: Web Inspector: support JavaScript variable mutation in protocol and V8 bindings
Product: WebKit Reporter: Peter Rybin <prybin>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apavlov, buildbot, dglazkov, haraken, japhet, keishi, loislo, pfeldman, pmuellr, rniwa, vsevik, web-inspector-bugs, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Peter Rybin 2013-01-24 08:14:24 PST
WebInspector lacks such feature as changing local variable value.
Add this feature to protocol, backend and V8 bindings (since V8 started to support this).
Comment 1 Peter Rybin 2013-01-24 08:17:01 PST
See the sister bug for JSC:
https://bugs.webkit.org/show_bug.cgi?id=107830
Comment 2 Peter Rybin 2013-01-24 08:30:52 PST
Created attachment 184509 [details]
Patch
Comment 3 Yury Semikhatsky 2013-01-24 09:01:26 PST
Comment on attachment 184509 [details]
Patch

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

> Source/WebCore/inspector/InjectedScriptSource.js:595
> +            setter = function(scopeIndex, variableName, newValue) {

style: { on next line

> Source/WebCore/inspector/InjectedScriptSource.js:603
> +            setter = function(scopeIndex, variableName, newValue) {

please use InjectedScriptHost.setFunctionVariableValue.bind(InjectedScriptHost,... instead

> Source/WebCore/inspector/Inspector.json:2847
> +                    { "name": "scopeNumber", "type": "integer", "description": "0-based number of scope as was listed in scope chain. Only 'local', 'closure' and 'catch' scope types are allowed. Other scopes could be manipulated manually." },

Please add a note to the changelog on why you don't want to deal with "with" scopes.

> Source/WebCore/inspector/Inspector.json:2850
> +                ],

Please hide this from the public protocol.
Comment 4 Andrey Adaikin 2013-01-24 09:08:35 PST
Comment on attachment 184509 [details]
Patch

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

> Source/WebCore/inspector/InjectedScript.cpp:119
> +void InjectedScript::setVariableValue(ErrorString* errorString, const ScriptValue& callFrames, const String* callFrameOpt, const String* functionObjectIdOpt, int scopeNumber, const String& variableName, const String& newValueStr)

callFrameOpt -> callFrameIdOpt

> Source/WebCore/inspector/InjectedScriptSource.js:585
> +     * @param {*} newValue, unmasked custom value.

@param {RuntimeAgent.CallArgument} newValue? or is it just a {string} here?

> Source/WebCore/inspector/InjectedScriptSource.js:586
> +     * @return {*}

@return {string|undefined}

> Source/WebCore/inspector/InjectedScriptSource.js:595
> +            setter = function(scopeIndex, variableName, newValue) {

IMO, inlined try-catch blocks would look much more readable. One-liner code dup saver of the "Failed..." string does nor worth it.

> Source/WebCore/inspector/Inspector.json:2845
> +                    { "name": "callFrame", "$ref": "CallFrameId", "optional": true, "description": "Id of callframe that holds variable" },

name it "callFrameId"?

> Source/WebCore/inspector/Inspector.json:2849
> +                    { "name": "newValue", "$ref": "Runtime.CallArgument", "description": "New variable value" }

dots at the end of descriptions are missing

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:587
> +        injectedScript = m_injectedScriptManager->injectedScriptForObjectId(*callFrame);

callFrame -> callFrameId
Comment 5 Peter Rybin 2013-01-24 10:33:23 PST
Created attachment 184524 [details]
Patch
Comment 6 Peter Rybin 2013-01-24 10:34:23 PST
> > Source/WebCore/inspector/InjectedScriptSource.js:595
> > +            setter = function(scopeIndex, variableName, newValue) {
> 
> style: { on next line

Done

> > Source/WebCore/inspector/InjectedScriptSource.js:603
> > +            setter = function(scopeIndex, variableName, newValue) {
> 
> please use InjectedScriptHost.setFunctionVariableValue.bind(InjectedScriptHost,... instead

Done

> > Source/WebCore/inspector/Inspector.json:2847
> > +                    { "name": "scopeNumber", "type": "integer", "description": "0-based number of scope as was listed in scope chain. Only 'local', 'closure' and 'catch' scope types are allowed. Other scopes could be manipulated manually." },
> Please add a note to the changelog on why you don't want to deal with "with" scopes.

Done

> > Source/WebCore/inspector/Inspector.json:2850
> > +                ],
> Please hide this from the public protocol.

Done
Comment 7 Peter Rybin 2013-01-24 10:35:31 PST
> > Source/WebCore/inspector/InjectedScript.cpp:119
> > +void InjectedScript::setVariableValue(ErrorString* errorString, const ScriptValue& callFrames, const String* callFrameOpt, const String* functionObjectIdOpt, int scopeNumber, const String& variableName, const String& newValueStr)
> callFrameOpt -> callFrameIdOpt

Done

> > Source/WebCore/inspector/InjectedScriptSource.js:585
> > +     * @param {*} newValue, unmasked custom value.
> @param {RuntimeAgent.CallArgument} newValue? or is it just a {string} here?

Yes, that's a bug. Thank you for noting this.
Done

> > Source/WebCore/inspector/InjectedScriptSource.js:586
> > +     * @return {*}
> @return {string|undefined}

Done

> > Source/WebCore/inspector/Inspector.json:2845
> > +                    { "name": "callFrame", "$ref": "CallFrameId", "optional": true, "description": "Id of callframe that holds variable" },
> name it "callFrameId"?

Done

> > Source/WebCore/inspector/Inspector.json:2849
> > +                    { "name": "newValue", "$ref": "Runtime.CallArgument", "description": "New variable value" }
> dots at the end of descriptions are missing

Done

> > Source/WebCore/inspector/InspectorDebuggerAgent.cpp:587
> > +        injectedScript = m_injectedScriptManager->injectedScriptForObjectId(*callFrame);
> callFrame -> callFrameId

Done
Comment 8 WebKit Review Bot 2013-01-24 18:29:00 PST
Comment on attachment 184524 [details]
Patch

Attachment 184524 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16110385

New failing tests:
inspector/console/command-line-api.html
Comment 9 Pavel Feldman 2013-01-24 22:51:27 PST
Comment on attachment 184524 [details]
Patch

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

> Source/WebCore/bindings/v8/DebuggerScript.js:99
> +    if (!mirror.isFunction()) {

nit: no need for {}

> Source/WebCore/bindings/v8/DebuggerScript.js:108
> +    if (!scopeMirror) {

ditto

> Source/WebCore/inspector/InjectedScriptSource.js:434
> +                    return String(e);

return e.toString()

> Source/WebCore/inspector/InjectedScriptSource.js:624
> +            return String(e);

ditto
Comment 10 WebKit Review Bot 2013-01-25 02:47:47 PST
Comment on attachment 184524 [details]
Patch

Attachment 184524 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16122225

New failing tests:
inspector/console/command-line-api.html
Comment 11 Build Bot 2013-01-25 03:15:04 PST
Comment on attachment 184524 [details]
Patch

Attachment 184524 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16115013

New failing tests:
svg/text/text-rect-precision.html
inspector/console/command-line-api.html
Comment 12 Build Bot 2013-01-25 05:53:36 PST
Comment on attachment 184524 [details]
Patch

Attachment 184524 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16124329

New failing tests:
inspector/console/command-line-api.html
Comment 13 Peter Rybin 2013-02-04 16:17:33 PST
Created attachment 186491 [details]
Patch
Comment 14 Build Bot 2013-02-04 19:17:28 PST
Comment on attachment 186491 [details]
Patch

Attachment 186491 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16376444

New failing tests:
inspector-protocol/debugger-setVariableValue.html
Comment 15 Build Bot 2013-02-04 20:55:26 PST
Comment on attachment 186491 [details]
Patch

Attachment 186491 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16374510

New failing tests:
inspector-protocol/debugger-setVariableValue.html
Comment 16 Pavel Feldman 2013-02-05 02:02:05 PST
Comment on attachment 186491 [details]
Patch

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

Mac EWS claims inspector-protocol/debugger-setVariableValue.html [ Failure ] is failing. Otherwise looks good.

> Source/WebCore/bindings/v8/DebuggerScript.js:99
> +    if (!mirror.isFunction()) {

no {} around single line blocks.

> Source/WebCore/bindings/v8/DebuggerScript.js:108
> +    if (!scopeMirror) {

ditto

> Source/WebCore/bindings/v8/JavaScriptCallFrame.cpp:142
> +        deprecatedV8String(variableName),

What is deprecatedV8String ?

> Source/WebCore/inspector/InjectedScript.cpp:146
> +    // Normal return.

WebKit discourages comments.

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:602
> +    String newValueStr = newValue->toJSONString();

netValueString (no abbreviations in WebKit). Why don't you pass it as RefPtr?
Comment 17 Peter Rybin 2013-02-06 14:47:22 PST
Created attachment 186925 [details]
Patch
Comment 18 Peter Rybin 2013-02-06 14:49:30 PST
> Mac EWS claims inspector-protocol/debugger-setVariableValue.html [ Failure ] is failing. Otherwise looks good.
Fixed

> > Source/WebCore/bindings/v8/DebuggerScript.js:99
> > +    if (!mirror.isFunction()) {
> no {} around single line blocks.
Done

> > Source/WebCore/bindings/v8/DebuggerScript.js:108
> > +    if (!scopeMirror) {
> ditto
Done

> > Source/WebCore/bindings/v8/JavaScriptCallFrame.cpp:142
> > +        deprecatedV8String(variableName),
> What is deprecatedV8String ?
I copy-pasted code that was under multi-step refactoring.

> > Source/WebCore/inspector/InspectorDebuggerAgent.cpp:602
> > +    String newValueStr = newValue->toJSONString();
> netValueString (no abbreviations in WebKit). Why don't you pass it as RefPtr?
Renamed. I did like similar command callFunctionOn is implemented.
Comment 19 Build Bot 2013-02-06 17:46:10 PST
Comment on attachment 186925 [details]
Patch

Attachment 186925 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16396449
Comment 20 WebKit Review Bot 2013-02-07 07:01:50 PST
Comment on attachment 186925 [details]
Patch

Clearing flags on attachment: 186925

Committed r142114: <http://trac.webkit.org/changeset/142114>
Comment 21 WebKit Review Bot 2013-02-07 07:01:56 PST
All reviewed patches have been landed.  Closing bug.