Web Inspector: Backend should detect sourceMappingURLs in CSS Resources

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

[Joseph Pecoraro]

The backend already detects sourceMappingURLs in Script evaluations. It should do the same for CSS Stylesheet resources. Detecting sourceMappingURL in Stylesheets, the frontend doesn't need to automatically request and parse the contents of every CSS file loaded by the page.

Note, this doesn't handle <style> blocks, just Stylesheets resources.

[Joseph Pecoraro]

Created attachment 198808 [details]
[PATCH] Stylesheet sourceMapURL proposed fix

I decided to add an ExtraResourceInfo object which could include all kinds of different optional info after the backend has processed a resource. Right now there is just this sourceMapURL field, but I could imagine more in the future. Since there are multiple paths that would want the same info I made an object they can share.

[WebKit Commit Bot]

Attachment 198808 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http/tests/inspector/network/css-source-mapping-url-expected.txt', u'LayoutTests/http/tests/inspector/network/css-source-mapping-url.html', u'LayoutTests/http/tests/inspector/network/resources/source-map-test-style.css', u'LayoutTests/http/tests/inspector/network/resources/source-map-test-style.css.map', u'LayoutTests/http/tests/inspector/network/resources/source-map-test-style.scss', u'Source/WebCore/ChangeLog', u'Source/WebCore/inspector/ContentSearchUtils.cpp', u'Source/WebCore/inspector/ContentSearchUtils.h', u'Source/WebCore/inspector/Inspector.json', u'Source/WebCore/inspector/InspectorDebuggerAgent.cpp', u'Source/WebCore/inspector/InspectorPageAgent.cpp', u'Source/WebCore/inspector/InspectorPageAgent.h', u'Source/WebCore/inspector/InspectorResourceAgent.cpp']" exit_code: 1
Source/WebCore/inspector/InspectorPageAgent.cpp:270:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Comment on attachment 198808 [details]
[PATCH] Stylesheet sourceMapURL proposed fix

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

Looks good otherwise.

> Source/WebCore/inspector/Inspector.json:947
>              {
> +                "id": "ExtraResourceInfo",
> +                "type": "object",
> +                "description": "Extra data extracted from a resource after it has completed loading.",
> +                "properties": [
> +                    { "name": "sourceMapURL", "type": "string", "optional": true, "description": "URL of source map associated with this resource (if any)." }
> +                ]
> +            },

I'm not sure ExtraResourceInfo helps much. What might we use it for later? If we keep it then FrameResourceTree should likely use it too. Otherwise everyone should just report sourceMapURL directly.

I suspect you want this to make it more compatible in the future to collect optional parameters. Instead maybe we should start using Safari's InspectorBackend "expectsResultObject" flag for more/all callbacks, which is good when there are a lot of optional parameters and for compatibility reasons.

>> Source/WebCore/inspector/InspectorPageAgent.cpp:270
>> +//static
> 
> Should have a space between // and comment  [whitespace/comments] [4]

I'd just drop the comment.

> Source/WebCore/inspector/InspectorPageAgent.cpp:273
> +    DEFINE_STATIC_LOCAL(String, sourceMapHttpHeader, (ASCIILiteral("X-SourceMap")));

HTTP

> Source/WebCore/inspector/InspectorPageAgent.cpp:280
> +    // Scripts are handled in a separate path.
> +
> +    if (cachedResource->type() != CachedResource::CSSStyleSheet)

Remove blank line.

> Source/WebCore/inspector/InspectorPageAgent.cpp:292
> +        String sourceMapURL = ContentSearchUtils::findStylesheetSourceMapURL(content);
> +        if (!sourceMapURL.isEmpty())
> +            return sourceMapURL;

Can't you just return ContentSearchUtils::findStylesheetSourceMapURL(content)?

[Timothy Hatcher]

Comment on attachment 198808 [details]
[PATCH] Stylesheet sourceMapURL proposed fix

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

>> Source/WebCore/inspector/InspectorPageAgent.cpp:280
>> +    if (cachedResource->type() != CachedResource::CSSStyleSheet)
> 
> Remove blank line.

Maybe scripts should be reported here to to be consistent?

[Joseph Pecoraro]

Created attachment 198912 [details]
[PATCH] Stylesheet sourceMapURL proposed fix - 2

Dropped the ExtraResourceInfo and addressed other review comments.

[WebKit Commit Bot]

Attachment 198912 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http/tests/inspector/network/css-source-mapping-url-expected.txt', u'LayoutTests/http/tests/inspector/network/css-source-mapping-url.html', u'LayoutTests/http/tests/inspector/network/resources/source-map-test-style.css', u'LayoutTests/http/tests/inspector/network/resources/source-map-test-style.css.map', u'LayoutTests/http/tests/inspector/network/resources/source-map-test-style.scss', u'Source/WebCore/ChangeLog', u'Source/WebCore/inspector/ContentSearchUtils.cpp', u'Source/WebCore/inspector/ContentSearchUtils.h', u'Source/WebCore/inspector/Inspector.json', u'Source/WebCore/inspector/InspectorDebuggerAgent.cpp', u'Source/WebCore/inspector/InspectorPageAgent.cpp', u'Source/WebCore/inspector/InspectorPageAgent.h', u'Source/WebCore/inspector/InspectorResourceAgent.cpp']" exit_code: 1
Source/WebCore/inspector/InspectorPageAgent.cpp:270:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

(In reply to comment #4)
> (From update of attachment 198808 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198808&action=review
> 
> >> Source/WebCore/inspector/InspectorPageAgent.cpp:280
> >> +    if (cachedResource->type() != CachedResource::CSSStyleSheet)
> > 
> > Remove blank line.
> 
> Maybe scripts should be reported here to to be consistent?

I thought about that. I'd like to handle that later, if we do decide to go that route. An advantage of the Debugger path handling Scripts is that it handles inline <script>s, and I wanted to avoid duplicating the ContentSearch for large JS resources right now. I certainly want to make this more consistent in the future though.

[Joseph Pecoraro]

Committed r148768
<http://trac.webkit.org/changeset/148768>