1216234 - add new DOMUtils method to list pseudo-elements

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Tom Tromey :tromey]

Bug 1215373 points out a list of pseudo elements in css-logic.js:

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/styleinspector/css-logic.js#46

It seems that it would be more future-proof to have a new DOMUtils
method to compute this list.

Maybe this would have to be a method on some actor, to correctly
support cross-debugging.

Comment 1

[Tom Tromey :tromey]

Attachment: add inIDOMUtils.getCSSPseudoElementNames

Preliminary patch.

The old list included ::selection, which isn't actually accepted by
firefox (only the prefixed ::moz-selection is).  I wasn't sure if this
was a leftover from some earlier platform change or whether it is
included in devtools for valence.

The old list also included some UA-only pseudo-elements; this patch
drops them.  It also drops some css-logic code that was apparently
unused.

Comment 2

[Tom Tromey :tromey]

Comment on attachment 8682003 [details] [diff] [review]
add inIDOMUtils.getCSSPseudoElementNames

This replaces a hand-maintained list of pseudo element names with a new
platform call to get the same list that is used internally.  UA-only
pseudo-elements are not listed, since AFAIK we don't deal with those in
devtools.  (Patrick, is that accurate?)

It updates the devtools to use this new API everywhere.
It deletes some apparently unused code in css-logic.js.

The new test is basic: it compares the result against a hard-coded list.
I wasn't sure what else would make sense.

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8682003 [details] [diff] [review]
add inIDOMUtils.getCSSPseudoElementNames

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

r=me on the non-JS files with the below addressed.

::: layout/inspector/inDOMUtils.cpp
@@ +1186,5 @@
>    // NotPseudoClass is ok.
>    return sPseudoClassStates[nsCSSPseudoClasses::GetPseudoType(atom)];
>  }
>  
> +NS_METHOD

This should be NS_IMETHODIMP.

@@ +1195,5 @@
> +  for (int i = 0; i < nsCSSPseudoElements::ePseudo_PseudoElementCount; ++i) {
> +    nsCSSPseudoElements::Type type = static_cast<nsCSSPseudoElements::Type>(i);
> +    if (!nsCSSPseudoElements::PseudoElementIsUASheetOnly(type)) {
> +      nsIAtom* atom = nsCSSPseudoElements::GetPseudoAtom(type);
> +      array.AppendElement(nsAtomString(atom));

I think this will make a copy of the atom's string.  We could avoid that by building up an array of nsIAtom*s instead of nsStrings, and then passing nsDependentAtomString(array[i]) to ToNewUnicode.

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8682003 [details] [diff] [review]
add inIDOMUtils.getCSSPseudoElementNames

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

What are UA-only pseudo-elements? Are they the list of pseudos we used to remove in styles.js:
:-moz-meter-bar, :-moz-list-bullet, :-moz-list-number, :-moz-focus-inner, :-moz-focus-outer, :-moz-math-anonymous, :-moz-math-stretchy ?
I'm not too sure about what we can or can't edit from devtools.

Apart from this, the code changes look good to me. I just made a comment about some code that I think should be removed, and about the test. Please let me know what you think and I can quickly R+ this.

::: devtools/server/actors/styles.js
@@ +37,4 @@
>  
> +loader.lazyGetter(this, "PSEUDO_ELEMENTS", () => {
> +  return ;
> +});

This block should be removed, right?

::: layout/inspector/tests/test_getCSSPseudoElementNames.html
@@ +38,5 @@
> +  for (let extra of expected) {
> +    info("extra element: " + extra);
> +  }
> +
> +  is(expected.size, 0, "no extra names are included");

This means the test will fail every time a new pseudo is added to the platform. I don't think this is what we want.
It would be a good way to make sure this test is up to date, for sure.
But is it a role of a devtools test to fail when this happens? Do we have UI in devtools that needs changing and/or checking when a new pseudo is added? If yes, then maybe we should keep this assertion, with a comment that says something like "please talk to devtools, adding a new pseudo has impact on devtools code, etc.". If no, then I think the assertion needs to be removed, or replaced by a todo() https://developer.mozilla.org/en/docs/Mochitest#Test_functions

Comment 5

[Tom Tromey :tromey]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #4)
> Comment on attachment 8682003 [details] [diff] [review]
> add inIDOMUtils.getCSSPseudoElementNames
> 

> What are UA-only pseudo-elements? Are they the list of pseudos we used to
> remove in styles.js:
> :-moz-meter-bar, :-moz-list-bullet, :-moz-list-number, :-moz-focus-inner,
> :-moz-focus-outer, :-moz-math-anonymous, :-moz-math-stretchy ?
> I'm not too sure about what we can or can't edit from devtools.

Yes, those are UA-only (except :-moz-math-stretchy, which no longer exists -- one
of the ways that the devtools were out of sync with platform).

A UA-only pseudo element is one that isn't available to content.
See https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPseudoElements.h#36

I think those are the ones that were previously dropped in styles.js.

It wasn't clear to me if we want to support those when browser styles are enabled?


> > +loader.lazyGetter(this, "PSEUDO_ELEMENTS", () => {
> > +  return ;
> > +});
> 
> This block should be removed, right?

Yes, oops.

> ::: layout/inspector/tests/test_getCSSPseudoElementNames.html
> @@ +38,5 @@
> > +  for (let extra of expected) {
> > +    info("extra element: " + extra);
> > +  }
> > +
> > +  is(expected.size, 0, "no extra names are included");
> 
> This means the test will fail every time a new pseudo is added to the
> platform. I don't think this is what we want.
> It would be a good way to make sure this test is up to date, for sure.
> But is it a role of a devtools test to fail when this happens? Do we have UI
> in devtools that needs changing and/or checking when a new pseudo is added?
> If yes, then maybe we should keep this assertion, with a comment that says
> something like "please talk to devtools, adding a new pseudo has impact on
> devtools code, etc.". If no, then I think the assertion needs to be removed,
> or replaced by a todo()
> https://developer.mozilla.org/en/docs/Mochitest#Test_functions

I think there's nothing in particular to do, so I will do the todo thing.

Comment 6

[Tom Tromey :tromey]

Attachment: add inIDOMUtils.getCSSPseudoElementNames

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8683351 [details] [diff] [review]
add inIDOMUtils.getCSSPseudoElementNames

Updated per review; see comments in bug for responses to your questions.

Comment 8

[Patrick Brosset <:pbro>]

(In reply to Tom Tromey :tromey from comment #5)
> It wasn't clear to me if we want to support those when browser styles are
> enabled?
Possibly, but maybe in another bug.

Comment 9

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c8ca27d36f3

Comment 10

[Tom Tromey :tromey]

Attachment: add inIDOMUtils.getCSSPseudoElementNames

Update to fix try issues.

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8684439 [details] [diff] [review]
add inIDOMUtils.getCSSPseudoElementNames

Patrick, I thought perhaps you should review the test changes I made.

In particular, in browser_ruleview_user-agent-styles.js, I think what
is going on is that we see multiple rule entries from getApplied.
However, a couple of these have different pseudo-elements but the same
underlying rule -- and in the UI this is shown just once, throwing off
the count in this test.  So, this arranges to de-duplicate the list in
the test.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8684439 [details] [diff] [review]
add inIDOMUtils.getCSSPseudoElementNames

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

The test changes look good to me.

::: devtools/client/styleinspector/test/browser_ruleview_user-agent-styles.js
@@ +169,5 @@
> +      entrySet.add(entry.rule);
> +      newEntries.push(entry);
> +    }
> +  }
> +  entries = newEntries;

nit: You can do the same thing without having to create 2 data structures:

let entryMap = new Map();
for (let entry of entries) {
  entryMap.set(entry.rule, entry);
}
entries = [...entryMap.entries()];

Comment 13

[Tom Tromey :tromey]

Attachment: add inIDOMUtils.getCSSPseudoElementNames

Simplify test case per review.

Comment 14

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d5914c73b74

Comment 15

[Tom Tromey :tromey]

You may be concerned by the dt6 failures in that try run.
However, locally I see these same failures without this patch.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f1d90a17ebcc

Comment 17

[Carsten Book [:Tomcat]]

sorry had to back this out again for dt6 failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5652372&repo=fx-team

Comment 18

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/0edf9ec8ff4c

Comment 19

[Tom Tromey :tromey]

TIL that the tests in question will time out on Xvfb or Xnest, but not if I run
them against the real X server.  This is why I thought they were already failing
locally :-(

I've reproduced the problem here, for real this time, and I'm working on it.

Comment 20

[Tom Tromey :tromey]

Attachment: add inIDOMUtils.getCSSPseudoElementNames

Rebased on the patch for bug 1223452.
The change is trivial, just a bit of differing context.

Comment 21

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=652ad41b7b78

Comment 22

[Carsten Book [:Tomcat]]

Hi Tim, this seems to have problems again, see patching file devtools/client/styleinspector/rule-view.js
Hunk #1 FAILED at 7
1 out of 2 hunks FAILED -- saving rejects to file devtools/client/styleinspector/rule-view.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1216234---add-inIDOMUtilsgetCSSPseudoElementNa.patch
Tomcats-MacBook-Pro-2:fx-team Tomcat$

Comment 23

[Tom Tromey :tromey]

Attachment: add inIDOMUtils.getCSSPseudoElementNames

Rebased.

Comment 24

[Tom Tromey :tromey]

Comment on attachment 8694211 [details] [diff] [review]
add inIDOMUtils.getCSSPseudoElementNames

I rebased the patch, which should help.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/479af4805b1e

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/479af4805b1e