Closed Bug 1013909 Opened 6 years ago Closed 6 years ago

CSS Coverage should be smarter in its handling of compressed style sheets

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: jwalker, Assigned: jwalker)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Bug 1007073 is the correct solution, but we can have a work-around for now where we don't markup compressed stylesheets
This is a work in progress.

https://github.com/joewalker/gecko-dev/commit/4813598fcd26818bfd2c1acc4bf2a2a8adce8830

It seems to work OK, but I'm not sure how to tell the user. We have have a stylesheet that is compressed, and have coverage info, but can't use it because the lines will be wrong. We'd just like a quick "By the way, we're not displaying coverage info because compression" temporary notice. Any thoughts on the best way to do that?

If we don't have any good ideas then I suggest we commit this (without the debug) because no markup and no reason is better than wrong markup.
Flags: needinfo?(fayearthur)
(In reply to Joe Walker [:jwalker] from comment #1)
> This is a work in progress.
> 
> https://github.com/joewalker/gecko-dev/commit/
> 4813598fcd26818bfd2c1acc4bf2a2a8adce8830
> 
> It seems to work OK, but I'm not sure how to tell the user. We have have a
> stylesheet that is compressed, and have coverage info, but can't use it
> because the lines will be wrong. We'd just like a quick "By the way, we're
> not displaying coverage info because compression" temporary notice. Any
> thoughts on the best way to do that?
> 
> If we don't have any good ideas then I suggest we commit this (without the
> debug) because no markup and no reason is better than wrong markup.

Could just throw up a notification box error saying "can't display coverage for minified sheets". emit an "error" event from StyleEditorUI.jsm to do this.

It's hidden right now, so I don't think it's a big deal, but we should solve it correctly by generating source maps for pretty-fied sheets ala bug 724505.
Flags: needinfo?(fayearthur)
Attached patch decompress-1013909.patch (obsolete) — Splinter Review
Assignee: nobody → jwalker
Status: NEW → ASSIGNED
Attachment #8427078 - Flags: review?(fayearthur)
Attached patch decompress-1013909.patch v2 (obsolete) — Splinter Review
Sorry wrong patch.

A breakdown of the changes is here: https://github.com/joewalker/gecko-dev/pull/3
Attachment #8427078 - Attachment is obsolete: true
Attachment #8427078 - Flags: review?(fayearthur)
Attachment #8427084 - Flags: review?(fayearthur)
Comment on attachment 8427084 [details] [diff] [review]
decompress-1013909.patch v2

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

A simple test would be nice. Also the commit message is quite big.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +469,5 @@
>            editor.onShow();
>  
> +          this.emit("editor-selected", editor);
> +
> +          // Is there any CSS coverage markup to include?

This is tiny, but this as a question I could see being confusing. Maybe make it a statement 'Check for any CSS coverage markup to include'.

@@ +481,5 @@
> +                let text = editor.sourceEditor.getText();
> +                // If the CSS text contains a '}' with some non-whitespace
> +                // after then we assume this is compressed CSS and stop
> +                // marking-up.
> +                if (!/}\s*\S+\s*\n/.test(text)) {

I'm testing this regex independently and it's not catching "a{b:7}c" for instance.
Attachment #8427084 - Flags: review?(fayearthur) → review+
(In reply to Heather Arthur [:harth] from comment #5)
> Comment on attachment 8427084 [details] [diff] [review]
> decompress-1013909.patch v2
> 
> Review of attachment 8427084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A simple test would be nice. Also the commit message is quite big.

I'm going to have to work out a better way to create hg patches from git commits. Thanks

> ::: browser/devtools/styleeditor/StyleEditorUI.jsm
> @@ +469,5 @@
> >            editor.onShow();
> >  
> > +          this.emit("editor-selected", editor);
> > +
> > +          // Is there any CSS coverage markup to include?
> 
> This is tiny, but this as a question I could see being confusing. Maybe make
> it a statement 'Check for any CSS coverage markup to include'.

OK

> @@ +481,5 @@
> > +                let text = editor.sourceEditor.getText();
> > +                // If the CSS text contains a '}' with some non-whitespace
> > +                // after then we assume this is compressed CSS and stop
> > +                // marking-up.
> > +                if (!/}\s*\S+\s*\n/.test(text)) {
> 
> I'm testing this regex independently and it's not catching "a{b:7}c" for
> instance.

Good spot. The \n should be \r.
(In reply to Joe Walker [:jwalker] from comment #6)
> (In reply to Heather Arthur [:harth] from comment #5)
> > @@ +481,5 @@
> > > +                let text = editor.sourceEditor.getText();
> > > +                // If the CSS text contains a '}' with some non-whitespace
> > > +                // after then we assume this is compressed CSS and stop
> > > +                // marking-up.
> > > +                if (!/}\s*\S+\s*\n/.test(text)) {
> > 
> > I'm testing this regex independently and it's not catching "a{b:7}c" for
> > instance.
> 
> Good spot. The \n should be \r.

Actually, that's not the whole story. The \n should be \r, but also the top line should read:

    let text = editor.sourceEditor.getText() + "\r";

In that way we treat the end of a file as if it was an end of line.
(In reply to Joe Walker [:jwalker] from comment #7)
> (In reply to Joe Walker [:jwalker] from comment #6)
> > (In reply to Heather Arthur [:harth] from comment #5)
> > > @@ +481,5 @@
> > > > +                let text = editor.sourceEditor.getText();
> > > > +                // If the CSS text contains a '}' with some non-whitespace
> > > > +                // after then we assume this is compressed CSS and stop
> > > > +                // marking-up.
> > > > +                if (!/}\s*\S+\s*\n/.test(text)) {
> > > 
> > > I'm testing this regex independently and it's not catching "a{b:7}c" for
> > > instance.
> > 
> > Good spot. The \n should be \r.
> 
> Actually, that's not the whole story. The \n should be \r, but also the top
> line should read:
> 
>     let text = editor.sourceEditor.getText() + "\r";
> 
> In that way we treat the end of a file as if it was an end of line.

So in summary: https://github.com/joewalker/gecko-dev/commit/62e6eb3bb26eee02ac5d6d21d8ac007588630cc4
https://hg.mozilla.org/mozilla-central/rev/1d03cb36396d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.