Bug 68891 - Web Inspector: add compiler source mapping support to RawSourceCode.
Summary: Web Inspector: add compiler source mapping support to RawSourceCode.
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 Podivilov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-27 05:12 PDT by Pavel Podivilov
Modified: 2011-10-04 05:32 PDT (History)
10 users (show)

See Also:


Attachments
Patch (22.68 KB, patch)
2011-09-27 05:14 PDT, Pavel Podivilov
no flags Details | Formatted Diff | Diff
Patch (29.42 KB, patch)
2011-09-29 02:10 PDT, Pavel Podivilov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Podivilov 2011-09-27 05:12:13 PDT
Web Inspector: add compiler source mapping support to RawSourceCode.
Comment 1 Pavel Podivilov 2011-09-27 05:14:46 PDT
Created attachment 108828 [details]
Patch
Comment 2 Pavel Feldman 2011-09-28 22:33:24 PDT
Comment on attachment 108828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108828&action=review

> LayoutTests/inspector/debugger/raw-source-code.html:365
> +                                if (lineNumber < 10)

6 levels of nesting is too much even for the test. Please consider extracting these to the top level.

> Source/WebCore/inspector/front-end/BreakpointManager.js:98
> +        var rawLocation = breakpoint.uiSourceCode.rawSourceCode.sourceMapping.uiLocationToRawLocation(breakpoint.uiSourceCode, breakpoint.lineNumber);

This breaks law of Demeter violently. You go 4 levels deep and pass reference to self to the call? This sounds like a candidate for UISourceCode.prototype.toRawLocation. It was already too much, but now it is just over the limit.

> Source/WebCore/inspector/front-end/SourceFile.js:282
> +WebInspector.RawSourceCode.CompilerSourceMapping.prototype = {

If sounds like you should have extracted RawSourceCode into its own class prior to introducing nested classes.

> Source/WebCore/inspector/front-end/SourceFile.js:318
> +WebInspector.CompilerSourceMappingProvider = function()

ditto

> Source/WebCore/inspector/front-end/SourceFile.js:454
> +WebInspector.CompilerSourceMappingContentProvider = function(sourceURL, compilerSourceMappingProvider)

ditto
Comment 3 Pavel Podivilov 2011-09-29 02:08:49 PDT
(In reply to comment #2)
> (From update of attachment 108828 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108828&action=review
> 
> > LayoutTests/inspector/debugger/raw-source-code.html:365
> > +                                if (lineNumber < 10)
> 
> 6 levels of nesting is too much even for the test. Please consider extracting these to the top level.

Done.

> 
> > Source/WebCore/inspector/front-end/BreakpointManager.js:98
> > +        var rawLocation = breakpoint.uiSourceCode.rawSourceCode.sourceMapping.uiLocationToRawLocation(breakpoint.uiSourceCode, breakpoint.lineNumber);
> 
> This breaks law of Demeter violently. You go 4 levels deep and pass reference to self to the call? This sounds like a candidate for UISourceCode.prototype.toRawLocation. It was already too much, but now it is just over the limit.

UISourceCode is an interface for ScriptsPanel, it should not mention rawLocations at all. UISourceCode::rawSourceCode is just a shortcut used by presentation model, it's not an actual link - UISourceCode and its clients should never use it. It could (and probably should) be replaced by UISourceCode->RawSourceCode mapping in the model to make things clear.


> 
> > Source/WebCore/inspector/front-end/SourceFile.js:282
> > +WebInspector.RawSourceCode.CompilerSourceMapping.prototype = {
> 
> If sounds like you should have extracted RawSourceCode into its own class prior to introducing nested classes.
> 
> > Source/WebCore/inspector/front-end/SourceFile.js:318
> > +WebInspector.CompilerSourceMappingProvider = function()
> 
> ditto
> 
> > Source/WebCore/inspector/front-end/SourceFile.js:454
> > +WebInspector.CompilerSourceMappingContentProvider = function(sourceURL, compilerSourceMappingProvider)
> 
> ditto

Done.
Comment 4 Pavel Podivilov 2011-09-29 02:10:37 PDT
Created attachment 109138 [details]
Patch
Comment 5 Pavel Feldman 2011-09-30 04:46:50 PDT
Comment on attachment 109138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109138&action=review

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:285
> +        var rawLocation = uiSourceCode.rawSourceCode.sourceMapping.uiLocationToRawLocation(uiSourceCode, lineNumber);

Please put FIXME suggesting doing uiSourceCode.toRawLocation().
Comment 6 Pavel Podivilov 2011-10-04 05:32:35 PDT
Committed r96585: <http://trac.webkit.org/changeset/96585>