[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