[webkit-reviews] review granted: [Bug 95498] [New Multicolumn] Implement unforced breaking in columns : [Attachment 161556] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Dave Hyatt <hyatt at apple.com>'s request for review:
Bug 95498: [New Multicolumn] Implement unforced breaking in columns
https://bugs.webkit.org/show_bug.cgi?id=95498

Attachment 161556: Patch
https://bugs.webkit.org/attachment.cgi?id=161556&action=review

------- Additional Comments from mitz at webkit.org
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?