Bug 96704 - Simplify some code in RenderBox::computePercentageLogicalHeight
Summary: Simplify some code in RenderBox::computePercentageLogicalHeight
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-13 16:23 PDT by Ojan Vafai
Modified: 2012-09-14 11:42 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.00 KB, patch)
2012-09-13 16:24 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (7.42 KB, patch)
2012-09-14 11:17 PDT, Ojan Vafai
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2012-09-13 16:23:12 PDT
Simplify some code in RenderBox::computePercentageLogicalHeight
Comment 1 Ojan Vafai 2012-09-13 16:24:18 PDT
Created attachment 163996 [details]
Patch
Comment 2 Tony Chang 2012-09-14 09:58:21 PDT
Comment on attachment 163996 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163996&action=review

> Source/WebCore/rendering/RenderBox.cpp:-2129
> -    // In quirks mode, blocks with auto height are skipped, and we keep looking for an enclosing
> -    // block that may have a specified height and then use it. In strict mode, this violates the
> -    // specification, which states that percentage heights just revert to auto if the containing
> -    // block has an auto height. We still skip anonymous containing blocks in both modes, though, and look

I kind of like the comment about how quirks mode and standards mode are different.  Maybe just rewrite it into a shorter sentence?

> Source/WebCore/rendering/RenderBox.cpp:2130
> +    while ((document()->inQuirksMode() || cb->isAnonymousBlock())
> +        && !cb->isRenderView() && !cb->isTableCell() && !cb->isOutOfFlowPositioned() && cb->style()->logicalHeight().isAuto() && isHorizontalWritingMode() == cb->isHorizontalWritingMode()) {

I find this impossible to read, even the old code was easier to read.  Can we make a helper function or something?
Comment 3 Tony Chang 2012-09-14 10:04:08 PDT
(In reply to comment #2)
> (From update of attachment 163996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163996&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:2130
> > +    while ((document()->inQuirksMode() || cb->isAnonymousBlock())
> > +        && !cb->isRenderView() && !cb->isTableCell() && !cb->isOutOfFlowPositioned() && cb->style()->logicalHeight().isAuto() && isHorizontalWritingMode() == cb->isHorizontalWritingMode()) {
> 
> I find this impossible to read, even the old code was easier to read.  Can we make a helper function or something?

Something like,
bool shouldTraverseParentForPercentHeight() {
    return document()->inQuirksMode() || cb->isAnonymousBlock();
}

and

bool useParentHeightForPrecentHeight() {
    return !cb->isTableCell() && !cb->isOutOfFlowPositioned() && cb->style()->logicalHeight().isAuto() && isHorizontalWritingMode() == cb->isHorizontalWritingMode();
}

So the while would be:

while (!cb->isRenderView() && shouldTraverseParentForPercentHeight() && useParentHeightForPercentHeight()) {
    ...
}
Comment 4 Ojan Vafai 2012-09-14 11:17:39 PDT
Created attachment 164195 [details]
Patch
Comment 5 Ojan Vafai 2012-09-14 11:42:26 PDT
Committed r128633: <http://trac.webkit.org/changeset/128633>