Bug 114853

Summary: Make loops in RenderObject::containingBlock homogeneous in their forms to simplify
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Layout and RenderingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, hyatt, kling, robert, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 113479    
Attachments:
Description Flags
Cleanup
none
Fixed builds
none
Fixed per comments
none
Removed an obsolete comment hyatt: review+

Description Ryosuke Niwa 2013-04-18 22:49:10 PDT
Make loops in RenderObject::containingBlock homogeneous in their forms to simplify
Comment 1 Ryosuke Niwa 2013-04-18 22:53:53 PDT
Created attachment 198805 [details]
Cleanup
Comment 2 Ryosuke Niwa 2013-04-18 22:56:01 PDT
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.
Comment 3 Ryosuke Niwa 2013-04-18 23:08:18 PDT
Created attachment 198807 [details]
Fixed builds
Comment 4 Simon Fraser (smfr) 2013-04-19 08:57:26 PDT
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"
Comment 5 Dave Hyatt 2013-04-19 10:43:46 PDT
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.
Comment 6 Ryosuke Niwa 2013-04-19 11:01:36 PDT
Created attachment 198900 [details]
Fixed per comments
Comment 7 Ryosuke Niwa 2013-04-19 11:14:30 PDT
Created attachment 198903 [details]
Removed an obsolete comment
Comment 8 Dave Hyatt 2013-04-19 12:00:22 PDT
Comment on attachment 198903 [details]
Removed an obsolete comment

r=me
Comment 9 Ryosuke Niwa 2013-04-19 12:50:31 PDT
Thanks for the review!
Comment 10 Ryosuke Niwa 2013-04-19 12:56:49 PDT
Committed r148759: <http://trac.webkit.org/changeset/148759>
Comment 11 Ryosuke Niwa 2013-04-19 15:48:50 PDT
Added a missing null pointer check in http://trac.webkit.org/changeset/148777.