[webkit-reviews] review denied: [Bug 85253] Apply animations and transitions for first-letter element : [Attachment 145416] Patch V7.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 5 11:30:29 PDT 2012
Simon Fraser (smfr) <simon.fraser at apple.com> has denied Igor Trindade Oliveira
<igor.oliveira at webkit.org>'s request for review:
Bug 85253: Apply animations and transitions for first-letter element
https://bugs.webkit.org/show_bug.cgi?id=85253
Attachment 145416: Patch V7.
https://bugs.webkit.org/attachment.cgi?id=145416&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.
More information about the webkit-reviews
mailing list