[webkit-reviews] review granted: [Bug 137704] Use is<>() / downcast<>() for RenderInline : [Attachment 239812] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 137704: Use is<>() / downcast<>() for RenderInline
https://bugs.webkit.org/show_bug.cgi?id=137704

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=239812&action=review


> Source/WebCore/rendering/InlineFlowBox.cpp:222
> -    toRenderInline(renderer()).lineBoxes().removeLineBox(this);
> +    downcast<RenderInline>(renderer()).lineBoxes().removeLineBox(this);

Seems like renderer() should return a RenderInline& if it’s guaranteed to be
one. Maybe there’s some reason not to do this.

> Source/WebCore/rendering/InlineFlowBox.cpp:1011
> +    for (InlineBox* current = lastChild(); current; current =
current->prevOnLine()) {

I think I would have called this “child” instead of “current”. I noticed one
like this in a previous patch, and probably should have commented then.

> Source/WebCore/rendering/RenderBox.cpp:2013
> +    ASSERT(renderer == container() || is<RenderRegion>(*renderer));

Looks like this function should take a RenderElement& given that assertion.
Maybe some day we can make that change.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:217
> +    while (is<RenderInline>(parent)) {

This would read better as a for loop. If we can’t fit it all on one line we
could leave out the initializer, but the termination and “move to parent” would
be fit nicely in a for.