Closed Bug 1306887 Opened 8 years ago Closed 7 years ago

x-sourcemap header doesn't work

Categories

(DevTools :: Inspector: Rules, defect, P2)

Product:
DevTools
Component:
Inspector: Rules
Version:
49 Branch
Type:
defect

Tracking

(firefox56 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- verified

People

(Reporter: dave, Assigned: tromey)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Screen Shot 2016-10-01 at 18.54.14.png
303.16 KB, image/png
Details
Bug 1306887 - keep SourceMap response header on CSS style sheets;
59 bytes, text/x-review-board-request
heycam
: review+
bzbarsky
: review+
Details
Bug 1306887 - use SourceMap response header in style sheet actor;
59 bytes, text/x-review-board-request
gl
: review+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.116 Safari/537.36

Steps to reproduce:

1. Go here: https://sourcemap-header-firefox-ienogvzqhe.now.sh/
2. Open developer tools
3. Go to inspector
4. Highlight h1 tag


Actual results:

In the right-hand css rules pane it links to main.css. It does not recognise the source map.


Expected results:

It should link to main.scss (Sass source file). Works correctly on Chrome and Safari.
Component: Untriaged → Developer Tools: CSS Rules Inspector
Inspector bug triage (filter on CLIMBING SHOES).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Blocks: source-maps
Note in bug 1346936 we're adding support for the unprefixed SourceMap header as well.
Assignee: nobody → ttromey
Comment on attachment 8882207 [details]
Bug 1306887 - keep SourceMap response header on CSS style sheets;

https://reviewboard.mozilla.org/r/153306/#review158588

Please remember to get a DOM peer's signoff on the .webidl change.

::: dom/webidl/StyleSheet.webidl:29
(Diff revision 1)
>    readonly attribute MediaList media;
>    [Pure]
>    attribute boolean disabled;
> +  // The SourceMap response header, if seen.
> +  [ChromeOnly]
> +  readonly attribute DOMString? sourceMapURL;

Since we never return null, we can just make this "DOMString" rather than "DOMString?".

::: layout/style/Loader.cpp:889
(Diff revision 1)
> +    nsAutoCString sourceMapURL;
> +    nsresult rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("SourceMap"), sourceMapURL);
> +    if (NS_FAILED(rv)) {
> +      rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("X-SourceMap"), sourceMapURL);
> +    }

This "check SourceMap and if not check X-SourceMap" seems duplicated in ScriptLoader::PrepareLoadedRequest.  Can you factor that out into a helper method?

::: layout/style/StyleSheet.cpp:497
(Diff revision 1)
> +  if (mInner) {
> +    mInner->mSourceMapURL = aSourceMapURL;
> +  }

mInner should really only be null when we're unlinking the object during cycle collection.  So I think you can just go ahead and use it without null checking it.  (Same in GetSourceMapURL.)
Attachment #8882207 - Flags: review?(cam) → review+
Comment on attachment 8882208 [details]
Bug 1306887 - use SourceMap response header in style sheet actor;

https://reviewboard.mozilla.org/r/153308/#review158640

::: devtools/server/actors/stylesheets.js:625
(Diff revision 1)
>     *         The text of the style sheet.
>     * @return {string}
>     *         Url of source map.
>     */
>    _extractSourceMapUrl: function (content) {
> +    if (this.rawSheet.sourceMapURL) {

I think adding a comment for this return would make sense. Based on your comments, adding something like "If a SourceMap response header was saved on a style sheet, use it in the
style sheet actor."
Attachment #8882208 - Flags: review?(gl) → review+
(In reply to Cameron McCormack (:heycam) from comment #5)

> Please remember to get a DOM peer's signoff on the .webidl change.

I had thought you were one based on some r= I saw in StyleSheet.webidl;
but no problem, I will add another r?.

> This "check SourceMap and if not check X-SourceMap" seems duplicated in
> ScriptLoader::PrepareLoadedRequest.  Can you factor that out into a helper
> method?

I'm happy to do this but I don't know where it ought to live.
Can you suggest somewhere?
Flags: needinfo?(cam)
(In reply to Tom Tromey :tromey from comment #7)
> I had thought you were one based on some r= I saw in StyleSheet.webidl;
> but no problem, I will add another r?.

(It might have been a DOM peer requesting review of me, which might pass the commit hook.  Not sure.)

> > This "check SourceMap and if not check X-SourceMap" seems duplicated in
> > ScriptLoader::PrepareLoadedRequest.  Can you factor that out into a helper
> > method?
> 
> I'm happy to do this but I don't know where it ought to live.
> Can you suggest somewhere?

How about a static method on nsContentUtils?  Or a new NS_Blah function in nsNetUtil.h.  Either sounds fine.
Flags: needinfo?(cam)
Comment on attachment 8882207 [details]
Bug 1306887 - keep SourceMap response header on CSS style sheets;

Probably could use another review.
Attachment #8882207 - Flags: review+ → review?(cam)
Comment on attachment 8882207 [details]
Bug 1306887 - keep SourceMap response header on CSS style sheets;

https://reviewboard.mozilla.org/r/153306/#review158786

::: layout/style/Loader.cpp:890
(Diff revision 2)
> +    if (nsContentUtils::GetSourceMapURL(httpChannel.get(), sourceMapURL)) {
> +      mSheet->SetSourceMapURL(NS_ConvertUTF8toUTF16(sourceMapURL));
> +    }

(I suppose there's not much difference between checking the return value, and unconditionally calling SetSourceMapURL, since the sheet's mSourceMapURL should already be an empty string.)

::: layout/style/StyleSheet.cpp:485
(Diff revision 2)
> +NS_IMETHODIMP
> +StyleSheet::GetSourceMapURL(nsAString& aSourceMapURL)
> +{
> +  aSourceMapURL.Assign(mInner->mSourceMapURL);
> +  return NS_OK;
> +}

Can this function just return void?
Attachment #8882207 - Flags: review?(cam) → review+
Comment on attachment 8882207 [details]
Bug 1306887 - keep SourceMap response header on CSS style sheets;

https://reviewboard.mozilla.org/r/153306/#review158786

> Can this function just return void?

Yes - I've changed that.
Comment on attachment 8882207 [details]
Bug 1306887 - keep SourceMap response header on CSS style sheets;

https://reviewboard.mozilla.org/r/153306/#review159000

r=me with the nits below.

::: dom/script/ScriptLoader.cpp:2846
(Diff revision 3)
>      if (NS_SUCCEEDED(rv) && !requestSucceeded) {
>        return NS_ERROR_NOT_AVAILABLE;
>      }
>  
>      nsAutoCString sourceMapURL;
> -    rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("SourceMap"), sourceMapURL);
> +    if (nsContentUtils::GetSourceMapURL(httpChannel.get(), sourceMapURL)) {

You don't need the .get() here.  nsCOMPtr<T> auto-converts to T*.

::: dom/webidl/StyleSheet.webidl:27
(Diff revision 3)
>    readonly attribute DOMString? title;
>    [Constant]
>    readonly attribute MediaList media;
>    [Pure]
>    attribute boolean disabled;
> +  // The SourceMap response header, if seen.

What if not seen?  Does it return empty string?  Something else?

Assuming we want to treat "sent, but empty string" the same as "not sent", just using empty string to represent "not sent" is fine.  But please document.

Also might be worth a comment about how it can change once we actually get back the HTTP response, so is not [Constant].  But it should probably be marked [Pure].

::: layout/style/Loader.cpp:890
(Diff revision 3)
>        mLoader->SheetComplete(this, NS_ERROR_NOT_AVAILABLE);
>        return NS_OK;
>      }
> +
> +    nsAutoCString sourceMapURL;
> +    if (nsContentUtils::GetSourceMapURL(httpChannel.get(), sourceMapURL)) {

No need for .get().

::: layout/style/StyleSheet.cpp:488
(Diff revision 3)
>  }
>  
> +void
> +StyleSheet::GetSourceMapURL(nsAString& aSourceMapURL)
> +{
> +  aSourceMapURL.Assign(mInner->mSourceMapURL);

Just `aSourceMapURL = mInner->mSourceMapURL`.

::: layout/style/StyleSheetInfo.h:64
(Diff revision 3)
>    // its hands on a child sheet that means we've already ensured unique infos
>    // throughout its parent chain and things are good.
>    RefPtr<StyleSheet>     mFirstChild;
>    AutoTArray<StyleSheet*, 8> mSheets;
>  
> +  // Source map URL from the SourceMap response header, if given.

Again, document what happens if the header is not present.
Attachment #8882207 - Flags: review?(bzbarsky) → review+
I've addressed the review comments.  Also I had to update the test to use async/await
in order for it to work after rebasing.
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a97c16caf744
keep SourceMap response header on CSS style sheets; r=bz,heycam
https://hg.mozilla.org/integration/autoland/rev/7023101ff34e
use SourceMap response header in style sheet actor; r=gl
https://hg.mozilla.org/mozilla-central/rev/a97c16caf744
https://hg.mozilla.org/mozilla-central/rev/7023101ff34e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
I have reproduced this Bug on Nightly 52.0a1 (2016-10-01) on Windows 10, 64 bit!

The bug's fix is now verified on latest  Nightly 56.0a1

Build ID    :	20170712030204
User Agent  : 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170712]
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: