Bug 109168 – Web Inspector: Implement position-based sourcemapping for stylesheets

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

[Alexander Pavlov (apavlov)]

Patch to follow.

[Alexander Pavlov (apavlov)]

Created attachment 187051 [details]
Patch

[Build Bot]

Comment on attachment 187051 [details]
Patch

Attachment 187051 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16426170

New failing tests:
http/tests/inspector/stylesheet-source-mapping.html
http/tests/inspector/compiler-script-mapping.html

[Build Bot]

Comment on attachment 187051 [details]
Patch

Attachment 187051 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16432194

New failing tests:
http/tests/inspector/stylesheet-source-mapping.html
http/tests/inspector/compiler-script-mapping.html

[Build Bot]

Comment on attachment 187051 [details]
Patch

Attachment 187051 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16427217

[Build Bot]

Comment on attachment 187051 [details]
Patch

Attachment 187051 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16415378

New failing tests:
http/tests/inspector/stylesheet-source-mapping.html
http/tests/inspector/compiler-script-mapping.html

[WebKit Review Bot]

Comment on attachment 187051 [details]
Patch

Attachment 187051 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16431326

New failing tests:
http/tests/inspector/stylesheet-source-mapping.html
http/tests/inspector/compiler-script-mapping.html

[Alexander Pavlov (apavlov)]

Created attachment 187281 [details]
For EWS

[WebKit Review Bot]

Comment on attachment 187281 [details]
For EWS

Attachment 187281 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16427593

New failing tests:
http/tests/inspector/stylesheet-source-mapping.html

[Alexander Pavlov (apavlov)]

Created attachment 187291 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 187291 [details]
Patch

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.

[Alexander Pavlov (apavlov)]

Created attachment 187528 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 187528 [details]
Patch

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

> Source/WebCore/inspector/front-end/SASSSourceMapping.js:218
> +        this._sourceMapByURL[completeSourceMapURL] = sourceMap;

This line should probably be before previous return (in case we already had a sourceMap but forceReload flag was set)

[Alexander Pavlov (apavlov)]

Committed r142445: <http://trac.webkit.org/changeset/142445>