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: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Pavel Podivilov
2011-01-19 06:44:29 PST
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> |