1014223 - CSS coverage should not use the same identifier for <style> elements in the same HTML document

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Joe Walker [:jwalker]]

See Bug 975522 comment 57.

Comment 1

[Joe Walker [:jwalker]]

Attachment: 0002-Bug-1014223-Use-unique-identifier-for-style-elements.patch

Comment 2

[Joe Walker [:jwalker]]

Green: https://tbpl.mozilla.org/?tree=Try&rev=3d41d14b4e69

Comment 3

[Joe Walker [:jwalker]]

FWIW, I'd like to change -> to → before we land this.

Comment 4

[Joe Walker [:jwalker]]

Attachment: 0001-Bug-1014223-Use-unique-identifier-for-style-elements.patch

Comment 5

[Joe Walker [:jwalker]]

That update did the -> to → that I mentioned.

Comment 6

[Heather Arthur [:harth]]

Comment on attachment 8438559 [details] [diff] [review]
0001-Bug-1014223-Use-unique-identifier-for-style-elements.patch

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

I'd like to get rid of the _testOnly methods if possible. Open to discussion though.

::: toolkit/devtools/server/actors/csscoverage.js
@@ +21,5 @@
> +  return require("devtools/server/actors/stylesheets");
> +});
> +loader.lazyGetter(this, "CssLogic", () => {
> +  return require("devtools/styleinspector/css-logic").CssLogic;
> +});

Doesn't look like you ended up using CssLogic.

@@ +438,3 @@
>     * For testing only. Is css coverage running.
>     */
>    _testOnly_isRunning: method(function() {

There's no reason for this to be test-only I think. If it's useful to us, it could be useful for someone else.

Another option is to keep a '.running' variable up-to-date on the Front. That's what I do for instance in the MediaRuleFront in stylesheets.js, I keep the .matches property up to date by listening for events.

@@ +445,5 @@
>  
> +  /**
> +   * For testing only. What pages did we visit.
> +   */
> +  _testOnly_visitedPages: method(function() {

There's got to be another way. Can you have rules on each page, and check that a rule from each page shows up?

@@ +454,5 @@
> +
> +  /**
> +   * For testing only. What pages/sheets/rules did we discover.
> +   */
> +  _testOnly_dump: method(function() {

This method isn't used anywhere. I'd get rid of this.

@@ +700,5 @@
> +  if (stylesheet.ownerNode) {
> +    let document = stylesheet.ownerNode.ownerDocument;
> +    let sheets = [...document.querySelectorAll("style")];
> +    let index = sheets.indexOf(stylesheet.ownerNode);
> +    return getURL(document) + ' → <style> index ' + index;

If you don't want to mess with "->", ":" would be good I think.

Comment 7

[Joe Walker [:jwalker]]

(In reply to Heather Arthur [:harth] from comment #6)
> Comment on attachment 8438559 [details] [diff] [review]
> 0001-Bug-1014223-Use-unique-identifier-for-style-elements.patch
> 
> Review of attachment 8438559 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to get rid of the _testOnly methods if possible. Open to discussion
> though.
> 
> ::: toolkit/devtools/server/actors/csscoverage.js
> @@ +21,5 @@
> > +  return require("devtools/server/actors/stylesheets");
> > +});
> > +loader.lazyGetter(this, "CssLogic", () => {
> > +  return require("devtools/styleinspector/css-logic").CssLogic;
> > +});
> 
> Doesn't look like you ended up using CssLogic.

One of the coverage commands is currently broken because it's missing. So it is needed even though I didn't add the 

> @@ +438,3 @@
> >     * For testing only. Is css coverage running.
> >     */
> >    _testOnly_isRunning: method(function() {
> 
> There's no reason for this to be test-only I think. If it's useful to us, it
> could be useful for someone else.
> 
> Another option is to keep a '.running' variable up-to-date on the Front.
> That's what I do for instance in the MediaRuleFront in stylesheets.js, I
> keep the .matches property up to date by listening for events.

The reason it's _testOnly_ is that it's dangerous to do "if (usage.isRunning) usage.stop", but I think it's something we should be testing, and in testing we can be sure that only one thing is attacking it at a time. So the theory is that normal use only needs start/stop/toggle.

Maybe we'll need this for a start/stop button in the UI though.

> @@ +445,5 @@
> >  
> > +  /**
> > +   * For testing only. What pages did we visit.
> > +   */
> > +  _testOnly_visitedPages: method(function() {
> 
> There's got to be another way. Can you have rules on each page, and check
> that a rule from each page shows up?

Is there a problem with _testOnly methods? I'm not sure what the issue is.

> @@ +700,5 @@
> > +  if (stylesheet.ownerNode) {
> > +    let document = stylesheet.ownerNode.ownerDocument;
> > +    let sheets = [...document.querySelectorAll("style")];
> > +    let index = sheets.indexOf(stylesheet.ownerNode);
> > +    return getURL(document) + ' → <style> index ' + index;
> 
> If you don't want to mess with "->", ":" would be good I think.

Got it working OK now, temporarily tripped up by the ACSII delivery of JS by mochitest. I kind of like →.

Comment 8

[Heather Arthur [:harth]]

(In reply to Joe Walker [:jwalker] from comment #7)
> Is there a problem with _testOnly methods? I'm not sure what the issue is.

API aesthetics and usage. I still think we could get rid of the _testOnly_dump since it's not being used.

Comment 9

[Joe Walker [:jwalker]]

(In reply to Heather Arthur [:harth] from comment #8)
> (In reply to Joe Walker [:jwalker] from comment #7)
> > Is there a problem with _testOnly methods? I'm not sure what the issue is.
> 
> API aesthetics and usage. I still think we could get rid of the
> _testOnly_dump since it's not being used.

I've got rid of _testOnly_dump.

I raised bug 1025044 which should allow us to get rid of _testOnly_isRunning, and I think we'll need it anyway.

I've got 3 options with _testOnly_visitedPages:

1. Create a non-ugly version that is exposed properly, and only used for testing except that
   we need to maintain backwards compatibility on it. I very much don't like this solution.
2. Just delete the test
3. Keep a deliberately ugly function that we can change without worrying about breaking things

I'm happy to hear better solutions, but right now 3. seems like the best option.

Comment 10

[Joe Walker [:jwalker]]

Attachment: 0001-Bug-1014223-Use-unique-identifier-for-style-elements.patch

Comment 11

[Heather Arthur [:harth]]

Comment on attachment 8439969 [details] [diff] [review]
0001-Bug-1014223-Use-unique-identifier-for-style-elements.patch

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

Sounds good to me.

Comment 12

[Joe Walker [:jwalker]]

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa3120af95db