[webkit-reviews] review granted: [Bug 52721] Web Inspector: [JSC] scripts have incorrect starting line (always 1) : [Attachment 79417] Patch.

Wed Jan 19 08:07:30 PST 2011

Yury Semikhatsky <yurys at chromium.org> has granted Pavel Podivilov
<podivilov at chromium.org>'s request for review:
Bug 52721: Web Inspector: [JSC] scripts have incorrect starting line (always 1)
https://bugs.webkit.org/show_bug.cgi?id=52721

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

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

> LayoutTests/inspector/debugger-scripts.html:6
> +<script>// First script

Is the comment necessary?

> LayoutTests/inspector/debugger-scripts.html:15
> +	   WebInspector.debuggerModel.queryScripts(function(script) { step2({
data: script.sourceID }) });

Why do you need to call step2 from 2 places, wouldn't adding it as
ParsedScriptSource listener be enough?

> LayoutTests/inspector/debugger-scripts.html:46
> +window.onload = runTest;

Why not declare it as body onload="runTest()" like in other inspector tests?

> LayoutTests/inspector/debugger-scripts.html:52
> +Tests that valid parsed script notifications are received by front-end.

Please add a link to the bug.

> LayoutTests/inspector/debugger-scripts.html:55
> +</body>

Opening <body> tag is missing, is it intentional? If so it should be
documented.

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:296
> +	   copy[i]->didParseSource(sourceID, url, data, lineOffset,
columnOffset, worldType);

We should change listeners API to pass TextPostion instead of line+column
separately