[webkit-reviews] review granted: [Bug 80772] Split the extra logic out of RenderBlock::updateFirstLetter : [Attachment 131247] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Igor Trindade Oliveira
<igor.oliveira at webkit.org>'s request for review:
Bug 80772: Split the extra logic out of RenderBlock::updateFirstLetter
https://bugs.webkit.org/show_bug.cgi?id=80772

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=131247&action=review


> Source/WebCore/rendering/RenderBlock.cpp:6000
> +    RenderStyle* pseudoStyle = styleForFirstLetter(firstLetterBlock,
firstLetterContainer);

Our only use of |firstLetterBlock| is for styleForFirstLetter in both
functions. I wonder if this could not be pushed to the caller (not necessarily
in this patch)

> Source/WebCore/rendering/RenderBlock.cpp:6149
> +    // to update it.

Missing first part of the comment:
// If the child already has style, then it has already been created, so we just
want

> Source/WebCore/rendering/RenderBlock.cpp:6162
> +    createFirstLetter(firstLetterBlock, currChild);

Comment missing:

// Create our pseudo style now that we have our firstLetterContainer
determined.

I wonder if we want to keep it though as it doesn't add much.

> Source/WebCore/rendering/RenderBlock.h:532
> +    void createFirstLetter(RenderObject* firstLetterBlock, RenderObject*
currentChild);

I am not super happy with the naming of this function. How about
createFirstLetterRenderer (though it doesn't return a RenderObject which is
weird for a create function). I can't think of better so if you have better
ideas feel free to throw them here :)