Bug 108168

Created attachment 186064 [details] WIP

The patch needs work like: - see how I can minimize the use of extra bitfields - see how it interacts with float clearance - solve block directionality for SEPARATE as well

Attachment 186064 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/block/margin-collapse/webkit-margin-collapse-siblings.html', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlock.h', u'Source/WebCore/rendering/style/RenderStyle.h']" exit_code: 1 Source/WebCore/rendering/RenderBlock.cpp:2007: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/rendering/RenderBlock.cpp:2028: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/rendering/RenderBlock.cpp:2052: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/rendering/RenderBlock.cpp:2054: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderBlock.cpp:6897: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/rendering/RenderBlock.cpp:6907: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 6 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Also, I need to see how to reset the margin computed values when discarding. I guess I need to set them to 0 somehow.

Comment on attachment 186064 [details] WIP Attachment 186064 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16295867 New failing tests: fast/block/margin-collapse/webkit-margin-collapse-siblings.html

Comment on attachment 186064 [details] WIP Attachment 186064 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16306948 New failing tests: fast/block/margin-collapse/webkit-margin-collapse-siblings.html

Created attachment 186401 [details] WIP v2

Comment on attachment 186401 [details] WIP v2 Attachment 186401 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16374311 New failing tests: fast/regions/text-region-split-vertical-rl.html fast/regions/autoheight-allregions.html fast/regions/text-region-split-after-resize.html fast/regions/autoheight-secondregion-breakoutside.html printing/css2.1/page-break-after-003.html fast/regions/autoheight-middleregion.html

(In reply to comment #8) > (From update of attachment 186401 [details]) > Attachment 186401 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/16374311 > > New failing tests: > fast/regions/text-region-split-vertical-rl.html > fast/regions/autoheight-allregions.html > fast/regions/text-region-split-after-resize.html > fast/regions/autoheight-secondregion-breakoutside.html > printing/css2.1/page-break-after-003.html > fast/regions/autoheight-middleregion.html This is because forced break after actually works now :).

Comment on attachment 186401 [details] WIP v2 View in context: https://bugs.webkit.org/attachment.cgi?id=186401&action=review r- > Source/WebCore/rendering/RenderBlock.cpp:1966 > + childDiscardMarginBefore = childDiscardMarginBefore || (childDiscardMarginAfter && child->isSelfCollapsingBlock()); I would store child->isSelfCollapsingBlock() in a local so that you don't have to call it multiple times. > Source/WebCore/rendering/RenderBlock.cpp:2051 > + // FIXME: Take writing direction into account for SEPARATE. > if (child->style()->marginBeforeCollapse() == MSEPARATE) { Good observation. Go ahead and fix it! Just need helper methods.... mustSeparateMarginBeforeForChild... etc. > Source/WebCore/rendering/RenderBlock.cpp:-2423 > - // Do not allow a collapse if the margin-before-collapse style is set to SEPARATE. > - RenderStyle* childStyle = child->style(); > - if (childStyle->marginBeforeCollapse() == MSEPARATE) { > - marginInfo.setAtBeforeSideOfBlock(false); > - marginInfo.clearMargin(); > - } Where did this code go? > Source/WebCore/rendering/RenderBlock.h:474 > + // FIXME: This function doesn't get called for table cells. Does it matter? It doesn't matter.

Created attachment 186860 [details] Patch

(In reply to comment #11) > Created an attachment (id=186860) [details] > Patch This patch doesn't include the fix for the after forced break. This one will be submitted for https://bugs.webkit.org/show_bug.cgi?id=104944 .

Created attachment 186894 [details] Patch v2 I forgot some code commented out after an experiment. Fixed now.

Created attachment 186898 [details] Patch v3 Ooops... forgot a parameter.

Comment on attachment 186898 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=186898&action=review r=me > Source/WebCore/rendering/RenderBlock.cpp:6921 > +bool RenderBlock::mustDiscardMarginBeforeForChild(const RenderBox* child) const You should probably assert that the child does not need a layout here, since you are asking for a RenderBlock bit that is set by the layout process. Same for all four of these methods. > Source/WebCore/rendering/RenderBlock.cpp:6927 > + if (child->isHorizontalWritingMode() == isHorizontalWritingMode()) > + return child->isRenderBlock() ? toRenderBlock(child)->mustDiscardMarginAfter() : (child->style()->marginAfterCollapse() == MDISCARD); > + return false; This warrants commenting I think. It's pretty subtle that you just return false for orthogonal blocks. Really the only reason we do so is because the properties are only specified for "before" and "after". If we did implement physical properties correctly (and this is something I could see us doing), then you would need to check the correct physical side. For now this is correct, though, but I think it warrants a comment.

Comment on attachment 186898 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=186898&action=review >> Source/WebCore/rendering/RenderBlock.cpp:6921 >> +bool RenderBlock::mustDiscardMarginBeforeForChild(const RenderBox* child) const > > You should probably assert that the child does not need a layout here, since you are asking for a RenderBlock bit that is set by the layout process. Same for all four of these methods. I use this method to estimate the child top position pre-layout. See estimateLogicalTopPosition where I've removed the fix-me stating we should also take into account -webkit-margin-collapse. Not sure such an assert could work if I use these methods there.

Comment on attachment 186898 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=186898&action=review > Source/WebCore/rendering/RenderBlock.cpp:6923 > + if (!child->isWritingModeRoot()) You should assert that the "child" RenderBox is actually a child of "this".

Created attachment 187101 [details] Patch v4

(In reply to comment #17) > (From update of attachment 186898 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186898&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:6923 > > + if (!child->isWritingModeRoot()) > > You should assert that the "child" RenderBox is actually a child of "this". I don't think an assertion is necessary. There are many "forChild" functions in WebKit's layout that don't have the assertion. In my opinion it would generate a lot of noise that's unnecessary because if such a mistake occurs it's easily spotted in a code review.

(In reply to comment #18) > Created an attachment (id=187101) [details] > Patch v4 I've changed a bit how margin estimation is made before the layout for a child. I've stopped using the forChild methods in marginBeforeEstimateForChild() because they were relying on the rare data bits which are not cleared at that point. I've added assertions only for selfNeedsLayout() because I need to use the flag value in estimateLogicalTopPosition().

Comment on attachment 187101 [details] Patch v4 r=me

Comment on attachment 187101 [details] Patch v4 Clearing flags on attachment: 187101 Committed r142974: <http://trac.webkit.org/changeset/142974>

All reviewed patches have been landed. Closing bug.