Bug 95498 - [New Multicolumn] Implement unforced breaking in columns
Summary: [New Multicolumn] Implement unforced breaking in columns
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: 2012-08-30 14:08 PDT by Dave Hyatt
Modified: 2012-08-31 08:46 PDT (History)
2 users (show)

See Also:


Attachments
Patch (20.63 KB, patch)
2012-08-30 14:22 PDT, Dave Hyatt
mitz: 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 2012-08-30 14:08:50 PDT
Implement proper unforced breaking inside columns. Lines and objects now break across boundaries correctly. Unforced breaks work. Forced ones are muddled and still use region rather than column. I'll cover that in a separate bug.
Comment 1 Dave Hyatt 2012-08-30 14:22:12 PDT
Created attachment 161556 [details]
Patch
Comment 2 mitz 2012-08-30 14:45:34 PDT
Comment on attachment 161556 [details]
Patch

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

> Source/WebCore/ChangeLog:36
> +        thus forcing us to repaginate everythign.

typo: everythign

> Source/WebCore/ChangeLog:42
> +        Always return the last region if it's a set, regardless of the extendLastRegion boolan.

typo: boolan

> Source/WebCore/rendering/RenderBlock.cpp:1455
> +    } else if (isRenderFlowThread()) {
> +        pageLogicalHeight = 1; // This is just a hack to always make sure we have a page logical height.
> +        pageLogicalHeightChanged = toRenderFlowThread(this)->pageLogicalHeightChanged();

I’d like to see RenderBlock doing less testing for known subclasses of itself and more calling virtual functions that have overrides in those subclasses. This is a general comment, though, and you don’t need to address it in this iteration.

> Source/WebCore/rendering/RenderRegion.h:111
> +    // The top of the nearest page inside the region. For RenderRegions, this is just the logical top of the
> +    // flow thread portion we contain. For sets, we have to figure out the top of the nearest column or
> +    // page.

“nearest” could imply that we search in both directions, but we never return the top of a page that’s *after* the offset, do we?
Comment 3 Dave Hyatt 2012-08-31 08:46:48 PDT
Landed in r127267.