Prefer auto-generation for some keyword props #13902

wafflespeanut · 2016-10-24T08:35:48Z

./mach build-geckolib does not report any errors

These changes do not require tests because it's a refactor

r? @Manishearth or @emilio

This change is

highfive · 2016-10-24T08:35:52Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/gecko.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/longhand/inherited_box.mako.rs, components/style/properties/data.py
@emilio: components/style/properties/gecko.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/longhand/inherited_box.mako.rs, components/style/properties/data.py

highfive · 2016-10-24T08:35:53Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

Manishearth · 2016-10-24T08:42:14Z

components/style/properties/longhand/inherited_box.mako.rs

+// And, firefox doesn't support 'pixelated' (https://bugzilla.mozilla.org/show_bug.cgi?id=856337)
+${helpers.single_keyword("image-rendering",
+                         "auto crispedges",
+                         extra_gecko_values="optimizequality optimizespeed",


this needs to be manual, since the serialization is custom, and there's some aliasing going on.

Okay, yeah, the problem lies in crisp-edges. I'll better add an option for that in python.

Manishearth · 2016-10-24T08:42:21Z

r? @emilio

wafflespeanut · 2016-10-25T08:32:57Z

@emilio The parsing/serialization code for image-rendering is there only for the sake of a gecko constant. It's CRISPEDGES in gecko, whereas we expect it to be CRISP_EDGES. So, I've added a tiny patch to ignore hyphens for such cases.

emilio · 2016-10-25T09:58:12Z

components/style/properties/data.py

+        # This is a workaround for gecko constants with unexpected names i.e., a constant
+        # corresponding to a keyword 'foo-bar' declared as FOOBAR instead of FOO_BAR
+        if self.keyword is not None:
+            self.keyword.ignore_hyphens = ignore_hyphens


I think this feels a lot like a hack. If we want to both work-around it and auto-generate it, I think instead of this we should be passing a "keyword to constant" map instead, and then look it up in gecko_constant or replace otherwise.

What do you think?

IMO workarounds like ignore_hyphens that get used just once probably shouldn't exist 😄 I'd prefer if we manually did image-rendering. Though I'm good with the map solution too.

wafflespeanut · 2016-10-25T15:49:40Z

@emilio @Manishearth So, I've gone for predicting constants based on their edit distance (python has a built-in object for that). It works quite well for all the constants we generate now. You think this would be useful? If it is, then I'll go ahead and fix the tests. If it's not, then I'll revert the changes to image-rendering stuff and we'll land this.

wafflespeanut · 2016-10-26T06:19:42Z

Well, I've fallen back to @emilio's idea of having a constants map. It'll also be useful in the future (#13668 for instance)

emilio

r=me with those addressed, thanks for doing this! :)

emilio · 2016-10-26T06:30:59Z

components/style/properties/helpers.mako.rs

@@ -312,6 +312,7 @@
        keyword_kwargs = {a: kwargs.pop(a, None) for a in [
            'gecko_constant_prefix', 'gecko_enum_prefix',
            'extra_gecko_values', 'extra_servo_values',
+            'custom_consts',


There's another instance of this same list below (line 359)

emilio · 2016-10-26T06:31:28Z

components/style/properties/longhand/inherited_box.mako.rs

-    pub mod computed_value {
-        use cssparser::ToCss;
-        use std::fmt;
+<% consts_map = { "crisp-edges": "CRISPEDGES" } %>


rename to image_rendering_custom_consts?

wafflespeanut · 2016-10-26T06:44:30Z

@bors-servo r=emilio

bors-servo · 2016-10-26T06:44:31Z

📌 Commit ff27e4f has been approved by emilio

bors-servo · 2016-10-26T07:16:41Z

⌛ Testing commit ff27e4f with merge bd94b5e...

@Manishearth

Prefer auto-generation for some keyword props  ---  - [x] `./mach build-geckolib` does not report any errors  - [x] These changes do not require tests because it's a refactor  r? @Manishearth or @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13902)

bors-servo · 2016-10-26T07:21:51Z

💔 Test failed - linux-rel-wpt

wafflespeanut · 2016-10-26T07:27:10Z

@bors-servo r=emilio

Oops. Forgot to change the enum variants in other crates :)

bors-servo · 2016-10-26T07:27:10Z

📌 Commit e7cbd10 has been approved by emilio

bors-servo · 2016-10-26T07:27:14Z

⌛ Testing commit e7cbd10 with merge 3c16dde...

@Manishearth

Prefer auto-generation for some keyword props  ---  - [x] `./mach build-geckolib` does not report any errors  - [x] These changes do not require tests because it's a refactor  r? @Manishearth or @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13902)

bors-servo · 2016-10-26T08:00:27Z

💔 Test failed - linux-rel-wpt

wafflespeanut · 2016-10-26T08:01:51Z

@bors-servo retry #13776

bors-servo · 2016-10-26T08:01:53Z

⚡ Previous build results for mac-dev-unit, windows-dev are reusable. Rebuilding only arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2...

bors-servo · 2016-10-26T08:41:44Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned Manishearth Oct 24, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 24, 2016

Manishearth reviewed Oct 24, 2016

View reviewed changes

highfive assigned emilio and unassigned Manishearth Oct 24, 2016

wafflespeanut added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 24, 2016

wafflespeanut force-pushed the keyword branch from ce6890d to 58f93c8 Compare October 25, 2016 08:24

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 25, 2016

emilio reviewed Oct 25, 2016

View reviewed changes

wafflespeanut force-pushed the keyword branch from 58f93c8 to 31fb345 Compare October 25, 2016 15:35

wafflespeanut force-pushed the keyword branch from 31fb345 to 1e7d854 Compare October 25, 2016 16:10

wafflespeanut added 3 commits October 26, 2016 11:56

Merge keyword and predefined type longhands

af8899e

Make use of existing codegen methods for font-*

1c34162

Keyword argument for custom constant mapping

12f429e

wafflespeanut force-pushed the keyword branch from dabef3f to 74265fd Compare October 26, 2016 06:27

emilio approved these changes Oct 26, 2016

View reviewed changes

wafflespeanut force-pushed the keyword branch 2 times, most recently from fb6a195 to ff27e4f Compare October 26, 2016 06:43

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 26, 2016

wafflespeanut mentioned this pull request Oct 26, 2016

Stylo: Implement font-variant-caps #13668

Closed

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 26, 2016

Auto-generate 'image-rendering' using 'single_keyword'

e7cbd10

wafflespeanut force-pushed the keyword branch from ff27e4f to e7cbd10 Compare October 26, 2016 07:26

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 26, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 26, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 26, 2016

bors-servo merged commit e7cbd10 into servo:master Oct 26, 2016

wafflespeanut deleted the keyword branch November 29, 2016 05:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer auto-generation for some keyword props #13902

Prefer auto-generation for some keyword props #13902

wafflespeanut commented Oct 24, 2016 •

edited by larsbergstrom

highfive commented Oct 24, 2016

highfive commented Oct 24, 2016

Manishearth Oct 24, 2016

wafflespeanut Oct 24, 2016

Manishearth commented Oct 24, 2016

wafflespeanut commented Oct 25, 2016 •

edited

emilio Oct 25, 2016

Manishearth Oct 25, 2016

wafflespeanut commented Oct 25, 2016

wafflespeanut commented Oct 26, 2016

emilio left a comment

emilio Oct 26, 2016

emilio Oct 26, 2016

wafflespeanut commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

wafflespeanut commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

wafflespeanut commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

Prefer auto-generation for some keyword props #13902

Prefer auto-generation for some keyword props #13902

Conversation

wafflespeanut commented Oct 24, 2016 • edited by larsbergstrom

highfive commented Oct 24, 2016

highfive commented Oct 24, 2016

Manishearth Oct 24, 2016

Choose a reason for hiding this comment

wafflespeanut Oct 24, 2016

Choose a reason for hiding this comment

Manishearth commented Oct 24, 2016

wafflespeanut commented Oct 25, 2016 • edited

emilio Oct 25, 2016

Choose a reason for hiding this comment

Manishearth Oct 25, 2016

Choose a reason for hiding this comment

wafflespeanut commented Oct 25, 2016

wafflespeanut commented Oct 26, 2016

emilio left a comment

Choose a reason for hiding this comment

emilio Oct 26, 2016

Choose a reason for hiding this comment

emilio Oct 26, 2016

Choose a reason for hiding this comment

wafflespeanut commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

wafflespeanut commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

wafflespeanut commented Oct 26, 2016

bors-servo commented Oct 26, 2016

bors-servo commented Oct 26, 2016

wafflespeanut commented Oct 24, 2016 •

edited by larsbergstrom

wafflespeanut commented Oct 25, 2016 •

edited