[webkit-reviews] review denied: [Bug 122512] Move m_floatingObjects to RenderBlockFlow from RenderBlock : [Attachment 213698] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Bem Jones-Bey <bjonesbe at adobe.com>'s
request for review:
Bug 122512: Move m_floatingObjects to RenderBlockFlow from RenderBlock
https://bugs.webkit.org/show_bug.cgi?id=122512

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

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


A number of methods have become virtual, but the virtualization is clearly
temporary. In some cases the virtualization only exists because methods haven't
moved yet. deleteLineBoxTree is a great example. I have it virtualized in my
own patch as well, but I marked it as temporary with a FIXME, because all of
the callers are ultimately going to be in RenderBlockFlow too. Some of your
virtualizing is similar, e.g., the logical offset for line functions (which
obviously will only be in RenderBlockFlow once lines are there too).

So basically, I'm fine with virtualizing, but I think nearly all of your
virtualizations are temporary and should be marked with FIXMEs so we know to
how to clean up RenderBlock once lines also move.

> Source/WebCore/rendering/RenderBlock.h:512
> +    virtual void addOverflowFromFloats() { };

Seems like this is fully moved to RenderBlockFlow, so you could just yank it
from RenderBlock and make RenderBlockFlow's non-virtual?

> Source/WebCore/rendering/RenderBlock.h:551
> -    void moveAllChildrenIncludingFloatsTo(RenderBlock* toBlock, bool
fullRemoveInsert);
> +    virtual void moveAllChildrenOnRemovalTo(RenderBlock* toBlock, bool
fullRemoveInsert) { moveAllChildrenTo(toBlock, fullRemoveInsert); }

I don't see any reason to rename this.