[webkit-reviews] review granted: [Bug 129372] [CSS Grid Layout] Fix positioning grid items using named grid lines/areas : [Attachment 225466] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 129372: [CSS Grid Layout] Fix positioning grid items using named grid
lines/areas
https://bugs.webkit.org/show_bug.cgi?id=129372

Attachment 225466: Patch
https://bugs.webkit.org/attachment.cgi?id=225466&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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;