Bug 68761 – Refactor computeInlineStaticDistance()

• Component: New Bugs
• Status: RESOLVED INVALID

[Robert Hogan]

Refactor computeInlineStaticDistance()

[Robert Hogan]

Created attachment 108586 [details]
Patch

[Robert Hogan]

Created attachment 108605 [details]
Patch

[Darin Adler]

Comment on attachment 108605 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2222
> +    // See bug 50576. If the immediate parent in an RTL is a RenderInline
> +    // we need to use the width of the containing block.

Comments need to say “why” things are true, not just repeat in words what the code says. This comment should be replaced by a “why” comment.

I’m also not at all fond of comments that point to a bug report.

[Robert Hogan]

Created attachment 108702 [details]
Patch

[Daniel Bates]

Comment on attachment 108702 [details]
Patch

I'm r-ing this patch because the function computeInlineStaticDistance() in RenderBox.cpp has significantly changed since this patch was proposed over 6 months ago. If this patch is still applicable then it will need to be rebased.

[Julien Chaffraix]

Comment on attachment 108702 [details]
Patch

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

More comments.

> Source/WebCore/ChangeLog:17
> +        (WebCore::RenderBox::computePositionedLogicalWidthReplaced): Ditto.

Typo parameter.

> Source/WebCore/rendering/RenderBox.cpp:2225
> +    if (container()->isRenderInline())
> +        return containingBlock()->width() - logicalRight();
> +    return toRenderBox(container())->width() - logicalRight();

Couldn't this just be a toRenderBoxModelObject(container())->width() - logicalRight() ? (btw container and containingBlock are not interchangeable, there is a very small differences)

> Source/WebCore/rendering/RenderBox.h:416
> +    LayoutUnit startOffset();

Shouldn't it be const?

> Source/WebCore/rendering/RenderBoxModelObject.h:98
> +    LayoutUnit borderStart(RenderStyle* style) const { return style->isHorizontalWritingMode() ? (style->direction() == LTR) ? borderLeft() : borderRight() : (style->direction() == LTR) ? borderTop() : borderBottom(); }

Could this be re-written to be less confusing?