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

stylo: Bug 1357357 - Make the parser of transition-property match the spec. #16614

Merged

Conversation

BorisChiou
Copy link
Contributor

@BorisChiou BorisChiou commented Apr 26, 2017

These are interdependent patches of Bug 1357357. We add one more arm, TransitionProperty::Unsupported, which stores the string of non-animatable, custom, or unrecognized property, so we can parse these kinds of properties and serialize them correctly. This is necessary because we need to start transitions even though some transition-properties are non-animatable, custom, or unrecognized.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix Bug 1357357.
  • There are tests for these changes.

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/gecko_bindings/bindings.rs, components/style/properties/gecko.mako.rs, components/style/build_gecko.rs, components/style/properties/properties.mako.rs, components/style/dom.rs and 4 more
  • @emilio: components/style/gecko_bindings/bindings.rs, components/style/properties/gecko.mako.rs, ports/geckolib/glue.rs, components/style/build_gecko.rs, components/style/properties/properties.mako.rs and 5 more

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 26, 2017
@BorisChiou
Copy link
Contributor Author

@bors-servo r=emilio

@bors-servo
Copy link
Contributor

📌 Commit e647a1a has been approved by emilio

@highfive highfive assigned emilio and unassigned Manishearth Apr 26, 2017
@highfive 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 Apr 26, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit e647a1a with merge 4e4b9ab...

bors-servo pushed a commit that referenced this pull request Apr 26, 2017
…y, r=emilio

stylo: Bug 1357357 - Make the parser of transition-property match the spec.

These are interdependent patches of Bug 1357357. We add one more arm, TransitionProperty::Unsupported, which stores the string of non-animatable, custom, or unrecognized property, so we can parse these kinds of properties and serialize them correctly. This is necessary because we need to start transitions even though some transition-properties are non-animatable, custom, or unrecognized.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [Bug 1357357](https://bugzilla.mozilla.org/show_bug.cgi?id=1357357).
- [X] There are tests for these changes.

<!-- 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/16614)
<!-- Reviewable:end -->
@nox
Copy link
Contributor

nox commented Apr 26, 2017

The commits should be squashed...

@BorisChiou
Copy link
Contributor Author

OK, I will squash them

@nox
Copy link
Contributor

nox commented Apr 26, 2017

Thanks! Didn't want to r- for this.

@BorisChiou BorisChiou force-pushed the stylo/transition/transition_property branch from e647a1a to 7ff4042 Compare April 26, 2017 10:27
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 26, 2017
@BorisChiou
Copy link
Contributor Author

@bors-servo r=emilio

squash commits.

@bors-servo
Copy link
Contributor

📌 Commit 7ff4042 has been approved by emilio

@highfive 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 Apr 26, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 7ff4042 with merge 1522645ec4429d961fb92a9a223968c2b27e7f76...

@BorisChiou BorisChiou force-pushed the stylo/transition/transition_property branch from 7ff4042 to 0c704e7 Compare April 26, 2017 10:28
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 26, 2017
@BorisChiou
Copy link
Contributor Author

@bors-servo r=emilio

Fix the commit message.

@bors-servo
Copy link
Contributor

📌 Commit 0c704e7 has been approved by emilio

@highfive highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Apr 26, 2017
bors-servo pushed a commit that referenced this pull request Apr 26, 2017
…y, r=emilio

stylo: Bug 1357357 - Make the parser of transition-property match the spec.

These are interdependent patches of Bug 1357357. We add one more arm, TransitionProperty::Unsupported, which stores the string of non-animatable, custom, or unrecognized property, so we can parse these kinds of properties and serialize them correctly. This is necessary because we need to start transitions even though some transition-properties are non-animatable, custom, or unrecognized.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [Bug 1357357](https://bugzilla.mozilla.org/show_bug.cgi?id=1357357).
- [X] There are tests for these changes.

<!-- 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/16614)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-css

@highfive 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 Apr 26, 2017
@BorisChiou BorisChiou force-pushed the stylo/transition/transition_property branch from 0c704e7 to 5ad3da2 Compare April 26, 2017 11:44
@highfive 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 Apr 26, 2017
@BorisChiou
Copy link
Contributor Author

@bors-servo r=emilio

Fix test failures. (I forgot to filter out unsupported cases in animation.rs.)

@bors-servo
Copy link
Contributor

📌 Commit 5ad3da2 has been approved by emilio

@highfive 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 Apr 26, 2017
@BorisChiou
Copy link
Contributor Author

@bors-servo p=1

@bors-servo
Copy link
Contributor

⌛ Testing commit 5ad3da2 with merge 2c4c1f0...

bors-servo pushed a commit that referenced this pull request Apr 26, 2017
…y, r=emilio

stylo: Bug 1357357 - Make the parser of transition-property match the spec.

These are interdependent patches of Bug 1357357. We add one more arm, TransitionProperty::Unsupported, which stores the string of non-animatable, custom, or unrecognized property, so we can parse these kinds of properties and serialize them correctly. This is necessary because we need to start transitions even though some transition-properties are non-animatable, custom, or unrecognized.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [Bug 1357357](https://bugzilla.mozilla.org/show_bug.cgi?id=1357357).
- [X] There are tests for these changes.

<!-- 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/16614)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-css

@highfive 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 Apr 26, 2017
1. We add a new arm to TransitionProperty, TransitionProperty::Unsupported,
   which contains an Atom, so it's better to remove the Copy trait from
   TransitionProperty.
2. TransitionProperty::Unsupported(Atom) represents any non-animatable, custom,
   or unrecognized property, and we use Atom to store the ident string for
   serialization.
@BorisChiou BorisChiou force-pushed the stylo/transition/transition_property branch from 5ad3da2 to 02fc178 Compare April 26, 2017 13:41
@highfive 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 Apr 26, 2017
@BorisChiou
Copy link
Contributor Author

@bors-servo r=emilio

Fix shorthand order.

@bors-servo
Copy link
Contributor

📌 Commit 02fc178 has been approved by emilio

@highfive 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 Apr 26, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 02fc178 with merge 8f1356d...

bors-servo pushed a commit that referenced this pull request Apr 26, 2017
…y, r=emilio

stylo: Bug 1357357 - Make the parser of transition-property match the spec.

These are interdependent patches of Bug 1357357. We add one more arm, TransitionProperty::Unsupported, which stores the string of non-animatable, custom, or unrecognized property, so we can parse these kinds of properties and serialize them correctly. This is necessary because we need to start transitions even though some transition-properties are non-animatable, custom, or unrecognized.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [Bug 1357357](https://bugzilla.mozilla.org/show_bug.cgi?id=1357357).
- [X] There are tests for these changes.

<!-- 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/16614)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ 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-msvc-dev
Approved by: emilio
Pushing 8f1356d to master...

@bors-servo bors-servo merged commit 02fc178 into servo:master Apr 26, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants