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

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Joe Walker [:jwalker]]

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

Comment 1

[Joe Walker [:jwalker]]

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.

Comment 2

[Heather Arthur [:harth]]

(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.

Comment 3

[Joe Walker [:jwalker]]

Attachment: decompress-1013909.patch

Comment 4

[Joe Walker [:jwalker]]

Attachment: decompress-1013909.patch v2

Sorry wrong patch.

A breakdown of the changes is here: https://github.com/joewalker/gecko-dev/pull/3

Comment 5

[Heather Arthur [:harth]]

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.

Comment 6

[Joe Walker [:jwalker]]

(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.

Comment 7

[Joe Walker [:jwalker]]

(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.

Comment 8

[Joe Walker [:jwalker]]

(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

Comment 9

[Joe Walker [:jwalker]]

Attachment: decompress-1013909.patch

Comment 10

[Joe Walker [:jwalker]]

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=2ea03e1c3655

Comment 11

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=f965b7cef97f
https://hg.mozilla.org/integration/fx-team/rev/1d03cb36396d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d03cb36396d