Web Inspector: [JSC] scripts has incorrect starting line (always 1).
Created attachment 79417 [details] Patch.
Attachment 79417 [details] did not build on mac: Build output: http://queues.webkit.org/results/7535205
Comment on attachment 79417 [details] Patch. 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
(In reply to comment #3) > (From update of attachment 79417 [details]) > 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? Not really :) > > > 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? When test is started there can already be scripts in DebuggerModel (at least for v8). > > > LayoutTests/inspector/debugger-scripts.html:46 > > +window.onload = runTest; > > Why not declare it as body onload="runTest()" like in other inspector tests? > Because JSC and v8 treat inlined handler scripts differently (there won't be parsedScript notification from v8). But it doesn't seem to be important enough to add separate expectation for chromium. > > LayoutTests/inspector/debugger-scripts.html:52 > > +Tests that valid parsed script notifications are received by front-end. > > Please add a link to the bug. Done. > > > LayoutTests/inspector/debugger-scripts.html:55 > > +</body> > > Opening <body> tag is missing, is it intentional? If so it should be documented. Fixed. > > > 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 TextPosition isn't used by v8 debugger, so we should probably keep pair of ints.
Committed r76129: <http://trac.webkit.org/changeset/76129>