Bug 139077 – [CSS Grid Layout] Simplify the interface of GridResolvedPosition

• Component: CSS
• Status: RESOLVED FIXED

[Sergio Villar Senin]

It's full of static methods that are only internally used.

Apart from that the code that creates GridSpans should be moved to GridCoordinate.h.

[Sergio Villar Senin]

Created attachment 242253 [details]
Patch

[Sergio Villar Senin]

Ping

[Darin Adler]

Comment on attachment 242253 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242253&action=review

> Source/WebCore/rendering/style/GridCoordinate.h:54
> +    static std::unique_ptr<GridSpan> create(const GridResolvedPosition& resolvedInitialPosition, const GridResolvedPosition& resolvedFinalPosition)
> +    {
> +        return std::make_unique<GridSpan>(resolvedInitialPosition, resolvedFinalPosition);
> +    }

It seems wasteful to have this class designed so you always have to create it on the heap. There’s no polymorphism here so I suggest these functions just return GridSpan, not unique_ptr<GridSpan>. We can make sure GridSpan has move semantics to avoid unnecessary copying of things. That way we avoid a new/delete pair that slow the code down. If there’s some reason to put it on the heap it’s easy to do that by explicitly calling make_unique to move it there.

I personally don’t like having all these functions inside the class definition. I find it hard to read the class definition when it’s like that. So I prefer to put these inlines separately after the class.

I also don’t think it’s useful to have a create function that simply calls make_unique with the same arguments. So I would normally omit it. It’s not like a RefCounted class where we want to keep the number of call sites of adoptRef to an absolute minimum. There a create function is justified to make it hard to misuse the class. But there’s nothing special about putting something onto the heap with make_unique that makes it something the class has to do itself; best to do that where the client has a reason to put it on the heap, not in the class’s own functions.

> Source/WebCore/rendering/style/GridCoordinate.h:60
> +        unsigned positionOffset = position.spanPosition() - 1;

What guarantees spanPosition is not zero?

> Source/WebCore/rendering/style/GridCoordinate.h:62
> +            GridResolvedPosition initialResolvedPosition = GridResolvedPosition(std::max<int>(0, resolvedOppositePosition.toInt() - positionOffset));

Not sure it’s good for the name GridResolvedPosition to be mentioned twice here. One way to reduce it is to use auto. Another is to use some other technique to convert an int to a GridResolvedPosition. Maybe construction syntax with { } is the best style here?

> Source/WebCore/rendering/style/GridCoordinate.h:63
> +            return GridSpan::create(initialResolvedPosition, resolvedOppositePosition);

No need to call GridSpan::create when already inside the GridSpan class. Just create should do.

> Source/WebCore/rendering/style/GridCoordinate.h:66
> +        return GridSpan::create(resolvedOppositePosition, GridResolvedPosition(resolvedOppositePosition.toInt() + positionOffset));

No need to call GridSpan::create when already inside the GridSpan class. Just create should do.

> Source/WebCore/rendering/style/GridCoordinate.h:82
> +        const unsigned* firstLineBeforeOppositePosition = std::lower_bound(gridLines.begin(), gridLines.end(), resolvedOppositePosition.toInt());

I think auto is a better type here than const unsigned*.

> Source/WebCore/rendering/style/GridCoordinate.h:93
> +        GridResolvedPosition resolvedGridLinePosition = gridLines[gridLineIndex];
> +        if (resolvedGridLinePosition > resolvedOppositePosition)
> +            resolvedGridLinePosition = resolvedOppositePosition;

Should we write this with std::min instead? Maybe without the local variable.

> Source/WebCore/rendering/style/GridCoordinate.h:94
> +        return GridSpan::create(resolvedGridLinePosition, resolvedOppositePosition);

No need to call GridSpan::create when already inside the GridSpan class. Just create should do.

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:239
> +    const GridPositionSide initialPositionSide = (direction == ForColumns) ? ColumnStartSide : RowStartSide;

I don’t think the const here adds much. Maybe omit it? Maybe use auto for the type?

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:241
> +    const GridPositionSide finalPositionSide = (direction == ForColumns) ? ColumnEndSide : RowEndSide;

I don’t think the const here adds much. Maybe omit it? Maybe use auto for the type?

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:246
> +        // FIXME: Implement properly "stack" value in auto-placement algorithm.

The word “properly” should come after the word “value” for proper grammar.

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:256
> +        const GridResolvedPosition finalResolvedPosition = resolveGridPositionFromStyle(gridContainerStyle, finalPosition, finalPositionSide);

I don’t think the const here adds much. Maybe omit it? Maybe we don’t need a local at all? Maybe use auto for the type?

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:262
> +        const GridResolvedPosition initialResolvedPosition = resolveGridPositionFromStyle(gridContainerStyle, initialPosition, initialPositionSide);

I don’t think the const here adds much. Maybe omit it? Maybe we don’t need a local at all? Maybe use auto for the type?

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:271
> +    if (resolvedFinalPosition < resolvedInitialPosition)
> +        resolvedFinalPosition = resolvedInitialPosition;

Again, how about std::min here instead?

[Sergio Villar Senin]

Comment on attachment 242253 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242253&action=review

The main objective of the change was just to simplify the interface but I agree we could use it to improve the code a bit.

>> Source/WebCore/rendering/style/GridCoordinate.h:54
>> +    }
> 
> It seems wasteful to have this class designed so you always have to create it on the heap. There’s no polymorphism here so I suggest these functions just return GridSpan, not unique_ptr<GridSpan>. We can make sure GridSpan has move semantics to avoid unnecessary copying of things. That way we avoid a new/delete pair that slow the code down. If there’s some reason to put it on the heap it’s easy to do that by explicitly calling make_unique to move it there.
> 
> I personally don’t like having all these functions inside the class definition. I find it hard to read the class definition when it’s like that. So I prefer to put these inlines separately after the class.
> 
> I also don’t think it’s useful to have a create function that simply calls make_unique with the same arguments. So I would normally omit it. It’s not like a RefCounted class where we want to keep the number of call sites of adoptRef to an absolute minimum. There a create function is justified to make it hard to misuse the class. But there’s nothing special about putting something onto the heap with make_unique that makes it something the class has to do itself; best to do that where the client has a reason to put it on the heap, not in the class’s own functions.

Yeah totally agree. I was trying to minimize the changes and do only refactorings but indeed the new functions don't look much better. I'll switch to returning GridSpan and ensuring the move semantics are properly used.

>> Source/WebCore/rendering/style/GridCoordinate.h:60
>> +        unsigned positionOffset = position.spanPosition() - 1;
> 
> What guarantees spanPosition is not zero?

As you can see above there is an ASSERT to verify that

>> Source/WebCore/rendering/style/GridResolvedPosition.cpp:262
>> +        const GridResolvedPosition initialResolvedPosition = resolveGridPositionFromStyle(gridContainerStyle, initialPosition, initialPositionSide);
> 
> I don’t think the const here adds much. Maybe omit it? Maybe we don’t need a local at all? Maybe use auto for the type?

auto looks good to me. The local is just to avoid a very long line of code bellow.

>> Source/WebCore/rendering/style/GridResolvedPosition.cpp:271
>> +        resolvedFinalPosition = resolvedInitialPosition;
> 
> Again, how about std::min here instead?

I guess std::max you mean

[Sergio Villar Senin]

Created attachment 253998 [details]
Patch for landing

[Sergio Villar Senin]

Created attachment 254000 [details]
Patch for landing

[Sergio Villar Senin]

Committed r185059: <http://trac.webkit.org/changeset/185059>