Bug 98803 – Refactor shouldAddBorderPaddingMargin()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Robert Hogan]

..

[Robert Hogan]

Comment on attachment 164760 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:289
> +static bool shouldAddBorderPaddingMargin(RenderObject* child, bool &checkSide)

This is incorrect formatting for the WebKit project. It should be bool& checkSide.

Also, I find the name and purpose of this argument to be quite confusing.

I suspect this function should be marked inline.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:291
> +    if (!child || (child->isText() && !toRenderText(child)->textLength()))

This needs a “why” comment. Why is this the correct rule? The change log says, but the code needs to say it too.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:294
> +    checkSide = false;
> +    return checkSide;

Why not just return false here?

This strange function that returns a boolean and also ands that same boolean with an argument isn}t really justified by the call site. This could just be a normal function and the rest of the logic could be done in a clearer way at the call site.

[Robert Hogan]

Created attachment 202318 [details]
Patch

[Robert Hogan]

Pinging Darin: is this more like it?

[Ryosuke Niwa]

Comment on attachment 202318 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:374
> +            if (checkStartEdge && parentContributesWidth) {

We don't need to check checkStartEdge here since we no longer update checkStartEdge in shouldAddBorderPaddingMargin.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:375
> +                checkStartEdge = parentContributesWidth;

You always set checkStartEdge to true here because parentContributesWidth is always true in this branch.
What you need to do instead is to add an else clause and set checkStartEdge to false there.
I'm surprised that none of our layout tests caught this. Maybe we need to a test for this?
r- because of this logic change.

[Robert Hogan]

Created attachment 202744 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 202744 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:374
> +            bool parentContributesWidth = checkStartEdge && shouldAddBorderPaddingMargin(previousInFlowSibling(child));
> +            if (parentContributesWidth)

Why do we need parentContributesWidth at all? Can't we just do:
checkStartEdge = checkStartEdge && shouldAddBorderPaddingMargin(previousInFlowSibling(child));
?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:378
> +            checkStartEdge = parentContributesWidth;
> +            parentContributesWidth = checkEndEdge && shouldAddBorderPaddingMargin(child->nextSibling());
> +            if (parentContributesWidth)

Ditto.

[Robert Hogan]

Created attachment 202749 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 202749 [details]
Patch

Clearing flags on attachment: 202749

Committed r150642: <http://trac.webkit.org/changeset/150642>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.