[webkit-reviews] review denied: [Bug 47148] CSS 2.1 failure: abspos-non-replaced-width-margin-000, abspos-replaced-width-margin-000 : [Attachment 106479] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Robert Hogan <robert at webkit.org>'s
request for review:
Bug 47148: CSS 2.1 failure: abspos-non-replaced-width-margin-000,
abspos-replaced-width-margin-000
https://bugs.webkit.org/show_bug.cgi?id=47148

Attachment 106479: Patch
https://bugs.webkit.org/attachment.cgi?id=106479&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=106479&action=review


> Source/WebCore/rendering/RenderBlock.h:132
> -    LayoutUnit startOffsetForLine(LayoutUnit position, bool firstLine) const
{ return style()->isLeftToRightDirection() ? logicalLeftOffsetForLine(position,
firstLine) : logicalRightOffsetForLine(position, firstLine); }
> +    LayoutUnit startOffsetForLine(LayoutUnit position, bool firstLine)
const;
>      LayoutUnit startAlignedOffsetForLine(RenderBox* child, LayoutUnit
position, bool firstLine);

This can still be inlined. You just needed to change it to this:

LayoutUnit startOffsetForLine(LayoutUnit position, bool firstLine) const {
return style()->isLeftToRightDirection() ? logicalLeftOffsetForLine(position,
firstLine) : width() - logicalRightOffsetForLine(position, firstLine); }

You can patch startAlignedOffsetForLine the same way... just do width() -
(final answer).

Seems like we need to make sure we have tests that cover this so we don't break
it again.

> Source/WebCore/rendering/RenderBox.cpp:2214
> -	   LayoutUnit staticPosition = child->layer()->staticInlinePosition() +
containerLogicalWidth + containerBlock->borderLogicalRight();
> +	   LayoutUnit staticPosition = child->layer()->staticInlinePosition() +
containerLogicalWidth + containerBlock->borderLogicalLeft();

I still don't understand why a left offset is being added in when the static
position is from the right side. I'm just not following this part. Can you
maybe make sure you have startOffsetForLine right and double-check? Or explain
this to me, because I don't get it. It's not clear to me why you add in a left
hand side coordinate to an offset from a right edge.