1454591 - Generate shorthand tables from Servo data

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

Those are currently encoded inside nsCSSProps.cpp. We should have them generated from Servo data so that when people adding new shorthands, they don't need to worry about two different places.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1454591 part 1 - Generate more structured data in ServoCSSPropList.py.

This patch changes ServoCSSPropList.py to use classes for properties.
This allows extending the data in the file without needing to update all
users of this file.

Sorting in GenerateCSSPropsGenerated.py is removed because the data file
has the right order already.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1454591 part 2 - Refactor GenerateCSSPropsGenerated.py.

This removes the extra template file and use the script to generate
the whole nsCSSPropsGenerated.inc file directly, because it doesn't
seem to really make much sense to have them separate.

One behavior change to this refactor is that, the static assertions
no longer include aliases. Other parts of the generated data all ignore
aliases, so checking property id of aliases isn't really useful. It
makes the code simpler everywhere to just strip aliases from the list
at the very beginning.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1454591 part 3 - Consistently use top-right-bottom-left order for physical sides.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1454591 part 5 - Generate subproperty lists from Servo data.

I manually diffed the generated lists and the original ones from in
nsCSSProps.cpp. All generated lists seem to contain the same set of
subproperties as their old correspondents.

There are still some differences:

Order of subproperties of many shorthands is changed. There are many
comments in the old lists stating that the order is important, but they
are mostly for serialization. I auditted all users of the subproperty
lists, and it doesn't seem to me any of them relies on the order.

gOutlineRadiusSubpropTable is renamed to gMozOutlineRadiusSubpropTable
which I don't think is a problem at all, but maybe worth mentioning.

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1454591 part 6 - Remove CSS_PROP_LIST_ONLY_COMPONENTS_OF_ALL_SHORTHAND.

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

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8973122 [details]
Bug 1454591 part 1 - Generate more structured data in ServoCSSPropList.py.

https://reviewboard.mozilla.org/r/241628/#review247446

::: devtools/shared/css/generated/mach_commands.py:57
(Diff revision 1)
>      def get_preferences(self):
>          """Get all of the preferences associated with enabling and disabling a property."""
>          # The data takes the following form:
>          # [ (name, prop, id, flags, pref, proptype), ... ]
>          dataPath = resolve_path(self.topobjdir, 'layout/style/ServoCSSPropList.py')
> -        with open(dataPath, "r") as f:
> +        data = runpy.run_path(dataPath).data

Nit: In the other files, you're using `["data"]` rather than `.data`.  Since this property should always be present, maybe use `.data` everywhere?

::: layout/style/GenerateCSSPropertyID.py:17
(Diff revision 1)
>  
>      longhand_count = 0
>      shorthand_count = 0
>      alias_count = 0
>      property_ids = []
> -    for name, method, id, flags, pref, prototype in data:
> +    for prop in data:

(Heh, I guess that should have been proptype rather than prototype...)

::: layout/style/ServoCSSPropList.mako.py:13
(Diff revision 1)
> +
> +
> +class Longhand(object):
> +    __slots__ = ["name", "method", "id", "flags", "pref"]
> +
> +    def __init__(self, *args):

As I'm not a Python programmer really, I don't know how idiomatic this __slots__ / *args stuff is, but I guess it's OK...

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8973123 [details]
Bug 1454591 part 2 - Refactor GenerateCSSPropsGenerated.py.

https://reviewboard.mozilla.org/r/241630/#review247450

::: commit-message-932e4:3
(Diff revision 1)
> +Bug 1454591 part 2 - Refactor GenerateCSSPropsGenerated.py. r?heycam
> +
> +This removes the extra template file and use the script to generate

"and uses the script"

::: layout/style/GenerateCSSPropsGenerated.py:31
(Diff revision 1)
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

These comments really applied to the separate .inc.in file, so I think we can just drop them here as we don't need them in the generated file.

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8973124 [details]
Bug 1454591 part 3 - Consistently use top-right-bottom-left order for physical sides.

https://reviewboard.mozilla.org/r/241632/#review247452

::: servo/components/style/properties/data.py:8
(Diff revision 1)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  import re
>  
> -PHYSICAL_SIDES = ["top", "left", "bottom", "right"]
> +PHYSICAL_SIDES = ["top", "right", "bottom", "left"]
>  LOGICAL_SIDES = ["block-start", "block-end", "inline-start", "inline-end"]

Should this be reordered too?

I just discovered that `margin: logical 1px 2px 3px 4px` is meant to set the logical properties in the order ["block-start", "inline-start", "block-end", "inline-end"], which is probably more logical (heh) than the physical version of a margin shorthand value, though the inconsistency could be confusing...

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8973122 [details]
Bug 1454591 part 1 - Generate more structured data in ServoCSSPropList.py.

https://reviewboard.mozilla.org/r/241628/#review247446

> Nit: In the other files, you're using `["data"]` rather than `.data`.  Since this property should always be present, maybe use `.data` everywhere?

So... this is wrong... I initially thought I can use `.data` and found out that it doesn't work, and I updated other places but not this, because I forgot :/

`runpy.run_path` returns a dictionary rather than a module object, so we can only use `["data"]` to access the data here.

> As I'm not a Python programmer really, I don't know how idiomatic this __slots__ / *args stuff is, but I guess it's OK...

`__slots__` is a magic variable in Python to save memory by only allocating slots for the list of fields, and not generating a general dictionary to the object, see https://docs.python.org/2/reference/datamodel.html#slots

I use it here mostly just for unifying the field names :)

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8973124 [details]
Bug 1454591 part 3 - Consistently use top-right-bottom-left order for physical sides.

https://reviewboard.mozilla.org/r/241632/#review247452

> Should this be reordered too?
> 
> I just discovered that `margin: logical 1px 2px 3px 4px` is meant to set the logical properties in the order ["block-start", "inline-start", "block-end", "inline-end"], which is probably more logical (heh) than the physical version of a margin shorthand value, though the inconsistency could be confusing...

Hmmm... I reorder PHYSICAL_SIDES here because I found that it affects the subproperties order in a confusing way.

IIRC, we don't currently support logical keyword in shorthands yet. We can probably delay reordering that when we do that...

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Subproperties for all shorthand listed in generated property db seems to be alarming... I need to investigate it later.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8973122 [details]
Bug 1454591 part 1 - Generate more structured data in ServoCSSPropList.py.

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

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8973123 [details]
Bug 1454591 part 2 - Refactor GenerateCSSPropsGenerated.py.

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

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8973124 [details]
Bug 1454591 part 3 - Consistently use top-right-bottom-left order for physical sides.

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

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8973125 [details]
Bug 1454591 part 5 - Generate subproperty lists from Servo data.

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

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8973126 [details]
Bug 1454591 part 6 - Remove CSS_PROP_LIST_ONLY_COMPONENTS_OF_ALL_SHORTHAND.

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

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

So, all shorthand have four fewer subproperties after this change, and those are:
* -moz-font-smoothing-background-color
* -moz-min-font-size-ratio
* -moz-top-layer
* -moz-window-shadow

I guess it doesn't matter a lot since they are all sorta internal properties.

For other properties, I've used the following script to compare and didn't find any difference:
> let a = require("./properties-db-a.js").CSS_PROPERTIES;
> let b = require("./properties-db-b.js").CSS_PROPERTIES;
> 
> for (let p in a) {
>   let subprops_a = a[p].subproperties.sort();
>   let subprops_b = b[p].subproperties.sort();
>   if (subprops_a.length != subprops_b.length) {
>     console.log(`${p}: length differ`);
>   } else {
>     for (let i = 0; i < subprops_a.length; i++) {
>       if (subprops_a[i] != subprops_b[i]) {
>         console.log(`${p}: expected ${subprops_a[i]} but get ${subprops_b[i]}`);
>       }
>     }
>   }
> }

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Hmmm... browser_styleinspector_tooltip-shorthand-fontfamily.js times out, probably because font-family was the first, and that test relies on that...

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1454591 part 4 - Have devtools test not rely on font-family being the first longhand of font shorthand.

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

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8973125 [details]
Bug 1454591 part 5 - Generate subproperty lists from Servo data.

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

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8973126 [details]
Bug 1454591 part 6 - Remove CSS_PROP_LIST_ONLY_COMPONENTS_OF_ALL_SHORTHAND.

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

Comment 22

[Tom Tromey :tromey]

Comment on attachment 8973154 [details]
Bug 1454591 part 4 - Have devtools test not rely on font-family being the first longhand of font shorthand.

https://reviewboard.mozilla.org/r/241656/#review247584

Thank you for the patch.  This seems reasonable to me.

Comment 23

[Cameron McCormack (:heycam)]

Comment on attachment 8973125 [details]
Bug 1454591 part 5 - Generate subproperty lists from Servo data.

https://reviewboard.mozilla.org/r/241634/#review247756

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 8973126 [details]
Bug 1454591 part 6 - Remove CSS_PROP_LIST_ONLY_COMPONENTS_OF_ALL_SHORTHAND.

https://reviewboard.mozilla.org/r/241636/#review247758

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61084cd28188
part 1 - Generate more structured data in ServoCSSPropList.py. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c07763d44a66
part 2 - Refactor GenerateCSSPropsGenerated.py. r=heycam
https://hg.mozilla.org/integration/autoland/rev/eff6d5eb51ce
part 3 - Consistently use top-right-bottom-left order for physical sides. r=heycam
https://hg.mozilla.org/integration/autoland/rev/d8252b50ec6c
part 4 - Have devtools test not rely on font-family being the first longhand of font shorthand. r=tromey
https://hg.mozilla.org/integration/autoland/rev/9f38c38a66cc
part 5 - Generate subproperty lists from Servo data. r=heycam
https://hg.mozilla.org/integration/autoland/rev/881670154df0
part 6 - Remove CSS_PROP_LIST_ONLY_COMPONENTS_OF_ALL_SHORTHAND. r=heycam

Comment 26

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/61084cd28188
https://hg.mozilla.org/mozilla-central/rev/c07763d44a66
https://hg.mozilla.org/mozilla-central/rev/eff6d5eb51ce
https://hg.mozilla.org/mozilla-central/rev/d8252b50ec6c
https://hg.mozilla.org/mozilla-central/rev/9f38c38a66cc
https://hg.mozilla.org/mozilla-central/rev/881670154df0