Bug 98803 - Refactor shouldAddBorderPaddingMargin()
Summary: Refactor shouldAddBorderPaddingMargin()
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Hogan
Depends on:
Reported: 2012-10-09 11:30 PDT by Robert Hogan
Modified: 2013-05-24 09:18 PDT (History)
5 users (show)

See Also:

Patch (3.54 KB, patch)
2013-05-20 14:09 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (3.46 KB, patch)
2013-05-23 15:37 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (3.31 KB, patch)
2013-05-23 15:59 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2012-10-09 11:30:52 PDT
Comment 1 Robert Hogan 2012-10-09 11:31:51 PDT
Comment on attachment 164760 [details]

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.
Comment 2 Robert Hogan 2013-05-20 14:09:28 PDT
Created attachment 202318 [details]
Comment 3 Robert Hogan 2013-05-20 14:37:57 PDT
Pinging Darin: is this more like it?
Comment 4 Ryosuke Niwa 2013-05-23 12:18:07 PDT
Comment on attachment 202318 [details]

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.
Comment 5 Robert Hogan 2013-05-23 15:37:17 PDT
Created attachment 202744 [details]
Comment 6 Ryosuke Niwa 2013-05-23 15:57:13 PDT
Comment on attachment 202744 [details]

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)

Comment 7 Robert Hogan 2013-05-23 15:59:31 PDT
Created attachment 202749 [details]
Comment 8 WebKit Commit Bot 2013-05-24 09:17:59 PDT
Comment on attachment 202749 [details]

Clearing flags on attachment: 202749

Committed r150642: <http://trac.webkit.org/changeset/150642>
Comment 9 WebKit Commit Bot 2013-05-24 09:18:02 PDT
All reviewed patches have been landed.  Closing bug.