1013887 - CSS coverage should look nicer

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Joe Walker [:jwalker]]

Which means:

* The report should have unused selectors grouped by source
* It should have a start/stop button and some way to see a report (in the profiler?)
* The report should be organized better
* Maybe a better rule-out color than red in the style editor

Comment 1

[Joe Walker [:jwalker]]

Attachment: analyse-panel.png

Comment 2

[Joe Walker [:jwalker]]

Rob - How does this feel from the POV of the perf work that you're doing?

Comment 3

[Rob Campbell [:rc] (:robcee)]

First, I think we're going towards "Performance" rather than "Analyse" for the tab name.

I'm not sure how this is going to sit, honestly. The stuff we're doing with the profiler is moving towards capturing performance information about a browser with the eventual goal of streaming that data live. Visuals will include framerate graphs, bar graphs of frames and flame charts for JS.

The CSS Coverage tool is presenting a static analysis of styles in use. If we had CSS rendering times for selectors, that might feel like a better fit but right now this feels like a bit of an impedance mismatch.

Comment 4

[Joe Walker [:jwalker]]

In addition to Rob's comment, keeping the button in the style editor puts it where the designers are more likely to look.
Also doing it in the style editor is easier, but that's more of a casting vote type thing.

Comment 5

[Girish Sharma [:Optimizer]]

My two cents:
I agree with Rob that Profiler is more of performance, while this is more of auditing (Thus the Audits tool in Chrome) and optimizations.

While placing the button in Style Editor is simple and a good way right now, what heppens when we have JS converage too ? Does that button go live in debugger ? Do we want to have a separate tool like Audits or Optimizations or Coverage which can host css/js coverage and other future similar mini-tools ?

Comment 6

[Joe Walker [:jwalker]]

Thanks, Style editor it is.

Comment 8

[Joe Walker [:jwalker]]

Attachment: Screen Shot 2014-05-24 at 10.39.15.png

Comment 9

[Joe Walker [:jwalker]]

Attachment: coveragelooks-1013887.patch

I've broken these changes out into separate commits - it will probably be much easier to get a hang of what's going on by looking at:

https://github.com/joewalker/gecko-dev/compare/ffc4f5d...coveragelooks-1013887

Non-cosmetic changes:

* Use column layout for the report so it flows better
* Group unused rules by CSS page rather than randomly and present in a paragraph rather than a list. This change meant re-organizing the report data a bit
* Add a summary pie chart
* Close the notification bar properly whenever coverage stops

Comment 10

[Joe Walker [:jwalker]]

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

Comment 11

[Heather Arthur [:harth]]

Comment on attachment 8428701 [details] [diff] [review]
coveragelooks-1013887.patch

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

::: browser/devtools/styleeditor/styleeditor.css
@@ +129,5 @@
> +  -moz-box-orient: vertical;
> +}
> +
> +.csscoverage-report-chart {
> +  margin: 0 50px;

Should probably put this one in browser/themes/shared/devtools/styleeditor.css.

::: toolkit/devtools/server/actors/csscoverage.js
@@ +321,4 @@
>     * the recommended changes to a page.
>     * Example:
>     *   {
> +   *     preload: [

What does 'preload' mean throughout this file? I'd like a short explanation of it in a comment somewhere.

::: toolkit/locales/en-US/chrome/global/devtools/csscoverage.dtd
@@ +19,4 @@
>  <!-- LOCALIZATION NOTE (csscoverage.unused, csscoverage.noMatch):
>    -  This is the heading and body text for the CSS usage part of the report -->
>  <!ENTITY csscoverage.unused "Unused Rules">
> +<!ENTITY csscoverage.noMatch "No matches found for the following rules:">

If I'm not mistaken, we have to change the string name here, even if it hasn't hit aurora yet.

Comment 12

[Joe Walker [:jwalker]]

Thanks for the review

(In reply to Heather Arthur [:harth] from comment #11)
> Comment on attachment 8428701 [details] [diff] [review]
> coveragelooks-1013887.patch
> 
> Review of attachment 8428701 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ...
>
> ::: toolkit/locales/en-US/chrome/global/devtools/csscoverage.dtd
> @@ +19,4 @@
> >  <!-- LOCALIZATION NOTE (csscoverage.unused, csscoverage.noMatch):
> >    -  This is the heading and body text for the CSS usage part of the report -->
> >  <!ENTITY csscoverage.unused "Unused Rules">
> > +<!ENTITY csscoverage.noMatch "No matches found for the following rules:">
> 
> If I'm not mistaken, we have to change the string name here, even if it
> hasn't hit aurora yet.

In the end I just reverted that whole section. It was more trouble than it was worth.

https://github.com/joewalker/gecko-dev/commit/cfb4ee52ceec3434251312a6b95a5cca5b83c9cb

Comment 13

[Joe Walker [:jwalker]]

Attachment: coveragelooks-1013887.patch

Rebased, review comments fixed.

Comment 14

[Joe Walker [:jwalker]]

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

Comment 15

[Joe Walker [:jwalker]]

Attachment: coveragelooks-1013887-part2.patch

I bailed out on some wording changes yesterday. These are a second try at those changes.

Comment 16

[Joe Walker [:jwalker]]

Part 2:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=aa06a0c23d46
https://hg.mozilla.org/integration/fx-team/rev/aa06a0c23d46

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f965b7cef97f
https://hg.mozilla.org/mozilla-central/rev/aa06a0c23d46