Start using on_refresh_driver_tick #5681 #5753

JIoJIaJIu · 2015-04-20T15:03:40Z

RequestAnimationFrame
Task

hoppipolla-critic-bot · 2015-04-20T15:03:45Z

Critic review: https://critic.hoppipolla.co.uk/r/4730

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

jdm · 2015-04-21T14:42:53Z

Does this cause any test result changes in test-wpt?

Reviewed files:

components/devtools/actors/framerate.rs @ r1
components/devtools/actors/timeline.rs @ r1
components/devtools_traits/lib.rs @ r1
components/layout/animation.rs @ r1
components/script/dom/bindings/trace.rs @ r1
components/script/dom/document.rs @ r1
components/script/dom/webidls/Window.webidl @ r1
components/script/dom/window.rs @ r1
components/script/lib.rs @ r1
components/script/script_task.rs @ r1
components/script_traits/lib.rs @ r1
tests/html/test_request_animation_frame.html @ r1

^{components/devtools/actors/framerate.rs, line 81 [r1] (raw file):}
Instead of creating a separate thread here (and for the regular timeline ticks as well), what if we sent a message to the main devtools event loop (via ScriptDevtoolControlMsg) with the actor name and the timestamp?

^{components/script/dom/document.rs, line 86 [r1] (raw file):}
std::iter::FromIterator

^{components/script/dom/document.rs, line 135 [r1] (raw file):}
No need for webappapis.html

^{components/script/dom/document.rs, line 137 [r1] (raw file):}
Cell

^{components/script/dom/document.rs, line 138 [r1] (raw file):}
No need for webappapis.html

^{components/script/dom/document.rs, line 774 [r1] (raw file):}
This isn't quite right - animations are driven by the compositor, so we should use that system. In particular, process_new_animations in animation.rs shows how we should notify the constellation that animations exist. We will need to differentiate between "there are active animations" and "the animation callback list is not empty" so that we continue receiving animation ticks even if a CSS animation finishes but there are still animation frame callbacks.

^{components/script/dom/document.rs, line 783 [r1] (raw file):}
No need for webappapis.html

^{components/script/dom/document.rs, line 784 [r1] (raw file):}
Use http://w3c.github.io/animation-timing/ instead of the TR/ version.

^{components/script/dom/document.rs, line 785 [r1] (raw file):}
invoke_animation_callbacks

^{components/script/dom/document.rs, line 786 [r1] (raw file):}
This will cause a panic if a callback calls requestAnimationFrame again, I'm pretty sure.

^{components/script/dom/webidls/Window.webidl, line 145 [r1] (raw file):}
http://w3c.github.io/animation-timing/#Window-interface-extensions

^{components/script/dom/window.rs, line 460 [r1] (raw file):}
No need for webappapis.html

^{components/script/dom/window.rs, line 465 [r1] (raw file):}
The spec says that any exceptions should be suppressed; let's use Report right now instead and add a TODO.

^{components/script/dom/window.rs, line 471 [r1] (raw file):}
No need for webappapis.html

^{components/script/dom/window.rs, line 886 [r1] (raw file):}
Unintentional change?

^{components/script/script_task.rs, line 917 [r1] (raw file):}
Move this into the devtools module like the other handlers.

Comments from the review on Reviewable.io

jdm · 2015-04-21T14:43:43Z

This is exciting! The main areas for improvement I can see are the integration with the existing animation ticking system and the use of threads for the devtools actor loops.

JIoJIaJIu · 2015-04-23T15:18:30Z

Does this cause any test result changes in test-wpt?

No, it should not

^{components/devtools/actors/framerate.rs, line 81 [r1] (raw file):}
ScriptDevtoolControlMsg does not used anywhere, is it legacy? I refactored with DevtoolsControlMsg

^{components/script/dom/document.rs, line 135 [r1] (raw file):}
Why is better do not use webappis? I thinked that whatwg specification is general

^{components/script/dom/window.rs, line 886 [r1] (raw file):}
My bad)

Comments from the review on Reviewable.io

jdm · 2015-04-23T15:47:42Z

Looks like we should enable the tests in http://mxr.mozilla.org/servo/source/tests/wpt/web-platform-tests/animation-timing/ :) Add the dir to http://mxr.mozilla.org/servo/source/tests/wpt/include.ini ?

jdm · 2015-04-23T15:51:55Z

^{components/devtools/actors/framerate.rs, line 81 [r1] (raw file):}
Yes, I'll file an issue about removing it.

^{components/script/dom/document.rs, line 135 [r1] (raw file):}
Oh, because the page.html links are not stable; only the #section-title links are stable, so ./mach test-tidy will complain about specific page links in the comments.

^{components/script/dom/window.rs, line 886 [r1] (raw file):}
(I accidentally marked this issue resolved, so ignore this comment I am making right now)

Comments from the review on Reviewable.io

JIoJIaJIu · 2015-04-23T16:03:02Z

Nice, thank you, I'll do it

Comments from the review on Reviewable.io

JIoJIaJIu · 2015-04-23T17:40:10Z

One test failed cause document.hidden is undefined, can I exclude someway this one?

 0:03.15 TEST_END: Thread-TestrunnerManager-1 Harness OK. Subtests passed 0/1. Unexpected 1
requestAnimationFrame callback is invoked at least once before the timeout
--------------------------------------------------------------------------
Expected PASS, got FAIL
assert_false: document.hidden must be exist and be false to run this test properly expected false got undefined
@http://localhost:8000/animation-timing/callback-invoked.html:4
Test.step@http://localhost:8000/resources/testharness.js:1295
@http://localhost:8000/animation-timing/callback-invoked.html:3
Test.step@http://localhost:8000/resources/testharness.js:1295
async_test@http://localhost:8000/resources/testharness.js:445
@http://localhost:8000/animation-timing/callback-invoked.html:9

Comments from the review on Reviewable.io

jdm · 2015-04-23T17:50:31Z

You can just add an expected failure in http://mxr.mozilla.org/servo/source/tests/wpt/metadata/ . See http://mxr.mozilla.org/servo/source/tests/wpt/metadata/XMLHttpRequest/data-uri.htm.ini for an example.

JIoJIaJIu · 2015-04-23T18:49:45Z

It's without tests, just for review, I'll add tests as soon as I can

bors-servo · 2015-04-23T19:14:53Z

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

jdm · 2015-04-23T19:25:48Z

Reviewed files:

components/compositing/compositor.rs @ r2
components/compositing/compositor_task.rs @ r2
components/compositing/constellation.rs @ r2
components/devtools/actors/framerate.rs @ r2
components/devtools/actors/timeline.rs @ r2
components/devtools/lib.rs @ r2
components/devtools_traits/lib.rs @ r2
components/layout/animation.rs @ r2
components/msg/constellation_msg.rs @ r2
components/script/devtools.rs @ r2
components/script/dom/bindings/trace.rs @ r2
components/script/dom/document.rs @ r2
components/script/dom/webidls/Window.webidl @ r2
components/script/dom/window.rs @ r2
components/script/script_task.rs @ r2
components/script_traits/lib.rs @ r2

^{components/compositing/compositor.rs, line 428 [r2] (raw file):}
This clone shouldn't be necessary.

^{components/compositing/compositor.rs, line 428 [r2] (raw file):}
I don't think this handles pages that use requestAnimationCallback as well as CSS animations at the same time. When the animations finish the page will report AnimationState::Stopped, right? I think the pipeline should have two booleans, and AnimiationState should have four states - NoAnimationsPresent, AnimationsPresent, AnimationCallbacksPresent, and NoAnimationCallbacksPresent. This method can toggle the booleans based on the states received. Does that make sense?

^{components/devtools/actors/framerate.rs, line 83 [r2] (raw file):}
fabric?

^{components/devtools/actors/framerate.rs, line 83 [r2] (raw file):}
Why a Box?

^{components/devtools/actors/framerate.rs, line 98 [r2] (raw file):}
This looks like it will never use the update value stored in the actor.

^{components/devtools/lib.rs, line 271 [r2] (raw file):}
Why is this block necessary? Can't we just use a different name instead of sender?

^{components/msg/constellation_msg.rs, line 236 [r2] (raw file):}
This can be Copy instead of Clone.

^{components/script/dom/document.rs, line 135 [r1] (raw file):}
The spec link is useful, it should just be https://html.spec.whatwg.org/multipage/#animation-frame-callback-identifier

^{components/script/dom/document.rs, line 138 [r1] (raw file):}
Same here; spec links are good :)

^{components/script/dom/document.rs, line 783 [r1] (raw file):}
This link can stay, just without webappapis.html :)

^{components/script/dom/window.rs, line 471 [r1] (raw file):}
This can stay, just without webappapis.html :)

Comments from the review on Reviewable.io

JIoJIaJIu · 2015-05-01T12:33:01Z

Execuse me for long silence

^{components/compositing/compositor.rs, line 428 [r2] (raw file):}
If I move animation_state I cannot use it below in comparison, isn't it?

^{components/devtools/actors/framerate.rs, line 83 [r2] (raw file):}
Is it bad name?

^{components/devtools/actors/framerate.rs, line 83 [r2] (raw file):}
May be Box<&bool>?
I want to send reference on self.is_recording
For avoid pointing lifetime I take Box

^{components/devtools/actors/framerate.rs, line 98 [r2] (raw file):}
I think problem is in Box, that I didn't send reference to self.is_recording and just ref to Box

Comments from the review on Reviewable.io

JIoJIaJIu · 2015-05-01T12:35:41Z

^{components/devtools/lib.rs, line 271 [r2] (raw file):}
I choosed that way for avoid bloating function's stack with many local variables

Comments from the review on Reviewable.io

jdm · 2015-05-01T14:28:26Z

^{components/compositing/compositor.rs, line 428 [r2] (raw file):}
If you making AnimationState derive Copy there is no need to clone it.

^{components/devtools/actors/framerate.rs, line 83 [r2] (raw file):}
Well, I don't understand what it means here :)

^{components/devtools/actors/framerate.rs, line 83 [r2] (raw file):}
I think we want Arc<Mutex<bool>> so that we can check the actual, shared value when the closure executes; otherwise we are sending the value that was present when the closure was created.

^{components/devtools/actors/framerate.rs, line 98 [r2] (raw file):}
I agree.

^{components/devtools/lib.rs, line 271 [r2] (raw file):}
I don't think that's a big concern here :)

Comments from the review on Reviewable.io

jdm · 2015-05-04T17:40:12Z

Reviewed files:

components/compositing/compositor.rs @ r3
components/compositing/compositor_task.rs @ r3
components/compositing/constellation.rs @ r3
components/devtools/actors/framerate.rs @ r3
components/devtools/lib.rs @ r3
components/devtools_traits/lib.rs @ r3
components/layout/animation.rs @ r3
components/msg/constellation_msg.rs @ r3
components/script/devtools.rs @ r3
components/script/dom/bindings/trace.rs @ r3
components/script/dom/document.rs @ r3
components/script/dom/window.rs @ r3
components/script/lib.rs @ r3
components/script/script_task.rs @ r3
components/script_traits/lib.rs @ r3
tests/wpt/include.ini @ r3
tests/wpt/metadata/animation-timing/callback-invoked.html.ini @ r3

^{components/compositing/compositor.rs, line 171 [r3] (raw file):}
Whether

^{components/compositing/compositor.rs, line 174 [r3] (raw file):}
Whether

^{components/devtools/actors/framerate.rs, line 83 [r3] (raw file):}
Let's just make the one from earlier mutable instead.

^{components/devtools/actors/framerate.rs, line 128 [r3] (raw file):}
Make the first one mutable instead.

^{components/layout/animation.rs, line 58 [r3] (raw file):}
This doesn't look right; this code executes if the running_animations vector is not empty.

^{components/script/dom/document.rs, line 819 [r3] (raw file):}
We don't seem to send NoAnimationCallbacksPresent anywhere in these changes.

^{tests/wpt/metadata/animation-timing/callback-invoked.html.ini, line 3 [r3] (raw file):}
Have you investigated why this fails?

Comments from the review on Reviewable.io

JIoJIaJIu · 2015-05-05T14:22:57Z

^{components/script/dom/document.rs, line 819 [r3] (raw file):}
Did I understand right? That NoAnimationCallbacksPresent sends nowhere?
Or I should in animation.rs create something like tick_animation_callbacks?

^{tests/wpt/metadata/animation-timing/callback-invoked.html.ini, line 3 [r3] (raw file):}
I think document.hidden should be implemented

async_test(function (t) {                                                                                                                                                               
    t.step(function() {                                                                                                                                                                   
        assert_false(document.hidden, "document.hidden must be exist and be false to run this test properly");                                                                              
    });                                                                                                                                                                                   
    window.requestAnimationFrame(function () {                                                                                                                                            
        t.step(function() { assert_true(true); t.done(); });                                                                                                                                
    });                                                                                                                                                                                   
}, "requestAnimationFrame callback is invoked at least once before the timeout");

Comments from the review on Reviewable.io

jdm · 2015-05-05T14:33:01Z

-S-awaiting-review +S-needs-code-changes

Reviewed files:

components/compositing/compositor.rs @ r4
components/devtools/actors/framerate.rs @ r4
components/layout/animation.rs @ r4
components/script/dom/document.rs @ r4

^{components/script/dom/document.rs, line 819 [r3] (raw file):}
We should have some code that runs after all animation callbacks have finished executing that checks if there any any new animation callbacks; if there are none, we should send NoAnimationCallbacksPresent.

Comments from the review on Reviewable.io

JIoJIaJIu · 2015-05-05T14:47:43Z

^{components/script/dom/document.rs, line 825 [r3] (raw file):}
Ahh, precise

Comments from the review on Reviewable.io

jdm · 2015-05-05T15:04:55Z

-S-awaiting-review +S-needs-rebase

Reviewed files:

components/script/dom/document.rs @ r5
components/script/dom/window.rs @ r5

Comments from the review on Reviewable.io

bors-servo · 2015-05-05T17:57:59Z

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

jdm · 2015-05-05T18:07:40Z

-S-awaiting-review

Reviewed files:

components/compositing/compositor.rs @ r6
components/devtools/actors/timeline.rs @ r6
components/devtools/lib.rs @ r6
components/devtools_traits/lib.rs @ r6
components/layout/animation.rs @ r6
components/script/dom/bindings/trace.rs @ r6
components/script/dom/document.rs @ r6
components/script/dom/window.rs @ r6
components/script/lib.rs @ r6
components/script/script_task.rs @ r6
components/script_traits/lib.rs @ r6

Comments from the review on Reviewable.io


        Start using on_refresh_driver_tick #5681

Final

jdm · 2015-05-06T05:47:50Z

@bors-servo: r+

bors-servo · 2015-05-06T05:47:51Z

📌 Commit be2cb66 has been approved by jdm

bors-servo · 2015-05-06T05:50:13Z

⌛ Testing commit be2cb66 with merge 5e59e77...


        Auto merge of #5753 - JIoJIaJIu:timeline, r=jdm

RequestAnimationFrame [Task](#5681)  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5753)

bors-servo · 2015-05-06T06:31:24Z

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

highfive added the S-awaiting-review label Apr 20, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Apr 21, 2015

JIoJIaJIu force-pushed the JIoJIaJIu:timeline branch from d24c73a to bfba267 Apr 23, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 23, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Apr 23, 2015

JIoJIaJIu force-pushed the JIoJIaJIu:timeline branch from bfba267 to e847870 May 3, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels May 3, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels May 4, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels May 5, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels May 5, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels May 5, 2015

jdm added S-needs-rebase and removed S-awaiting-review labels May 5, 2015

JIoJIaJIu force-pushed the JIoJIaJIu:timeline branch from 132b098 to b6cf6c5 May 5, 2015

highfive added the S-awaiting-review label May 5, 2015

jdm removed the S-awaiting-review label May 5, 2015

Start using on_refresh_driver_tick #5681

Loading status checks…

be2cb66

Final

JIoJIaJIu force-pushed the JIoJIaJIu:timeline branch from b6cf6c5 to be2cb66 May 5, 2015

highfive added the S-awaiting-review label May 5, 2015

jdm added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels May 6, 2015

bors-servo merged commit be2cb66 into servo:master May 6, 2015
2 checks passed

2 checks passed

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

homu Test successful
Details

bors-servo mentioned this pull request May 6, 2015

Support WebDriver takeScreenshot command. #5884

Merged

JIoJIaJIu deleted the JIoJIaJIu:timeline branch May 9, 2015

servo / servo

Start using on_refresh_driver_tick #5681 #5753

Start using on_refresh_driver_tick #5681 #5753

JIoJIaJIu commented Apr 20, 2015

hoppipolla-critic-bot commented Apr 20, 2015

jdm commented Apr 21, 2015

jdm commented Apr 21, 2015

JIoJIaJIu commented Apr 23, 2015

jdm commented Apr 23, 2015

jdm commented Apr 23, 2015

JIoJIaJIu commented Apr 23, 2015

JIoJIaJIu commented Apr 23, 2015

jdm commented Apr 23, 2015

JIoJIaJIu commented Apr 23, 2015

bors-servo commented Apr 23, 2015

jdm commented Apr 23, 2015

JIoJIaJIu commented May 1, 2015

JIoJIaJIu commented May 1, 2015

jdm commented May 1, 2015

jdm commented May 4, 2015

JIoJIaJIu commented May 5, 2015

jdm commented May 5, 2015

JIoJIaJIu commented May 5, 2015

jdm commented May 5, 2015

bors-servo commented May 5, 2015

jdm commented May 5, 2015

jdm commented May 6, 2015

bors-servo commented May 6, 2015

bors-servo commented May 6, 2015

bors-servo commented May 6, 2015

servo / servo

Join GitHub today

Start using on_refresh_driver_tick #5681 #5753

Start using on_refresh_driver_tick #5681 #5753

Conversation

JIoJIaJIu commented Apr 20, 2015

hoppipolla-critic-bot commented Apr 20, 2015

jdm commented Apr 21, 2015

jdm commented Apr 21, 2015

JIoJIaJIu commented Apr 23, 2015

jdm commented Apr 23, 2015

jdm commented Apr 23, 2015

JIoJIaJIu commented Apr 23, 2015

JIoJIaJIu commented Apr 23, 2015

jdm commented Apr 23, 2015

JIoJIaJIu commented Apr 23, 2015

bors-servo commented Apr 23, 2015

jdm commented Apr 23, 2015

JIoJIaJIu commented May 1, 2015

JIoJIaJIu commented May 1, 2015

jdm commented May 1, 2015

jdm commented May 4, 2015

JIoJIaJIu commented May 5, 2015

jdm commented May 5, 2015

JIoJIaJIu commented May 5, 2015

jdm commented May 5, 2015

bors-servo commented May 5, 2015

jdm commented May 5, 2015

jdm commented May 6, 2015

bors-servo commented May 6, 2015

bors-servo commented May 6, 2015

bors-servo commented May 6, 2015