Bug 127365 – [New Multicolumn] Table cells and list items need to work as multicolumn blocks

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

Right now, the presence of RenderMultiColumnBlock is a problem. By having a concrete class for multi-column blocks, I created a situation where table cells and list items (and the RenderView too) cannot be multi-column. The old code worked as mix-in to RenderBlock, and so it works.

This bug is tracking the elimination of RenderMultiColumnBlock. I'll be folding its code into RenderBlockFlow. The first step is to eliminate all of RenderMultiColumnBlock's member variables and move them elsewhere.

[Dave Hyatt]

Created attachment 221778 [details]
Patch

[Dave Hyatt]

Created attachment 221783 [details]
Patch

[WebKit Commit Bot]

Attachment 221783 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderMultiColumnFlowThread.h:64:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/rendering/RenderMultiColumnFlowThread.h:66:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 221783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221783&action=review

> Source/WebCore/rendering/RenderMultiColumnBlock.cpp:43
> +    RenderMultiColumnFlowThread* flowThread = new RenderMultiColumnFlowThread(document(), RenderStyle::createAnonymousStyleWithDisplay(&style(), BLOCK));
> +    flowThread->initializeStyle();
> +    RenderBlockFlow::addChild(flowThread);
> +    setMultiColumnFlowThread(flowThread);

Trying to work out the lifetime / ownership of the flowThread.  We create it here, and set it as both a child and as a member of the RenderBlockFlow rareData.  Does that mean it gets deleted as a result of the RenderBlockFlow's normal child deleting and we assume that corresponds with the rare data being torn down so we don't worry about the dangling pointer?

[Dave Hyatt]

(In reply to comment #4)
> (From update of attachment 221783 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221783&action=review
> 
> > Source/WebCore/rendering/RenderMultiColumnBlock.cpp:43
> > +    RenderMultiColumnFlowThread* flowThread = new RenderMultiColumnFlowThread(document(), RenderStyle::createAnonymousStyleWithDisplay(&style(), BLOCK));
> > +    flowThread->initializeStyle();
> > +    RenderBlockFlow::addChild(flowThread);
> > +    setMultiColumnFlowThread(flowThread);
> 
> Trying to work out the lifetime / ownership of the flowThread.  We create it here, and set it as both a child and as a member of the RenderBlockFlow rareData.  Does that mean it gets deleted as a result of the RenderBlockFlow's normal child deleting and we assume that corresponds with the rare data being torn down so we don't worry about the dangling pointer?

Correct. It's torn down by anonymous child cleanup.

[Dave Hyatt]

Note I didn't change the ownership. I just moved it from one class to another.

[Simon Fraser (smfr)]

Comment on attachment 221783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221783&action=review

> Source/WebCore/rendering/RenderBlockFlow.cpp:2895
> +    RenderBlockFlowRareData& rareData = ensureRareBlockFlowData();

Can flowThread be null and thus not require the creation of rareData?

> Source/WebCore/rendering/RenderBlockFlow.h:154
> +        RenderMultiColumnFlowThread* m_multiColumnFlowThread;

Could be a reference? There are lots of unchecked dereferences.

> Source/WebCore/rendering/RenderMultiColumnBlock.cpp:95
> +    multiColumnFlowThread()->setInBalancingPass(true); // Prevent re-entering this method (and recursion into layout).

This should use TemporaryChange<> or some other early-return-proof class.

> Source/WebCore/rendering/RenderMultiColumnFlowThread.h:48
> +    bool computeColumnCountAndWidth();

Add a comment to say what the return values means.

> Source/WebCore/rendering/RenderMultiColumnFlowThread.h:65
> +    LayoutUnit m_columnWidth; // since a multi-column block that is split across variable width pages or regions will have different column counts and widths in each.

since a -> A

> Source/WebCore/rendering/RenderMultiColumnFlowThread.h:68
> +    bool m_inBalancingPass; // Set when relayouting for column balancing.

Comment doesn't add anything.

[Dave Hyatt]

First round landed in r162702.