| Summary: | [CSS Grid Layout] Fix positioning grid items using named grid lines/areas | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||||
| Component: | CSS | Assignee: | Sergio Villar Senin <svillar> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | allan.jensen, bfulgham, commit-queue, darin, esprehn+autocc, glenn, gyuyoung.kim, hyatt, jfernandez, kling, koivisto, kondapallykalyan, macpherson, menard, mihnea, rego, simon.fraser, svillar | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
Created attachment 225376 [details]
Patch
Comment on attachment 225376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225376&action=review review- because of the GridPosition object storage management issues > Source/WebCore/css/StyleResolver.cpp:1402 > + default: > + ASSERT_NOT_REACHED(); > + return false; It’s much better to remove the default case and put the ASSERT_NOT_REACHED after the switch statement. That way some compilers (clang and gcc at least) can warn fi we add a new enum value and forgot to include it in the switch statement. > Source/WebCore/css/StyleResolver.cpp:1406 > +GridPosition* StyleResolver::adjustNamedGridItemPosition(const NamedGridAreaMap& gridAreaMap, const NamedGridLinesMap& namedLinesMap, const GridPosition& position, GridPositionSide side) const This return value should be std::unique_ptr, not a raw pointer. Or maybe we should return a GridPosition rather than a pointer to one. > Source/WebCore/css/StyleResolver.cpp:1411 > + GridPosition* adjustedPosition = nullptr; This local variable should be std::unique_ptr, not a raw pointer. > Source/WebCore/css/StyleResolver.cpp:1419 > + String namedGridAreaOrGridLine = position.namedGridLine(); > + const bool hasStartSuffix = namedGridAreaOrGridLine.endsWith("-start"); > + const bool hasEndSuffix = namedGridAreaOrGridLine.endsWith("-end"); > + const bool isStartSide = side == ColumnStartSide || side == RowStartSide; > + const bool hasStartSuffixForStartSide = hasStartSuffix && isStartSide; > + const bool hasEndSuffixForEndSide = hasEndSuffix && !isStartSide; > + const size_t suffixLength = hasStartSuffix ? strlen("-start") : strlen("-end"); I’m not sure I understand the use of const here. Why mark all the bools and the size_t const, but not the String? I suggest omitting all of these const. > Source/WebCore/css/StyleResolver.cpp:1433 > + adjustedPosition = new GridPosition(); This should be using std::make_unique. Also, I suggest allocating the GridPosition object outside the if statements, since we always allocate one in every code path. > Source/WebCore/css/StyleResolver.cpp:1485 > + GridPosition* adjustedPosition = adjustNamedGridItemPosition(gridAreaMap, namedGridLines, position, side); \ > + if (adjustedPosition) \ > + style.setGridItem##Prop(*adjustedPosition); \ It looks to me like there is a storage leak here. adjustNamedGridItemPosition returns a newly allocated grid position, and we never deallocate it. (In reply to comment #2) > (From update of attachment 225376 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225376&action=review > > review- because of the GridPosition object storage management issues Yeah I had that patch with OwnPtr/PassOwnPtr stuff but don't know how I uploaded the one with raw pointers. Anyway I would update it to a more C++11 version. (In reply to comment #2) > (From update of attachment 225376 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225376&action=review > > Source/WebCore/css/StyleResolver.cpp:1406 > > +GridPosition* StyleResolver::adjustNamedGridItemPosition(const NamedGridAreaMap& gridAreaMap, const NamedGridLinesMap& namedLinesMap, const GridPosition& position, GridPositionSide side) const > > This return value should be std::unique_ptr, not a raw pointer. Or maybe we should return a GridPosition rather than a pointer to one. The reason why I'm returning a pointer is because sometimes I want to return the nullptr, which means that the position shouldn't be adjusted as it's correct as it is. Comment on attachment 225376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225376&action=review >>> Source/WebCore/css/StyleResolver.cpp:1406 >>> +GridPosition* StyleResolver::adjustNamedGridItemPosition(const NamedGridAreaMap& gridAreaMap, const NamedGridLinesMap& namedLinesMap, const GridPosition& position, GridPositionSide side) const >> >> This return value should be std::unique_ptr, not a raw pointer. Or maybe we should return a GridPosition rather than a pointer to one. > > The reason why I'm returning a pointer is because sometimes I want to return the nullptr, which means that the position shouldn't be adjusted as it's correct as it is. That’s what I thought when I originally saw the function, but I looked carefully at the implementation you posted in this patch, and it never returns a nullptr. Comment on attachment 225376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225376&action=review > Source/WebCore/css/StyleResolver.cpp:1441 > + return adjustedPosition; So this is the point were we precisely return nullptr. If none of the conditions above is true (neither the if nor the else) then adjustedPosition is left unchanged. Created attachment 225466 [details]
Patch
Comment on attachment 225376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225376&action=review >> Source/WebCore/css/StyleResolver.cpp:1441 >> + return adjustedPosition; > > So this is the point were we precisely return nullptr. If none of the conditions above is true (neither the if nor the else) then adjustedPosition is left unchanged. I see. I misread the code above as "else" rather than else if. This code would be much clearer without the adjustedPosition local variable. All these various branches should end in return statements for clarity. Then this code would say return nullptr. Comment on attachment 225466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225466&action=review > Source/WebCore/css/StyleResolver.cpp:1411 > + std::unique_ptr<GridPosition> adjustedPosition; Code would read clearer if the unique_ptr for position was local to each clause and return statement. Having a shared one declared up here makes the code hard to read for no good reason. > Source/WebCore/css/StyleResolver.cpp:1430 > + StringBuilder gridLineNameBuilder; > + gridLineNameBuilder.append(namedGridAreaOrGridLine); > + if (isStartSide && !hasStartSuffix) > + gridLineNameBuilder.append("-start"); > + else if (!isStartSide && !hasEndSuffix) > + gridLineNameBuilder.append("-end"); > + > + String gridLineName = gridLineNameBuilder.toString(); A StringBuilder is not helpful for a single append that we then convert back to a string. This code would be more direct and also more efficient: String gridLineName; if (isStartSide && !hasStartSuffix) gridLineName = namedGridAreaOrGridLine + "-start"; else if (!isStartSide && !hasEndSuffix) gridLineName = namedGridAreaOrGridLine + "-end"; else gridLineName = namedGridAreaOrGridLine; And this idiom would be efficient if we added an overload of String::append for const char*; but without one it would do an extra string allocation (sadly): String gridLineName = namedGridAreaOrGridLine; if (isStartSide && !hasStartSuffix) gridLineName.append("-start"); else if (!isStartSide && !hasEndSuffix) gridLineName.append("-end"); If we did stick with StringBuilder I would suggest appendLiteral instead of append. > Source/WebCore/css/StyleResolver.cpp:1434 > + adjustedPosition = std::make_unique<GridPosition>(); > + adjustedPosition->setExplicitPosition(1, gridLineName); So here it would say: auto adjustedPosition = std::make_unique<GridPosition>(); adjustedPosition->setExplicitPosition(1, gridLineName); return adjustedPosition; > Source/WebCore/css/StyleResolver.cpp:1438 > + adjustedPosition = std::make_unique<GridPosition>(); > + adjustedPosition->setNamedGridArea(gridAreaName); Here it would say: auto adjustedPosition = std::make_unique<GridPosition>(); adjustedPosition->setNamedGridArea(gridAreaName); return adjustedPosition; > Source/WebCore/css/StyleResolver.cpp:1441 > + return adjustedPosition; Here it would say: return nullptr; > Source/WebCore/css/StyleResolver.cpp:1447 > + adjustedPosition = std::make_unique<GridPosition>(); > + adjustedPosition->setExplicitPosition(1, namedGridAreaOrGridLine); > + return adjustedPosition; Here it would say: auto adjustedPosition = std::make_unique<GridPosition>(); adjustedPosition->setExplicitPosition(1, gridLineName); return adjustedPosition; Committed r164869: <http://trac.webkit.org/changeset/164869> (In reply to comment #9) > (From update of attachment 225466 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225466&action=review > > > Source/WebCore/css/StyleResolver.cpp:1411 > > + std::unique_ptr<GridPosition> adjustedPosition; > > Code would read clearer if the unique_ptr for position was local to each clause and return statement. Having a shared one declared up here makes the code hard to read for no good reason. > Yeah very nice suggestion. I finally applied it and converted the if-else in two ifs as they're both returning. |
Currently our code assumes that a <custom-ident> in -webkit-grid-{column|row}-{start|end} and -webkit-grid-{column|row} is always a grid area. That's wrong because the <custom-ident> could be also a explicitly named grid line. Our resolution code is not correct either. The specs mandates us to: * first we try to match any existing grid area. * if there is a named grid line with the name <custom-ident>-{start|end} for -webkit-grid-{column|row}-{start|end} defined before the grid area then we use it instead of the grid area. * otherwise if there is a named grid line we resolve to the first such line. * otherwise we treat it as 'auto'.