Bug 114854 - Web Inspector: Backend should detect sourceMappingURLs in CSS Resources
Summary: Web Inspector: Backend should detect sourceMappingURLs in CSS Resources
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-18 23:15 PDT by Joseph Pecoraro
Modified: 2013-04-19 16:01 PDT (History)
4 users (show)

See Also:


Attachments
[PATCH] Stylesheet sourceMapURL proposed fix (23.79 KB, patch)
2013-04-18 23:26 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Stylesheet sourceMapURL proposed fix - 2 (22.76 KB, patch)
2013-04-19 13:34 PDT, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2013-04-18 23:15:38 PDT
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.
Comment 1 Joseph Pecoraro 2013-04-18 23:26:35 PDT
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.
Comment 2 WebKit Commit Bot 2013-04-18 23:27:19 PDT
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.
Comment 3 Timothy Hatcher 2013-04-19 06:41:33 PDT
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)?
Comment 4 Timothy Hatcher 2013-04-19 06:59:57 PDT
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?
Comment 5 Joseph Pecoraro 2013-04-19 13:34:47 PDT
Created attachment 198912 [details]
[PATCH] Stylesheet sourceMapURL proposed fix - 2

Dropped the ExtraResourceInfo and addressed other review comments.
Comment 6 WebKit Commit Bot 2013-04-19 13:37:09 PDT
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.
Comment 7 Joseph Pecoraro 2013-04-19 13:53:37 PDT
(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.
Comment 8 Joseph Pecoraro 2013-04-19 16:01:22 PDT
Committed r148768
<http://trac.webkit.org/changeset/148768>