Bug 109168 - Web Inspector: Implement position-based sourcemapping for stylesheets
Summary: Web Inspector: Implement position-based sourcemapping for stylesheets
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: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks: 105285
  Show dependency treegraph
 
Reported: 2013-02-07 03:44 PST by Alexander Pavlov (apavlov)
Modified: 2013-02-11 03:07 PST (History)
12 users (show)

See Also:


Attachments
Patch (25.67 KB, patch)
2013-02-07 04:03 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
For EWS (25.86 KB, patch)
2013-02-08 03:39 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (27.31 KB, patch)
2013-02-08 04:46 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (48.10 KB, patch)
2013-02-11 01:57 PST, Alexander Pavlov (apavlov)
vsevik: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2013-02-07 03:44:59 PST
Patch to follow.
Comment 1 Alexander Pavlov (apavlov) 2013-02-07 04:03:06 PST
Created attachment 187051 [details]
Patch
Comment 2 Build Bot 2013-02-07 06:42:56 PST
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
Comment 3 Build Bot 2013-02-07 07:12:34 PST
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
Comment 4 Build Bot 2013-02-07 09:57:18 PST
Comment on attachment 187051 [details]
Patch

Attachment 187051 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16427217
Comment 5 Build Bot 2013-02-07 10:17:12 PST
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
Comment 6 WebKit Review Bot 2013-02-07 14:48:22 PST
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
Comment 7 Alexander Pavlov (apavlov) 2013-02-08 03:39:38 PST
Created attachment 187281 [details]
For EWS
Comment 8 WebKit Review Bot 2013-02-08 04:30:24 PST
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
Comment 9 Alexander Pavlov (apavlov) 2013-02-08 04:46:55 PST
Created attachment 187291 [details]
Patch
Comment 10 Vsevolod Vlasov 2013-02-08 06:50:55 PST
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.
Comment 11 Alexander Pavlov (apavlov) 2013-02-11 01:57:22 PST
Created attachment 187528 [details]
Patch
Comment 12 Vsevolod Vlasov 2013-02-11 02:55:41 PST
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)
Comment 13 Alexander Pavlov (apavlov) 2013-02-11 03:07:26 PST
Committed r142445: <http://trac.webkit.org/changeset/142445>