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:20Z

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)


        Merge keyword and predefined type longhands


        Make use of existing codegen methods for font-*


        Keyword argument for custom constant mapping

emilio

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

emilio · 2016-10-26T06:32:46Z

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:32:46Z

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...


        Auto merge of #13902 - Wafflespeanut:keyword, r=emilio

@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


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

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...


        Auto merge of #13902 - Wafflespeanut:keyword, r=emilio

@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 label Oct 24, 2016

Manishearth reviewed Oct 24, 2016

View changes

highfive assigned emilio and unassigned Manishearth Oct 24, 2016

wafflespeanut added S-needs-code-changes and removed S-awaiting-review labels Oct 24, 2016

wafflespeanut force-pushed the wafflespeanut:keyword branch from ce6890d to 58f93c8 Oct 25, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 25, 2016

emilio reviewed Oct 25, 2016

View changes

wafflespeanut force-pushed the wafflespeanut:keyword branch from 58f93c8 to 31fb345 Oct 25, 2016

wafflespeanut force-pushed the wafflespeanut:keyword branch from 31fb345 to 1e7d854 Oct 25, 2016

wafflespeanut added 3 commits Oct 22, 2016

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 wafflespeanut:keyword branch from dabef3f to 74265fd Oct 26, 2016

emilio approved these changes Oct 26, 2016

View changes

wafflespeanut force-pushed the wafflespeanut:keyword branch 2 times, most recently from fb6a195 to ff27e4f Oct 26, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 26, 2016

wafflespeanut mentioned this pull request Oct 26, 2016

Stylo: Implement font-variant-caps #13668

Closed

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 26, 2016

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

Loading status checks…

e7cbd10

wafflespeanut force-pushed the wafflespeanut:keyword branch from ff27e4f to e7cbd10 Oct 26, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Oct 26, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 26, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 26, 2016

bors-servo merged commit e7cbd10 into servo:master Oct 26, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

wafflespeanut deleted the wafflespeanut:keyword branch Nov 29, 2016

servo / servo

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

This comment has been minimized.

This comment has been minimized.

Manishearth commented Oct 24, 2016

wafflespeanut commented Oct 25, 2016 •

edited

This comment has been minimized.

This comment has been minimized.

wafflespeanut commented Oct 25, 2016

wafflespeanut commented Oct 26, 2016

emilio left a comment

This comment has been minimized.

This comment has been minimized.

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

servo / servo

Join GitHub today

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

This comment has been minimized.

Manishearth Oct 24, 2016

This comment has been minimized.

wafflespeanut Oct 24, 2016

Manishearth commented Oct 24, 2016

wafflespeanut commented Oct 25, 2016 • edited

This comment has been minimized.

emilio Oct 25, 2016

This comment has been minimized.

Manishearth Oct 25, 2016

wafflespeanut commented Oct 25, 2016

wafflespeanut commented Oct 26, 2016

emilio left a comment

This comment has been minimized.

emilio Oct 26, 2016

This comment has been minimized.

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

wafflespeanut commented Oct 24, 2016 •

edited by larsbergstrom

wafflespeanut commented Oct 25, 2016 •

edited