Bug 114853 – Make loops in RenderObject::containingBlock homogeneous in their forms to simplify

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ryosuke Niwa]

Make loops in RenderObject::containingBlock homogeneous in their forms to simplify

[Ryosuke Niwa]

Created attachment 198805 [details]
Cleanup

[Ryosuke Niwa]

Comment on attachment 198805 [details]
Cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=198805&action=review

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

Oops, I obviously didn't upload the right patch.

[Ryosuke Niwa]

Created attachment 198807 [details]
Fixed builds

[Simon Fraser (smfr)]

Comment on attachment 198807 [details]
Fixed builds

View in context: https://bugs.webkit.org/attachment.cgi?id=198807&action=review

I'd prefer hyatt do final review. Be careful here, this is hot code!

> Source/WebCore/rendering/RenderObject.cpp:807
> +        // For a realtively positioned inline, return its nearest non-anonymous containing block,

"realtively"

[Dave Hyatt]

Comment on attachment 198807 [details]
Fixed builds

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.

[Ryosuke Niwa]

Created attachment 198900 [details]
Fixed per comments

[Ryosuke Niwa]

Created attachment 198903 [details]
Removed an obsolete comment

[Dave Hyatt]

Comment on attachment 198903 [details]
Removed an obsolete comment

r=me

[Ryosuke Niwa]

Thanks for the review!

[Ryosuke Niwa]

Committed r148759: <http://trac.webkit.org/changeset/148759>

[Ryosuke Niwa]

Added a missing null pointer check in http://trac.webkit.org/changeset/148777.