Bug 86099 - Web Inspector: pass source mapping into UISourceCode's constructor; move formatting outside mapping.
Summary: Web Inspector: pass source mapping into UISourceCode's constructor; move form...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on: 86337
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-10 07:33 PDT by Pavel Feldman
Modified: 2012-05-13 23:35 PDT (History)
12 users (show)

See Also:


Attachments
Patch (82.07 KB, patch)
2012-05-10 07:38 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (83.28 KB, patch)
2012-05-11 06:43 PDT, Pavel Feldman
vsevik: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2012-05-10 07:33:48 PDT
This changes moves formatting out of the source mapping machinery.
Comment 1 Pavel Feldman 2012-05-10 07:38:57 PDT
Created attachment 141169 [details]
Patch
Comment 2 Pavel Podivilov 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.
Comment 3 Pavel Feldman 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!
Comment 4 Pavel Feldman 2012-05-11 06:41:32 PDT
> 
> I don't think it does. I'll double check.

It does. Fixed.
Comment 5 Pavel Feldman 2012-05-11 06:43:07 PDT
Created attachment 141401 [details]
Patch
Comment 6 Pavel Podivilov 2012-05-11 07:12:42 PDT
looks good!
Comment 7 Vsevolod Vlasov 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.
Comment 8 Pavel Feldman 2012-05-11 09:13:14 PDT
Committed r116775: <http://trac.webkit.org/changeset/116775>
Comment 9 Csaba Osztrogonác 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?
Comment 10 Csaba Osztrogonác 2012-05-13 23:35:17 PDT
New bug report filed on it - https://bugs.webkit.org/show_bug.cgi?id=86337