Bug 133225

Created attachment 251634 [details] Patch

Comment on attachment 251634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251634&action=review > Source/WebCore/ChangeLog:9 > + different wirting-modes and direction. Margins, borders and paddings are also wirting > Source/WebCore/rendering/RenderGrid.cpp:1289 > + // A margin basically has three types: fixed, percentage, and auto (variable). > + // Auto and percentage margins simply become 0 when computing min/max width. > + // Fixed margins can be added in as is. > + Length marginLeft = child.style().marginLeft(); > + Length marginRight = child.style().marginRight(); > + LayoutUnit margin = 0; > + if (marginLeft.isFixed()) > + margin += marginLeft.value(); > + if (marginRight.isFixed()) > + margin += marginRight.value(); > + return margin; How about writing this with helper functions? // A margin has three types: fixed, percentage, and auto (variable). // Auto and percentage margins become 0 when computing min/max width. // Fixed margins should be added in as is. static LayoutUnit fixedValue(const Length& length) { return length.isFixed() ? length.value() : 0; } static LayoutUnit marginWidthForChild(const RenderBox& child) { return fixedValues(child.style().marginLeft()) + fixedValue(child.style().marginRight()); } static LayoutUnit marginHeightForChild(const RenderBox& child) { return fixedValue(child.style().marginTop()) + fixedValue(child.style().marginBottom()); } I think this is easier to read than the longer functions with the local variables. Inline as needed. Name the helper differently if you like. > Source/WebCore/rendering/RenderGrid.cpp:1361 > + bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode(); > + switch (RenderStyle::resolveAlignment(style(), child.style(), ItemPositionStretch)) { Given this whole function ends up just deciding on start, end or centered, maybe we should write it that way with an enum rather than sprinkling in all the different function calls throughout. We should look into it again with an eye toward clarity when reading the function. > Source/WebCore/rendering/RenderGrid.cpp:1363 > + // If orthogonal writing-modes, this computes to 'start'. Generally speaking these should be: return <boolean expression about whether to use start or end> ? startOfRowForChild(child) : endOfRowForChild(child); Rather than this sequence of multiple return statements. > Source/WebCore/rendering/RenderGrid.cpp:1373 > + return startOfRowForChild(child); > + case ItemPositionSelfEnd: Paragraphing here is strange. If you are including blank lines within cases, then please leave a blank line before the next case too. > Source/WebCore/rendering/RenderGrid.cpp:1392 > + // The alignment axis (column axis) and the inline axis are parallell in > + // orthogonal writing mode. > + // FIXME: grid track sizing and positioning do not support orthogonal modes yet. > + if (hasOrthogonalWritingMode) > + return startOfRowForChild(child); > + > + // Otherwise this this is equivalent to 'start'. > + return startOfRowForChild(child); This seems to be written out too long. I think a ? : would be much better. Also, this is startOfRowForChild in both places. Is that because of the FIXME? Lets not write it this strange way. > Source/WebCore/rendering/RenderGrid.cpp:1401 > + // The alignment axis (column axis) and the inline axis are parallell in > + // orthogonal writing mode. > + // FIXME: grid track sizing and positioning do not support orthogonal modes yet. > + if (hasOrthogonalWritingMode) > + return endOfRowForChild(child); > + > + // Otherwise this this is equivalent to 'start'. > + return startOfRowForChild(child); This seems to be written out too long. I think a ? : would be much better. > Source/WebCore/rendering/RenderGrid.cpp:1406 > + // Only used in flex layout, for other layout, it's equivalent to 'start'. > + case ItemPositionFlexStart: > + case ItemPositionStart: Indentation of this comment is wrong. It should be moved left or put on the same line with FlexStart. Indented like this it seems to be part of the previous case. > Source/WebCore/rendering/RenderGrid.cpp:1410 > + // Only used in flex layout, for other layout, it's equivalent to 'end'. > + case ItemPositionFlexEnd: > + case ItemPositionEnd: Ditto. > Source/WebCore/rendering/RenderGrid.h:123 > + LayoutUnit startOfRowForChild(const RenderBox& child) const; > + LayoutUnit endOfRowForChild(const RenderBox& child) const; > + LayoutUnit centeredRowPositionForChild(const RenderBox&) const; > + LayoutUnit rowPositionForChild(const RenderBox&) const; Should either include or omit the argument name “child” consistently. > Source/WebCore/rendering/RenderGrid.h:133 > + bool allowedToStretchLogicalHeightForChild(const RenderBox& child) const; > + bool needToStretchChildLogicalHeight(const RenderBox&) const; > + LayoutUnit marginLogicalHeightForChild(const RenderBox&) const; > + LayoutUnit computeMarginLogicalHeightForChild(const RenderBox&) const; Should either include or omit the argument name “child” consistently.

Comment on attachment 251634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251634&action=review >> Source/WebCore/rendering/RenderGrid.cpp:1289 >> + return margin; > > How about writing this with helper functions? > > // A margin has three types: fixed, percentage, and auto (variable). > // Auto and percentage margins become 0 when computing min/max width. > // Fixed margins should be added in as is. > > static LayoutUnit fixedValue(const Length& length) > { > return length.isFixed() ? length.value() : 0; > } > > static LayoutUnit marginWidthForChild(const RenderBox& child) > { > return fixedValues(child.style().marginLeft()) + fixedValue(child.style().marginRight()); > } > > static LayoutUnit marginHeightForChild(const RenderBox& child) > { > return fixedValue(child.style().marginTop()) + fixedValue(child.style().marginBottom()); > } > > I think this is easier to read than the longer functions with the local variables. Inline as needed. Name the helper differently if you like. Yeah, that's definitively how I should have do it. However, I ended up removing that code because it seems that Flexbox stretching logic uses child.verticalMarginExtent() or child.horizontalMarginExtent() functions, which are precisely what I need to implement the Gird stretching logic too. >> Source/WebCore/rendering/RenderGrid.cpp:1361 >> + switch (RenderStyle::resolveAlignment(style(), child.style(), ItemPositionStretch)) { > > Given this whole function ends up just deciding on start, end or centered, maybe we should write it that way with an enum rather than sprinkling in all the different function calls throughout. We should look into it again with an eye toward clarity when reading the function. Even that it's not implemented yet, Baseline alignment would imply a different result. Anyway, do you mean translating ItemPosition to an internal enumeration with just START, END and CENTER values ? In that case, we would need a function to return the corresponding layout point. >> Source/WebCore/rendering/RenderGrid.cpp:1363 >> + // If orthogonal writing-modes, this computes to 'start'. > > Generally speaking these should be: > > return <boolean expression about whether to use start or end> ? startOfRowForChild(child) : endOfRowForChild(child); > > Rather than this sequence of multiple return statements. Agree. >> Source/WebCore/rendering/RenderGrid.cpp:1373 >> + case ItemPositionSelfEnd: > > Paragraphing here is strange. If you are including blank lines within cases, then please leave a blank line before the next case too. I'll avoid paragraphing. >> Source/WebCore/rendering/RenderGrid.cpp:1392 >> + return startOfRowForChild(child); > > This seems to be written out too long. I think a ? : would be much better. Also, this is startOfRowForChild in both places. Is that because of the FIXME? Lets not write it this strange way. Sure. >> Source/WebCore/rendering/RenderGrid.cpp:1401 >> + return startOfRowForChild(child); > > This seems to be written out too long. I think a ? : would be much better. Done.

Created attachment 251708 [details] Patch

Created attachment 251709 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 251709 [details]: media/track/track-active-cues.html bug 144236 (authors: annacc@chromium.org and eric.carlson@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 251709 [details] Patch Clearing flags on attachment: 251709 Committed r183370: <http://trac.webkit.org/changeset/183370>

All reviewed patches have been landed. Closing bug.