[webkit-reviews] review denied: [Bug 123013] Bad cast with toRenderBoxModelObject in RenderBlock::updateFirstLetter() : [Attachment 214584] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has denied Joone Hur <joone at webkit.org>'s
request for review:
Bug 123013: Bad cast with toRenderBoxModelObject in
RenderBlock::updateFirstLetter()
https://bugs.webkit.org/show_bug.cgi?id=123013

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

------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=214584&action=review


Can we make a test for this?

> Source/WebCore/rendering/RenderBlock.cpp:5179
> +		   RenderObject* oldRemainingText =
oldFirstLetter->firstLetterRemainingText();

You should use a tighter type than RenderObject* here, since
firstLetterRemainingText() returns a RenderTextFragment.
I would use auto, like so:

    auto oldRemainingText = oldFirstLetter->firstLetterRemainingText();

> Source/WebCore/rendering/RenderBlock.cpp:5180
> +		   if (oldRemainingText && oldRemainingText->isText()) {

isText() here will always be true.

> Source/WebCore/rendering/RenderBlock.cpp:5183
> +		      
toRenderText(oldRemainingText)->setText(toText(oldRemainingText->node())->data(
).impl());

If you use a tighter type like I suggested above, this line could become:
oldRemainingText->setText(oldRemainingText->textNode()->data());