[webkit-reviews] review denied: [Bug 86861] Web Inspector: Expose function (closure) scopes in remote protocol : [Attachment 143084] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon May 21 23:01:28 PDT 2012


Yury Semikhatsky <yurys at chromium.org> has denied Peter Rybin
<prybin at chromium.org>'s request for review:
Bug 86861: Web Inspector: Expose function (closure) scopes in remote protocol
https://bugs.webkit.org/show_bug.cgi?id=86861

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

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=143084&action=review


We will need a test for this change. See
LayoutTests/inspector/debugger/function-details.html

> Source/WebCore/bindings/v8/ScriptDebugServer.h:117
> +// V8-specific interface section.

Just move the method to the public section above. The comment should be more
detailed, otherwise it is confusing as there are other v8-specific methods
already. I'd just drop the comment.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:188
> +    InjectedScriptHost* host =
V8InjectedScriptHost::toNative(args.Holder());

What would it take to support this for JSC as well? If you can't implement it
for JSC, please file a bug and put FIXME with the bug # in the corresponding JS
bindings.

> Source/WebCore/inspector/InjectedScriptHost.cpp:186
> +ScriptDebugServer& InjectedScriptHost::getScriptDebugServer()

We don't use get prefixes in WebKit, simply scriptDebugServer()

> Source/WebCore/inspector/InjectedScriptSource.js:215
> +		   // FIXME: fix group id

Group id should be passed as a parameter of the protocol command.

> Source/WebCore/inspector/InjectedScriptSource.js:545
> +InjectedScript.CallFrameProxy._createScopeJson = (function() {

No need to use extra closure here. If you are concerned about the performance
you may move the types map to InjectedScript.CallFrameProxy.ScopeTypeNames.

> Source/WebCore/inspector/InjectedScriptSource.js:559
> +    return function(scope_type_code, scope_object, group_id) {

style: use camelCase for variable names


More information about the webkit-reviews mailing list