Open Bug 1320607 Opened 8 years ago Updated 1 year ago

Investigate removing devtools/shared/css/generated/properties-db.js

Categories

(Firefox Build System :: General, defect)

defect

Tracking

(firefox53 affected)

ASSIGNED
Tracking Status
firefox53 --- affected

People

(Reporter: MatsPalmgren_bugz, Assigned: ochameau)

References

Details

Attachments

(1 file)

In bug 1319958 I added a few new CSS property shorthands. This made some devtools test fail with this error message: TEST-UNEXPECTED-FAIL | devtools/shared/tests/unit/test_css-properties-db.js | run_test/< - [run_test/< : 78] The static database and platform do not agree on the property "place-content" If this assertion fails, then the client side CSS properties list in devtools is out of sync with the CSS properties on the platform. To fix this assertion run `mach devtools-css-db` to re-generate the client side properties. - false == true This sucks! If devtools/shared/css/generated/properties-db.js is auto-generated (as it seems) then it's something the build system should do!
`mach devtools-css-db` requires the xpcshell binary, which means we need binaries built before we generate this file. It also means we need a *host* binary, which means auto-generating this file on cross-compilation builds is significantly harder since we don't have a host native xpcshell binary and obtaining one requires a host native Gecko, which would take minutes to compile. We dealt with a similar problem in bug 903149 where we wanted to use a JS interpreter for minifying shipped JS sources. tl;dr we gave up and used a Python JS minifier. Anyway, our preferred solution to this type of problem is "use Python" (since that's our tooling language of choice). But it looks like generate-properties-db.js calls into Gecko to enumerate CSS properties. And calling into Gecko requires host native Gecko binaries. Is there any way we could extract the CSS properties without requiring Gecko binaries? Extracting from WebIDL, parsing source code, using a manifest file to define properties, or listing properties or properties files in moz.build files are some alternatives. Not sure how practical they are.
Flags: needinfo?(mats)
OK, I agree it seems hard to automate this at build time. > Is there any way we could extract the CSS properties without requiring Gecko binaries? It's probably possible to extract the CSS property names, but it's unlikely the values for each property can be extracted without querying libxul code in some way. (That said, I don't know how `mach devtools-css-db` actually generates the set of property values.) Adding new properties in Gecko is done in layout/style/nsCSSPropList.h, but adding/modifying values is done in other (.cpp) files and not easily accessible. However, doing that requires rebuilding layout/style/ so perhaps we can at least semi-automate this by adding something in the build rules for that directory? Given a list of files, would it be possible to run `mach devtools-css-db` when any of those files change? That would at least alert the developer by producing changes to devtools/shared/tests/unit/test_css-properties-db.js that needs to be merged.
Flags: needinfo?(mats) → needinfo?(gps)
We /could/ write build rules to run things when certain files change. We couldn't do this in automation due to aforementioned reasons. And, there is no precedent today for automatically running processes as part of the build that updates files under source control. Instead, we try to have the build system treat the source directory as read-only. I concede for this use case that magic updating is in the developer's best interest. If we know that changes to certain source files require changes to other checked in files, we can write VCS hooks to detect a failure to update things and prevent the commit/push from succeeding. On the server, we need to keep these hooks as dumb as possible (e.g. little to no file parsing smarts and little to no dependencies on random packages, libraries, etc). The hooks also need to be written in Python so they can access Mercurial's internals. Of course, you /could/ launch a process from Python to do additional processing. That being said, if data influencing change is embedded within .h and .cpp files, there are many changes to .h/.cpp files that won't cause generated files from changing and the hook would need to differentiate those to prevent false positives. So we're back to square 1 of figuring out how to extract the property names and values. FWIW, there is another longer term solution: autoland. We could teach the autolanding tool to regenerate checked in files when certain files change. The benefit is developers don't need to worry about regenerating some things locally. But there are still cases where developers do need to regen locally. But autoland does solve the problem of forgetting to do the regen before you land the commit.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #3) > We /could/ write build rules to run things when certain files change. We > couldn't do this in automation due to aforementioned reasons. Yeah, I agree running things on the server side won't work. VCS hooks probably isn't the right solution either since some changes to these files won't generate changes to properties-db.js so then you'd have to do a manual "dummy" change to that file just to land which doesn't seem like a good idea. > FWIW, there is another longer term solution: autoland. We could teach the > autolanding tool to regenerate checked in files when certain files change. That sounds great, except that it might take years before it happens. ;-) I still think we need some temporary solution, regardless how crude it might be. Auto-running "mach devtools-css-db" whenever some files under layout/style/ are rebuilt seems good enough to me. I think that will prevent me from pushing if it generates uncommited changes to properties-db.js (?)
BTW, running "mach devtools-css-db" on Aurora generates changes to properties-db.js which means we're actually shipping a devtools that is out-of-sync with the style system implementation on that branch. That doesn't seem good. (I've filed bug 1321982) The changeset in comment 4 shows that too - I think the "-moz-tab-size" changes there are related to an earlier unrelated commit.
[Adding dependency on bug 1290988 which is where this file (properties-db.js) & command (./mach devtools-css-db) were created.]
Depends on: 1290988
I tried checking if we're up-to-date on Beta, but "./mach devtools-css-db" fails for me on that branch (filed bug 1321987). Can someone else check please?
Pinging Greg.
Flags: needinfo?(gtatum)
:gps enumerates the problems we've run into quite well. The static database is currently being used in devtools as a fallback when our debuggee can't provide us with an up-to-date database. The most common case in devtools is that you are using devtools that are hosted in the same browser, plus that browser will be able to provide an up-to-date list of CSS properties. This static database is only a fallback. If the fallback is used, there is no guarantee that the debugger and debuggee agree on what properties are supported, so we decided that it was fine to only stick with what Nightly knows. Trying to keep this up to date with every build meant that uplifting from Nightly to Aurora, or Aurora to Beta would fail, as the database would need to be re-generated. If we could automate this using only python, it would be awesome, although I gave my best attempt at doing it already, and had to compromise with using the generate-properties-db.js
Flags: needinfo?(gtatum)
For CSS property names we have precedent right there in layout/style: https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/style/moz.build#263 We generate nsCSSPropsGenerated.inc by running PythonCSSProps.h through the C preprocessor, where that file defines some macros for nsCSSPropList.h to use, then #includes that header with the output of CPP being Python data structures(!) that this script then interprets and writes to another header: https://dxr.mozilla.org/mozilla-central/source/layout/style/GenerateCSSPropsGenerated.py That is all sort of awful, but it works. It looks like that's only about half of what properties-db.js wants, though. As a counterpoint, perhaps we could take the CSS property definitions from nsCSSPropList.h and all the other data that winds up in properties-db.js, generate a data file that we could commit as the ground source of truth, and then have scripts to generate header files for the C++ code to use, as well as the js file for devtools? I don't know what the source of the rest of the data looks like and how feasible that would be.
Anything we can do on the platform side to make the devtools CSS properties database less hacky would be amazing. The problems I was running into was that a lot of the behavior was dynamic in how it was being used, and I couldn't find a way to statically analyze it. There are prefs for whether CSS properties are enabled, and then some were turned on by flags.
Product: Core → Firefox Build System

Is there any reason we can't generate this database in the object directory?

This is the single biggest backout cause when touching the style system :)

Flags: needinfo?(nchevobbe)
Flags: needinfo?(jdescottes)

It needs a fully built binary to generate the list for the reasons listed above. It might be able to be generated if there is a point where you can run the binary to query it.

At this point it would be useful to evaluate whether it can be removed, since this was part of the devtools de-chromifying requirements whenever DevTools failed to move to GitHub. I don't really have the requirements in my head, but it could be that it could be removed. It's also one of the bigger files shipped to end users in the packaged build. If I recall, this call is mostly to get a list of properties from the debuggee.

Severity: normal → S3

As Greg said, this was done for an unsuccessful project, and we could probably remove it entirely

Flags: needinfo?(nchevobbe)
Summary: The build system should auto-generate devtools/shared/css/generated/properties-db.js → Investigate removing devtools/shared/css/generated/properties-db.js
Depends on: 1815877
Flags: needinfo?(jdescottes)

It looks like this was useless in today's codebase.
This was only used by tests codepath, where we can use data queried directly from the runtime.
I added an assertion to see if we miss cssProperties and try looks green:
https://treeherder.mozilla.org/jobs?repo=try&revision=fc32fd0b9e1f10db2581501bb3bf3038253d05ce
Also tested remote debugging without seeing anything wrong.

This wasn't really used anymore.
We are fetching the database from the server runtime in order to support
remote debugging correctly, where frontend CSS may be different from debuggee CSS.

Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: