[webkit-reviews] review granted: [Bug 46519] Convert the implementation of computeLogicalWidth to work with block-flow : [Attachment 68766] Clean up the ChangeLog typos.

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Dave Hyatt <hyatt at apple.com>'s request
for review:
Bug 46519: Convert the implementation of computeLogicalWidth to work with
block-flow
https://bugs.webkit.org/show_bug.cgi?id=46519

Attachment 68766: Clean up the ChangeLog typos.
https://bugs.webkit.org/attachment.cgi?id=68766&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68766&action=review

> WebCore/rendering/RenderBox.cpp:1492
> +    // Fieldsets do this.

This comment could be improved.

> WebCore/rendering/RenderBox.cpp:1613
> +    Length marginStartLength =
style()->marginStartUsing(containingBlock->style());
> +    Length marginEndLength =
style()->marginEndUsing(containingBlock->style());
> +    const RenderStyle* containingBlockStyle = containingBlock->style();
> +
> +    // Case One: The object is being centered in the containing block's
available logical width.
> +    if ((marginStartLength.isAuto() && marginEndLength.isAuto() &&
childWidth < containerWidth)
> +	   || (!marginStartLength.isAuto() && !marginEndLength.isAuto() &&
containingBlock->style()->textAlign() == WEBKIT_CENTER)) {
> +	   setMarginStartUsing(containingBlockStyle, max(0, (containerWidth -
childWidth) / 2));
> +	   setMarginEndUsing(containingBlockStyle, containerWidth - childWidth
- marginStartUsing(containingBlockStyle));
> +	   return;
> +    } 
> +    
> +    // Case Two: The object is being pushed to the start of the containing
block's available logical width.
> +    if (marginEndLength.isAuto() && childWidth < containerWidth) {
> +	   setMarginStartUsing(containingBlockStyle,
marginStartLength.calcValue(containerWidth));
> +	   setMarginEndUsing(containingBlockStyle, containerWidth - childWidth
- marginStartUsing(containingBlockStyle));
> +	   return;
> +    } 
> +    
> +    // Case Three: The object is being pushed to the end of the containing
block's available logical width.
> +    bool pushToEndFromTextAlign = !marginEndLength.isAuto() &&
((containingBlockStyle->direction() == RTL && containingBlockStyle->textAlign()
== WEBKIT_LEFT)
> +	   || (containingBlockStyle->direction() == LTR &&
containingBlockStyle->textAlign() == WEBKIT_RIGHT));
> +    if ((marginStartLength.isAuto() && childWidth < containerWidth) ||
pushToEndFromTextAlign) {
> +	   setMarginEndUsing(containingBlockStyle,
marginEndLength.calcValue(containerWidth));
> +	   setMarginStartUsing(containingBlockStyle, containerWidth -
childWidth - marginEndUsing(containingBlockStyle));
> +	   return;
> +    } 
> +    
> +    // Case Four: No auto margins. Just compute normally.
> +    // This makes auto margins 0 if we failed a logicalWidth() <
containerWidth test above (css2.1, 10.3.3).
> +    setMarginStartUsing(containingBlockStyle,
marginStartLength.calcMinValue(containerWidth));
> +    setMarginEndUsing(containingBlockStyle,
marginEndLength.calcMinValue(containerWidth));

I am not sure how hot these functions are, but we could avoid some branches by
adding marginStartAndEndUsing()/setMarginStartAndEndUsing() style functions to
get/set both at once. You might also consider having the setMargin functions
return the length they set, since you often use it on the next line.

> WebCore/rendering/RenderBox.h:198
> +    // of the containing block..

Typo. Double period.