Remove usage of 'keyword_list' #13917

wafflespeanut · 2016-10-25T11:22:03Z

We're using keyword_list to generate code for some of the animation properties. This could be achieved with single_keyword (passing vector=True).

./mach build -d does not report any errors
./mach build-geckolib does not report any errors

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

r? @emilio

This change is

highfive · 2016-10-25T11:22:08Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/helpers.mako.rs, components/style/properties/longhand/box.mako.rs
@emilio: components/style/properties/helpers.mako.rs, components/style/properties/longhand/box.mako.rs

highfive · 2016-10-25T11:22:09Z

Warning

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

Manishearth · 2016-10-25T11:37:01Z

components/style/properties/helpers.mako.rs

@@ -75,6 +75,10 @@
            }
            pub mod computed_value {
                pub use super::single_value::computed_value as single_value;
+                % if requires_single:
+                pub use super::single_value::computed_value::T as SingleComputedValue;


I don't think we need requires_single. Refactor the code that needs SingleFoo to use single_value::computed_value::Foo. Alternatively, unconditionally add the reexport.

Manishearth · 2016-10-25T11:37:36Z

components/style/properties/helpers.mako.rs

@@ -145,6 +151,19 @@
                    }).map(SpecifiedValue)
                % endif
            }
+
+            % if requires_single:


We should get rid of this and refactor code using these functions to use the single_value functions instead

wafflespeanut · 2016-10-25T11:41:17Z

I really thought I could escape from refactoring a lot of code 😆

bors-servo · 2016-11-07T06:21:02Z

☔ The latest upstream changes (presumably #14089) made this pull request unmergeable. Please resolve the merge conflicts.

wafflespeanut · 2016-11-16T14:52:00Z

r? @emilio or @Manishearth

emilio

Just one question, that I think @SimonSapin should be able to answer.

So the idea of not having T::get_initial_value, as far as I understand, is that you might want to properties with the same type, but different initial values for each of them, right?

If that's the case, that change probably should be reverted. Otherwise the simplification looks pretty neat :)

emilio · 2016-11-16T16:14:23Z

components/style/properties/helpers.mako.rs

@@ -75,6 +74,7 @@
            }
            pub mod computed_value {
                pub use super::single_value::computed_value as single_value;
+                pub use super::single_value::computed_value::T as SingleComputedValue;


nit: pub use single_value::T as SingleComputedValue is shorter.

Re-exporting as self::single_value::T works, nvm :)

emilio · 2016-11-16T16:14:23Z

components/style/properties/helpers.mako.rs

    <%call expr="longhand(name, **kwargs)">
-        % if product == "gecko" or not gecko_only:


Manishearth · 2016-11-16T16:16:32Z

If that's the case, that change probably should be reverted.

Yeah, that could be the case. In that case I guess you can just wrap in a struct.

SimonSapin · 2016-11-16T16:20:46Z

Just one question, that I think @SimonSapin should be able to answer.

I have no idea what is the context or motivation for this PR. (By the way, it would be nicer to include that in the PR message.) But yes, in CSS specs initial values are define per individual property, even if some properties may share some syntax / grammar.

Manishearth · 2016-11-16T16:22:16Z

I have no idea what is the context or motivation for this PR.

We have both helpers.keyword(vector=True) and helpers.keyword_list() which do the same thing but slightly differently.

I guess we shouldn't move get_initial to a method, then.

wafflespeanut · 2016-11-16T17:17:22Z

So the idea of not having T::get_initial_value, as far as I understand, is that you might want to properties with the same type, but different initial values for each of them, right?

No, it was because initially, I was unable to have the get_initial_value function in each module. Now, I've done a workaround. T::get_initial_value is no longer a method :)

Manishearth · 2016-11-16T17:21:48Z

No, it was because initially, I was unable to have the get_initial_value function in each module. Now, I've done a workaround. T::get_initial_value is no longer a method :)

I still don't get why this is necessary. The vector=True helper creates a single_value mod, just put the second function inside that?

wafflespeanut · 2016-11-16T17:21:54Z

Updated the PR message :)

Manishearth · 2016-11-16T17:23:25Z

You should rewrite callers to use property::single_value::computed_value::get_initial. Don't add a conflicting alias.

Manishearth · 2016-11-16T17:24:12Z

Or, property::computed_value::single_value::get_initial. Both work.

Manishearth · 2016-11-16T21:26:39Z

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

        use style_traits::ToCss;

        pub use Atom as SingleComputedValue;

+        impl Parse for Atom {


I don't think this should be there. Atom is too generic to be tied to the parsing details for a single property.

Yes, writing a newtype for this is adequate I think

Okay, I've gone for a new type AnimationName(pub Atom) and impl'd the necessary traits.

emilio · 2016-11-16T22:35:57Z

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

+        impl Parse for AnimationIterationCount {
+            fn parse(input: &mut ::cssparser::Parser) -> Result<Self, ()> {
+                if input.try(|input| input.expect_ident_matching("infinite")).is_ok() {
+                    Ok(AnimationIterationCount::Infinite)


nit: return early and deindent the next block if you wish.

emilio · 2016-11-16T22:35:57Z

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

        use style_traits::ToCss;

        pub use Atom as SingleComputedValue;

+        impl Parse for Atom {


Yes, writing a newtype for this is adequate I think

wafflespeanut · 2016-11-17T14:53:21Z

Updated the PR.

Manishearth · 2016-11-17T17:43:33Z

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

+            }
+        }
+
+        impl Deref for AnimationName {


In general try to use deref for things that involve actual dereferencing. Just refactor everything else to use .0 here

Removed the impls.


        Remove usage of keyword_list (and prefer single_keyword)


        Remove Mako helper: keyword_list

Manishearth · 2016-11-17T18:16:28Z

@bors-servo r+

bors-servo · 2016-11-17T18:16:29Z

📌 Commit b5bc2a1 has been approved by Manishearth


        Auto merge of #13917 - Wafflespeanut:keywords, r=Manishearth

@emilio

Remove usage of 'keyword_list'  We're using `keyword_list` to generate code for some of the animation properties. This could be achieved with `single_keyword` (passing `vector=True`). ---  - [x] `./mach build -d` does not report any errors - [x] `./mach build-geckolib` does not report any errors  - [x] These changes do not require tests because it's a cleanup  r? @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13917)

bors-servo · 2016-11-17T20:34:34Z

⌛ Testing commit b5bc2a1 with merge b3ad713...

bors-servo · 2016-11-17T21:34: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 emilio Oct 25, 2016

highfive added the S-awaiting-review label Oct 25, 2016

Manishearth reviewed Oct 25, 2016

View changes

highfive added the S-needs-rebase label Nov 7, 2016

wafflespeanut closed this Nov 16, 2016

wafflespeanut force-pushed the wafflespeanut:keywords branch from c0c0c76 to bfd4a0e Nov 16, 2016

wafflespeanut reopened this Nov 16, 2016

wafflespeanut force-pushed the wafflespeanut:keywords branch from 66470c9 to 7ccb868 Nov 16, 2016

emilio reviewed Nov 16, 2016

View changes

wafflespeanut force-pushed the wafflespeanut:keywords branch from 7ccb868 to 8749862 Nov 16, 2016

wafflespeanut closed this Nov 16, 2016

wafflespeanut reopened this Nov 16, 2016

wafflespeanut force-pushed the wafflespeanut:keywords branch from 8749862 to 5a6b8db Nov 16, 2016

Manishearth reviewed Nov 16, 2016

View changes

emilio reviewed Nov 16, 2016

View changes

KiChjang removed the S-needs-rebase label Nov 17, 2016

wafflespeanut force-pushed the wafflespeanut:keywords branch 3 times, most recently from 3c7122d to df8d9da Nov 17, 2016

Manishearth reviewed Nov 17, 2016

View changes

wafflespeanut added 2 commits Nov 16, 2016

Remove usage of keyword_list (and prefer single_keyword)

c51db9f

Remove Mako helper: keyword_list

Loading status checks…

b5bc2a1

wafflespeanut force-pushed the wafflespeanut:keywords branch from df8d9da to b5bc2a1 Nov 17, 2016

highfive assigned Manishearth and unassigned emilio Nov 17, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 17, 2016

bors-servo merged commit b5bc2a1 into servo:master Nov 17, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 17, 2016

servo / servo

Remove usage of 'keyword_list' #13917

Remove usage of 'keyword_list' #13917

wafflespeanut commented Oct 25, 2016 •

edited

highfive commented Oct 25, 2016

highfive commented Oct 25, 2016

This comment has been minimized.

This comment has been minimized.

wafflespeanut commented Oct 25, 2016

bors-servo commented Nov 7, 2016

wafflespeanut commented Nov 16, 2016

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Nov 16, 2016 •

edited

SimonSapin commented Nov 16, 2016

Manishearth commented Nov 16, 2016

wafflespeanut commented Nov 16, 2016

Manishearth commented Nov 16, 2016

wafflespeanut commented Nov 16, 2016

Manishearth commented Nov 16, 2016

Manishearth commented Nov 16, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wafflespeanut commented Nov 17, 2016

This comment has been minimized.

This comment has been minimized.

Manishearth commented Nov 17, 2016

bors-servo commented Nov 17, 2016

bors-servo commented Nov 17, 2016

bors-servo commented Nov 17, 2016

		<%call expr="longhand(name, **kwargs)">
		% if product == "gecko" or not gecko_only:

servo / servo

Join GitHub today

Remove usage of 'keyword_list' #13917

Remove usage of 'keyword_list' #13917

Conversation

wafflespeanut commented Oct 25, 2016 • edited

highfive commented Oct 25, 2016

highfive commented Oct 25, 2016

This comment has been minimized.

This comment has been minimized.

wafflespeanut commented Oct 25, 2016

bors-servo commented Nov 7, 2016

wafflespeanut commented Nov 16, 2016

emilio left a comment

This comment has been minimized.

This comment has been minimized.

wafflespeanut Nov 17, 2016 • edited

This comment has been minimized.

Manishearth commented Nov 16, 2016 • edited

SimonSapin commented Nov 16, 2016

Manishearth commented Nov 16, 2016

wafflespeanut commented Nov 16, 2016

Manishearth commented Nov 16, 2016

wafflespeanut commented Nov 16, 2016

Manishearth commented Nov 16, 2016

Manishearth commented Nov 16, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wafflespeanut Nov 17, 2016 • edited

This comment has been minimized.

This comment has been minimized.

wafflespeanut commented Nov 17, 2016

This comment has been minimized.

This comment has been minimized.

Manishearth commented Nov 17, 2016

bors-servo commented Nov 17, 2016

bors-servo commented Nov 17, 2016

bors-servo commented Nov 17, 2016

wafflespeanut commented Oct 25, 2016 •

edited

wafflespeanut Nov 17, 2016 •

edited

Manishearth commented Nov 16, 2016 •

edited

wafflespeanut Nov 17, 2016 •

edited