Closed Bug 975522 Opened 10 years ago Closed 10 years ago

Display CSS Coverage analysis information

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: jwalker, Assigned: jwalker)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 8 obsolete files)

We need some way to gather usage information over something like a test run, and display the used/unused values in the toolbox
Attached patch coverage.patch (obsolete) — Splinter Review
This would be very cool. I've been relying on the old jscoverage every time I've tried it and it has always been cumbersome. Looking at how simple it seems to be, did I miss an existing tool that will gather coverage data when running xpcshell tests and output it to a report file of some sort? If not, it would be very nice if it were possible after this bug.
Assignee: nobody → jwalker
I'm going to concentrate on CSS coverage in this bug. Other bugs track JS coverage testing.
Summary: Display JS/CSS Coverage analysis information → Display CSS Coverage analysis information
I don't know exactly what the scope of this bug is, but maybe you want to look at bug 834865.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #4)
> I don't know exactly what the scope of this bug is, but maybe you want to
> look at bug 834865.

I like the idea of splitting CSS into 3 - Inlined/Used/Unused.
Thanks.
How do you defined "unused"? Unused across a timed session? When the user stats the analysis, will he be allowed to load other documents?
s/stats/starts/
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #7)
> How do you defined "unused"? Unused across a timed session? When the user
> starts the analysis, will he be allowed to load other documents?

The idea is to have an on/off switch. Turn it on run your test suite, turn it off, and the style editor will be annotated.
And how do you know that a rule has been used?
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #11)
> And how do you know that a rule has been used?

We need some heuristic, but broadly:

While the switch is on, we register listeners on DOMContentLoaded and as a mutation observer, and at each change we check to see if the known css rules match anything.
Can't we do something at the platform level instead? This sounds very expensive.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #13)
> Can't we do something at the platform level instead? This sounds very
> expensive.

Expensive in terms of page performance, maybe - but you'd only turn this on for a specific reason, and I think you'd be willing to pay the cost for a one off.

Expensive in terms of development cost however is a different question.
Attached patch coverage-975522.patch (obsolete) — Splinter Review
Heather - here is the WIP of the css coverage code.

Before r?:

* Add tests
* localization
* Remote. It's designed to be easy to remote, but to keep things simple I've not
  bothered so far

* Fix report to show unused styles properly
* Check that start/stop work. We probably need to un-attach and re-attach the
  ReflowListener for each navigation

Before we can land/demo - Should talk to harth about these:

* UsageReport.createPageReport: How do we jump style editor to
  this.href:this.start.line
* Move editor_* functions to the style editor
* Get a rule formatter working (see formatRule)
* How do we style unused rules in the style editor. red strike is ugly
* Line numbers are off for inline style elements because the DOMUtils
  gives the line num relative to the HTML, and the style editor just uses
  the contents of the style element
* Fix digging into imported sheets (a.k.a I need to get a list of all the
  @imported sheets too - I guess there's code for this already)
* Make the report look good (use a textarea for the cut/paste preload CSS?)
Attachment #8379908 - Attachment is obsolete: true
See Also: → 1007006
Depends on: 1007006
This is now remoted, localized and has tests.

r? gps for build changes.
Will r? harth when we've talked over a few changes that I'd like to make and added more tests.
Attachment #8417281 - Attachment is obsolete: true
Attachment #8419326 - Flags: review?(gps)
Attachment #8419326 - Flags: review?(gps) → review?(mshal)
How do you use it? I assume it's the 'coverage' command, but I get errors when I use any of the subcommands ('start', 'oneshot'): "Error: No type from output of coverage oneshot"
Flags: needinfo?(jwalker)
(In reply to Heather Arthur [:harth] from comment #18)
> How do you use it? I assume it's the 'coverage' command, but I get errors
> when I use any of the subcommands ('start', 'oneshot'): "Error: No type from
> output of coverage oneshot"

Hmm - did you apply both patches?

We were talking about the connection lost problem. Is this what you see?
https://tbpl.mozilla.org/?tree=Try&rev=77de976a4040
Flags: needinfo?(jwalker)
Comment on attachment 8419326 [details] [diff] [review]
0002-Bug-975522-Add-CSS-coverage-commands-r-harth.patch

>diff --git a/toolkit/devtools/gcli/moz.build b/toolkit/devtools/gcli/moz.build
>index 58ce5e2..6fbe815 100644
>--- a/toolkit/devtools/gcli/moz.build
>+++ b/toolkit/devtools/gcli/moz.build
>@@ -1,5 +1,3 @@
>-# vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>-

Just some nits on the build pieces:

1) Why are you changing the header for these moz.build files? The vast majority use this:

# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
# vim: set filetype=python:
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

>diff --git a/toolkit/devtools/jar.mn b/toolkit/devtools/jar.mn
>new file mode 100644
>index 0000000..cdacb70
>--- /dev/null
>+++ b/toolkit/devtools/jar.mn
>@@ -0,0 +1,4 @@
>+
>+toolkit.jar:
>+   content/global/devtools/gcli/commands/coverage.xhtml     (gcli/commands/coverage.xhtml)
>+   content/global/devtools/gcli/commands/coverage-report.js (gcli/commands/coverage-report.js)

2) The jar.mn file should also probably have the MPL (most of them do):

# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
Attachment #8419326 - Flags: review?(mshal) → review+
(In reply to Michael Shal [:mshal] from comment #20)
> Comment on attachment 8419326 [details] [diff] [review]
> 0002-Bug-975522-Add-CSS-coverage-commands-r-harth.patch
> 
> >diff --git a/toolkit/devtools/gcli/moz.build b/toolkit/devtools/gcli/moz.build
> >index 58ce5e2..6fbe815 100644
> >--- a/toolkit/devtools/gcli/moz.build
> >+++ b/toolkit/devtools/gcli/moz.build
> >@@ -1,5 +1,3 @@
> >-# vim: set filetype=python:
> > # This Source Code Form is subject to the terms of the Mozilla Public
> > # License, v. 2.0. If a copy of the MPL was not distributed with this
> > # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >-
> 
> Just some nits on the build pieces:
> 
> 1) Why are you changing the header for these moz.build files? The vast
> majority use this:
> 
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40
> -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.

In modelines are evil for reasons that I'm sure don't need listing, and I would generally take them out but file didn't have any substantive changes so I'll revert it.

> >diff --git a/toolkit/devtools/jar.mn b/toolkit/devtools/jar.mn
> ...
> 2) The jar.mn file should also probably have the MPL (most of them do):
> 
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.

Ah - good spot, thanks.
Attached patch coverage-975522.patch (obsolete) — Splinter Review
Attachment #8419326 - Attachment is obsolete: true
Attachment #8421017 - Flags: review?(fayearthur)
This code listens to reflow. Reflow == change in the layout. Change in the layout doesn't mean a new rule has been used. You'll trigger _onChange many times for no good reason, and you'll miss plenty of the CSS changes.

Use mutations (selector changes) and resize events (for some media queries).

But I still believe that we should fire an event from gecko when a rule is being used or a way to flag rules has "been used".
(In reply to Joe Walker [:jwalker] from comment #19)
> (In reply to Heather Arthur [:harth] from comment #18)
> > How do you use it? I assume it's the 'coverage' command, but I get errors
> > when I use any of the subcommands ('start', 'oneshot'): "Error: No type from
> > output of coverage oneshot"
> 
> Hmm - did you apply both patches?

Aha, did not, sorry. I'll try again.

> 
> We were talking about the connection lost problem. Is this what you see?
> https://tbpl.mozilla.org/?tree=Try&rev=77de976a4040

Yep, that's it. Innocuous so far, but I'd like to figure it out.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #23)
> This code listens to reflow. Reflow == change in the layout. Change in the
> layout doesn't mean a new rule has been used. You'll trigger _onChange many
> times for no good reason, and you'll miss plenty of the CSS changes.
> 
> Use mutations (selector changes) and resize events (for some media queries).

We don't want to track media query changes because we're not tracking them separately.

My plan was to use mutation observers and reflows (to catch CSSOM changes). But for now I'm documenting that we don't support CSSOM changes until we've more experience with how people are affected by the performance of this.
(In reply to Joe Walker [:jwalker] from comment #25)
> (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from
> comment #23)
> > This code listens to reflow. Reflow == change in the layout. Change in the
> > layout doesn't mean a new rule has been used. You'll trigger _onChange many
> > times for no good reason, and you'll miss plenty of the CSS changes.
> > 
> > Use mutations (selector changes) and resize events (for some media queries).
> 
> We don't want to track media query changes because we're not tracking them
> separately.
> 
> My plan was to use mutation observers and reflows (to catch CSSOM changes).
> But for now I'm documenting that we don't support CSSOM changes until we've
> more experience with how people are affected by the performance of this.

Also there are better ways to track CSSOM changes.
What would tracking reflow bring exactly? What kind of changes at the CSS level would happen that would be found after a reflow but not after a mutation? Beside mediaQueries on page resize, I don't see any, but I might miss something obvious.
Note for later: nsRuleNode.cpp:nsRuleNode::WalkRuleTree might be a good location for a platform hook (not tested).
Attached patch coverage-975522.patch v3 (obsolete) — Splinter Review
Quick update so we can start reviewing.
Will track diff of diffs here https://github.com/joewalker/gecko-dev/pull/2
Attachment #8421017 - Attachment is obsolete: true
Attachment #8421017 - Flags: review?(fayearthur)
Attachment #8423237 - Flags: review?(fayearthur)
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #28)
> Note for later: nsRuleNode.cpp:nsRuleNode::WalkRuleTree might be a good
> location for a platform hook (not tested).

Each time we debate this I go away thinking that the platform may be able to help more, and then later think that it can't help as much as previously thought.

Coverage is not usage. For our purposes, a rule is covered if we want to keep it in our CSS files. There a many reasons why we may want to keep a rule even when it is not used:

* media rules prevent it from being used even though that's just a matter of how the browser window is displayed
* x:hover, x:lang, x:visited, x:target, x:active, x:dir, etc are covered by an x that isn't hovered, visited, etc, and similar for other pseudo-classes
* x::first-letter is not used in a document with a single empty x, although it is still covered.

So moving this code into the platform, looks more and more like just re-writing the algorithm in another language to me.

The primary benefit of moving the code was performance. However:

* People expect a performance degradation when coverage testing is on. That doesn't mean we shouldn't try to make it fast, but it does mean that it's not #1 priority
* If badly implemented (e.g. how we discussed - marking a rule as used when it was applied to the document) could easily degrade performance, for everyone, whether coverage was running or not.
Can we have an overview of the algorithm you'll be using here?

Then, I'll see if part of it can be done at platform level, and if so, I'll see how big the performance difference would be, and how much better/weaker the coverage would be, and how hard it would be to implement.

Without platform work, I fail to see how we can get a coverage that doesn't miss plenty of changes (all the CSS changes that are no triggered by DOM mutations and refows), and how this can be done without a huge impact on performance (on one change (triggered by a reflow or a mutation), do you need to go through all the DOM nodes of a document and check all the rules of each nodes?).

What ever we do at the platform level, I don't believe it will be able to replace what ever you're doing here, but I'm pretty sure the core operations that you will do in JS/DOM/CSSOM could be done in C++. And maybe in an even simpler way :)
Could this approach be extended to report selectors that only match one element? With that information, web authors could detect sloppy selectors and merge those selectors into a broader rule.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #32)
> Can we have an overview of the algorithm you'll be using here?
> 
> Then, I'll see if part of it can be done at platform level, and if so, I'll
> see how big the performance difference would be, and how much better/weaker
> the coverage would be, and how hard it would be to implement.
> 
> Without platform work, I fail to see how we can get a coverage that doesn't
> miss plenty of changes (all the CSS changes that are no triggered by DOM
> mutations and refows), and how this can be done without a huge impact on
> performance (on one change (triggered by a reflow or a mutation), do you
> need to go through all the DOM nodes of a document and check all the rules
> of each nodes?).

I understand your worries a little better now. There is a much better algorithm - On each change you only need to check that the selectors that previously had qsa.length == 0, still have that. So it's not going through all the nodes and it's not checking all the rules. Simple, and in the testing that I've done so far has very little performance impact.

With platform help, we could speed this up with a "this style-rule used" event which would knock the "still unused" list down faster. More useful would be a "need to check usage" event to use in place of mutation observers. I think it would be a mistake to get into either without knowing that we need to.
(In reply to Anthony Ricaud (:rik) from comment #33)
> Could this approach be extended to report selectors that only match one
> element? With that information, web authors could detect sloppy selectors
> and merge those selectors into a broader rule.

It could, but is it valuable? Is it true that selectors that have only a single match are necessarily sloppy, and should be merged? I guess I'm not following your point.
(In reply to Joe Walker [:jwalker] from comment #34)
> There is a much better
> algorithm - On each change you only need to check that the selectors that
> previously had qsa.length == 0, still have that. So it's not going through
> all the nodes and it's not checking all the rules. Simple, and in the
> testing that I've done so far has very little performance impact.

Meh, I haven't thought of that. I thought something much more complex was needed. I like it. Could we try that on the homescreen of Firefox OS running on a Keon and see if it can run properly (I can run some tests for you if you want).

> With platform help, we could speed this up with a "this style-rule used"
> event which would knock the "still unused" list down faster. More useful
> would be a "need to check usage" event to use in place of mutation
> observers. I think it would be a mistake to get into either without knowing
> that we need to.

What about if we can flag the document as docshell.trackSelectorUsage = true/false, and then, be able to do: docshell.getUsedSelectorsDuringCoverage()? It would return the list of used selectors, with a hit count (for how many different nodes it's been used).

Then we would not need to listen to any mutations, and we would cover more use cases.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #36)
> > With platform help, we could speed this up with a "this style-rule used"
> > event which would knock the "still unused" list down faster. More useful
> > would be a "need to check usage" event to use in place of mutation
> > observers. I think it would be a mistake to get into either without knowing
> > that we need to.
> 
> What about if we can flag the document as docshell.trackSelectorUsage =
> true/false, and then, be able to do:
> docshell.getUsedSelectorsDuringCoverage()? It would return the list of used
> selectors, with a hit count (for how many different nodes it's been used).
> 
> Then we would not need to listen to any mutations, and we would cover more
> use cases.

But this confuses usage with coverage...
Attached patch coverage-975522.patch v4 (obsolete) — Splinter Review
Attachment #8423237 - Attachment is obsolete: true
Attachment #8423237 - Flags: review?(fayearthur)
Attachment #8425043 - Flags: review?(fayearthur)
(In reply to Joe Walker [:jwalker] from comment #37)
> (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from
> comment #36)
> > > With platform help, we could speed this up with a "this style-rule used"
> > > event which would knock the "still unused" list down faster. More useful
> > > would be a "need to check usage" event to use in place of mutation
> > > observers. I think it would be a mistake to get into either without knowing
> > > that we need to.
> > 
> > What about if we can flag the document as docshell.trackSelectorUsage =
> > true/false, and then, be able to do:
> > docshell.getUsedSelectorsDuringCoverage()? It would return the list of used
> > selectors, with a hit count (for how many different nodes it's been used).
> > 
> > Then we would not need to listen to any mutations, and we would cover more
> > use cases.
> 
> But this confuses usage with coverage...

Wouldn't that just replace the need to do querySelector on each mutation? And it would also add support for pseudo classes (don't trigger mutations) and pseudo elements (not reachable via querySelector).
If I'm not mistaken, from /toolkit/, you reference strings that are stored in /browser/, this will break in B2G.
What would be the procedure to run this code on a remote target? I would be very cool to analyze the code of the the Firefox OS homescreen :)
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #42)
> What would be the procedure to run this code on a remote target? I would be
> very cool to analyze the code of the the Firefox OS homescreen :)
Wait, isn't remotable a requirement for every new feature?
(In reply to Anthony Ricaud (:rik) from comment #43)
> (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from
> comment #42)
> > What would be the procedure to run this code on a remote target? I would be
> > very cool to analyze the code of the the Firefox OS homescreen :)
> Wait, isn't remotable a requirement for every new feature?

It is remotable. There's just no UI yet.
Attached patch coverage-975522.patch v5 (obsolete) — Splinter Review
I'm going to restrict changes to fixing bugs, review problems and maybe adding new tests.

There are 2 changes in this patch:

* I added a notification bar so people can see when coverage is running, and have an obvious way to stop it, which you can see here: https://github.com/joewalker/gecko-dev/commit/40f51ede31be0dbc96e4ba1738aa0b6b8f215db6

* I move the l10n files to toolkit, so this should work remotely: https://github.com/joewalker/gecko-dev/commit/a5e52f0307c439196f4cc9aa18aa2f0d4f96cc73
Attachment #8425043 - Attachment is obsolete: true
Attachment #8425043 - Flags: review?(fayearthur)
Attachment #8425412 - Flags: review?(fayearthur)
How to run coverage without a command line:

    const coverage = require("devtools/server/actors/coverage");
    let target = <whatever>;
    coverage.getUsage(target).then(usage => { usage.start(); });

Also:

    usage.stop();
    gDevTools.showToolbox(target, "styleeditor");
(In reply to Joe Walker [:jwalker] from comment #35)
> It could, but is it valuable? Is it true that selectors that have only a
> single match are necessarily sloppy, and should be merged? I guess I'm not
> following your point.
I'm not entirely sure it would be useful (my gut tells me yes) but that's for another bug anyway. I just wanted to know about the possibility. So, it's cool.
(In reply to Joe Walker [:jwalker] from comment #3)
> I'm going to concentrate on CSS coverage in this bug. Other bugs track JS
> coverage testing.

Joe, what are these other bugs ?
Comment on attachment 8425412 [details] [diff] [review]
coverage-975522.patch v5

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

This is great. The main issue I found is that it doesn't work on pages with multiple inline style sheets, see comments.

::: browser/devtools/commandline/test/browser_cmd_coverage_startstop.js
@@ +22,5 @@
> +
> +  yield usage.start();
> +
> +  let running = yield usage._testOnly_isRunning();
> +console.log(running);

take out log.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +72,5 @@
>    this._prefObserver = new PrefObserver("devtools.styleeditor.");
>    this._prefObserver.on(PREF_ORIG_SOURCES, this._onNewDocument);
> +
> +  this._loadDeferred = promise.defer();
> +  this.loadPromise = this._loadDeferred.promise;

Can't find where these are used.

@@ +232,5 @@
>  
>      this._root.classList.add("loading");
>    },
>  
> +  getTokens: function(href) {

Comment header for this function. `getTokens` is pretty generic. Can't find `getTokens` being used anywhere though, take it out?

@@ +246,5 @@
> +
> +    return parsePromise;
> +  },
> +
> +  fetchSource: function(href) {

Comment for this function. Also not used except for by 'getTokens', so take it out if we're not using it.

@@ +507,5 @@
>            editor.onShow();
>  
> +          coverage.getUsage(this._target).then(usage => {
> +            let href = editor.styleSheet.href || editor.styleSheet.nodeHref;
> +            usage.createEditorReport(href).then(data => {

This won't work for inline stylesheets ('href' is null). http://google.com is a good test case with a bunch of inline stylesheets. In that instance, every stylesheet has the 1st and 9th line crossed out.

Maybe we could just get a list of unused selectors and find the rules in the stylesheet that match those, to avoid doing it by line number.

::: toolkit/devtools/gcli/commands/coverage.js
@@ +12,5 @@
> +TODO: UI review.
> +TODO: Patrick: Line numbers are off for inline style elements because the
> +      DOMUtils gives the line num relative to the HTML, and the style editor
> +      just uses the contents of the style element
> +TODO: Add tests for start/stop and page report and more selectors

Update this list? looks like there are at least tests for start and stop

::: toolkit/devtools/server/actors/coverage.js
@@ +95,5 @@
> +    this._running = true;
> +    this._tooMuchUnused = false;
> +
> +    this._onTabLoad = this._onTabLoad.bind(this);
> +    this._onChange = this._onChange.bind(this);

why not put these binds in 'initialize'.

@@ +119,5 @@
> +
> +    // Note on dictionary mode. Normally we'd "this.blah = undefined", however
> +    // that would make the next call to start() fail, so we do it properly
> +    delete this._onTabLoad;
> +    delete this._onChange;

Confused about why this is necessary, why do we delete these functions?

@@ +253,5 @@
> +        continue;
> +      }
> +
> +      try {
> +        // If there are more than 100,000 rules to check then

This comment doesn't seem to be related to the code below it.
Attachment #8425412 - Flags: review?(fayearthur) → review-
Since this just does CSS coverage, maybe call the command csscoverage?
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #41)
> If I'm not mistaken, from /toolkit/, you reference strings that are stored
> in /browser/, this will break in B2G.

This https://github.com/joewalker/gecko-dev/commit/3bdaed881dae0fab9b0e4457840fe2fddd4e22a5 is built into the next patch.
(In reply to Heather Arthur [:harth] from comment #50)
> Comment on attachment 8425412 [details] [diff] [review]
> coverage-975522.patch v5
> 
> Review of attachment 8425412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is great. The main issue I found is that it doesn't work on pages with
> multiple inline style sheets, see comments.
> 
> ::: browser/devtools/commandline/test/browser_cmd_coverage_startstop.js
> @@ +22,5 @@
> > +
> > +  yield usage.start();
> > +
> > +  let running = yield usage._testOnly_isRunning();
> > +console.log(running);
> 
> take out log.

https://github.com/joewalker/gecko-dev/commit/5eb8e9f6bf73872bf05b38e6b3176d620f9a07cd

> ::: browser/devtools/styleeditor/StyleEditorUI.jsm
> @@ +72,5 @@
> >    this._prefObserver = new PrefObserver("devtools.styleeditor.");
> >    this._prefObserver.on(PREF_ORIG_SOURCES, this._onNewDocument);
> > +
> > +  this._loadDeferred = promise.defer();
> > +  this.loadPromise = this._loadDeferred.promise;
> 
> Can't find where these are used.

They were used when I was injecting changes into the style editor, but getting the style editor to ask for them turned out to be much cleaner. However I forgot to remove this bit. Good find.

https://github.com/joewalker/gecko-dev/commit/6adeb74f6d51f856ec6c8377c44db38361d50a50

> @@ +232,5 @@
> >  
> >      this._root.classList.add("loading");
> >    },
> >  
> > +  getTokens: function(href) {
> 
> Comment header for this function. `getTokens` is pretty generic. Can't find
> `getTokens` being used anywhere though, take it out?

I used to have code to find the end of a rule using a CSS parser, but there was no need for this because we can only mark lines in the source editor anyway. So this code needs to go for now.

https://github.com/joewalker/gecko-dev/commit/2579371fcddc6ab28f395bc903fc0c108fa5cd94

> @@ +246,5 @@
> > +
> > +    return parsePromise;
> > +  },
> > +
> > +  fetchSource: function(href) {
> 
> Comment for this function. Also not used except for by 'getTokens', so take
> it out if we're not using it.

It's gone. See above.

> @@ +507,5 @@
> >            editor.onShow();
> >  
> > +          coverage.getUsage(this._target).then(usage => {
> > +            let href = editor.styleSheet.href || editor.styleSheet.nodeHref;
> > +            usage.createEditorReport(href).then(data => {
> 
> This won't work for inline stylesheets ('href' is null). http://google.com
> is a good test case with a bunch of inline stylesheets. In that instance,
> every stylesheet has the 1st and 9th line crossed out.
> 
> Maybe we could just get a list of unused selectors and find the rules in the
> stylesheet that match those, to avoid doing it by line number.

When editor.styleSheet.href == null, then we use editor.styleSheet.nodeHref, so I don't think that's the cause of the bug that you saw.

I suspect that the problem was bug 1013896 where the lines get reported wrongly, in conjunction with bug 1013909 (be smarter about marking up compressed stylesheets)

I've added tests for multiple inline <style> elements here:

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

I've also checked out google.com with the fix for bug 1013896, and taking the compression bug into account, it looks right now.

> ::: toolkit/devtools/gcli/commands/coverage.js
> @@ +12,5 @@
> > +TODO: UI review.
> > +TODO: Patrick: Line numbers are off for inline style elements because the
> > +      DOMUtils gives the line num relative to the HTML, and the style editor
> > +      just uses the contents of the style element
> > +TODO: Add tests for start/stop and page report and more selectors
> 
> Update this list? looks like there are at least tests for start and stop

All gone.
https://github.com/joewalker/gecko-dev/commit/1019e8753726f78bb142fc17383d06a7ac2101a1

> ::: toolkit/devtools/server/actors/coverage.js
> @@ +95,5 @@
> > +    this._running = true;
> > +    this._tooMuchUnused = false;
> > +
> > +    this._onTabLoad = this._onTabLoad.bind(this);
> > +    this._onChange = this._onChange.bind(this);
> 
> why not put these binds in 'initialize'.

Done
https://github.com/joewalker/gecko-dev/commit/36646ecf23b3ec3667889264924b327a94686b44

> @@ +119,5 @@
> > +
> > +    // Note on dictionary mode. Normally we'd "this.blah = undefined", however
> > +    // that would make the next call to start() fail, so we do it properly
> > +    delete this._onTabLoad;
> > +    delete this._onChange;
> 
> Confused about why this is necessary, why do we delete these functions?

See above

> @@ +253,5 @@
> > +        continue;
> > +      }
> > +
> > +      try {
> > +        // If there are more than 100,000 rules to check then
> 
> This comment doesn't seem to be related to the code below it.

Fixed:

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

Also I fixed a couple of typos and added a const for the number, and reduced it to 10,000.
(In reply to Heather Arthur [:harth] from comment #51)
> Since this just does CSS coverage, maybe call the command csscoverage?

The same argument went for the modules called coverage.js so the patch turned out to be large, but it's largely just a s/coverage/csscoverage/g

https://github.com/joewalker/gecko-dev/commit/afb6fed01b9ece5a315b96e17b6c2a303040c185
The last try (see comment 46) had an xpcshell failure, which was trivial to fix - just lazy load gDevTools.

https://github.com/joewalker/gecko-dev/commit/68debb3d0d769b8396293e04d09cc9a3c1eb0aee
Attached patch coverage-975522.patch v6 (obsolete) — Splinter Review
This patch includes the changes from comment 52 to 55 and bug 1013896 and bug 1013897, and nothing else.

https://tbpl.mozilla.org/?tree=Try&rev=b168e3c66fdd
Attachment #8425412 - Attachment is obsolete: true
Attachment #8426385 - Flags: review?(fayearthur)
(In reply to Joe Walker [:jwalker] from comment #53)
> > This won't work for inline stylesheets ('href' is null). http://google.com
> > is a good test case with a bunch of inline stylesheets. In that instance,
> > every stylesheet has the 1st and 9th line crossed out.
> >
> 
> When editor.styleSheet.href == null, then we use editor.styleSheet.nodeHref,
> so I don't think that's the cause of the bug that you saw.

nodeHref would be the same for every inline stylesheet I think, on google.com they'd all have a ownerNode.baseURI of 'http://google.com'.
(In reply to Heather Arthur [:harth] from comment #57)
> (In reply to Joe Walker [:jwalker] from comment #53)
> > > This won't work for inline stylesheets ('href' is null). http://google.com
> > > is a good test case with a bunch of inline stylesheets. In that instance,
> > > every stylesheet has the 1st and 9th line crossed out.
> > >
> > 
> > When editor.styleSheet.href == null, then we use editor.styleSheet.nodeHref,
> > so I don't think that's the cause of the bug that you saw.
> 
> nodeHref would be the same for every inline stylesheet I think, on
> google.com they'd all have a ownerNode.baseURI of 'http://google.com'.

Ah, yes. I raise bug 1014223 and made it block turning this on. I'll take a look tomorrow.
Comment on attachment 8426385 [details] [diff] [review]
coverage-975522.patch v6

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

Talked on irc, but besides large commit message and this variable name, looks good.

::: toolkit/devtools/server/actors/stylesheets.js
@@ +804,5 @@
> +
> +     let absoluteStartLine;
> +     let i = 0;
> +     while (absoluteStartLine == null) {
> +       let r = sheet.cssRules[i];

this one-letter variable
Attachment #8426385 - Flags: review?(fayearthur) → review+
(In reply to Heather Arthur [:harth] from comment #59)
> Comment on attachment 8426385 [details] [diff] [review]
> coverage-975522.patch v6
> 
> Review of attachment 8426385 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Talked on irc, but besides large commit message and this variable name,
> looks good.
> 
> ::: toolkit/devtools/server/actors/stylesheets.js
> @@ +804,5 @@
> > +
> > +     let absoluteStartLine;
> > +     let i = 0;
> > +     while (absoluteStartLine == null) {
> > +       let r = sheet.cssRules[i];
> 
> this one-letter variable

https://github.com/joewalker/gecko-dev/commit/5ba5819f4e54fc1c9dca51f9d9b1eb6fdec11058
Attachment #8426385 - Attachment is obsolete: true
Supergreen https://tbpl.mozilla.org/?tree=Try&rev=b168e3c66fdd
Only changed a variable name, and patch comments since. Will push.
https://hg.mozilla.org/mozilla-central/rev/97304755abe8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
> Keywords: dev-doc-needed

There is https://developer.mozilla.org/en-US/docs/Tools/CSS_Coverage - do we still need this flag?
Flags: needinfo?(saneyuki.s.snyk)
(In reply to Joe Walker [:jwalker] from comment #65)
> There is https://developer.mozilla.org/en-US/docs/Tools/CSS_Coverage - do we
> still need this flag?

Oops! I think we don't need "dev-doc-needed".
Flags: needinfo?(saneyuki.s.snyk)
(In reply to Joe Walker [:jwalker] from comment #61)
> Created attachment 8426914 [details] [diff] [review]
> coverage-975522.patch v7

<!ENTITY csscoverage.optimize "Opimizable Pages">
Opimizable -> Optimizable.
Flags: needinfo?(jwalker)
(In reply to YF (Yang) from comment #67)
> (In reply to Joe Walker [:jwalker] from comment #61)
> > Created attachment 8426914 [details] [diff] [review]
> > coverage-975522.patch v7
> 
> <!ENTITY csscoverage.optimize "Opimizable Pages">
> Opimizable -> Optimizable.

Thanks. Will fix, probably in bug 1013887.
Flags: needinfo?(jwalker)
Blocks: 1016330
Depends on: 1023943
No longer depends on: 1023943
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.