Support CSS source maps

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Developer Tools: Style Editor
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: harth, Assigned: harth)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

unspecified
Firefox 29
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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)

Updated

4 years ago
Assignee: nobody → fayearthur
(Assignee)

Comment 1

4 years ago
Created attachment 8336995 [details] [diff] [review]
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.
(Assignee)

Comment 2

4 years ago
Created attachment 8341633 [details] [diff] [review]
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.
Attachment #8336995 - Attachment is obsolete: true
(Assignee)

Comment 3

4 years ago
Created attachment 8343457 [details] [diff] [review]
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://.
Attachment #8341633 - Attachment is obsolete: true
Attachment #8343457 - Flags: review?(dcamp)
Blocks: 947119

Comment 4

4 years ago
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+
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/8deb6f225d0a
Whiteboard: [fixed-in-fx-team]
(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
(Assignee)

Comment 8

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

Comment 9

4 years ago
Created attachment 8344229 [details] [diff] [review]
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.
Attachment #8343457 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/4204ff8d0234
https://hg.mozilla.org/mozilla-central/rev/4204ff8d0234
Status: NEW → RESOLVED
Last Resolved: 4 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 → ---
Created attachment 8346539 [details] [diff] [review]
Fix styleeditor actor on b2g

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

Comment 14

4 years ago
(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

4 years ago
(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.
(Assignee)

Comment 16

4 years ago
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 ?
(Assignee)

Comment 18

4 years ago
Compatibility: bug 949556
(Assignee)

Comment 19

4 years ago
(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)
(Assignee)

Comment 23

4 years ago
I have a patch to fix the compatibility. I need to test it more tomorrow, but it'll be in bug 949556.
Blocks: 888429
Blocks: 893306

Comment 24

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

Let's land that.
Attachment #8346539 - Flags: review?(dcamp) → review+

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f3edfca0d514
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f3edfca0d514
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Duplicate of this bug: 949391

Updated

4 years ago
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)
(Assignee)

Comment 30

4 years ago
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)
Keywords: dev-doc-needed → dev-doc-complete

Updated

2 years ago
Duplicate of this bug: 898011
You need to log in before you can comment on or make changes to this bug.