Bug 144235

Created attachment 252049 [details] Patch

Comment on attachment 252049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252049&action=review Awesome fix+test. I've some comments though > Source/WebCore/rendering/RenderGrid.cpp:1241 > +static LayoutUnit computeOverflowAlignmentOffset(OverflowAlignment overflow, LayoutUnit startOfTrack, LayoutUnit endOfTrack, LayoutUnit childBreadth) You better pass directly the trackBreadth here as neither endOfTrack nor startOfTrack are used for anything else. > Source/WebCore/rendering/RenderGrid.cpp:1254 > + return offset; I prefer to have a switch here with all the potential values for OverflowAlignment as it's safer against future additions/removals in the enum. Something like switch(overflow) { case Safe: return std::max(...) case True: case Default: return trackBreadth - childBreath; } ASSERT_NOT_REACHED() return 0; > Source/WebCore/rendering/style/RenderStyle.cpp:-39 > -#include "StyleInheritedData.h" Why are you removing this? > LayoutTests/fast/css-grid-layout/grid-align-justify-overflow-expected.txt:10 > +PASS It's a pity that a gorgeous set of test cases as the ones you're adding have this miserable output :(. Anyway I guess we could eventually replace some of this checkLayout() tests. > LayoutTests/fast/css-grid-layout/grid-align-justify-overflow.html:120 > + Nit: perhaps we can write all these in a single line each so the test is more compact. .justifySelfEnd { justify-self: end; } etc....

Comment on attachment 252049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252049&action=review >> Source/WebCore/rendering/RenderGrid.cpp:1241 >> +static LayoutUnit computeOverflowAlignmentOffset(OverflowAlignment overflow, LayoutUnit startOfTrack, LayoutUnit endOfTrack, LayoutUnit childBreadth) > > You better pass directly the trackBreadth here as neither endOfTrack nor startOfTrack are used for anything else. the same would appliy to trackBreadh and childBreadth; the thing is that if I pass that, I end up computing the same in 2 functions computeOverflowAlignmentOffset is kind of helpers function to clearly describe how the overflow is handled such logic can be moved to both, row-axis and column-axis positioning logic but I thought haveing a single function with the shared code was clearer. If you don't mind, I'll keep the function as it is. >> Source/WebCore/rendering/RenderGrid.cpp:1254 >> + return offset; > > I prefer to have a switch here with all the potential values for OverflowAlignment as it's safer against future additions/removals in the enum. Something like > > switch(overflow) { > case Safe: > return std::max(...) > case True: > case Default: > return trackBreadth - childBreath; > } > ASSERT_NOT_REACHED() > return 0; Ok >> Source/WebCore/rendering/style/RenderStyle.cpp:-39 >> -#include "StyleInheritedData.h" > > Why are you removing this? Wow, good catch !!! it was a mistake. >> LayoutTests/fast/css-grid-layout/grid-align-justify-overflow-expected.txt:10 >> +PASS > > It's a pity that a gorgeous set of test cases as the ones you're adding have this miserable output :(. Anyway I guess we could eventually replace some of this checkLayout() tests. Agree. >> LayoutTests/fast/css-grid-layout/grid-align-justify-overflow.html:120 >> + > > Nit: perhaps we can write all these in a single line each so the test is more compact. > > .justifySelfEnd { justify-self: end; } > > etc.... ok.

Created attachment 252089 [details] Patch

Comment on attachment 252089 [details] Patch Clearing flags on attachment: 252089 Committed r183660: <http://trac.webkit.org/changeset/183660>

All reviewed patches have been landed. Closing bug.