Bug 214315 – [css-grid] CSSOM gridTemplateColumns/Rows reports wrong line names when leading implicit tracks are present

• Component: CSS
• Status: RESOLVED FIXED

[Oriol Brufau]

These WPT are failing:
imported/w3c/web-platform-tests/css/css-grid/parsing/grid-template-columns-computed-implicit-track.html
imported/w3c/web-platform-tests/css/css-grid/parsing/grid-template-rows-computed-implicit-track.html

They were added in https://crbug.com/790988, the Chromium patch needs to be ported to WebKit.

[Oriol Brufau]

Created attachment 404272 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 404272 [details]
Patch

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

On a quick look, it seems good. But I'd split it in 2 patches (one for the refactoring and another for the actual fix). Thanks!

> Source/WebCore/ChangeLog:20
> +        This patch also renames SmallestTrackStart to ExplicitGridStart.
> +        The SmallestTrackStart method used to return a signed int which was
> +        never positive, representing the smallest untranslated start among all
> +        grid items, clamped by 0 as a maximum.

Can we do this change on a different patch?

This is basically a refactoring and should have no behavior changes, so we can land that one first, and then the actual fix for this bug. WDYT?

[Oriol Brufau]

(In reply to Manuel Rego Casasnovas from comment #2)
> This is basically a refactoring and should have no behavior changes, so we
> can land that one first, and then the actual fix for this bug. WDYT?
OK, filed bug 214347.

[Oriol Brufau]

Created attachment 404335 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 404335 [details]
Patch

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

Thanks for the fix.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:875
> +    ASSERT((unsigned)end <= tracks.size());

Do we need this cast?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:920
> +        // Named grid line indices are relative to the explicit grid, but we are
> +        // including all tracks. So we need to subtract the number of leading
> +        // implicit tracks in order to get the proper line index.

Nit: Comments lines can be longer in WebKit, no need to wrap to 80-chars.

[Oriol Brufau]

Created attachment 404393 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 404393 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:875
>      ASSERT(end <= tracks.size());

So it looks like the cast was actually needed:
./css/CSSComputedStyleDeclaration.cpp:875:16: error: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
    ASSERT(end <= tracks.size());

Probably you can do something like:
    ASSERT(end <= static_cast<int>(tracks.size()));

[Oriol Brufau]

Created attachment 404435 [details]
Patch

[Oriol Brufau]

Created attachment 404451 [details]
Patch

[EWS]

Committed r264465: <https://trac.webkit.org/changeset/264465>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404451 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65672557>