Bug 128555 – [New Multicolumn] Make columns work with line grids

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dave Hyatt]

Make columns work with line grids. They need to set up the line grid pagination origin properly, and regions/flow threads have a bug or two that needs to be fixed.

[Dave Hyatt]

Created attachment 223752 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 223752 [details]
Patch

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

> Source/WebCore/rendering/LayoutState.h:89
> +    LayoutSize pageOffset() const { return m_pageOffset; }

Is this really offsetFromPage()? that would be a clearer name than pageOffset (which could be the offset of a page).

[Dave Hyatt]

(In reply to comment #2)
> (From update of attachment 223752 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223752&action=review
> 
> > Source/WebCore/rendering/LayoutState.h:89
> > +    LayoutSize pageOffset() const { return m_pageOffset; }
> 
> Is this really offsetFromPage()? that would be a clearer name than pageOffset (which could be the offset of a page).

It's actually really complicated what that represents. It is the cumulative offset of the logical top of the first page within the layout state. I agree it needs a better name, but should probably be a separate patch, since it's a member variable and that term is used all over.

[Morten Stenshorne]

Comment on attachment 223752 [details]
Patch

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

> Source/WebCore/rendering/LayoutState.cpp:-227
> -void LayoutState::computeLineGridPaginationOrigin(RenderBox* renderer)

Should remove the method declaration from the .h file too.

> Source/WebCore/rendering/RenderBox.cpp:-4414
> -        || isMultiColumnBlockFlow();

With this change, isMultiColumnBlockFlow() will become unused, won't it?
Anyway, I wonder if you should keep it here, since paginating a multicol still doesn't work.

[Dave Hyatt]

Landed in r163878