Move m_floatingObjects to RenderBlockFlow from RenderBlock
Created attachment 213698 [details] Patch
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.
(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.
Created attachment 213713 [details] Patch Update for review comments
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.
Created attachment 213726 [details] Patch Rebase
Comment on attachment 213726 [details] Patch ran all tests, looks good on the EWS, passing off to CQ.
Comment on attachment 213726 [details] Patch Clearing flags on attachment: 213726 Committed r157144: <http://trac.webkit.org/changeset/157144>
All reviewed patches have been landed. Closing bug.