[webkit-reviews] review denied: [Bug 85253] Apply animations and transitions for first-letter element : [Attachment 144960] Patch V5.

Thu May 31 14:45:58 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 144960: Patch V5.
https://bugs.webkit.org/attachment.cgi?id=144960&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=144960&action=review

r- pending answers to my questions.

> LayoutTests/animations/first-letter-animation.html:11
> +	 -webkit-animation: boxAnimation 0.5s 0.2s linear;

0.5s is still pretty long for one test. Imagine 100 of these; you've eaten 50s
of every developer's time who runs tests.

> LayoutTests/animations/first-letter-animation.html:28
> +	      }

Style rules would suggest this paren be aligned with the 'from'.

> LayoutTests/animations/first-letter-play-state.html:5
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
"http://www.w3.org/TR/html4/loose.dtd">
> +<html lang="en">
> +<head>
> +  <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
> +  <title>Test of -webkit-animation-play-state</title>

Could be simplified at match the other test.

> Source/WebCore/ChangeLog:8
> +	   Instead of call RenderOject::node() in the animations code,

"of calling"

> Source/WebCore/rendering/RenderBlock.cpp:6020
> -    RenderStyle* pseudoStyle = styleForFirstLetter(firstLetterBlock,
firstLetterContainer);
> +    RefPtr<RenderStyle> pseudoStyle =
animation()->updateAnimations(firstLetter,
styleForFirstLetter(firstLetterBlock, firstLetterContainer));

Why do we have to call updateAnimations() explicitly in this case, but in
createFirstLetterRenderer() we can rely on setAnimatableStyle() to call
updateAnimations() for us?

> Source/WebCore/rendering/RenderObject.cpp:2227
> +Node* RenderObject::generatingNode() const
> +{
> +    Node* node = m_node == document() ? 0 : m_node;
> +    if (node)
> +	   return node;
> +
> +    for (RenderObject* object = parent(); object; object = object->parent())
{
> +	   if (Node* node = (object->node() == object->document() ? 0 :
object->node()))
> +	       return node;
> +    }
> +    return 0;
> +}

Does this behavior change affect other calls to generatingNode(), like the
counter code?