Bug 145520

Created attachment 254093 [details] Patch

Attachment 254093 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/RenderGrid.cpp:417: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 254097 [details] Patch

Comment on attachment 254097 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254097&action=review > Source/WebCore/rendering/RenderGrid.cpp:132 > + ContentAlignmentData() > + : positionOffset(-1) > + , distributionOffset(-1) > + { > + } > + > + ContentAlignmentData(LayoutUnit position, LayoutUnit distribution) > + : positionOffset(position) > + , distributionOffset(distribution) > + { > + } We should omit these. > Source/WebCore/rendering/RenderGrid.cpp:135 > + LayoutUnit positionOffset; > + LayoutUnit distributionOffset; We should initialize these like this: LayoutUnit positionOffset { -1 }; LayoutUnit distributionOffset { -1 }; > Source/WebCore/rendering/RenderGrid.cpp:137 > + bool isValid() { return positionOffset > -1 && distributionOffset > -1; }; Should be const. Using -1 as a magic number to mean invalid is really ugly. Do we need to follow this pattern? > Source/WebCore/rendering/RenderGrid.cpp:1527 > + return ContentAlignmentData(); Should just be: return { }; > Source/WebCore/rendering/RenderGrid.cpp:1534 > + return ContentAlignmentData(0, availableFreeSpace / (numberOfGridTracks - 1)); Should just be: return { 0 , availableFreeSpace / (numberOfGridTracks - 1) }; > Source/WebCore/rendering/RenderGrid.cpp:1629 > + // crbug.com/234191 Hmm, lets not use a crbug URL in WebKit.

*** Bug 145521 has been marked as a duplicate of this bug. ***

Created attachment 254178 [details] Patch Applied suggested changes.

Comment on attachment 254178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254178&action=review > Source/WebCore/ChangeLog:16 > + This does not look correct. It isn't only for columns right? > Source/WebCore/rendering/RenderGrid.cpp:411 > + distributeSpaceToTracks<MaximizeTracks>(tracksForDistribution, needToStretch ? &tracksToStretch : nullptr, availableLogicalSpace); Hmm I don't understand this change. The second argument of distributeSpaceToTracks should be used just to specify the tracks that should grow over the limits, something that is clearly specified in the algorithm. You are using this for a completely different thing. Also it was nullptr on purpose because on the step 3 of the algorithm we should just grow tracks to their maximums, we should not grow over that limit at this point. > Source/WebCore/rendering/RenderGrid.cpp:1272 > + m_columnPositions.resize(numberOfColumnTracks + 1); Why resize() instead of resizeToFit() ? > Source/WebCore/rendering/RenderGrid.cpp:1279 > + m_rowPositions.resize(numberOfRowTracks + 1); Ditto. > Source/WebCore/rendering/RenderGrid.cpp:1525 > + return nullptr; Perhaps move this check above the previous one. > Source/WebCore/rendering/RenderGrid.cpp:1557 > + ContentAlignmentData* contentAlignment = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfGridTracks); You're leaking the returned object as it was created with new. > Source/WebCore/rendering/RenderGrid.cpp:1598 > + ContentAlignmentData* contentAlignment = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfGridTracks); You're leaking the returned object as it was created with new. > Source/WebCore/rendering/RenderGrid.h:43 > +struct ContentAlignmentData; Seems like this is not needed here?

Comment on attachment 254178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254178&action=review >> Source/WebCore/ChangeLog:16 >> + > > This does not look correct. It isn't only for columns right? Sure, it was like that in the first versions of the patch, but I ended up implementing logic for both properties. I'll rephrase it accordingly. >> Source/WebCore/rendering/RenderGrid.cpp:411 >> + distributeSpaceToTracks<MaximizeTracks>(tracksForDistribution, needToStretch ? &tracksToStretch : nullptr, availableLogicalSpace); > > Hmm I don't understand this change. The second argument of distributeSpaceToTracks should be used just to specify the tracks that should grow over the limits, something that is clearly specified in the algorithm. You are using this for a completely different thing. > > Also it was nullptr on purpose because on the step 3 of the algorithm we should just grow tracks to their maximums, we should not grow over that limit at this point. umm, it's really a good catch, indeed. I must look for an alternative approach to implement stretch which should happen after the track sizing algorithm is executed. >> Source/WebCore/rendering/RenderGrid.cpp:1272 >> + m_columnPositions.resize(numberOfColumnTracks + 1); > > Why resize() instead of resizeToFit() ? It was not my intention to change that; I just wanted to use numberOfColumnTracks so I'll keep the original function. >> Source/WebCore/rendering/RenderGrid.cpp:1525 >> + return nullptr; > > Perhaps move this check above the previous one. We can t do that because we need to resolve the fallback position before returning. >> Source/WebCore/rendering/RenderGrid.cpp:1557 >> + ContentAlignmentData* contentAlignment = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfGridTracks); > > You're leaking the returned object as it was created with new. Sure, I'll fix that. >> Source/WebCore/rendering/RenderGrid.cpp:1598 >> + ContentAlignmentData* contentAlignment = contentDistributionOffset(availableFreeSpace, position, distribution, numberOfGridTracks); > > You're leaking the returned object as it was created with new. I need to fix this too, indeed. >> Source/WebCore/rendering/RenderGrid.h:43 >> +struct ContentAlignmentData; > > Seems like this is not needed here? Yes it is, some header functions return that type.

Comment on attachment 254178 [details] Patch Let's not land this

Created attachment 254265 [details] Patch Fixed commented issues and changed the approach to deal with stretch and spans.

Comment on attachment 254265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254265&action=review Looks good in general. Some nits here and there. I'm setting r- because the cache thing is still not completely well resolved. We also need the CSSWG clarifications for some of the stuff you're adding before landing. > Source/WebCore/rendering/RenderGrid.cpp:371 > + sizingData.invalidateTrackPositionsCache(direction); Wouldn't it make sense just to explicitly initialize the invalid flag in GridSizingData ? > Source/WebCore/rendering/RenderGrid.cpp:1189 > + // We consider auto-sized tracks as content-sized (min-content, max-content, auto). We need a clarification in the specs for this. > Source/WebCore/rendering/RenderGrid.cpp:1191 > + unsigned numberAutoSizedTracks = autoSizedTracksIndex.size(); You're copying a Vector here just to get the size, used sizingData.contentSizedTracksIndex.size() directly instead > Source/WebCore/rendering/RenderGrid.cpp:1195 > + return; Actually all these checks doesn't require checking the size. I'd modify the order of the checks and put sizingData.contentSizedTracksIndex.size < 1, at the very end so that we don't normally need to even check it. > Source/WebCore/rendering/RenderGrid.cpp:1204 > + // FIXME: Respecting the constraints imposed by max-height/max-width. I'll wait for the CSSWG reply before giving the r+ as it wouldn't make sense to approve this to change it soon. > Source/WebCore/rendering/RenderGrid.cpp:1293 > + if (!trackPositionsCacheIsValid) This does not make sense, you have an ASSERT(trackPositionsCacheIsValid) just two lines above, so it's either something that is always true (and thus the ASSERT) or an allowed value (and thus the "if" check) > Source/WebCore/rendering/RenderGrid.cpp:1325 > + sizingData.rowTracksPositionsCacheValid = true; Do we ever modify the m_rowPositions without modifying m_columnPositions or viceversa? If so we just need 1 boolean instead of two. Also it's a bit weird to have a function to invalidate the cache and at the same time validate it directly by setting the flags to true. So I guess we better have a setWhateverCacheValid(bool) instead.

Comment on attachment 254265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254265&action=review >> Source/WebCore/rendering/RenderGrid.cpp:371 >> + sizingData.invalidateTrackPositionsCache(direction); > > Wouldn't it make sense just to explicitly initialize the invalid flag in GridSizingData ? I'm not sure. I think this method should explicitly invalidate the cache because it builds again the track position vectors. It's true that in the current code we always pass a newly created GridSizingData instance, but I think we shouldn't trust on that., make the logic as independent as possible in order to be safe against future code refactoring. >> Source/WebCore/rendering/RenderGrid.cpp:1189 >> + // We consider auto-sized tracks as content-sized (min-content, max-content, auto). > > We need a clarification in the specs for this. Agreed. >> Source/WebCore/rendering/RenderGrid.cpp:1191 >> + unsigned numberAutoSizedTracks = autoSizedTracksIndex.size(); > > You're copying a Vector here just to get the size, used sizingData.contentSizedTracksIndex.size() directly instead Well, it's used as well in the loop below. This way we only copy it once. >> Source/WebCore/rendering/RenderGrid.cpp:1195 >> + return; > > Actually all these checks doesn't require checking the size. I'd modify the order of the checks and put sizingData.contentSizedTracksIndex.size < 1, at the very end so that we don't normally need to even check it. Good idea. >> Source/WebCore/rendering/RenderGrid.cpp:1204 >> + // FIXME: Respecting the constraints imposed by max-height/max-width. > > I'll wait for the CSSWG reply before giving the r+ as it wouldn't make sense to approve this to change it soon. Agreed. >> Source/WebCore/rendering/RenderGrid.cpp:1293 >> + if (!trackPositionsCacheIsValid) > > This does not make sense, you have an ASSERT(trackPositionsCacheIsValid) just two lines above, so it's either something that is always true (and thus the ASSERT) or an allowed value (and thus the "if" check) I tried to implement a safe path for release, but yeah, not a lot of sense, indeed. >> Source/WebCore/rendering/RenderGrid.cpp:1325 >> + sizingData.rowTracksPositionsCacheValid = true; > > Do we ever modify the m_rowPositions without modifying m_columnPositions or viceversa? If so we just need 1 boolean instead of two. > > Also it's a bit weird to have a function to invalidate the cache and at the same time validate it directly by setting the flags to true. So I guess we better have a setWhateverCacheValid(bool) instead. Again, I'd rather keep logic independence for this cache thing. We may want to modify either row or column tracks only so I think it's better to maintain separated caches for them.

Created attachment 262053 [details] New patch using now the min/maxTrackBreadth auto values logic, which landed recently.

Comment on attachment 262053 [details] New patch using now the min/maxTrackBreadth auto values logic, which landed recently. Attachment 262053 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/222998 New failing tests: fast/repaint/justify-self-overflow-change.html fast/repaint/justify-items-overflow-change.html

Created attachment 262054 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5

Comment on attachment 262053 [details] New patch using now the min/maxTrackBreadth auto values logic, which landed recently. Attachment 262053 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/222994 New failing tests: fast/repaint/justify-self-overflow-change.html fast/repaint/justify-items-overflow-change.html

Created attachment 262055 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Created attachment 262062 [details] Fixed layout tests failures.

Comment on attachment 262062 [details] Fixed layout tests failures. View in context: https://bugs.webkit.org/attachment.cgi?id=262062&action=review > Source/WebCore/rendering/RenderGrid.cpp:127 > + ContentAlignmentData() { }; > + ContentAlignmentData(LayoutUnit position, LayoutUnit distribution) > + : positionOffset(position) > + , distributionOffset(distribution) > + { > + } These constructors aren’t needed. We can already construct this object with default values, or with explicit values in aggregate syntax. Don’t need constructors. > Source/WebCore/rendering/RenderGrid.cpp:1257 > + GridTrack* track = tracks.data() + trackIndex; I’d write: GridTrack& track = tracks[trackIndex]; > Source/WebCore/rendering/RenderGrid.cpp:1359 > + const Vector<GridTrack>& tracks = (direction == ForColumns) ? sizingData.columnTracks : sizingData.rowTracks; > + const GridCoordinate& coordinate = cachedGridCoordinate(child); > + const GridSpan& span = (direction == ForColumns) ? coordinate.columns : coordinate.rows; > + const Vector<LayoutUnit>& linePositions = (direction == ForColumns) ? m_columnPositions : m_rowPositions; I’d use more auto here. The types aren’t so important. > Source/WebCore/rendering/RenderGrid.cpp:1683 > +{ > + return (distribution == ContentDistributionStretch || ContentDistributionStretch == ContentDistributionDefault) ? LayoutUnit(0) : trackPositions[1] - trackPositions[0] - childBreadth; > + > +} Stray blank line here.

Created attachment 262158 [details] Applied suggested changes.

Created attachment 262159 [details] Patch Patch rebased and applied suggested changes.

Created attachment 262244 [details] Patch Fixed compilation errors.

Comment on attachment 262244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262244&action=review It's a r+ for me too but please correct the few minor things I spotted. > Source/WebCore/rendering/RenderGrid.cpp:1343 > + // Track Positions vector stores the 'start' grid line of each track, so w have to add last track's baseSize. Nit: "so we". Also perhaps add some empty lines before and after defining the xxxTrackPosition just to make this block more readable. > Source/WebCore/rendering/RenderGrid.cpp:1662 > + return (distribution == ContentDistributionStretch || ContentDistributionStretch == ContentDistributionDefault) ? LayoutUnit(0) : trackPositions[1] - trackPositions[0] - childBreadth; Nit: just use LayoutUnit() which is a bit faster than LayoutUnit(0) > Source/WebCore/rendering/RenderGrid.cpp:1668 > + size_t childStartLine = coordinate.rows.resolvedInitialPosition.toInt(); In Webkit we switched from size_t to unsigned so better use unsigned. > Source/WebCore/rendering/RenderGrid.cpp:1679 > + size_t childEndLine = coordinate.rows.resolvedFinalPosition.next().toInt(); Ditto unsigned. > Source/WebCore/rendering/RenderGrid.cpp:1697 > + size_t childStartLine = coordinate.columns.resolvedInitialPosition.toInt(); Ditto. > Source/WebCore/rendering/RenderGrid.cpp:1708 > + size_t childEndLine = coordinate.columns.resolvedFinalPosition.next().toInt(); Ditto.

Created attachment 262326 [details] Patch Applied additional suggested changes.

Comment on attachment 262326 [details] Patch Clearing flags on attachment: 262326 Committed r190484: <http://trac.webkit.org/changeset/190484>

All reviewed patches have been landed. Closing bug.