If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

replace uses of inIDOMUtils.getCSSPseudoElementNames

RESOLVED FIXED in Firefox 50

Status

()

Firefox
Developer Tools: Framework
P1
enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: tromey, Assigned: gregtatum)

Tracking

unspecified
Firefox 50
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
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
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d018785d9a7d
Created attachment 8768842 [details] [diff] [review]
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.
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+
Created attachment 8770127 [details] [diff] [review]
replace uses of inIDOMUtils.getCSSPseudoElementNames

Fixes the pseudo typos and updates the test failure messages.
Attachment #8770127 - Flags: review+
(Assignee)

Updated

a year ago
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.
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 7

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/c194fb332a11
replace uses of inIDOMUtils.getCSSPseudoElementNames r=pbro
Keywords: checkin-needed

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c194fb332a11
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.