Summary: | [css-grid] Fix grid-template-columns|rows computed style with content alignment | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Manuel Rego Casasnovas <rego> | ||||||||
Component: | Layout and Rendering | Assignee: | Manuel Rego Casasnovas <rego> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, jfernandez, rego, simon.fraser, svillar | ||||||||
Priority: | P2 | Keywords: | BlinkMergeCandidate | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 60731 | ||||||||||
Attachments: |
|
Description
Manuel Rego Casasnovas
2016-04-20 04:48:55 PDT
Created attachment 276818 [details]
Patch
Created attachment 276903 [details]
Patch
Comment on attachment 276903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276903&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1073 > + const auto& grid = downcast<RenderGrid>(*renderer); I don’t think the const here is helpful. Just auto& would be fine. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1074 > + const Vector<LayoutUnit>& trackPositions = direction == ForColumns ? grid.columnPositions() : grid.rowPositions(); I think this would be better if it said auto& instead of writing out the type. > Source/WebCore/rendering/RenderGrid.h:68 > + LayoutUnit offsetBetweenTracks(GridTrackSizingDirection direction) const > + { > + return direction == ForColumns ? m_offsetBetweenColumns : m_offsetBetweenRows; > + } Normally it’s easier to read class definitions if they don’t have multi-line function definitions inside them. Would you consider putting the inline function definition separately after the class definition and just declaring the function here? Created attachment 277235 [details]
Patch for landing
Thanks for the review. (In reply to comment #3) > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1073 > > + const auto& grid = downcast<RenderGrid>(*renderer); > > I don’t think the const here is helpful. Just auto& would be fine. Done. > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1074 > > + const Vector<LayoutUnit>& trackPositions = direction == ForColumns ? grid.columnPositions() : grid.rowPositions(); > > I think this would be better if it said auto& instead of writing out the > type. Done. > > Source/WebCore/rendering/RenderGrid.h:68 > > + LayoutUnit offsetBetweenTracks(GridTrackSizingDirection direction) const > > + { > > + return direction == ForColumns ? m_offsetBetweenColumns : m_offsetBetweenRows; > > + } > > Normally it’s easier to read class definitions if they don’t have multi-line > function definitions inside them. Would you consider putting the inline > function definition separately after the class definition and just declaring > the function here? Moved the implementation to the .cpp. Comment on attachment 277235 [details] Patch for landing Clearing flags on attachment: 277235 Committed r199981: <http://trac.webkit.org/changeset/199981> All reviewed patches have been landed. Closing bug. |