Staticize CASCADE_PROPERTIES, (temporarily) fix stylo path for animations, and introduce the long-term path to follow #11972

emilio · 2016-06-30T21:51:38Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors

These changes do not require tests because refactoring + geckolib

@bholley: I was going to do this "the good way", but that involves quite a few properties, so I thought I'd rather unlock stylo before :)

r? @bholley

This change is


        style: Make CASCADE_PROPERTY a real static array.

Why wasn't this done before?

highfive · 2016-06-30T21:51:41Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/properties.mako.rs, components/style/animation.rs, components/style/properties/data.py

highfive · 2016-06-30T21:51:43Z

Warning

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

bholley · 2016-07-01T00:20:09Z

I don't understand why we introduce need_borrow and then remove it again?

bholley · 2016-07-01T00:20:45Z

(the reason I did clone instead of borrow is that borrowing will almost never be possible from geckolib, because we need to convert the types)

emilio · 2016-07-01T00:52:34Z

Yup, I'm doing some changes, sorry for the early review request

emilio · 2016-07-01T02:25:50Z

Ok, now it should be ready for review.

r? @bholley


        style: Add a temporary workaround for geckolib animations.

With needs_borrow we will be able to remove this, though we'll have to implement clone_ and borrow_ methods for a bunch of properties.


        style: Simplify generation of need_clone methods in servo

bholley · 2016-07-01T19:29:00Z

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

-<%helpers:longhand name="transition-timing-function" animatable="False">
+<%helpers:longhand name="transition-timing-function"
+                   need_index="True"
+                   animatable="False">


Nit - the order here isn't consistent.

bholley · 2016-07-01T19:36:04Z

ports/geckolib/properties.mako.rs

 </%def>

-<%def name="impl_color(ident, gecko_ffi_name, color_flags_ffi_name=None)">
+<%def name="impl_color(ident, gecko_ffi_name, color_flags_ffi_name=None, need_clone=None)">


This should be False, not None.

bholley · 2016-07-01T19:37:38Z

ports/geckolib/properties.mako.rs

@@ -245,15 +246,42 @@ def set_gecko_property(ffi_name, expr):
    }
 </%def>

+<%def name="impl_color_setter(ident, gecko_ffi_name, color_flags_ffi_name=None)">


Looks like this is a duplicate of the above - remove it?

Same reason, my fault

bholley · 2016-07-01T19:40:08Z

ports/geckolib/properties.mako.rs

 <%def name="impl_color_copy(ident, gecko_ffi_name, color_flags_ffi_name=None)">
    fn copy_${ident}_from(&mut self, other: &Self) {
        % if color_flags_ffi_name:
-        ${clear_color_flags(color_flags_ffi_name)}


Why are we removing the call to clear_color_flags?

Whoops, good catch. At one point I wrote another version of these functions, but then backed it out, and this seems that it got lost in the meantime.

bholley · 2016-07-01T19:44:09Z

ports/geckolib/values.rs

 }

-pub trait ToGeckoStyleCoord {
+pub trait ToGeckoStyleCoord : Sized {


Might want to rename this to GeckoStyleCoordConversion, or some potentially-more-idiomatic name that reflects the bidirectional conversion.

bholley · 2016-07-01T20:00:21Z

r=me with those fixes.

@bors-servo delegate+

bors-servo · 2016-07-01T20:00:22Z

✌️ @emilio can now approve this pull request

emilio · 2016-07-01T20:11:46Z

@bors-servo: r=bholley

bors-servo · 2016-07-01T20:11:47Z

📌 Commit 808f5ab has been approved by bholley

bors-servo · 2016-07-01T20:20:21Z

⌛ Testing commit 808f5ab with merge 540aeaa...


        Auto merge of #11972 - emilio:style-thingies, r=bholley

@bholley

Staticize CASCADE_PROPERTIES, (temporarily) fix stylo path for animations, and introduce the long-term path to follow  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] These changes do not require tests because refactoring + geckolib  @bholley: I was going to do this "the good way", but that involves quite a few properties, so I thought I'd rather unlock stylo before :) r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11972)

bors-servo · 2016-07-01T21:12:53Z

💔 Test failed - mac-rel-css


        Auto merge of #11972 - emilio:style-thingies, r=bholley

@bholley

Staticize CASCADE_PROPERTIES, (temporarily) fix stylo path for animations, and introduce the long-term path to follow  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] These changes do not require tests because refactoring + geckolib  @bholley: I was going to do this "the good way", but that involves quite a few properties, so I thought I'd rather unlock stylo before :) r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11972)

bors-servo · 2016-07-02T11:47:28Z

💥 Test timed out

cbrewster · 2016-07-02T17:51:45Z

@bors-servo retry

timeout?

bors-servo · 2016-07-02T18:42:09Z

⌛ Testing commit 30963dd with merge bdfad79...


        Auto merge of #11972 - emilio:style-thingies, r=bholley

@bholley

Staticize CASCADE_PROPERTIES, (temporarily) fix stylo path for animations, and introduce the long-term path to follow  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] These changes do not require tests because refactoring + geckolib  @bholley: I was going to do this "the good way", but that involves quite a few properties, so I thought I'd rather unlock stylo before :) r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11972)

bors-servo · 2016-07-02T19:33:56Z

💔 Test failed - linux-rel

highfive · 2016-07-02T19:33:58Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/refe</span><span class="stdout">rence/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728

cbrewster · 2016-07-02T19:35:07Z

@bors-servo retry

#11574

bors-servo · 2016-07-02T20:27:26Z

⌛ Testing commit 30963dd with merge a77cc99...


        Auto merge of #11972 - emilio:style-thingies, r=bholley

@bholley

Staticize CASCADE_PROPERTIES, (temporarily) fix stylo path for animations, and introduce the long-term path to follow  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] These changes do not require tests because refactoring + geckolib  @bholley: I was going to do this "the good way", but that involves quite a few properties, so I thought I'd rather unlock stylo before :) r? @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11972)

bors-servo · 2016-07-02T21:19:40Z

💔 Test failed - linux-rel

cbrewster · 2016-07-02T21:21:35Z

@bors-servo retry

#11703

bors-servo · 2016-07-03T01:38:12Z

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

bors-servo · 2016-07-03T02:30:08Z

💔 Test failed - linux-rel

jdm · 2016-07-03T03:08:18Z

@bors-servo: retry

#11703

bors-servo · 2016-07-03T03:08:20Z

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

bors-servo · 2016-07-03T04:01:28Z

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

style: Make CASCADE_PROPERTY a real static array.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

2faaf95

Why wasn't this done before?

highfive assigned bholley Jun 30, 2016

highfive added the S-awaiting-review label Jun 30, 2016

emilio added 2 commits Jun 30, 2016

style: Simplify generation of need_clone methods in servo

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

07da4e4

emilio force-pushed the emilio:style-thingies branch from 1a708cd to 8a6d5b9 Jul 1, 2016

bholley reviewed Jul 1, 2016
View changes

emilio force-pushed the emilio:style-thingies branch from d55ffc3 to 808f5ab Jul 1, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 1, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 1, 2016

emilio force-pushed the emilio:style-thingies branch from 808f5ab to a49a519 Jul 1, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 2, 2016

highfive removed the S-awaiting-merge label Jul 2, 2016

highfive added the S-tests-failed label Jul 2, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 2, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 2, 2016

highfive added the S-tests-failed label Jul 3, 2016

bors-servo merged commit 30963dd into servo:master Jul 3, 2016
2 checks passed

2 checks passed

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

homu Test successful
Details

This was referenced Jul 3, 2016

style: Add support to the animation shorthand and fix parsing of animation-name #12118

Merged

Bug 10104 - Only restyle nodes that uses viewport percentage units on viewport size change #11890

Merged

emilio deleted the emilio:style-thingies branch Jul 3, 2016

servo / servo

Staticize CASCADE_PROPERTIES, (temporarily) fix stylo path for animations, and introduce the long-term path to follow #11972

Staticize CASCADE_PROPERTIES, (temporarily) fix stylo path for animations, and introduce the long-term path to follow #11972

emilio commented Jun 30, 2016 •

edited by larsbergstrom

highfive commented Jun 30, 2016

highfive commented Jun 30, 2016

bholley commented Jul 1, 2016

bholley commented Jul 1, 2016

emilio commented Jul 1, 2016

emilio commented Jul 1, 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley commented Jul 1, 2016

bors-servo commented Jul 1, 2016

emilio commented Jul 1, 2016

bors-servo commented Jul 1, 2016

bors-servo commented Jul 1, 2016

bors-servo commented Jul 1, 2016

bors-servo commented Jul 2, 2016

cbrewster commented Jul 2, 2016

bors-servo commented Jul 2, 2016

bors-servo commented Jul 2, 2016

highfive commented Jul 2, 2016

cbrewster commented Jul 2, 2016

bors-servo commented Jul 2, 2016

bors-servo commented Jul 2, 2016

cbrewster commented Jul 2, 2016

bors-servo commented Jul 3, 2016

bors-servo commented Jul 3, 2016

jdm commented Jul 3, 2016

bors-servo commented Jul 3, 2016

bors-servo commented Jul 3, 2016

servo / servo

Join GitHub today

Staticize CASCADE_PROPERTIES, (temporarily) fix stylo path for animations, and introduce the long-term path to follow #11972

Staticize CASCADE_PROPERTIES, (temporarily) fix stylo path for animations, and introduce the long-term path to follow #11972

Conversation

emilio commented Jun 30, 2016 • edited by larsbergstrom

highfive commented Jun 30, 2016

highfive commented Jun 30, 2016

bholley commented Jul 1, 2016

bholley commented Jul 1, 2016

emilio commented Jul 1, 2016

emilio commented Jul 1, 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley commented Jul 1, 2016

bors-servo commented Jul 1, 2016

emilio commented Jul 1, 2016

bors-servo commented Jul 1, 2016

bors-servo commented Jul 1, 2016

bors-servo commented Jul 1, 2016

bors-servo commented Jul 2, 2016

cbrewster commented Jul 2, 2016

bors-servo commented Jul 2, 2016

bors-servo commented Jul 2, 2016

highfive commented Jul 2, 2016

cbrewster commented Jul 2, 2016

bors-servo commented Jul 2, 2016

bors-servo commented Jul 2, 2016

cbrewster commented Jul 2, 2016

bors-servo commented Jul 3, 2016

bors-servo commented Jul 3, 2016

jdm commented Jul 3, 2016

bors-servo commented Jul 3, 2016

bors-servo commented Jul 3, 2016

emilio commented Jun 30, 2016 •

edited by larsbergstrom