1290988 - Generate CSS properties database correctly for different versions.

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P1)

Description

[Greg Tatum [:gregtatum]]

The CSS property database currently generates the list of the properties on the server [0], and there is a test that enforces that the static client list is in sync with the server [1]. Unfortunately some of the properties do get uplifted from Nightly to Aurora, as they are protected by an #ifdef as they are in development (see [2]). During the uplifts this test will begin to fail as the client and server properties list will no longer be in sync.

[0] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/css-properties-db.js
[1] https://dxr.mozilla.org/mozilla-central/source/devtools/server/tests/mochitest/test_css-properties.html
[2] https://dxr.mozilla.org/mozilla-central/rev/465d150bc8be5bbf9f02a8607d4552b6a5e1697c/modules/libpref/init/all.js#4788

Comment 1

[Greg Tatum [:gregtatum]]

Attachment: Bug 1290988 - Collect devtools/shared/css-* files into a folder;

In preparation for the additional files in the `mach generate-css-db`
command, collect the CSS files into a folder.

Review commit: https://reviewboard.mozilla.org/r/75006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75006/

Comment 2

[Greg Tatum [:gregtatum]]

Attachment: Bug 1290988 - Add the mach generate-css-db command;

Provide a single mach command to automatically generate the static
database of CSS properties that devtools uses for the inspector
and various editors.

Review commit: https://reviewboard.mozilla.org/r/75008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75008/

Comment 3

[Greg Tatum [:gregtatum]]

Attachment: Bug 1290988 - Take into account prefs for the CSS properties database;

This test was causing uplifts to fail because certain CSS properties
were disabled via preferences, and the devtools client and platform were
out of sync with what properties were supported. This test now uses the
preference information to allow in-development features to not break
this test.

Review commit: https://reviewboard.mozilla.org/r/75010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75010/

Comment 4

[Greg Tatum [:gregtatum]]

chmanchester: I asked in #build on IRC for a good person to ping for a review. I'm adding a mach command here and writing some python for it. I also flagged tromey on that commit for the JS-side of things.

Comment 5

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54ca739b43b7

Comment 6

[Tom Tromey :tromey]

Comment on attachment 8786052 [details]
Bug 1290988 - Collect devtools/shared/css-* files into a folder;

https://reviewboard.mozilla.org/r/75006/#review73204

Thanks for doing this.  It looks good to me.  I think it's a bit tidier.

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8786052 [details]
Bug 1290988 - Collect devtools/shared/css-* files into a folder;

https://reviewboard.mozilla.org/r/75006/#review73206

Actually I also wanted to ask whether a change to .eslintignore is needed.
I tend to think so.

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8786053 [details]
Bug 1290988 - Add the mach generate-css-db command;

https://reviewboard.mozilla.org/r/75008/#review73208

Thank you.  This seems reasonable.  I'm not really the person to review a new mach command, though I did read through it a bit.

::: devtools/shared/css/generated/mach_commands.py:8
(Diff revision 1)
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +"""
> +This script implements the `mach generate-css-db` command. It runs the C preprocessor
> +on the CSS properties header file to get the list of preferences associated with
> +a specific property, and it runs an xpcshell script that uses nsiDOMUtils to query

It's actually called "inIDOMUtils", not "nsiDOMUtils".

::: devtools/shared/css/generated/mach_commands.py:42
(Diff revision 1)
> +    headerPath = resolve_path(here, '../../../../layout/style/PythonCSSProps.h')
> +
> +    cpp = buildconfig.substs['CPP']
> +
> +    if not cpp:
> +        print "Unable to find the cpp program. Please do a full, non-artifact"

Are we actually guaranteed that mach is run using Python 2?  Other mach commands seem to use print() to be Py3 compatible.

::: devtools/shared/css/generated/mach_commands.py:52
(Diff revision 1)
> +    cmd += shellutil.split(buildconfig.substs['ACDEFINES'])
> +    cmd.append(headerPath)
> +
> +    # The preprocessed list takes the following form:
> +    # [ (name, prop, id, flags, pref, proptype), ... ]
> +    preprocessed = eval(subprocess.check_output(cmd))

Funny.

::: devtools/shared/css/generated/mach_commands.py:95
(Diff revision 1)
> +    print('The database was successfully generated at ' + destination_path)
> +
> +@CommandProvider
> +class MachCommands(MachCommandBase):
> +    @Command(
> +        'generate-css-db', category='post-build',

I tend to think the new command should mention devtools somehow.  Otherwise people may think it has something to do with ordinary uses of CSS.  Unfortunately that would mean a couple of comment updates elsewhere.

Comment 9

[Tom Tromey :tromey]

Comment on attachment 8786054 [details]
Bug 1290988 - Take into account prefs for the CSS properties database;

https://reviewboard.mozilla.org/r/75010/#review73214

Looks good.  Thank you.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8786053 [details]
Bug 1290988 - Add the mach generate-css-db command;

https://reviewboard.mozilla.org/r/75008/#review73274

::: devtools/shared/css/generated/mach_commands.py:37
(Diff revision 1)
> +    return os.path.normpath(os.path.join(start, relativePath))
> +
> +def get_preferences():
> +    """Get all of the preferences associated with enabling and disabling a property."""
> +    # Build the command to run the preprocessor on PythonCSSProps.h
> +    headerPath = resolve_path(here, '../../../../layout/style/PythonCSSProps.h')

Instead of using `here` throughout you can use `buildconfig.topsrcdir` as a starting point for building srcdir paths.

::: devtools/shared/css/generated/mach_commands.py:46
(Diff revision 1)
> +    cmd = shellutil.split(cpp)
> +    cmd += shellutil.split(buildconfig.substs['ACDEFINES'])
> +    cmd.append(headerPath)

We already have Python code in the tree doing this, in `layout/style/GenerateCSSPropsGenerated.py`. Can you re-use that?

::: devtools/shared/css/generated/mach_commands.py:56
(Diff revision 1)
> +    # [ (name, prop, id, flags, pref, proptype), ... ]
> +    preprocessed = eval(subprocess.check_output(cmd))
> +
> +    # Map this list
> +    # (name, prop, id, flags, pref, proptype) => (name, pref)
> +    preferences = [(x[0], x[4]) for x in preprocessed if 'CSS_PROPERTY_INTERNAL' not in x[3]]

This can be `preferences = [name, pref for name, prop, id, flags, pref, proptype in preprocessed if 'CSS_PROPERTY_INTERNAL' not in flags]` if that seems clearer.

::: devtools/shared/css/generated/mach_commands.py:67
(Diff revision 1)
> +    build = MozbuildObject.from_environment()
> +
> +    # Get the paths
> +    script_path = resolve_path(here, 'generate-properties-db.js')
> +    xpcshell_path = build.get_binary_path(what='xpcshell')
> +    browser_path = resolve_path(xpcshell_path, '../../Resources/browser')

This looks Mac specific. If this is required let's bail out early if someone tries to run this elsewhere.

::: devtools/shared/css/generated/mach_commands.py:79
(Diff revision 1)
> +    handle = open(js_template_path, 'r')
> +    js_template = handle.read()
> +    handle.close()

`with` statement syntax is preferred for handling files.  This example would become:

    with open(js_template_path, 'r') as handle:
        js_template = handle.read()

Writing the destination below can be handled in a similar fashion.

Comment 11

[Greg Tatum [:gregtatum]]

Comment on attachment 8786053 [details]
Bug 1290988 - Add the mach generate-css-db command;

https://reviewboard.mozilla.org/r/75008/#review73274

> We already have Python code in the tree doing this, in `layout/style/GenerateCSSPropsGenerated.py`. Can you re-use that?

Yeah that's how I came to my solution. The only thing is that my code is needing to check for the existence of the CPP property having a proper value in buildconfig.substs as it's being run from a mach command and not from a mach build.

Are you thinking of having me modify GenerateCSSPropsGenerated.py with something like this?

```python
def preprocess_header(headerPath):
    cpp = buildconfig.substs['CPP']

    # Check for the existence of cpp in case this is run in a context where it may
    # not exist.
    if not cpp:
        print("Unable to find the cpp program. Please do a full, non-artifact")
        print("build and try this again.")
        sys.exit(1)

    cmd = shellutil.split(cpp)
    cmd += shellutil.split(buildconfig.substs['ACDEFINES'])
    cmd.append(headerPath)

    return eval(subprocess.check_output(cmd))
    
def get_properties(preprocessorHeader):
    preprocessed = preprocess_header(preprocessorHeader)
    
    ...
```

The only thing is that now I don't know how to get the function over to my own file. I'm not sure how the python module system works in Gecko. I read through the Python manual page on modules, but I didn't find it helpful. Do you know where this function should live and how I would share it between the 2 files?

> This looks Mac specific. If this is required let's bail out early if someone tries to run this elsewhere.

I found the same thing in obj-*/dist/bin/browser. It appears on Mac, Linux, and Windows.

> `with` statement syntax is preferred for handling files.  This example would become:
> 
>     with open(js_template_path, 'r') as handle:
>         js_template = handle.read()
> 
> Writing the destination below can be handled in a similar fashion.

That is much nicer, thanks.

Comment 12

[Greg Tatum [:gregtatum]]

Comment on attachment 8786052 [details]
Bug 1290988 - Collect devtools/shared/css-* files into a folder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75006/diff/1-2/

Comment 13

[Greg Tatum [:gregtatum]]

Comment on attachment 8786053 [details]
Bug 1290988 - Add the mach generate-css-db command;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75008/diff/1-2/

Comment 14

[Greg Tatum [:gregtatum]]

Comment on attachment 8786054 [details]
Bug 1290988 - Take into account prefs for the CSS properties database;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75010/diff/1-2/

Comment 15

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8786053 [details]
Bug 1290988 - Add the mach generate-css-db command;

https://reviewboard.mozilla.org/r/75008/#review74158

::: devtools/shared/css/generated/mach_commands.py:103
(Diff revisions 1 - 2)
> +        # buildconfig is not available when defining the command, only when running it.
> +        import buildconfig
> +

MachCommandsBase inherits from MozbuildObject, so you should have things like `self.substs` and `self.topobjdir` available from here.

Comment 16

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8786053 [details]
Bug 1290988 - Add the mach generate-css-db command;

https://reviewboard.mozilla.org/r/75008/#review73274

> Yeah that's how I came to my solution. The only thing is that my code is needing to check for the existence of the CPP property having a proper value in buildconfig.substs as it's being run from a mach command and not from a mach build.
> 
> Are you thinking of having me modify GenerateCSSPropsGenerated.py with something like this?
> 
> ```python
> def preprocess_header(headerPath):
>     cpp = buildconfig.substs['CPP']
> 
>     # Check for the existence of cpp in case this is run in a context where it may
>     # not exist.
>     if not cpp:
>         print("Unable to find the cpp program. Please do a full, non-artifact")
>         print("build and try this again.")
>         sys.exit(1)
> 
>     cmd = shellutil.split(cpp)
>     cmd += shellutil.split(buildconfig.substs['ACDEFINES'])
>     cmd.append(headerPath)
> 
>     return eval(subprocess.check_output(cmd))
>     
> def get_properties(preprocessorHeader):
>     preprocessed = preprocess_header(preprocessorHeader)
>     
>     ...
> ```
> 
> The only thing is that now I don't know how to get the function over to my own file. I'm not sure how the python module system works in Gecko. I read through the Python manual page on modules, but I didn't find it helpful. Do you know where this function should live and how I would share it between the 2 files?

One way to go about this would be to do the CPP existence check before calling the other script (or check buildconfig.substs['MOZ_ARTIFACT_BUILDS'] here), and then add that directory to sys.path to make it importable: http://searchfox.org/mozilla-central/rev/3611a7607db7554b9d7065ebfef3d5111f7171b8/accessible/xpcom/AccEventGen.py#13 is an example of this. It's not an extremely clean solution, feel free to leave this to a follow up.

Comment 17

[Greg Tatum [:gregtatum]]

Comment on attachment 8786053 [details]
Bug 1290988 - Add the mach generate-css-db command;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75008/diff/2-3/

Comment 18

[Greg Tatum [:gregtatum]]

Comment on attachment 8786054 [details]
Bug 1290988 - Take into account prefs for the CSS properties database;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75010/diff/2-3/

Comment 19

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b8cce8fc78e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This can't autoland because it has unresolved issues on MozReview. And trying to manually import it leads to a pile of conflicts.

Comment 21

[Greg Tatum [:gregtatum]]

Comment on attachment 8786052 [details]
Bug 1290988 - Collect devtools/shared/css-* files into a folder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75006/diff/2-3/

Comment 22

[Greg Tatum [:gregtatum]]

Comment on attachment 8786053 [details]
Bug 1290988 - Add the mach generate-css-db command;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75008/diff/3-4/

Comment 23

[Greg Tatum [:gregtatum]]

Comment on attachment 8786054 [details]
Bug 1290988 - Take into account prefs for the CSS properties database;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75010/diff/3-4/

Comment 24

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26e0f8306d32

Comment 25

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4986d14c8559

Comment 26

[Greg Tatum [:gregtatum]]

Comment on attachment 8786052 [details]
Bug 1290988 - Collect devtools/shared/css-* files into a folder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75006/diff/3-4/

Comment 27

[Greg Tatum [:gregtatum]]

Comment on attachment 8786053 [details]
Bug 1290988 - Add the mach generate-css-db command;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75008/diff/4-5/

Comment 28

[Greg Tatum [:gregtatum]]

Comment on attachment 8786054 [details]
Bug 1290988 - Take into account prefs for the CSS properties database;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75010/diff/4-5/

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bca17123507
Collect devtools/shared/css-* files into a folder; r=tromey
https://hg.mozilla.org/integration/autoland/rev/d67ee15cd875
Add the mach generate-css-db command; r=chmanchester,tromey
https://hg.mozilla.org/integration/autoland/rev/307a8048995a
Take into account prefs for the CSS properties database; r=tromey

Comment 30

[Greg Tatum [:gregtatum]]

Attachment: Fix require path for devtools/shared/css/lexer

MozReview-Commit-ID: EVGhNmz5YZD

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a569d019fd4a
Fix require path for devtools/shared/css/lexer to fix dt test failures; a=tomcat-sheriffduty

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for xpcshell failures (test_rewriteDeclarations.js) and devtools failures (browser_styleeditor_syncAddProperty.js):

https://hg.mozilla.org/integration/autoland/rev/51d254b8787c45ea15de620e4a71c053f70df011
https://hg.mozilla.org/integration/autoland/rev/83b7f7ee97d1019c22a99d65fcdbde21fa94d565
https://hg.mozilla.org/integration/autoland/rev/e723d5e04ad873cfe9a5898f5c01f8450f856b79
https://hg.mozilla.org/integration/autoland/rev/5b33055d5bc3e990ae91f31ffffd8cf74869f236

Follow-up push still showing failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a569d019fd4a1e2a7be700555f9e81322e40e846

xpcshell failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=3431665&repo=autoland

15:41:45  WARNING -  TEST-UNEXPECTED-FAIL | devtools/client/shared/test/unit/test_rewriteDeclarations.js | xpcshell return code: 0
15:41:45     INFO -  TEST-INFO took 1271ms
15:41:45     INFO -  >>>>>>>
...
15:41:45     INFO -  TEST-PASS | devtools/client/shared/test/unit/test_rewriteDeclarations.js | run_test - [run_test : 502] output for enable sanitize unpaired brace - "walrus: \\\\};" == "walrus: \\\\};"
15:41:45     INFO -  TEST-PASS | devtools/client/shared/test/unit/test_rewriteDeclarations.js | run_test - [run_test : 510] changed result for enable sanitize unpaired brace - {"0":"\\\\}"} deepEqual {"0":"\\\\}"}
15:41:45     INFO -  TEST-PASS | devtools/client/shared/test/unit/test_rewriteDeclarations.js | run_test - [run_test : 502] output for disabled declaration does not need semicolon insertion - "/*! no: semicolon */\\nwalrus: zebra;\\n" == "/*! no: semicolon */\\nwalrus: zebra;\\n"
15:41:45     INFO -  TEST-PASS | devtools/client/shared/test/unit/test_rewriteDeclarations.js | run_test - [run_test : 510] changed result for disabled declaration does not need semicolon insertion - {} deepEqual {}
15:41:45  WARNING -  TEST-UNEXPECTED-FAIL | devtools/client/shared/test/unit/test_rewriteDeclarations.js | run_test - [run_test : 502] output for create commented-out property - "p: v;shoveler: duck;" == "p: v;/*! shoveler: duck; */"
15:41:45     INFO -  /home/worker/workspace/build/tests/xpcshell/tests/devtools/client/shared/test/unit/test_rewriteDeclarations.js:run_test:502
15:41:46     INFO -  /home/worker/workspace/build/tests/xpcshell/head.js:_execute_test:535
15:41:46     INFO -  -e:null:1

devtools failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=3428415&repo=autoland
07:56:48     INFO -  537 INFO TEST-PASS | devtools/client/styleeditor/test/browser_styleeditor_syncAddProperty.js | The new property editor has focus -
07:56:48     INFO -  538 INFO Pressing return to commit and focus the new value field
07:56:48     INFO -  539 INFO Console message: [JavaScript Warning: "XUL box for h3 element contained an inline #text child, forcing all its children to be wrapped in a block." {file: "chrome://devtools/content/styleeditor/styleeditor.xul" line: 0}]
07:56:48     INFO -  540 INFO TEST-UNEXPECTED-FAIL | devtools/client/styleeditor/test/browser_styleeditor_syncAddProperty.js | selector edits are synced - Got
07:56:48     INFO -    body {
07:56:48     INFO -      border-width: 15px;
07:56:48     INFO -      color: red;
07:56:48     INFO -    }
07:56:48     INFO -    #testid {
07:56:48     INFO -      font-size: 4em;
07:56:48     INFO -      background-color: yellow;
07:56:48     INFO -    }
07:56:48     INFO -    , expected
07:56:48     INFO -    body {
07:56:48     INFO -      border-width: 15px;
07:56:48     INFO -      color: red;
07:56:48     INFO -    }
07:56:48     INFO -    #testid {
07:56:48     INFO -      font-size: 4em;
07:56:48     INFO -      /*! background-color: yellow; */
07:56:48     INFO -    }
07:56:48     INFO -  Stack trace:
07:56:48     INFO -      chrome://mochikit/content/browser-test.js:test_is:960
07:56:48     INFO -      chrome://mochitests/content/browser/devtools/client/styleeditor/test/browser_styleeditor_syncAddProperty.js:null:44
07:56:48     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:784:9
07:56:48     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:704:7
07:56:48     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:743:59

Comment 33

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb775f1e77f9

Comment 34

[Greg Tatum [:gregtatum]]

Attachment: Collect devtools/shared/css-* files into a folder; r=tromey

In preparation for the additional files in the `mach generate-css-db`
command, collect the CSS files into a folder.

MozReview-Commit-ID: 9JRVsC2NMK8

Comment 35

[Greg Tatum [:gregtatum]]

Attachment: Add the mach generate-css-db command; r=tromey, r=chmanchester

Provide a single mach command to automatically generate the static
database of CSS properties that devtools uses for the inspector
and various editors.

MozReview-Commit-ID: 8E2jwxF0KbM

Comment 36

[Greg Tatum [:gregtatum]]

Attachment: Take into account prefs for the CSS properties database; r=tromey

This test was causing uplifts to fail because certain CSS properties
were disabled via preferences, and the devtools client and platform were
out of sync with what properties were supported. This test now uses the
preference information to allow in-development features to not break
this test.

MozReview-Commit-ID: 767xRFMHyw9

Comment 37

[Greg Tatum [:gregtatum]]

The review board patches were backed out. I wasn't sure how to update them there, so I attached the new patches to land onto this bug in the old way.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/23dbff2ec614
Collect devtools/shared/css-* files into a folder. r=tromey
https://hg.mozilla.org/integration/fx-team/rev/6ef033c9dcc3
Add the mach generate-css-db command. r=tromey, r=chmanchester
https://hg.mozilla.org/integration/fx-team/rev/7d70ac7d03de
Take into account prefs for the CSS properties database. r=tromey

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23dbff2ec614
https://hg.mozilla.org/mozilla-central/rev/6ef033c9dcc3
https://hg.mozilla.org/mozilla-central/rev/7d70ac7d03de