Summary: | Make computeLogicalHeight block-flow-aware. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dave Hyatt <hyatt> | ||||
Component: | Layout and Rendering | Assignee: | 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
Dave Hyatt
2010-09-27 11:41:55 PDT
Created attachment 68942 [details]
Patch
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 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. (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. 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 It did. See https://bugs.webkit.org/show_bug.cgi?id=46649 I will have a fix shortly. |