[webkit-reviews] review granted: [Bug 110169] Encapsulate FloatingObject's constructor inside create : [Attachment 188967] Cleanup

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 110169: Encapsulate FloatingObject's constructor inside create
https://bugs.webkit.org/show_bug.cgi?id=110169

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

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


> Source/WebCore/rendering/RenderBlock.cpp:7714
> +void RenderBlock::ensureFloatingObjects()

I’m not such a big fan of the naming here. I understand, though, that we’re
using the word “ensure” for this in many places in WebKit now. But I have two
thoughts:

1) We should find a better name or short phrase for this that is less like
jargon if we can.
2) In other places, I think this is only used when the function has a return
value.

To me this is just createFloatingObjects. The fact that it's smart enough not
to recreate it if it already exists seems like a detail that need not be in its
name.

> Source/WebCore/rendering/RenderBlock.cpp:7717
> +    if (!m_floatingObjects)
> +	   m_floatingObjects = FloatingObjects::create(this,
isHorizontalWritingMode());

I like early return when we already have it better than nesting like this, but
it's just a matter of taste.

> Source/WebCore/rendering/RenderBlock.cpp:7720
> +inline PassOwnPtr<RenderBlock::FloatingObjects>
RenderBlock::FloatingObjects::create(const RenderBlock* renderer, bool
horizontalWritingMode)

With older compilers, the inline needs to before the code that uses it. Thus, I
suggest reordering these three things in exactly the reverse order they
currently appear.