Bug 68891 – Web Inspector: add compiler source mapping support to RawSourceCode.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Podivilov]

Web Inspector: add compiler source mapping support to RawSourceCode.

[Pavel Podivilov]

Created attachment 108828 [details]
Patch

[Pavel Feldman]

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

[Pavel Podivilov]

(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.

[Pavel Podivilov]

Created attachment 109138 [details]
Patch

[Pavel Feldman]

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().

[Pavel Podivilov]

Committed r96585: <http://trac.webkit.org/changeset/96585>