Closed Bug 1290988 Opened 4 years ago Closed 3 years ago

Generate CSS properties database correctly for different versions.

Categories

(DevTools :: Framework, defect, P1)

50 Branch
defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Iteration:
51.3 - Sep 19
Tracking Status
firefox51 --- fixed

People

(Reporter: gregtatum, Assigned: gregtatum)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-html])

Attachments

(6 files, 1 obsolete file)

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
Whiteboard: [devtools-html]
Priority: P2 → --
Whiteboard: [devtools-html] → [devtools-html] [triage]
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [devtools-html] [triage] → [reserve-html]
Assignee: nobody → gtatum
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.
Status: NEW → ASSIGNED
Iteration: --- → 51.2 - Aug 29
Priority: P3 → P1
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.
Attachment #8786052 - Flags: review?(ttromey) → review+
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 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.
Attachment #8786053 - Flags: review?(ttromey) → review+
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.
Attachment #8786054 - Flags: review?(ttromey) → review+
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.
Attachment #8786053 - Flags: review?(cmanchester)
Iteration: 51.2 - Aug 29 → 51.3 - Sep 12
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 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.
Attachment #8786053 - Flags: review?(cmanchester) → review+
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.
Keywords: checkin-needed
This can't autoland because it has unresolved issues on MozReview. And trying to manually import it leads to a pile of conflicts.
Keywords: checkin-needed
Pushed by gtatum@mozilla.com:
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
MozReview-Commit-ID: EVGhNmz5YZD
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a569d019fd4a
Fix require path for devtools/shared/css/lexer to fix dt test failures; a=tomcat-sheriffduty
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
Flags: needinfo?(gtatum)
Attachment #8790721 - Attachment is obsolete: true
Flags: needinfo?(gtatum)
In preparation for the additional files in the `mach generate-css-db`
command, collect the CSS files into a folder.

MozReview-Commit-ID: 9JRVsC2NMK8
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
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
Keywords: checkin-needed
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.
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/23dbff2ec614
https://hg.mozilla.org/mozilla-central/rev/6ef033c9dcc3
https://hg.mozilla.org/mozilla-central/rev/7d70ac7d03de
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.