RESOLVED FIXED 52721
Web Inspector: [JSC] scripts have incorrect starting line (always 1) 
https://bugs.webkit.org/show_bug.cgi?id=52721
Summary Web Inspector: [JSC] scripts have incorrect starting line (always 1)
Pavel Podivilov
Reported 2011-01-19 06:44:29 PST
Web Inspector: [JSC] scripts has incorrect starting line (always 1).
Attachments
Patch. (18.06 KB, patch)
2011-01-19 06:45 PST, Pavel Podivilov 		yurys: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Pavel Podivilov
Comment 1 2011-01-19 06:45:59 PST
Created attachment 79417 [details] Patch.
WebKit Review Bot
Comment 2 2011-01-19 07:32:49 PST
Attachment 79417 [details] did not build on mac: Build output: http://queues.webkit.org/results/7535205
Yury Semikhatsky
Comment 3 2011-01-19 08:07:31 PST
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
Pavel Podivilov
Comment 4 2011-01-19 08:57:47 PST
(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.
Pavel Podivilov
Comment 5 2011-01-19 09:01:45 PST
Committed r76129: <http://trac.webkit.org/changeset/76129>
Note You need to log in before you can comment on or make changes to this bug.