[webkit-reviews] review denied: [Bug 68891] Web Inspector: add compiler source mapping support to RawSourceCode. : [Attachment 108828] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Pavel Podivilov
<podivilov at chromium.org>'s request for review:
Bug 68891: Web Inspector: add compiler source mapping support to RawSourceCode.
https://bugs.webkit.org/show_bug.cgi?id=68891

Attachment 108828: Patch
https://bugs.webkit.org/attachment.cgi?id=108828&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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(bre
akpoint.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