Closed Bug 1454591 Opened 7 years ago Closed 7 years ago

Generate shorthand tables from Servo data

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

Bug 1454591 part 1 - Generate more structured data in ServoCSSPropList.py.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1454591 part 2 - Refactor GenerateCSSPropsGenerated.py.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1454591 part 3 - Consistently use top-right-bottom-left order for physical sides.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1454591 part 5 - Generate subproperty lists from Servo data.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1454591 part 6 - Remove CSS_PROP_LIST_ONLY_COMPONENTS_OF_ALL_SHORTHAND.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1454591 part 4 - Have devtools test not rely on font-family being the first longhand of font shorthand.
(deleted), text/x-review-board-request
tromey
: review+
Details
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.
Depends on: 1452542
Blocks: 1457178
Assignee: nobody → xidorn+moz
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 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 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+
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 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...
Subproperties for all shorthand listed in generated property db seems to be alarming... I need to investigate it later.
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]}`); > } > } > } > }
Hmmm... browser_styleinspector_tooltip-shorthand-fontfamily.js times out, probably because font-family was the first, and that test relies on that...
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 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 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+
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.

Attachment

General

Creator:
Created:
Updated:
Size: