Generate shorthand tables from Servo data

RESOLVED FIXED in Firefox 61

Status

()

RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

7 months ago
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.
(Assignee)

Updated

7 months ago
Depends on: 1452542
(Assignee)

Updated

7 months ago
Blocks: 1457178
(Assignee)

Updated

7 months ago
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

7 months ago
mozreview-review
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...
Attachment #8973122 - Flags: review?(cam) → review+

Comment 7

7 months ago
mozreview-review
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.
Attachment #8973123 - Flags: review?(cam) → review+

Comment 8

7 months ago
mozreview-review
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...
Attachment #8973124 - Flags: review?(cam) → review+
(Assignee)

Comment 9

7 months ago
mozreview-review-reply
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 :)
(Assignee)

Comment 10

7 months ago
mozreview-review-reply
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...
(Assignee)

Comment 11

7 months ago
Subproperties for all shorthand listed in generated property db seems to be alarming... I need to investigate it later.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

7 months ago
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]}`);
>       }
>     }
>   }
> }
(Assignee)

Comment 18

7 months ago
Hmmm... browser_styleinspector_tooltip-shorthand-fontfamily.js times out, probably because font-family was the first, and that test relies on that...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

7 months ago
mozreview-review
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.
Attachment #8973154 - Flags: review?(ttromey) → review+

Comment 23

6 months ago
mozreview-review
Comment on attachment 8973125 [details]
Bug 1454591 part 5 - Generate subproperty lists from Servo data.

https://reviewboard.mozilla.org/r/241634/#review247756
Attachment #8973125 - Flags: review?(cam) → review+

Comment 24

6 months ago
mozreview-review
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
Attachment #8973126 - Flags: review?(cam) → review+

Comment 25

6 months ago
Pushed by xquan@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.