[webkit-reviews] review granted: [Bug 46639] Make computeLogicalHeight block-flow-aware. : [Attachment 68942] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Dave Hyatt <hyatt at apple.com>'s request
for review:
Bug 46639: Make computeLogicalHeight block-flow-aware.
https://bugs.webkit.org/show_bug.cgi?id=46639

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68942&action=review

> WebCore/rendering/RenderBox.cpp:1759
> +	   if (document()->printing())
> +	       visHeight = static_cast<int>(view()->pageHeight());
> +	   else if (style()->isVerticalBlockFlow())
> +	       visHeight = view()->viewHeight();
> +	   else
> +	       visHeight = view()->viewWidth();

I think this might read clearer as an if-else:

if (document()->printing()
   foo()
else {
    if (style->verticalBlockFlow())
       bar()
    else
       baz();
}

That way, when paginated support comes, it is more clear where the branch needs
to go.

> LayoutTests/ChangeLog:39
> +2010-09-27  David Hyatt  <hyatt at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Need a short description and bug URL (OOPS!)
> +
> +	   * fast/blockflow/auto-margins-across-boundaries-expected.txt: Added.

> +	   * fast/blockflow/auto-margins-across-boundaries.html: Added.
> +	   * fast/blockflow/block-formatting-context-expected.txt: Added.
> +	   * fast/blockflow/block-formatting-context.html: Added.
> +	   * fast/blockflow/percentage-padding.html:
> +	   * fast/blockflow/relative-positioning-percentages-expected.txt:
Added.
> +	   * fast/blockflow/relative-positioning-percentages.html: Added.
> +

Double changelog entry!

It think it probably makes sense to add some layout tests that don't use
getClientRects (dumps the render tree the old fashion way) just incase that
function is not returning the right results.