Bug 130010 – [CSS Grid Layout] getComputedStyle() must return the specified value for positioning properties

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sergio Villar Senin]

A recent discussion in www-style mailing list

http://lists.w3.org/Archives/Public/www-style/2014Mar/0162.html

made clear that we basically cannot adjust the style of grid items because we'd be losing the specified value of the property, which is the one we should return in getComputedStyle(). Instead, we should handle that as any other resolution code in the RenderGrid.

[Sergio Villar Senin]

Created attachment 227742 [details]
Patch

[Darin Adler]

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.

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

Created attachment 227848 [details]
Patch

[Darin Adler]

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&.

[Sergio Villar Senin]

(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.

[Darin Adler]

(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.

[Sergio Villar Senin]

(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.

[Sergio Villar Senin]

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