Bug 130010

Created attachment 227742 [details] Patch

Comment on attachment 227742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227742&action=review > Source/WebCore/rendering/RenderGrid.cpp:871 > + size_t namedGridLineFirstDefinition = GridPosition::adjustGridPositionForSide(namedLinesMap.get(gridLineName)[0], side); Doing a get() here will result in copying what we get from the map. We should find a way to write this where we extract the item without copying it out, using find rather than get. The code will be a bit longer, but likely more efficient. > Source/WebCore/rendering/RenderGrid.cpp:907 > + if (gridAreaMap.contains(gridAreaName)) { What guarantees that gridAreaName is not an empty string? Double hash table lookups here are unfortunate. Here we are calling contains and the function inside will be calling get. > Source/WebCore/rendering/RenderGrid.cpp:916 > + if (namedLinesMap.contains(gridLineName) && gridLineDefinedBeforeGridArea(gridLineName, gridAreaName, gridAreaMap, namedLinesMap, side)) { What guarantees that gridLineName is not an empty string? Double hash table lookups here are unfortunate. Here we are calling contains and the function inside will be calling get.Double hash table lookups here are unfortunate. Here we are calling contains and the function inside will be calling get. > Source/WebCore/rendering/style/GridPosition.h:80 > + static bool isColumnSide(GridPositionSide side) > + { > + return side == ColumnStartSide || side == ColumnEndSide; > + } I don’t see any reason for this to be a member function. It should just be a free namespace-level function since GridPositionSide is a namespace-level enum. > LayoutTests/fast/css-grid-layout/grid-item-area-get-set.html:109 > + testGridAreaCSSParsing("fourValueMixedGridArea", "firstArea", "span 1", "nonExistent", "1 foobar"); Strange to capitalize the letter “e” here. The word nonexistent doesn’t have a capital letter in it.

Comment on attachment 227742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227742&action=review Thanks for the review. >> Source/WebCore/rendering/RenderGrid.cpp:871 >> + size_t namedGridLineFirstDefinition = GridPosition::adjustGridPositionForSide(namedLinesMap.get(gridLineName)[0], side); > > Doing a get() here will result in copying what we get from the map. We should find a way to write this where we extract the item without copying it out, using find rather than get. The code will be a bit longer, but likely more efficient. Good point, will change it. >> Source/WebCore/rendering/RenderGrid.cpp:907 >> + if (gridAreaMap.contains(gridAreaName)) { > > What guarantees that gridAreaName is not an empty string? > > Double hash table lookups here are unfortunate. Here we are calling contains and the function inside will be calling get. This is an interesting comment, I hadn't thought about the possibility of authors using just -start or -end. Will add checks for that, in those cases we don't need to check for the existence of the named area, and we could consider that it's just the name of a grid line. Regarding the double lookups will replace them with finds. >> Source/WebCore/rendering/style/GridPosition.h:80 >> + } > > I don’t see any reason for this to be a member function. It should just be a free namespace-level function since GridPositionSide is a namespace-level enum. Fair enough. >> LayoutTests/fast/css-grid-layout/grid-item-area-get-set.html:109 >> + testGridAreaCSSParsing("fourValueMixedGridArea", "firstArea", "span 1", "nonExistent", "1 foobar"); > > Strange to capitalize the letter “e” here. The word nonexistent doesn’t have a capital letter in it. Well I am not really capitalizing anything, it's just the name of a grid line already present in the test, but I can change it as well.

Created attachment 227848 [details] Patch

Comment on attachment 227848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227848&action=review > Source/WebCore/rendering/RenderGrid.cpp:890 > + const auto& linesIterator = namedLinesMap.find(lineName); Should just be auto, rather than const auto&. > Source/WebCore/rendering/RenderGrid.cpp:910 > + const auto& areaIterator = gridAreaMap.find(namedGridAreaOrGridLine); Should just be auto rather than const auto&. > Source/WebCore/rendering/RenderGrid.cpp:921 > + String gridAreaName = namedGridAreaOrGridLine.substring(0, namedGridAreaOrGridLine.length() - suffixLength); I know I asked this last time, but what guarantees this is not an empty string? If namedGridAreaOrGridLine is "-end" then I think this code will call find on an empty string, which does not work. > Source/WebCore/rendering/RenderGrid.cpp:923 > + const auto& areaIterator = gridAreaMap.find(gridAreaName); Should just be auto rather than const auto&.

(In reply to comment #5) > (From update of attachment 227848 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=227848&action=review > > Source/WebCore/rendering/RenderGrid.cpp:921 > > + String gridAreaName = namedGridAreaOrGridLine.substring(0, namedGridAreaOrGridLine.length() - suffixLength); > > I know I asked this last time, but what guarantees this is not an empty string? If namedGridAreaOrGridLine is "-end" then I think this code will call find on an empty string, which does not work. Oh true, I was assuming that HashTraits<String> was using isEmpty() for isEmptyValue() implementation but I've just checked that it uses isNull() so you're totally right indeed. I'll add a check for that case before landing.

(In reply to comment #6) > I was assuming that HashTraits<String> was using isEmpty() for isEmptyValue() implementation but I've just checked that it uses isNull() so you're totally right indeed. Actually, I think you were right. Given that HashTraits for strings use null as the special value, that means it’s OK to call find with a non-null empty value. As long as we are guaranteed it’s empty and non-null. The empty value is the one that is *unsafe* to use with our hash-table-based collections. Along with the deleted value. Other values, like non-null empty strings, are safe to use. I think the way to clarify this is to make sure we have a test case that covers that code path.

(In reply to comment #7) > (In reply to comment #6) > > I was assuming that HashTraits<String> was using isEmpty() for isEmptyValue() implementation but I've just checked that it uses isNull() so you're totally right indeed. > > Actually, I think you were right. > > Given that HashTraits for strings use null as the special value, that means it’s OK to call find with a non-null empty value. As long as we are guaranteed it’s empty and non-null. > > The empty value is the one that is *unsafe* to use with our hash-table-based collections. Along with the deleted value. Other values, like non-null empty strings, are safe to use. > > I think the way to clarify this is to make sure we have a test case that covers that code path. Let's add another test for that then.

Committed r166299: <http://trac.webkit.org/changeset/166299>