Bug 87525 – [New MultiColumn] Create the flow thread and make sure children get placed inside it.

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

[Dave Hyatt]

Create the flow thread and make sure children get placed inside it.

[Dave Hyatt]

Created attachment 144119 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 144119 [details]
Patch

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

LGTM, other than the comments being a bit odd.  Won't this break the multi-column painting?  (since presumably the children used to be direct decendents of MultiColumBlock?)  Or does it happen to work since the flowThread is just a block and so it just seems to have an anonymous block inside it, but otherwise works?

> Source/WebCore/rendering/RenderMultiColumnBlock.cpp:83
> +        RenderBlock::addChild(m_flowThread); // Always put the flow thread at the end.

Interesting.  I thought regions made all their flow threads direct children of the body element.  I guess in that case, they can flow into any element, where here you're contained within the block marked for multicolumn?

> Source/WebCore/rendering/RenderMultiColumnBlock.cpp:87
> +    // of the flow thread itself.

It would be nice if the thread could be a non-displayed child of some form.  Since with the exception of render region, we don't generally have children which are not displayed.

> Source/WebCore/rendering/RenderMultiColumnBlock.cpp:89
> +        RenderBlock::addChild(newChild, beforeChild);

I'm confused why the comment above says to always put the flow-thread at the end, when this code won't keep it at the end.  It seems the flow ends up always being the first child.

[Dave Hyatt]

(In reply to comment #2)
> (From update of attachment 144119 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144119&action=review
> 
> LGTM, other than the comments being a bit odd.  Won't this break the multi-column painting?  (since presumably the children used to be direct decendents of MultiColumBlock?)  Or does it happen to work since the flowThread is just a block and so it just seems to have an anonymous block inside it, but otherwise works?

This is a brand new implementation. It's not building on previous code. Nothing can "break" since nobody is using this code. It's all turned off in ToT. Painting is not implemented yet.

> 
> > Source/WebCore/rendering/RenderMultiColumnBlock.cpp:83
> > +        RenderBlock::addChild(m_flowThread); // Always put the flow thread at the end.
> 
> Interesting.  I thought regions made all their flow threads direct children of the body element.  I guess in that case, they can flow into any element, where here you're contained within the block marked for multicolumn?

Yes, this is a difference (and the reason for the conceptual split between RenderNamedFlowThread and RenderFlowThread). Named flow threads are document-level. Multi-column (and eventually paged) flow threads are children of the block that spawned them.

> 
> > Source/WebCore/rendering/RenderMultiColumnBlock.cpp:87
> > +    // of the flow thread itself.
> 
> It would be nice if the thread could be a non-displayed child of some form.  Since with the exception of render region, we don't generally have children which are not displayed.

The flow thread does get displayed. It just does so through the regions. It's not undisplayed though.
> 
> > Source/WebCore/rendering/RenderMultiColumnBlock.cpp:89
> > +        RenderBlock::addChild(newChild, beforeChild);
> 
> I'm confused why the comment above says to always put the flow-thread at the end, when this code won't keep it at the end.  It seems the flow ends up always being the first child.

You're looking at the column set case not the flow thread case. The comment was on this line:

RenderBlock::addChild(m_flowThread); // Always put the flow thread at the end.

Which does put the flow thread at the end.

When I do get around to adding column sets (not yet implemented), they'll always be inserted before the flow thread, so this function doesn't have to special case that at all.

[Build Bot]

Comment on attachment 144119 [details]
Patch

Attachment 144119 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12791608

[Eric Seidel (no email)]

Comment on attachment 144119 [details]
Patch

OK.  thanks.  You may need to fix Win.

[Dave Hyatt]

(In reply to comment #5)
> (From update of attachment 144119 [details])
> OK.  thanks.  You may need to fix Win.

Yup, will do. Just need to add the multi-column files to RenderingAllInOne.cpp.

[Dave Hyatt]

Fixed in r118552.

[Darin Adler]

Comment on attachment 144119 [details]
Patch

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

> Source/WebCore/rendering/RenderMultiColumnBlock.h:51
> +
>  private:
> +    RenderMultiColumnFlowThread* m_flowThread;

An extra private here we could delete.

> Source/WebCore/rendering/RenderMultiColumnFlowThread.cpp:38
> +RenderMultiColumnFlowThread::~RenderMultiColumnFlowThread()
> +{
> +}

Why was this added?

> Source/WebCore/rendering/RenderMultiColumnFlowThread.h:37
> +    ~RenderMultiColumnFlowThread();

Normally we explicitly make it virtual.

> Source/WebCore/rendering/RenderMultiColumnSet.h:48
> +    virtual bool isRenderMultiColumnSet() const OVERRIDE { return true; }

Should be private, not public.

> Source/WebCore/rendering/RenderObject.h:366
>      virtual bool isRenderNamedFlowThread() const { return false; }
> +    
> +    virtual bool isRenderMultiColumnSet() const { return false; }
> +
>      virtual bool isRenderScrollbarPart() const { return false; }

Why add these blank lines?