Closed Bug 1265785 Opened 4 years ago Closed 4 years ago

replace uses of inIDOMUtils.getCSSPseudoElementNames

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox50 --- fixed

People

(Reporter: tromey, Assigned: gregtatum)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file, 1 obsolete file)

Replace uses of inIDOMUtils.getCSSPseudoElementNames for devtools
de-chrome-ification project.
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.
Flags: qe-verify-
Priority: -- → P2
Priority: P2 → P1
Assignee: nobody → gtatum
Status: NEW → ASSIGNED
Iteration: --- → 50.1
Iteration: 50.1 → 50.2
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
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.
Attachment #8768842 - Flags: review?(pbrosset)
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
Attachment #8768842 - Flags: review?(pbrosset) → review+
Fixes the pseudo typos and updates the test failure messages.
Attachment #8770127 - Flags: review+
Attachment #8768842 - Attachment is obsolete: true
Thanks Patrick. That all sounds like a great plan. I filed Bug 1286232 for automating and Bug 1286238 for testing.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/c194fb332a11
replace uses of inIDOMUtils.getCSSPseudoElementNames r=pbro
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c194fb332a11
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.