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

style: Add support to test animations programatically. #12392

Merged
merged 2 commits into from Jul 20, 2016

Conversation

@emilio
Copy link
Member

emilio commented Jul 11, 2016


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

r? @SimonSapin for the style changes, @Ms2ger or @jdm for the dom and test changes


This change is Reviewable

@highfive
Copy link

highfive commented Jul 11, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/lib.rs, components/style/timer.rs, components/style/context.rs, components/style/animation.rs, components/style/matching.rs
  • @KiChjang: components/script/dom/testbinding.rs, components/script/dom/webidls/TestBinding.webidl, components/script/script_thread.rs, components/script/dom/window.rs, components/script_layout_interface/message.rs, components/script_traits/lib.rs, components/script_traits/lib.rs
@SimonSapin
Copy link
Member

SimonSapin commented Jul 12, 2016

Reviewed 2 of 2 files at r1, 19 of 19 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


components/servo/Cargo.lock, line 1168 [r2] (raw file):

 "style 0.0.1",
 "style_traits 0.0.1",
 "time 0.1.35 (registry+https://github.com/rust-lang/crates.io-index)",

You probably want to run something like ./mach cargo-update -p layout to update ports/cef/Cargo.lock.


components/style/matching.rs, line 461 [r2] (raw file):

                                     context: &SharedStyleContext<<Self::ConcreteElement as Element>::Impl>,
                                     style: &mut Option<&mut
                                     Arc<Self::ConcreteComputedValues>>)

This is an usual place to insert a line break. Is it needed at all?


components/style/timer.rs, line 52 [r2] (raw file):

        if let TimerMode::Test(ref mut val) = self.mode {
            *val += by;
        }

Should this panic in non-test mode? I think an else { panic!(…) } here could replace the assertion in handle_advance_clock_ms.


Comments from Reviewable

@emilio emilio force-pushed the emilio:test-animations branch from 5349d14 to 1fad86e Jul 12, 2016
@emilio
Copy link
Member Author

emilio commented Jul 12, 2016

Review status: all files reviewed at latest revision, 3 unresolved discussions.


components/servo/Cargo.lock, line 1168 [r2] (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

You probably want to run something like ./mach cargo-update -p layout to update ports/cef/Cargo.lock.

Done

components/style/matching.rs, line 461 [r2] (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

This is an usual place to insert a line break. Is it needed at all?

Whoops, sorry, bad autoformatting :)

components/style/timer.rs, line 52 [r2] (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Should this panic in non-test mode? I think an else { panic!(…) } here could replace the assertion in handle_advance_clock_ms.

Makes sense, done :)

Comments from Reviewable

@SimonSapin
Copy link
Member

SimonSapin commented Jul 20, 2016

@bors-servo r+


Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2016

📌 Commit 1fad86e has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2016

🔒 Merge conflict

@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2016

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

emilio added 2 commits Jul 8, 2016
…_thread.rs
…ic infrastructure for controlling animations.
@emilio emilio force-pushed the emilio:test-animations branch from 1fad86e to 0b67b21 Jul 20, 2016
@emilio
Copy link
Member Author

emilio commented Jul 20, 2016

@bors-servo: r=SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2016

📌 Commit 0b67b21 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2016

Testing commit 0b67b21 with merge 14aeccc...

bors-servo added a commit that referenced this pull request Jul 20, 2016
style: Add support to test animations programatically.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12120

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

r? @SimonSapin for the style changes, @Ms2ger or @jdm for the dom and test 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/12392)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2016

@bors-servo bors-servo merged commit 0b67b21 into servo:master Jul 20, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@emilio emilio deleted the emilio:test-animations branch Oct 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.