[webkit-reviews] review denied: [Bug 114853] Make loops in RenderObject::containingBlock homogeneous in their forms to simplify : [Attachment 198807] Fixed builds

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 114853: Make loops in RenderObject::containingBlock homogeneous in their
forms to simplify
https://bugs.webkit.org/show_bug.cgi?id=114853

Attachment 198807: Fixed builds
https://bugs.webkit.org/attachment.cgi?id=198807&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=198807&action=review


r-

> Source/WebCore/rendering/RenderObject.cpp:774
> +static inline bool isNonReplacedInlineInFlowPosition(RenderObject* object)
> +{
> +    return object->style()->hasInFlowPosition() && object->isInline() &&
!object->isReplaced();
> +}

Pretty sure this function has no reason for existing and is just historical
from when RenderBlocks could be inline flows because of run-in/compact. I think
this can just be isRenderInline() now.

> Source/WebCore/rendering/RenderObject.cpp:778
> +    // FIXME: This set of conditions can be simplified by combinging the
first and the last conditions.

Typo. combinging should be combining.

> Source/WebCore/rendering/RenderObject.cpp:785
> +	   || isNonReplacedInlineInFlowPosition(object);

I'm pretty sure this can just be isRenderInline().

> Source/WebCore/rendering/RenderObject.cpp:791
> +static inline bool isNonInlineRenderBlock(RenderObject* object)
> +{
> +    return object->isRenderBlock() && (!object->isInline() ||
object->isReplaced());
> +}

I would invert this function and make it:

isNonRenderBlockInline and make the definition

(o->isInline() && !o->isReplaced()) || !o->isRenderBlock())

Your version makes a virtual function call right off the bat. Preserve the
original code that avoided the virtual function call by using inline/replaced
checks first.

> Source/WebCore/rendering/RenderObject.cpp:816
> +	   while (o && !isNonInlineRenderBlock(o))

This can be while (o && isNonRenderBlockInline())

This reads better to me without the negation.