[webkit-reviews] review denied: [Bug 109168] Web Inspector: Implement position-based sourcemapping for stylesheets : [Attachment 187291] Patch

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 109168: Web Inspector: Implement position-based sourcemapping for
stylesheets
https://bugs.webkit.org/show_bug.cgi?id=109168

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

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=187291&action=review


> Source/WebCore/inspector/front-end/CSSStyleModel.js:543
>	   var sourceMapping = this._sourceMappings[rawLocation.url];

We should always fallback to default mapping.
if (sourceMapping) {
    var uiLocation = sourceMapping.rawLocationToUILocation(rawLocation);
    if (uiLocation)
	return uiLocation;
}

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:170
> +	   if (sourceMap && (forceRebind ||
!this._sourceMapByStyleSheetURL[cssURL])) {

Why would we go to loadSourceMapForStyleSheet() when (!forceRebind &&
this._sourceMapByStyleSheetURL[cssURL]) holds in the first place?

if (!forceRebind && this._sourceMapByStyleSheetURL[cssURL])
    return;
var sourceMap = this.loadSourceMapForStyleSheet(match[1], cssURL, forceRebind);

if (!sourceMap)
    return;
this._sourceMapByStyleSheetURL[cssURL] = sourceMap;
this._bindUISourceCode(cssURL, sourceMap);

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:210
> +	   var payload =
WebInspector.SourceMap.loadPayload(completeSourceMapURL);

We can now rename loadPayload to SourceMap.load() and move this 4 lines there,
like

function loadSourceMap(sourceMapURL, compiledURL) {
    var payload = ... // load payload for sourceMapURL
    if (!payload)
	return null;
    var baseURL = sourceMapURL.startsWith("data:") ? compiledURL :
completeSourceMapURL;
    return new WebInspector.PositionBasedSourceMap(baseURL, payload);
}

We should also remove RangeBasedSourceMap since we don't need it anymore and
merge PositionBasedSourceMap and SourceMap.

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:254
> +	   if (!entry)

if (!entry || entry.length === 2)
    return null;

> LayoutTests/http/tests/inspector/stylesheet-source-mapping.html:21
> +    function checkMapping(compiledLineNumber, compiledColumnNumber,
sourceURL, sourceLineNumber, sourceColumnNumber, mapping)

Unused?

> LayoutTests/http/tests/inspector/stylesheet-source-mapping.html:81
> +		   InspectorTest.checkUILocation(scssUISourceCode, 0, 0,
rawLocationToUILocation(0, 0));

Should be InspectorTest.checkUILocation(cssUISourceCode, 0, 0,
rawLocationToUILocation(0, 0)), this is why it does not fail although the code
is incorrect.