Bug 122512 – Move m_floatingObjects to RenderBlockFlow from RenderBlock

• Component: New Bugs
• Status: RESOLVED FIXED

[Bem Jones-Bey]

Move m_floatingObjects to RenderBlockFlow from RenderBlock

[Bem Jones-Bey]

Created attachment 213698 [details]
Patch

[Dave Hyatt]

Comment on attachment 213698 [details]
Patch

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.

[Bem Jones-Bey]

(In reply to comment #2)
> (From update of attachment 213698 [details])
> 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.

The only ones I didn't add the FIXMEs to are computeOverflow and adjustForBorderFit. Does that jibe with what you expect?

> 
> > 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?

Yup. Must have forgotten to remove that.

> 
> > 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.

Ok, I've put the name back.

[Bem Jones-Bey]

Created attachment 213713 [details]
Patch

Update for review comments

[Dave Hyatt]

Comment on attachment 213713 [details]
Patch

r=me. Note the patch doesn't apply, so make sure you run layout tests, etc. on tip of tree.

[Bem Jones-Bey]

Created attachment 213726 [details]
Patch

Rebase

[Bem Jones-Bey]

Comment on attachment 213726 [details]
Patch

ran all tests, looks good on the EWS, passing off to CQ.

[WebKit Commit Bot]

Comment on attachment 213726 [details]
Patch

Clearing flags on attachment: 213726

Committed r157144: <http://trac.webkit.org/changeset/157144>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.