Bug 70557 - avoid unnecessary layouts of flex items during the flex pass
Summary: avoid unnecessary layouts of flex items during the flex pass
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: Tony Chang
URL:
Keywords:
Depends on:
Blocks: 62048
  Show dependency tree / graph
 
Reported: 2011-10-20 15:44 PDT by Tony Chang
Modified: 2011-10-25 12:25 PDT (History)
2 users (show)

See Also:

Attachments
Patch (6.60 KB, patch)
2011-10-20 15:49 PDT, Tony Chang 		no flags Details | Formatted Diff | Diff
Patch (6.61 KB, patch)
2011-10-25 11:45 PDT, Tony Chang 		no flags Details | Formatted Diff | Diff
Patch for landing (6.57 KB, patch)
2011-10-25 12:14 PDT, Tony Chang 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2011-10-20 15:44:01 PDT 
avoid unnecessary layouts of flex items during the flex pass
Comment 1 Tony Chang 2011-10-20 15:49:14 PDT 
Created attachment 111862 [details]
Patch
Comment 2 Ojan Vafai 2011-10-20 18:12:08 PDT 
Comment on attachment 111862 [details]
Patch

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

Patch looks good to me.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:496
> +    Length fixedWidth = isHorizontalFlow() ? child->style()->width() : child->style()->height();

Nit: I think the code would be simpler if you added flowAwareWidthStyleForChild instead of isFlowAwareLogicalWidthAutoForChild.
Comment 3 Dave Hyatt 2011-10-25 11:17:09 PDT 
Comment on attachment 111862 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:224
> +    Length width = isHorizontalFlow() ? child->style()->width() : child->style()->height();
> +    return width.isAuto();

Please name the local variable logicalWidth and not just width.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:497
> +    return fixedWidth.calcMinValue(flowAwareContentLogicalWidth());

Adding flowAwareLogicalWidthLengthForChild as Ojan suggests seems cleaner.
Comment 4 Tony Chang 2011-10-25 11:45:00 PDT 
Created attachment 112368 [details]
Patch
Comment 5 Ojan Vafai 2011-10-25 12:11:03 PDT 
Comment on attachment 112368 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:506
> +    if (flowAwareLogicalWidthLengthForChild(child).isAuto()) {
>          LayoutUnit logicalWidth = hasOrthogonalFlow(child) ? child->logicalHeight() : child->maxPreferredLogicalWidth();
>          return logicalWidth - logicalBorderAndPaddingWidthForChild(child) - logicalScrollbarHeightForChild(child);
>      }
> -    return isHorizontalFlow() ? child->contentWidth() : child->contentHeight();
> +    Length fixedWidth = isHorizontalFlow() ? child->style()->width() : child->style()->height();
> +    return fixedWidth.calcMinValue(flowAwareContentLogicalWidth());

nit: i would do this like so:
Length logicalWidthLength = flowAwareLogicalWidthLengthForChild(child);
if (logicalWidthLength.isAuto()) {
...
}
return logicalWidthLength.calcMinValue(...);
Comment 6 Tony Chang 2011-10-25 12:14:33 PDT 
Created attachment 112372 [details]
Patch for landing
Comment 7 Tony Chang 2011-10-25 12:25:04 PDT 
Committed r98373: <http://trac.webkit.org/changeset/98373>