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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 225376: Patch
https://bugs.webkit.org/attachment.cgi?id=225376&action=review

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