[webkit-reviews] review denied: [Bug 107829] Web Inspector: support JavaScript variable mutation in protocol and V8 bindings : [Attachment 186491] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Peter Rybin
<prybin at chromium.org>'s request for review:
Bug 107829: Web Inspector: support JavaScript variable mutation in protocol and
V8 bindings
https://bugs.webkit.org/show_bug.cgi?id=107829

Attachment 186491: Patch
https://bugs.webkit.org/attachment.cgi?id=186491&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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?