Closed
Bug 1265785
Opened 9 years ago
Closed 8 years ago
replace uses of inIDOMUtils.getCSSPseudoElementNames
Categories
(DevTools :: Framework, enhancement, P1)
DevTools
Framework
Tracking
(firefox50 fixed)
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: tromey, Assigned: gregtatum)
References
Details
(Whiteboard: [devtools-html])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
gregtatum
:
review+
|
Details | Diff | Splinter Review |
Replace uses of inIDOMUtils.getCSSPseudoElementNames for devtools
de-chrome-ification project.
Comment 1•9 years ago
|
||
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.
Updated•9 years ago
|
Flags: qe-verify-
Priority: -- → P2
Updated•8 years ago
|
Priority: P2 → P1
Updated•8 years ago
|
Assignee: nobody → gtatum
Status: NEW → ASSIGNED
Iteration: --- → 50.1
Updated•8 years ago
|
Iteration: 50.1 → 50.2
Updated•8 years ago
|
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
Fixes the pseudo typos and updates the test failure messages.
Attachment #8770127 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8768842 -
Attachment is obsolete: true
Assignee | ||
Comment 6•8 years ago
|
||
Thanks Patrick. That all sounds like a great plan. I filed Bug 1286232 for automating and Bug 1286238 for testing.
Assignee | ||
Updated•8 years ago
|
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
Comment 8•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•