Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix animation shorthand parsing #15793

Merged
merged 6 commits into from Mar 3, 2017

Conversation

Projects
None yet
8 participants
@upsuper
Copy link
Member

commented Mar 2, 2017

which is somehow broken after #15779.

But it seems there are various issue around the animation shorthand parsing, so I try to fix it to make it match the spec.

I expect this change to fix most parsing failures in Gecko's test suite, although I haven't tested.


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Mar 2, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/gecko.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/shorthand/box.mako.rs
  • @emilio: components/style/properties/gecko.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/shorthand/box.mako.rs
@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

⌛️ Trying commit a472e38 with merge 18faec5...

bors-servo added a commit that referenced this pull request Mar 2, 2017

Auto merge of #15793 - upsuper:animation, r=<try>
Fix animation shorthand parsing

which is somehow broken after #15779.

But it seems there are various issue around the animation shorthand parsing, so I try to fix it to make it match the spec.

I expect this change to fix most parsing failures in Gecko's test suite, although I haven't tested.
@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

@highfive highfive assigned Manishearth and unassigned cbrewster Mar 2, 2017

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

And transition shorthand also needs fix, but that is a bit more trickier than animation, because transition-property cannot be a list including none, while animation-name can.

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

Also, the serialization of animation-name is apparently wrong when it goes to quoted string as animation name. It would need to output escaped string, and it need a special handling of none keyword. That could be done in a separate issue, and that can actually be an easy issue.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

💔 Test failed - linux-rel-css

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

@bors-servo retry

bors-servo added a commit that referenced this pull request Mar 2, 2017

Auto merge of #15793 - upsuper:animation, r=<try>
Fix animation shorthand parsing

which is somehow broken after #15779.

But it seems there are various issue around the animation shorthand parsing, so I try to fix it to make it match the spec.

I expect this change to fix most parsing failures in Gecko's test suite, although I haven't tested.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15793)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

⌛️ Trying commit a472e38 with merge d9b3e50...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

💔 Test failed - linux-rel-css

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

That failure doesn't seem to be related... I have no idea why it happens.

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

r? @emilio

@highfive highfive assigned emilio and unassigned Manishearth Mar 2, 2017

@emilio

emilio approved these changes Mar 2, 2017

Copy link
Member

left a comment

r=me with the debug_assert nit addressed, the comment, and optionally that code sharing thing, though the result may end up not much cleaner, so at your discrection :)

@@ -1727,15 +1727,13 @@ fn static_assert() {

pub fn set_animation_name(&mut self, v: longhands::animation_name::computed_value::T) {
use nsstring::nsCString;

assert!(v.0.len() > 0);

This comment has been minimized.

Copy link
@emilio

emilio Mar 2, 2017

Member

debug_assert!(!v.0.is_empty())?

This comment has been minimized.

Copy link
@upsuper

upsuper Mar 2, 2017

Author Member

I follow lots of assert! around this file in the same format... probably we can convert all of them into debug_assert!(!v.0.is_empty()) in a separate commit.

unsafe { self.gecko.mAnimations[0].mName.truncate(); }
self.gecko.mAnimationNameCount = v.0.len() as u32;
for (servo, gecko) in v.0.into_iter().zip(self.gecko.mAnimations.iter_mut()) {
gecko.mName.assign_utf8(&nsCString::from(servo.0.to_string()));

This comment has been minimized.

Copy link
@emilio

emilio Mar 2, 2017

Member

So, this is really inefficient, but it's pre-existing. I believe bug 1329169 is supposed to fix this, could you reference it from here in a comment?

@@ -153,32 +153,26 @@ macro_rules! try_parse_one {
animation-fill-mode animation-play-state"
allowed_in_keyframe_block="False"
spec="https://drafts.csswg.org/css-animations/#propdef-animation">
<%
props = "name duration timing_function delay iteration_count \
direction fill_mode play_state".split()

This comment has been minimized.

Copy link
@emilio

emilio Mar 2, 2017

Member

It seems to me that sharing code with the sub_properties field is doable. Not sure if super-worth it, though, since not keeping them in sync is a compile-time error. Do it if you wish and think it's cleaner, not needed to land.

@upsuper upsuper force-pushed the upsuper:animation branch from a472e38 to 55abc74 Mar 2, 2017

@highfive highfive removed the S-tests-failed label Mar 2, 2017

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

@bors-servo r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

📌 Commit 55abc74 has been approved by emilio

@upsuper upsuper force-pushed the upsuper:animation branch from 6784489 to 5302772 Mar 3, 2017

@highfive highfive removed the S-tests-failed label Mar 3, 2017

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

⌛️ Trying commit 5302772 with merge ac6af20...

bors-servo added a commit that referenced this pull request Mar 3, 2017

Auto merge of #15793 - upsuper:animation, r=<try>
Fix animation shorthand parsing

which is somehow broken after #15779.

But it seems there are various issue around the animation shorthand parsing, so I try to fix it to make it match the spec.

I expect this change to fix most parsing failures in Gecko's test suite, although I haven't tested.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15793)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

@emilio so it seems it is indeed because of the cache thing you mentioned.

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

@bors-servo r=emilio,bholley

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

📌 Commit 5302772 has been approved by emilio,bholley

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

@bors-servo r- try- clean

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

@bors-servo r=emilio,bholley

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

📌 Commit 5302772 has been approved by emilio,bholley

@nox

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

@bors-servo try- retry

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

It seems animation stuff is still broken after this pr... I'll need to work with stylo build then...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

⌛️ Testing commit 5302772 with merge 7cd4c69...

bors-servo added a commit that referenced this pull request Mar 3, 2017

Auto merge of #15793 - upsuper:animation, r=emilio,bholley
Fix animation shorthand parsing

which is somehow broken after #15779.

But it seems there are various issue around the animation shorthand parsing, so I try to fix it to make it match the spec.

I expect this change to fix most parsing failures in Gecko's test suite, although I haven't tested.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15793)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: emilio,bholley
Pushing 7cd4c69 to master...

@bors-servo bors-servo merged commit 5302772 into servo:master Mar 3, 2017

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

@upsuper upsuper deleted the upsuper:animation branch Mar 3, 2017

bholley added a commit to bholley/servo that referenced this pull request Mar 3, 2017

bors-servo added a commit that referenced this pull request Mar 3, 2017

Auto merge of #15814 - bholley:backout_15793, r=bholley
Revert #15793 for crashes and test failures on stylo

Looks like #15793 busted autoland. Given that it's the weekend for Xidorn I'm going to back out the patch.

CC @upsuper
@bholley

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

bors-servo added a commit that referenced this pull request Mar 6, 2017

Auto merge of #15836 - upsuper:animation, r=<try>
Fix animation shorthand parsing (again)

This is from [bug 1343168](https://bugzilla.mozilla.org/show_bug.cgi?id=1343168) and mostly same as #15793 which was backed out in #15814.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15836)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Mar 7, 2017

Auto merge of #15836 - upsuper:animation, r=emilio,bholley
Fix animation shorthand parsing (again)

This is from [bug 1343168](https://bugzilla.mozilla.org/show_bug.cgi?id=1343168) and mostly same as #15793 which was backed out in #15814.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15836)
<!-- Reviewable:end -->

clementmiao added a commit to clementmiao/servo that referenced this pull request Apr 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.