Bug 86861 – Web Inspector: Expose function (closure) scopes in remote protocol

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

[Peter Rybin]

Each function in JavaScript is a closure. However currently remote protocol doesn't let see the internal scope of function. It only gets visible when function is activated (as a call frame variables).
Since the variables are always there, make them accessible via FunctionDetails structure.

[Peter Rybin]

Created attachment 143084 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 143084 [details]
Patch

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

[Pavel Feldman]

Comment on attachment 143084 [details]
Patch

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

> Source/WebCore/bindings/v8/DebuggerScript.js:70
> +        return void 0;

we don't use such notation, simply return here instead.

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

You don't need this annotation in the header under bindings/v8. You should also keep this method private to the debug server for the encapsulation purposes.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:191
> +    v8::Handle<v8::Value> scopes = debugServer.callDebuggerMethod("getFunctionScopes", 1, argv);

Should be debugServer.functionScopes(blah).

> Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:193
> +        result->Set(v8::String::New("scopes_raw"), scopes);

rawScopes ?

> Source/WebCore/inspector/InjectedScriptHost.cpp:188
> +    return m_debuggerAgent->scriptDebugServer();

You should be careful about accessing debugServer before debug domain is enabled and scripts are sent to the front-end. This won't work.

[Yury Semikhatsky]

Comment on attachment 143084 [details]
Patch

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

>> Source/WebCore/bindings/v8/DebuggerScript.js:70
>> +        return void 0;
> 
> we don't use such notation, simply return here instead.

return null would be more consistent with the rest code as this function is expected to return a value.

[Peter Rybin]

(In reply to comment #2)
> (From update of attachment 143084 [details])
> 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

function-details is extended. Will be a problem with JSC builds? Do we do any kind of conditioning for V8/JSC?
 
> > 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.

You said we have polymorphic part and V8-specific part. Should we keep it clear? Anyway, the method is redone now.

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

I made a bug and added FIXME. I haven't investigated JSC yet on this.

> > Source/WebCore/inspector/InjectedScriptHost.cpp:186
> > +ScriptDebugServer& InjectedScriptHost::getScriptDebugServer()
> We don't use get prefixes in WebKit, simply scriptDebugServer()

Done

> > Source/WebCore/inspector/InjectedScriptSource.js:215
> > +                // FIXME: fix group id
> Group id should be passed as a parameter of the protocol command.
The comment was obsolete. Sorry about this. I inherit group id from the function id (similar to how getProperties works I believe).

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

Old approach restored.

> > Source/WebCore/inspector/InjectedScriptSource.js:559
> > +    return function(scope_type_code, scope_object, group_id) {
> style: use camelCase for variable names
Done

[Peter Rybin]

> > Source/WebCore/bindings/v8/DebuggerScript.js:70
> > +        return void 0;
> we don't use such notation, simply return here instead.
Done

> > Source/WebCore/bindings/v8/ScriptDebugServer.h:117
> > +// V8-specific interface section.
> You don't need this annotation in the header under bindings/v8. You should also keep this method private to the debug server for the encapsulation purposes.

Yury said you have cross-engine polymorphic interface part and the engine-specific part. Wanted to keep it clear.
Removed now.

> > Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:191
> > +    v8::Handle<v8::Value> scopes = debugServer.callDebuggerMethod("getFunctionScopes", 1, argv);
> Should be debugServer.functionScopes(blah).
Done

> > Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:193
> > +        result->Set(v8::String::New("scopes_raw"), scopes);
> rawScopes ?
Done

> > Source/WebCore/inspector/InjectedScriptHost.cpp:188
> > +    return m_debuggerAgent->scriptDebugServer();
> You should be careful about accessing debugServer before debug domain is enabled and scripts are sent to the front-end. This won't work.

This is still and issue.

[Peter Rybin]

(In reply to comment #4)
> (From update of attachment 143084 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143084&action=review
> 
> >> Source/WebCore/bindings/v8/DebuggerScript.js:70
> >> +        return void 0;
> > 
> > we don't use such notation, simply return here instead.
> 
> return null would be more consistent with the rest code as this function is expected to return a value.

Done

[Peter Rybin]

Created attachment 143441 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 143441 [details]
Patch

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

> Source/WebCore/bindings/v8/DebuggerScript.js:69
> +    if (count == 0) {

style nit: no need for {} around 1-line statement

> Source/WebCore/bindings/v8/DebuggerScript.js:297
> +    case ScopeType.Catch:

You will need to rebaseline after http://trac.webkit.org/changeset/118161

> Source/WebCore/bindings/v8/ScriptDebugServer.cpp:411
> +

Please revert this.

> Source/WebCore/inspector/InjectedScriptHost.cpp:188
> +    // FIX BEFORE COMMIT: (pfeldman) You should be careful about accessing debugServer before debug domain is enabled and scripts are sent to the front-end. This won't work.

r- for this.

> Source/WebCore/inspector/InjectedScriptHost.h:52
> +class ScriptDebugServer;

Mind sorting order.

> Source/WebCore/inspector/InjectedScriptHost.h:126
> +    InspectorDebuggerAgent* m_debuggerAgent;

It should be guarded with #if ENABLE(JAVASCRIPT_DEBUGGER)

> Source/WebCore/inspector/InjectedScriptSource.js:211
> +            var scopes_raw = details.rawScopes;

style nit: scopes_raw -> scopesRaw

> Source/WebCore/inspector/InjectedScriptSource.js:214
> +            for (var i = 0; i < scopes_raw.length; i++) {

style nit: no need for {}

> LayoutTests/http/tests/inspector/debugger-test.js:63
> +        if (!caseName) {

style nit: no need for {}

> LayoutTests/http/tests/inspector/inspector-test.js:248
> +        var caseName = nextTest.testCaseName;

Please revert this and adhere to the style used in the existing inspector tests. Instead of calling 
  createStandardTestCase("firstLineFunction", "testGetFirstLineFunctionDetails")
below you could do something like 

InspectorTest.runDebuggerTestSuite([
    function testGetFirstLineFunctionDetails(next)
    {
        dumpScopesForExpression("firstLineFunction", next);
    },
...
]);

> LayoutTests/inspector/debugger/function-details-expected.txt:9
> +columnNumber: 36

JSC doesn't provide column number, you should update this file with new JSC results and LayoutTests/platform/chromium/inspector/debugger/function-details-expected.txt with new Chromium test results.

> LayoutTests/inspector/debugger/function-details.html:68
> +            var var_array = [];

style: camelCase for variable names

> LayoutTests/inspector/debugger/function-details.html:70
> +                d = propertyDescriptors[i];

var d = ...

> LayoutTests/inspector/debugger/function-details.html:73
> +                    if (d.value.value) {

style nit: no need for {}

[Peter Rybin]

Created attachment 143902 [details]
All but excpected file for JSC

[WebKit Review Bot]

Comment on attachment 143902 [details]
All but excpected file for JSC

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

New failing tests:
inspector/debugger/function-details.html

[WebKit Review Bot]

Created attachment 143932 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Yury Semikhatsky]

Comment on attachment 143902 [details]
All but excpected file for JSC

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

> Source/WebCore/inspector/InjectedScriptHost.h:131
> +#if ENABLE(JAVASCRIPT_DEBUGGER)

Wrong field is guarded.

> LayoutTests/inspector/debugger/function-details.html:127
> +            return Function.prototype.toString.call(this).replace("UnnamedTestCase", name);

I still don't understand the reason for this complication while we can follow the same style that is used in other inspector tests.

[Peter Rybin]

Created attachment 144211 [details]
Patch

[WebKit Review Bot]

Comment on attachment 144211 [details]
Patch

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

New failing tests:
inspector/debugger/function-details.html

[WebKit Review Bot]

Created attachment 144212 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 144211 [details]
Patch

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

New failing tests:
inspector/debugger/function-details.html

[WebKit Review Bot]

Created attachment 144213 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Peter Rybin]

Created attachment 144246 [details]
with sorted properties

[WebKit Review Bot]

Comment on attachment 144246 [details]
with sorted properties

Rejecting attachment 144246 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
commit-queue/Source/WebKit/chromium/third_party/v8-i18n --revision 104 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
47>At revision 104.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/12845138

[Peter Rybin]

Created attachment 144352 [details]
rebase

[WebKit Review Bot]

Comment on attachment 144352 [details]
rebase

Clearing flags on attachment: 144352

Committed r118685: <http://trac.webkit.org/changeset/118685>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

(In reply to comment #22)
> (From update of attachment 144352 [details])
> Clearing flags on attachment: 144352
> 
> Committed r118685: <http://trac.webkit.org/changeset/118685>

This change broke compilation of InjectedScriptSource.js:


Compiling InjectedScriptSource.js...
Source/WebCore/inspector/InjectedScriptSource.js:32: WARNING - Suspicious code. This code lacks side-effects. Is there a bug?
(function (InjectedScriptHost, inspectedWindow, injectedScriptId) {
 ^

Source/WebCore/inspector/InjectedScriptSource.js:203: WARNING - Property rawScopes never defined on details
            var rawScopes = details.rawScopes;
                            ^

Source/WebCore/inspector/InjectedScriptSource.js:205: WARNING - Property rawScopes never defined on details
            delete details.rawScopes;
                   ^

0 error(s), 3 warning(s), 66.0% typed