Bug 46639

Summary: Make computeLogicalHeight block-flow-aware.
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 46123    
Attachments:
Description Flags
Patch sam: review+

Description Dave Hyatt 2010-09-27 11:41:55 PDT
Make computeLogicalHeight block-flow-aware, and get layout limping enough that boxes have the correct widths/heights in vertical text mode.
Comment 1 Dave Hyatt 2010-09-27 12:04:38 PDT
Created attachment 68942 [details]
Patch
Comment 2 WebKit Review Bot 2010-09-27 12:08:14 PDT
Attachment 68942 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/RenderBox.cpp:1834:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Sam Weinig 2010-09-27 12:23:30 PDT
Comment on attachment 68942 [details]
Patch

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@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.
Comment 4 Dave Hyatt 2010-09-27 12:39:46 PDT
(In reply to comment #3)
> (From update of attachment 68942 [details])
> 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.
> 

Yeah, sounds good.

> > LayoutTests/ChangeLog:39
> > +2010-09-27  David Hyatt  <hyatt@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.

The reason I am not doing this yet is just because the x/y positions aren't right yet.  This way I have tests that pass as long as width/height are good and that won't break as I work on x/y.   As more of layout comes online, I'll fall back to traditional render tree dumps yes.
Comment 5 Dave Hyatt 2010-09-27 12:43:09 PDT
Fixed in r68417.
Comment 6 WebKit Review Bot 2010-09-27 14:05:15 PDT
http://trac.webkit.org/changeset/68417 might have broken Leopard Intel Debug (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/68416
http://trac.webkit.org/changeset/68417
http://trac.webkit.org/changeset/68415
Comment 7 Dave Hyatt 2010-09-27 14:06:08 PDT
It did.

See https://bugs.webkit.org/show_bug.cgi?id=46649

I will have a fix shortly.