926014 - Support CSS source maps

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Heather Arthur [:harth]]

If a stylesheet specifies a source map we should:

1) Open the original source (e.g. sass file) in the Style Editor
2) Links in the inspector should be to the location in the original file

Showing the original sources should be behind a pref, similar to what the JS debugger tool does.

Comment 1

[Heather Arthur [:harth]]

Attachment: WIP show sources in Style Editor and link in Inspector

Here's a work in progress patch. If a stylesheet uses source maps, the original sources will be shown in the Style Editor instead of the generated style sheet. Additionally, links in the inspector's rule view will link to the location in original source.

No tests yet.

Comment 2

[Heather Arthur [:harth]]

Attachment: WIP 2

WIP version 2. Cleanup and all the existing tests are passing now. Left to do: new tests for showing original sources and inspector links.

Comment 3

[Heather Arthur [:harth]]

Attachment: css-source-maps.patch

This patch adds partial CSS source map support. It's behind a pref right now, so you'll have toggle "devtools.styleeditor.source-maps-enabled".

Functionality:
1) Original sources will be shown instead of their respective style sheets in the Style Editor. These are in read-only.
2) Links in rule view and computed view will be to the location of the rule in the original source.

This changes quite a bit of the Style Editor code. The big change is that the Style Editor actors now use the protocol.js framework, so the code is nicer and now integrates better with the inspector. It's not backwards compatible.

This patch also adds a couple tests for the style editor and inspector links.

Follow up bugs for these important things:
1) UI for pref. Immediate effects when toggling.
2) Watch for file changes to the generated source if we're on a file:// url, and allow live editing of original sources for file://.

Comment 4

[Dave Camp (:dcamp)]

Comment on attachment 8343457 [details] [diff] [review]
css-source-maps.patch

Review of attachment 8343457 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/profile/firefox.js
@@ -1158,5 @@
>  pref("devtools.scratchpad.recentFilesMax", 10);
>  
>  // Enable the Style Editor.
>  pref("devtools.styleeditor.enabled", true);
> -pref("devtools.styleeditor.transitions", true);

Did you mean to remove the transitions pref?

::: browser/devtools/styleeditor/styleeditor-panel.js
@@ +8,5 @@
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
> +
> +let promise = require("sdk/core/promise");

Conflicts with benvie's promise work incoming!

::: browser/devtools/styleinspector/test/sourcemaps.scss
@@ +1,2 @@
> +
> +$paulrougetpink: #f06;

hehe

::: toolkit/devtools/server/actors/styleeditor.js
@@ +237,3 @@
>  
>  /**
> + * The corresponding Front object for the ShaderActor.

ShaderActor

@@ +854,5 @@
> + * @returns Promise
> + *        A promise of the document at that URL, as a string.
> + */
> +function fetch(aURL, aOptions={ loadFromCache: true, window: null,
> +                                charset: null}) {

This seems like a generally useful function, maybe we should move it somewhere central at some point.

Comment 5

[Heather Arthur [:harth]]

Thanks for the review. Try results coming in here: https://tbpl.mozilla.org/?tree=Try&rev=c285bc535ab7

(In reply to Dave Camp (:dcamp) from comment #4)
> Did you mean to remove the transitions pref?

Yeah, turns out we weren't using it anywhere. We can add it back later if we start.

> @@ +854,5 @@
> > + * @returns Promise
> > + *        A promise of the document at that URL, as a string.
> > + */
> > +function fetch(aURL, aOptions={ loadFromCache: true, window: null,
> > +                                charset: null}) {
> 
> This seems like a generally useful function, maybe we should move it
> somewhere central at some point.

It is, the debugger uses a very similar function, we should create a shared dir in toolkit soon.

Comment 6

[Heather Arthur [:harth]]

https://hg.mozilla.org/integration/fx-team/rev/8deb6f225d0a

Comment 7

[Carsten Book [:Tomcat]]

(In reply to Heather Arthur [:harth] from comment #6)
> https://hg.mozilla.org/integration/fx-team/rev/8deb6f225d0a

Hi,

had to backout this change seems it caused failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=31610509&tree=Fx-Team on OSX and maybe this one https://tbpl.mozilla.org/php/getParsedLog.php?id=31610600&tree=Fx-Team

Comment 8

[Heather Arthur [:harth]]

Thanks for backing out, sorry about that. Running *all* the tests on try now: https://tbpl.mozilla.org/?tree=Try&rev=a5bbdfbc37c6

Comment 9

[Heather Arthur [:harth]]

Attachment: Support source maps - bug fixes

New patch with test fixes, try is green: https://tbpl.mozilla.org/?tree=Try&rev=057fe48e3801.

Will probably check in after the merge, though.

Comment 10

[Heather Arthur [:harth]]

https://hg.mozilla.org/integration/fx-team/rev/4204ff8d0234

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4204ff8d0234

Comment 12

[Alexandre Poirot [:ochameau]]

This patch broke b2g support in two ways:
- Miss b2g actors registering update:
http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#1065
That miss breaks DebuggerServer initialization, so that we can connect but that's pretty much it (bug 949391)
I could easily have fixed that, but there is also:
- The switch from "old fashion actor" to protocol.js make FF Desktop <29 incompatible with FF OS >=29, nor FF Desktop >= 29 with FF OS <29.

I'm not sure we want to keep such compatiblity breaker change?

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Fix styleeditor actor on b2g

Just fix DebuggerServer init, doesn't address compability issue.

Comment 14

[Paul Rouget [:paul]]

(In reply to Alexandre Poirot (:ochameau) from comment #12)
> I'm not sure we want to keep such compatiblity breaker change?

The protocol should not change.

We should be able to debug gecko 26 (fxos 1.2) with gecko 29.

You need to change the actor name if you want a new API.

Comment 15

[Paul Rouget [:paul]]

(In reply to Paul Rouget [:paul] from comment #14)
> (In reply to Alexandre Poirot (:ochameau) from comment #12)
> > I'm not sure we want to keep such compatiblity breaker change?
> 
> The protocol should not change.
> 
> We should be able to debug gecko 26 (fxos 1.2) with gecko 29.
> 
> You need to change the actor name if you want a new API.

And have a fallback mechanism if the expected actor is not present.

Comment 16

[Heather Arthur [:harth]]

Sorry I forgot to leave a comment about this, I talked offline with dcamp about what to do here.

The actor name changed to styleSheetsActor. I plan to add a styleEditorActor that uses protocol.js and implements the old API. The UI will see if the styleSheetsActor exists, and if not will use the old API. ETA on this is soon, before the next merge for sure. I'll link to the bug when I file it.

Comment 17

[Julien Wajsberg [:julienw]]

So, should this be backed out until this is ironed out ?

Comment 18

[Heather Arthur [:harth]]

Compatibility: bug 949556

Comment 19

[Heather Arthur [:harth]]

(In reply to Julien Wajsberg [:julienw] from comment #17)
> So, should this be backed out until this is ironed out ?

Practically, there are changes on top of it checked in. I'm going to work on this right now.

Comment 20

[Julien Wajsberg [:julienw]]

Heather, will the remote debugger in a newer Nightly work with the current Gecko? If yes then I'm fine with waiting for your other changes. Otherwise, it really breaks one of our main tools :/

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot (:ochameau) from comment #13)
> Created attachment 8346539 [details] [diff] [review]
> Fix styleeditor actor on b2g
> 
> Just fix DebuggerServer init, doesn't address compability issue.

I think we should at least land this patch now, even if fixing the style editor for real will take more time.  At least this way, the other tools and remote features will be working again.

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8346539 [details] [diff] [review]
Fix styleeditor actor on b2g

(In reply to J. Ryan Stinnett [:jryans] from comment #21)
> (In reply to Alexandre Poirot (:ochameau) from comment #13)
> > Created attachment 8346539 [details] [diff] [review]
> > Fix styleeditor actor on b2g
> > 
> > Just fix DebuggerServer init, doesn't address compability issue.
> 
> I think we should at least land this patch now, even if fixing the style
> editor for real will take more time.  At least this way, the other tools and
> remote features will be working again.

+1, either that or backout, we can't let b2g support completely broken.

Comment 23

[Heather Arthur [:harth]]

I have a patch to fix the compatibility. I need to test it more tomorrow, but it'll be in bug 949556.

Comment 24

[Paul Rouget [:paul]]

Comment on attachment 8346539 [details] [diff] [review]
Fix styleeditor actor on b2g

Let's land that.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f3edfca0d514

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3edfca0d514

Comment 28

[Jean-Yves Perrier [:teoli]]

Adding dev-doc-needed so it goes on Will radar.

Comment 29

[Will Bamberg [:wbamberg]]

I've added a section on source maps to the Style Editor doc: https://developer.mozilla.org/en-US/docs/Tools/Style_Editor#Source_map_support. Please let me know if it looks reasonable to you.

Comment 30

[Heather Arthur [:harth]]

This looks good, thanks for adding that. I'm doing a blog post soon with some more details about the file watching that we could add on to that, stay tuned.