[Webkit-unassigned] [Bug 86861] Web Inspector: Expose function (closure) scopes in remote protocol

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=86861


Yury Semikhatsky <yurys at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #143441|review?                     |review-
               Flag|                            |




--- Comment #9 from Yury Semikhatsky <yurys at chromium.org>  2012-05-23 05:27:11 PST ---
(From update of attachment 143441)
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 {}

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.