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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mats Palmgren (inactive)]

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!

Comment 1

[Gregory Szorc [:gps]]

`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.

Comment 2

[Mats Palmgren (inactive)]

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.

Comment 3

[Gregory Szorc [: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.

Comment 4

[Mats Palmgren (inactive)]

People keep tripping over this: https://hg.mozilla.org/integration/mozilla-inbound/rev/71fff6495d23e355d5147d9673dfd5df30d3953c

Comment 5

[Mats Palmgren (inactive)]

(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 (?)

Comment 6

[Mats Palmgren (inactive)]

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.

Comment 7

[Daniel Holbert [:dholbert]]

[Adding dependency on bug 1290988 which is where this file (properties-db.js) & command (./mach devtools-css-db) were created.]

Comment 8

[Mats Palmgren (inactive)]

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?

Comment 9

[Tom Tromey :tromey]

Pinging Greg.

Comment 10

[Greg Tatum [:gregtatum]]

: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

Comment 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[Greg Tatum [:gregtatum]]

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.

Comment 13

[Tom Tromey :tromey]

There is also data that isn't in declarative form, like
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/inspector/inDOMUtils.cpp#739

Comment 14

[Emilio Cobos Álvarez (:emilio)]

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 :)

Comment 15

[Greg Tatum [:gregtatum]]

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.

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 17

[Alexandre Poirot [:ochameau]]

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.

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1320607 - [devtools] Remove the generated CSS Properties database.

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.

Comment 19

[Daniel Holbert [:dholbert]]

We layout folks are thrilled to see this being addressed. :) Thanks, everyone!

Comment 20

[Alexandre Poirot [:ochameau]]

(In reply to Daniel Holbert [:dholbert] from comment #19)

We layout folks are thrilled to see this being addressed. :) Thanks, everyone!

In the name of DevTools, sorry for that thing that bug your team for a little while...

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb19de542f01
[devtools] Remove the generated CSS Properties database. r=devtools-reviewers,nchevobbe

Comment 22

[Atila Butkovits]

Backed out for causing python failures complaining about test_devtools_database_parsing .

Backout link: https://hg.mozilla.org/integration/autoland/rev/5ed2d82563db9ab07b081fa10544fb713c174dc2

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=UC77my7nTT6A3jUsYMEk2Q.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=fb19de542f01cf7f7cbddaab20c3447fb25896f9

Failure log: https://treeherder.mozilla.org/logviewer?job_id=428295903&repo=autoland&lineNumber=648

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a515d873b097
[devtools] Remove the generated CSS Properties database. r=devtools-reviewers,nchevobbe

Comment 24

[Alexandre Poirot [:ochameau]]

I didn't know about that test coverage. I hope that's the last bit related to this generated file.

Comment 25

[Serban Stanca [:SerbanS]]

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

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Does probe-scraper still need this file? See https://phabricator.services.mozilla.com/D187492#6210969

Comment 27

[Chris H-C :chutten]

probe-scraper expects that file, yes. If it can't be retrieved, it'll try to be resilient and assume that those use counters no longer exist.

Now, since we can't actually delete columns, this shouldn't have an immediate deleterious effect on things... but the next CSS property added will not be reflected in the pipeline.

If this isn't terribly important to get in quickly, perhaps make this depend on bug 1852098. Glean doesn't know how to understand properties-db.js and doesn't want to learn, so moving Use Counters to Glean means we'll be able to tidy this up.

Comment 28

[Alexandre Poirot [:ochameau]]

It is hard for me to make the final call.

Should I revert or is it fine to wait for bug 1852098 to be fixed?

Comment 29

[Emilio Cobos Álvarez (:emilio)]

What's the ETA for bug 1852098? Not having use counters for new properties isn't a huge deal if it's not for a very very long time I guess.

Comment 30

[Chris H-C :chutten]

No priority set and no assignee so we'd best ask Nick.

Comment 31

[Nick Alexander :nalexander [he/him]]

(In reply to Chris H-C :chutten from comment #30)

No priority set and no assignee so we'd best ask Nick.

I have no particular ability to get this resourced (and no time to push on it myself). Switching discussion to the ticket itself.

Comment 32

[Nick Alexander :nalexander [he/him]]

Dexter: unping; I'll move the conversation to Bug 1852098.

Comment 33

[Chris H-C :chutten]

In looking into this for bug 1852098, I wonder... Are there any CSS (non-shorthand, non-alias) properties that we'd find in properties-db.js but not in ServoCssPropList.py? It looks like the db's generated solely from the proplist.

If not, then there should actually be no difference with the removal of the db from Use Counter histogram generation, since we also process ServoCSSPropList.py.

Comment 34

[Emilio Cobos Álvarez (:emilio)]

All properties are in ServoCSSPropList.py indeed. I thought we needed the properties-db.js stuff because ServoCSSPropList.py is not in the repo so can't be fetched (without a build) by probe-scraper?

At least that's my recollection from bug 1578661 comment 21 and https://github.com/mozilla/probe-scraper/pull/118.

Comment 35

[Chris H-C :chutten]

Ohhhhh, the non-mozbuild case. Yeah, you're right.

Every time I think I understand this, I find a new way to be wrong. At least it doesn't affect bug 1852098 as it'll only generate things during the build, so it can use the proplist directly.

Sorry for the noise.