[Webkit-unassigned] [Bug 85253] Apply animations and transitions for first-letter element

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=85253


Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #145416|review?                     |review-
               Flag|                            |




--- Comment #17 from Simon Fraser (smfr) <simon.fraser at apple.com>  2012-06-05 11:30:29 PST ---
(From update of attachment 145416)
View in context: https://bugs.webkit.org/attachment.cgi?id=145416&action=review

I think this needs one more round, but it's close. If you rename generatingNode(), you should do it as a first step in a separate patch.

> LayoutTests/animations/first-letter-play-state.html:11
> +      -webkit-animation: "move1" 0.2s linear;

Keyframe names are IDENTS and should not be quoted.

> LayoutTests/animations/first-letter-play-state.html:17
> +    @-webkit-keyframes "move1" {

Ditto.

> LayoutTests/animations/first-letter-play-state.html:21
> +    @-webkit-keyframes "firstLetterAnimation" {

Ditto.

> LayoutTests/animations/first-letter-play-state.html:43
> +    function stop()
> +    {
> +        document.getElementById("box1").style.webkitAnimationPlayState = "paused";
> +    }
> +
> +    function start()
> +    {
> +        document.getElementById("box1").style.webkitAnimationPlayState = "running";
> +    }

This isn't testing play-state on the pseudo animation, which I think is the interesting thing.

> LayoutTests/animations/first-letter-play-state.html:61
> +        }
> +            
> +    }

Extra line here.

> LayoutTests/transitions/first-letter-color-transition.html:42
> +    window.addEventListener( 'webkitTransitionEnd', transitionEnded, false );

Extra spaces inside the parens.

> Source/WebCore/rendering/RenderObject.h:585
> +    Node* styledGeneratingNode() const;

The comment for generatingNode() seems totally wrong, so maybe that one should be renamed. I'm not a big fan of styledGeneratingNode().

> Source/WebCore/rendering/RenderObject.h:668
> +    void setAnimatableStyle(PassRefPtr<RenderStyle>, bool isGeneratedContent = false);

I don't like the boolean parameter here. It's hard to read at the call site. Maybe two methods would be clearer.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.