Bug 128555 - [New Multicolumn] Make columns work with line grids
Summary: [New Multicolumn] Make columns work with line grids
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-10 14:37 PST by Dave Hyatt
Modified: 2014-02-11 08:47 PST (History)
6 users (show)

See Also:


Attachments
Patch (25.84 KB, patch)
2014-02-10 14:43 PST, Dave Hyatt
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2014-02-10 14:37:11 PST
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.
Comment 1 Dave Hyatt 2014-02-10 14:43:18 PST
Created attachment 223752 [details]
Patch
Comment 2 Simon Fraser (smfr) 2014-02-10 15:21:15 PST
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).
Comment 3 Dave Hyatt 2014-02-10 21:38:13 PST
(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.
Comment 4 Morten Stenshorne 2014-02-11 02:10:52 PST
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.
Comment 5 Dave Hyatt 2014-02-11 08:47:05 PST
Landed in r163878