Bug 86099

Summary: Web Inspector: pass source mapping into UISourceCode's constructor; move formatting outside mapping.
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, ossy, pfeldman, pmuellr, podivilov, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 86337    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch vsevik: review+

Pavel Feldman
Reported 2012-05-10 07:33:48 PDT
This changes moves formatting out of the source mapping machinery.
Attachments
Patch (82.07 KB, patch)
2012-05-10 07:38 PDT, Pavel Feldman 		no flags
DetailsFormatted DiffDiff
Patch (83.28 KB, patch)
2012-05-11 06:43 PDT, Pavel Feldman 		vsevik: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Pavel Feldman
Comment 1 2012-05-10 07:38:57 PDT
Created attachment 141169 [details] Patch
Pavel Podivilov
Comment 2 2012-05-11 03:36:10 PDT
Comment on attachment 141169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141169&action=review > Source/WebCore/inspector/front-end/BreakpointManager.js:70 > + if (breakpoint._breakpointSourceCodeStorageId !== sourceFileId) _sourceCodeStorageId? > Source/WebCore/inspector/front-end/BreakpointManager.js:347 > + var rawLocation = this._primaryUILocation.uiSourceCode.uiLocationToRawLocation(this._primaryUILocation.lineNumber, 0); Looks like a UILocation's method. > Source/WebCore/inspector/front-end/CallStackSidebarPane.js:150 > + this._statusMessageElement.appendChild(status); Shouldn't you remove this._statusMessageElement's children? > Source/WebCore/inspector/front-end/JavaScriptSource.js:51 > + WebInspector.debuggerPresentationModel.breakpointManager.restoreBreakpoints(this); Maybe move this call to ScriptsPanel? > Source/WebCore/inspector/front-end/JavaScriptSource.js:145 > + this._togglingFormatter = true; Maybe include this info in event? > Source/WebCore/inspector/front-end/RawSourceCode.js:118 > + return WebInspector.debuggerModel.createRawLocationByURL(this.url, lineNumber, columnNumber); Wouldn't that broke breakpoints in anonymous scripts? > Source/WebCore/inspector/front-end/ResourceScriptMapping.js:131 > var rawSourceCode = /** @type {WebInspector.RawSourceCode} */ event.target; Not used.
Pavel Feldman
Comment 3 2012-05-11 06:28:52 PDT
Comment on attachment 141169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141169&action=review >> Source/WebCore/inspector/front-end/BreakpointManager.js:70 >> + if (breakpoint._breakpointSourceCodeStorageId !== sourceFileId) > > _sourceCodeStorageId? Done. >> Source/WebCore/inspector/front-end/BreakpointManager.js:347 >> + var rawLocation = this._primaryUILocation.uiSourceCode.uiLocationToRawLocation(this._primaryUILocation.lineNumber, 0); > > Looks like a UILocation's method. Done. >> Source/WebCore/inspector/front-end/CallStackSidebarPane.js:150 >> + this._statusMessageElement.appendChild(status); > > Shouldn't you remove this._statusMessageElement's children? Good catch. Done. >> Source/WebCore/inspector/front-end/JavaScriptSource.js:51 >> + WebInspector.debuggerPresentationModel.breakpointManager.restoreBreakpoints(this); > > Maybe move this call to ScriptsPanel? I would need to copy it in the tests then. >> Source/WebCore/inspector/front-end/JavaScriptSource.js:145 >> + this._togglingFormatter = true; > > Maybe include this info in event? I'll re-implement this piece as we agreed offline. >> Source/WebCore/inspector/front-end/RawSourceCode.js:118 >> + return WebInspector.debuggerModel.createRawLocationByURL(this.url, lineNumber, columnNumber); > > Wouldn't that broke breakpoints in anonymous scripts? I don't think it does. I'll double check. >> Source/WebCore/inspector/front-end/ResourceScriptMapping.js:131 >> var rawSourceCode = /** @type {WebInspector.RawSourceCode} */ event.target; > > Not used. It is!
Pavel Feldman
Comment 4 2012-05-11 06:41:32 PDT
> > I don't think it does. I'll double check. It does. Fixed.
Pavel Feldman
Comment 5 2012-05-11 06:43:07 PDT
Created attachment 141401 [details] Patch
Pavel Podivilov
Comment 6 2012-05-11 07:12:42 PDT
looks good!
Vsevolod Vlasov
Comment 7 2012-05-11 07:25:42 PDT
Comment on attachment 141401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141401&action=review > Source/WebCore/inspector/front-end/ScriptsPanel.js:520 > + this._editorContainer.uiSourceCodeAdded(uiLocation.uiSourceCode); Please keep the old behaviour as discussed offline.
Pavel Feldman
Comment 8 2012-05-11 09:13:14 PDT
Committed r116775: <http://trac.webkit.org/changeset/116775>
Csaba Osztrogonác
Comment 9 2012-05-12 12:17:15 PDT
(In reply to comment #8) > Committed r116775: <http://trac.webkit.org/changeset/116775> It made a test fail on Qt: --- /ramdisk/qt-linux-64-release/build/layout-test-results/inspector/debugger/script-formatter-breakpoints-expected.txt +++ /ramdisk/qt-linux-64-release/build/layout-test-results/inspector/debugger/script-formatter-breakpoints-actual.txt @@ -14,10 +14,14 @@ script-formatter-breakpoints.html:18 var f = 0; + script-formatter-breakpoints.html:25 + var sourceFrame; Breakpoint sidebar pane while paused in raw script-formatter-breakpoints.html:11 var f=0; + script-formatter-breakpoints.html:18 + var sourceFrame; Script execution resumed. Debugger was disabled. Could you check it, please?
Csaba Osztrogonác
Comment 10 2012-05-13 23:35:17 PDT
New bug report filed on it - https://bugs.webkit.org/show_bug.cgi?id=86337
Note You need to log in before you can comment on or make changes to this bug.