Summary: | Refactor shouldAddBorderPaddingMargin() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robert Hogan <robert> | ||||||||
Component: | WebCore Misc. | Assignee: | Robert Hogan <robert> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, darin, esprehn+autocc, glenn, robert | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Robert Hogan
2012-10-09 11:30:52 PDT
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. Created attachment 202318 [details]
Patch
Pinging Darin: is this more like it? 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. Created attachment 202744 [details]
Patch
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. Created attachment 202749 [details]
Patch
Comment on attachment 202749 [details] Patch Clearing flags on attachment: 202749 Committed r150642: <http://trac.webkit.org/changeset/150642> All reviewed patches have been landed. Closing bug. |