[webkit-reviews] review granted: [Bug 37352] Animations are cleared when stylesheets are dynamically inserted : [Attachment 53018] Patch with testcase

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 37352: Animations are cleared when stylesheets are dynamically inserted
https://bugs.webkit.org/show_bug.cgi?id=37352

Attachment 53018: Patch with testcase
https://bugs.webkit.org/attachment.cgi?id=53018&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/dom/Document.cpp
> ===================================================================

>      recalcStyleSelector();
> +    // This recalcStyle initialtes a new recalc cycle. We need to bracket it
to
> +    // make sure animations get the correct update time
> +    if (m_frame)
> +	   m_frame->animation()->beginAnimationUpdate();
>      recalcStyle(Force);
> +    if (m_frame)
> +	   m_frame->animation()->endAnimationUpdate();

I don't think the comment is necessary; the code speaks for itself.

> Index: WebCore/page/animation/KeyframeAnimation.cpp
> ===================================================================
> --- WebCore/page/animation/KeyframeAnimation.cpp	(revision 57355)
> +++ WebCore/page/animation/KeyframeAnimation.cpp	(working copy)
> @@ -68,6 +68,11 @@ void KeyframeAnimation::getKeyframeAnima
>      double elapsedTime = getElapsedTime();
>  
>      double t = m_animation->duration() ? (elapsedTime /
m_animation->duration()) : 1;
> +
> +    ASSERT(t >= 0);
> +    if (t < 0)
> +	   t = 0;

Or t = max(t, 0);

> Index: LayoutTests/animations/dynamic-stylesheet-loading.html
> ===================================================================
> +  <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
> +  <meta name="viewport" content="width=device-width, minimum-scale=1.0,
maximum-scale=1.0, user-scalable=no">

No need for these meta tags.

> +  <title>Test of dynamic stylesheet loading</title>
> +  <link rel="stylesheet"
href="resources/dynamic-stylesheet-insertion-main.css">
> +    <script src="animation-test-helpers.js" type="text/javascript"
charset="utf-8"></script>
> +    <script type="text/javascript" charset="utf-8">
> +    
> +    const expectedValues = [
> +	 // [animation-name, time, element-id, property, expected-value,
tolerance]
> +	 ["splashdown", 0.3, "splash", "webkitTransform.5", 100, 0.1],
> +    ];
> +    
> +    var controller = {};
> +
> +    controller.init = function () {
> +	   // put a listener on the initial splash animation
> +	   this.splash = document.getElementById('splash');
> +	   this.splash.addEventListener('webkitAnimationStart', this, false);
> +	   this.splash.addEventListener('webkitAnimationEnd', this, false);
> +
> +	   runAnimationTest(expectedValues, undefined, undefined, true);
> +    };
> +
> +    controller.handleEvent = function (event) {
> +	   if (event.type == 'webkitAnimationStart') {
> +	       // pre-load the stylesheet
> +	       var link = document.createElement('link');
> +	       link.rel = 'stylesheet';
> +	       link.href =
'resources/dynamic-stylesheet-insertion-inserted.css';
> +	       document.head.appendChild(link);
> +	   }
> +    };

I'd simplify this JS to not be objecty, and to make the names like 'splash'
more generic.

> Index:
LayoutTests/animations/resources/dynamic-stylesheet-insertion-inserted.css
> ===================================================================

Maybe add a comment saying "/* This file deliberately left blank */ ?

> Index: LayoutTests/animations/resources/dynamic-stylesheet-insertion-main.css

> ===================================================================
> --- LayoutTests/animations/resources/dynamic-stylesheet-insertion-main.css   
(revision 0)
> +++ LayoutTests/animations/resources/dynamic-stylesheet-insertion-main.css   
(revision 0)
> @@ -0,0 +1,37 @@
> +body {
> +  width: 320px;
> +  height: 480px;
> +}
> +
> +#splash {
> +  position: absolute;
> +  left: 20px;
> +  left: 60px;
> +  width: 100px;
> +  height: 100px;
> +  background-color: blue;
> +  -webkit-transform: translate3d(0,80px,0);
> +  -webkit-animation-name: splashdown;
> +  -webkit-animation-duration: 0.6s;
> +  -webkit-animation-delay: 0.1s;
> +}

Should remove rules that don't pertain to the issue, and pref. avoid rules that
kick us into compositing mode.

r=me with more simplified testcase.