[webkit-reviews] review granted: [Bug 38402] Paginate and column-break at layout time rather than when painting : [Attachment 67819] New patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dave Hyatt <hyatt at apple.com>'s
request for review:
Bug 38402: Paginate and column-break at layout time rather than when painting
https://bugs.webkit.org/show_bug.cgi?id=38402

Attachment 67819: New patch
https://bugs.webkit.org/attachment.cgi?id=67819&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context:
https://bugs.webkit.org/attachment.cgi?id=67819&action=prettypatch

I reviewed a lot but I’m still not sure I read enough of the actual algorithms.
I am going to say review+ but you probably want at least a bit of review from
people who will read more of the layout algorithms.

> WebCore/rendering/ColumnInfo.h:60
> +    void setColumnCountAndHeight(int c, int h)

I would prefer argument names of count and height rather than c and h.

> WebCore/rendering/ColumnInfo.h:97
> +    int m_forcedBreaks; // FIXME: We will ultimately need to cache way more
information to balance around forced breaks properly.

I don’t think “way more” is way better than “more” here.

I don’t know what “balance around” means, but maybe experts would know.

> WebCore/rendering/LayoutState.cpp:38
> +LayoutState::LayoutState(LayoutState* prev, RenderBox* renderer, const
IntSize& offset, int pageHeight, ColumnInfo* colInfo)
> +    : m_columnInfo(colInfo)

I’d prefer “columnInfo” to “colInfo”.

> WebCore/rendering/LayoutState.cpp:88
> +	   m_pageOffset = IntSize(m_layoutOffset.width() +
renderer->borderLeft() + renderer->paddingLeft(),
> +				  m_layoutOffset.height() +
renderer->borderTop() + renderer->paddingTop());

I know it’s pretty, but we normally don’t line up the second line with the
parenthesis like this. Makes it look bad if you ever rename things.

> WebCore/rendering/LayoutState.h:66
> +    bool paginatingColumns() const { return m_columnInfo; }
> +    bool paginated() const { return m_pageHeight || m_columnInfo; }

I think these would be more readable as isPaginatingColumns and isPaginated.

> WebCore/rendering/LayoutState.h:81
> +    IntSize m_layoutDelta; // Transient offset from the final position of
the object
> +			      // used to ensure that repaints happen in the
correct place.
> +			      // This is a total delta accumulated from the
root.

Same comment about indented lines. It’s better not to do this, so things stay
good even if we rename later. I know the existing code already did it.

> WebCore/rendering/RenderBlock.cpp:1141
> +    ColumnInfo* colInfo = columnInfo();

I’d prefer columnInfo to colInfo.

> WebCore/rendering/RenderBlock.cpp:2916
> +    // Our location is irrelevant if we're unsplittable or no pagination is
in effect.  Just go ahead
> +    // and lay out the float.

Would read better if you broke lines where you broke sentences.

> WebCore/rendering/RenderBlock.cpp:2956
> +void RenderBlock::removeFloatingObjects(FloatingObject* f, int y)

Instead of “f” it would be good to name this in some way that made clear what
the argument is.

I think this function is confusing. It’s “remove floatingObjects after this one
that are below this y coordinate”? Is there some better way to explain that in
the function name?

> WebCore/rendering/RenderBlock.cpp:4442
> +		       int distanceBetweenBreaks =
max(colInfo->maximumDistanceBetweenForcedBreaks(),
> +						      
view()->layoutState()->pageY(borderTop() + paddingTop() + contentHeight()) -
colInfo->forcedBreakOffset());

Better not to try to line up lines like these as I said before.

> WebCore/rendering/RenderBlock.h:606
> +    RenderBlockRareData* m_rareData;

Can we use an OwnPtr for rare data? If it’s really rare, can we use a hash map
and a bit for it instead of a pointer?

> WebCore/rendering/RenderTable.h:145
> -
> +    

You should just revert the whitespace-only change to RenderTable.h.