1265785 - replace uses of inIDOMUtils.getCSSPseudoElementNames

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

Replace uses of inIDOMUtils.getCSSPseudoElementNames for devtools
de-chrome-ification project.

Comment 1

[Patrick Brosset <:pbro>]

This is used in one spot only in the toolbox client's code, in
devtools\client\inspector\rules\models\element-style.js

In summary, this file does something like:

let domUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
domUtils.getCSSPseudoElementNames();

which returns (at the moment):

:after,:before,:backdrop,:first-letter,:first-line,:-moz-selection,:-moz-focus-inner,:-moz-focus-outer,:-moz-list-bullet,:-moz-list-number,:-moz-math-anonymous,:-moz-progress-bar,:-moz-range-track,:-moz-range-progress,:-moz-range-thumb,:-moz-meter-bar,:-moz-placeholder,:-moz-color-swatch

This is typically platform-dependent information that should instead be retrieved async from whichever backend the toolbox is attached to.

Comment 2

[Greg Tatum [:gregtatum]]

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

Comment 3

[Greg Tatum [:gregtatum]]

Attachment: replace uses of inIDOMUtils.getCSSPseudoElementNames

I implemented both the static list of pseudo element names, plus a dynamic list
on the CssProperties actor. I also started a test for the css-properties-db.
I plan on fleshing the test out a bit more on my other open bugs.

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8768842 [details] [diff] [review]
replace uses of inIDOMUtils.getCSSPseudoElementNames

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

As we discussed before, we'll need to think of a way to start generating css-properties-db.js automatically. It's becoming hard to maintain manually.
Could you file a bug for it?
Maybe we should have some instructions in the file that explain which data is generated and how. I think it might be enough to have a big comment just before the generated data to avoid people manually changing it, and then have a script that people can run and then copy the data back to the file, or something simple like this. Having a fully automated thing as part of the build might be hard.

Thanks for adding the new test. We'll need to add checks for css properties too, right? But probably not in this bug, so, could you please file another one for this?

::: devtools/shared/tests/unit/test_css-properties-db.js
@@ +10,5 @@
> +
> +const {PSEUDO_ELEMENTS} = require("devtools/shared/css-properties-db");
> +
> +function run_test() {
> +  // Check that the platform and client match for psuedo elements.

s/psuedo/pseudo

@@ +16,5 @@
> +  let foundPseudoElements = 0;
> +
> +  for (let element of PSEUDO_ELEMENTS) {
> +    const hasElement = platformPseudoElements.includes(element);
> +    ok(hasElement, `${element} psuedo element is on the platform.`);

So, if this assertion (and the one further below) fails, it means pseudos have been added, removed or changed. In which case we'll want to update css-properties-db.js.
The assertion should therefore say this. It's very likely that when this happens, it will be as a result of someone making C++ changes to the platform, and probably not knowing why this test is failing.
I believe the assertion should be very self explanatory. Something like: "Devtools maintains a static list of pseudo elements in css-properties-db.js that now seem to be out of sync with what the platform supports. css-properties-db.js should be updated, please read the update instructions in the file, or NI? someone from devtools on your bug"

@@ +21,5 @@
> +    foundPseudoElements += hasElement ? 1 : 0;
> +  }
> +
> +  equal(foundPseudoElements, platformPseudoElements.length,
> +        "The client side database of psuedo element names were all found on " +

s/psuedo/pseudo

Comment 5

[Greg Tatum [:gregtatum]]

Attachment: replace uses of inIDOMUtils.getCSSPseudoElementNames

Fixes the pseudo typos and updates the test failure messages.

Comment 6

[Greg Tatum [:gregtatum]]

Thanks Patrick. That all sounds like a great plan. I filed Bug 1286232 for automating and Bug 1286238 for testing.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c194fb332a11
replace uses of inIDOMUtils.getCSSPseudoElementNames r=pbro

Comment 8

[Carsten Book [:Tomcat]]

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