Bug 107829 – Web Inspector: support JavaScript variable mutation in protocol and V8 bindings

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

[Peter Rybin]

WebInspector lacks such feature as changing local variable value.
Add this feature to protocol, backend and V8 bindings (since V8 started to support this).

[Peter Rybin]

See the sister bug for JSC:
https://bugs.webkit.org/show_bug.cgi?id=107830

[Peter Rybin]

Created attachment 184509 [details]
Patch

[Yury Semikhatsky]

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.

[Andrey Adaikin]

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

[Peter Rybin]

Created attachment 184524 [details]
Patch

[Peter Rybin]

> > 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

[Peter Rybin]

> > 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

[WebKit Review Bot]

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

[Pavel Feldman]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Build Bot]

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

[Peter Rybin]

Created attachment 186491 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Pavel Feldman]

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?

[Peter Rybin]

Created attachment 186925 [details]
Patch

[Peter Rybin]

> 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.

[Build Bot]

Comment on attachment 186925 [details]
Patch

Attachment 186925 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16396449

[WebKit Review Bot]

Comment on attachment 186925 [details]
Patch

Clearing flags on attachment: 186925

Committed r142114: <http://trac.webkit.org/changeset/142114>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.