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)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: jwalker, Assigned: jwalker)
References
Details
Attachments
(1 file, 2 obsolete files)
7.47 KB,
patch
|
Details | Diff | Splinter Review |
Bug 1007073 is the correct solution, but we can have a work-around for now where we don't markup compressed stylesheets
Assignee | ||
Comment 1•11 years ago
|
||
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)
Comment 2•11 years ago
|
||
(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)
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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+
Assignee | ||
Comment 6•11 years ago
|
||
(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.
Assignee | ||
Comment 7•11 years ago
|
||
(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.
Assignee | ||
Comment 8•11 years ago
|
||
(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
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #8427084 -
Attachment is obsolete: true
Assignee | ||
Comment 10•11 years ago
|
||
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=2ea03e1c3655
Assignee | ||
Comment 11•11 years ago
|
||
Comment 12•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•