Bug 52721

Summary:

Web Inspector: [JSC] scripts have incorrect starting line (always 1)

Product:

WebKit

Reporter:

Pavel Podivilov <podivilov>

Component:

Web Inspector (Deprecated)

Assignee:

Pavel Podivilov <podivilov>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch.	yurys: review+

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+	Details Formatted Diff Diff
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.