Closed Bug 1013909 Opened 11 years ago Closed 11 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

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
Attachment #8427084 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 11 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.

Attachment

General

Created:
Updated:
Size: