Bug 46639 - Make computeLogicalHeight block-flow-aware.
Summary: Make computeLogicalHeight block-flow-aware.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks: 46123
  Show dependency treegraph
 
Reported: 2010-09-27 11:41 PDT by Dave Hyatt
Modified: 2010-09-27 14:06 PDT (History)
3 users (show)

See Also:


Attachments
Patch (53.61 KB, patch)
2010-09-27 12:04 PDT, Dave Hyatt
sam: 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 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.