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

[bugzilla-daemon at webkit.org]

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 143441: Patch
https://bugs.webkit.org/attachment.cgi?id=143441&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
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 {}