Closed Bug 926014 Opened 11 years ago Closed 10 years ago

Support CSS source maps

Categories

(DevTools :: Style Editor, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: harth, Assigned: harth)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 3 obsolete files)

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.
Assignee: nobody → fayearthur
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.
Attached patch WIP 2 (obsolete) — Splinter Review
WIP version 2. Cleanup and all the existing tests are passing now. Left to do: new tests for showing original sources and inspector links.
Attachment #8336995 - Attachment is obsolete: true
Attached patch css-source-maps.patch (obsolete) — Splinter Review
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://.
Attachment #8341633 - Attachment is obsolete: true
Attachment #8343457 - Flags: review?(dcamp)
Blocks: 947119
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.
Attachment #8343457 - Flags: review?(dcamp) → review+
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.
Thanks for backing out, sorry about that. Running *all* the tests on try now: https://tbpl.mozilla.org/?tree=Try&rev=a5bbdfbc37c6
New patch with test fixes, try is green: https://tbpl.mozilla.org/?tree=Try&rev=057fe48e3801.

Will probably check in after the merge, though.
Attachment #8343457 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4204ff8d0234
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
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?
Blocks: 949391
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Just fix DebuggerServer init, doesn't address compability issue.
(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.
(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.
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.
So, should this be backed out until this is ironed out ?
Compatibility: bug 949556
(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.
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 :/
(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 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.
Attachment #8346539 - Flags: review?(dcamp)
I have a patch to fix the compatibility. I need to test it more tomorrow, but it'll be in bug 949556.
Comment on attachment 8346539 [details] [diff] [review]
Fix styleeditor actor on b2g

Let's land that.
Attachment #8346539 - Flags: review?(dcamp) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f3edfca0d514
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 952600
Adding dev-doc-needed so it goes on Will radar.
Keywords: dev-doc-needed
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.
Flags: needinfo?(fayearthur)
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.
Flags: needinfo?(fayearthur)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.