Bug 155014 – [css-grid] Initial support for implicit grid before explicit grid

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

Next step is to port the following patch from Blink:
https://codereview.chromium.org/1529083006/

Main change is the change in GridSpan to start storing ints instead of unsigned.

[Manuel Rego Casasnovas]

Created attachment 273225 [details]
Patch

[Darin Adler]

Comment on attachment 273225 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:1167
> +            coordinate.rows.translate(abs(m_smallestRowStart));

Please use std::abs, which won’t silently convert a floating point value, or a uint32_t or size_t, to an int, rather than abs, which despite its generic name *will* do those things.

For example, abs(4294967295U) returns 1, while std::abs(4294967295U) gives us a compile time error because of overload ambiguity. And abs(-1.1) returns 1, while std::abs(-1.1) returns 1.1.

Also note that the return value of abs and std::abs can be negative if it’s passed the most negative integer. Not sure if this is an issue for the correctness of our code.

> Source/WebCore/rendering/RenderGrid.cpp:1169
> +            coordinate.columns.translate(abs(m_smallestColumnStart));

Ditto.

> Source/WebCore/rendering/RenderGrid.cpp:1237
> +    m_grid.grow(maximumRowIndex + abs(m_smallestRowStart));

Ditto.

> Source/WebCore/rendering/RenderGrid.cpp:1239
> +        column.grow(maximumColumnIndex + abs(m_smallestColumnStart));

Ditto.

> Source/WebCore/rendering/style/GridCoordinate.h:168
> +        if (resolvedInitialPosition >= 0)
> +            m_resolvedInitialPosition = std::min(resolvedInitialPosition, kGridMaxTracks - 1);
> +        else
> +            m_resolvedInitialPosition = std::max(resolvedInitialPosition, -kGridMaxTracks);

I normally prefer to write this in a way that brackets the value with the minimum and maximum values. More elegant than comparing with zero, I think.

    m_resolvedInitialPosition = std::max(-kGridMaxTracks, std::min(resolvedInitialPosition, kGridMaxTracks - 1));

> Source/WebCore/rendering/style/GridCoordinate.h:172
> +        if (resolvedFinalPosition >= 0)
> +            m_resolvedFinalPosition = std::min(resolvedFinalPosition, kGridMaxTracks);
> +        else
> +            m_resolvedFinalPosition = std::max(resolvedFinalPosition, -kGridMaxTracks + 1);

Ditto.

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:253
>          unsigned resolvedPosition = abs(position.integerPosition()) - 1;

Same issue with abs rather than std::abs.

[Manuel Rego Casasnovas]

Created attachment 273340 [details]
Patch

[Manuel Rego Casasnovas]

Thanks for the review.

(In reply to comment #2)
> Comment on attachment 273225 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273225&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:1167
> > +            coordinate.rows.translate(abs(m_smallestRowStart));
> 
> Please use std::abs, which won’t silently convert a floating point value, or
> a uint32_t or size_t, to an int, rather than abs, which despite its generic
> name *will* do those things.
> 
> For example, abs(4294967295U) returns 1, while std::abs(4294967295U) gives
> us a compile time error because of overload ambiguity. And abs(-1.1) returns
> 1, while std::abs(-1.1) returns 1.1.
> 
> Also note that the return value of abs and std::abs can be negative if it’s
> passed the most negative integer. Not sure if this is an issue for the
> correctness of our code.

Thanks for the explanation, I've changed it to std::abs() in all the different places.

I think we're safe regarding the most negative integer as we're setting the limit in -kGridMaxTracks.

> > Source/WebCore/rendering/style/GridCoordinate.h:168
> > +        if (resolvedInitialPosition >= 0)
> > +            m_resolvedInitialPosition = std::min(resolvedInitialPosition, kGridMaxTracks - 1);
> > +        else
> > +            m_resolvedInitialPosition = std::max(resolvedInitialPosition, -kGridMaxTracks);
> 
> I normally prefer to write this in a way that brackets the value with the
> minimum and maximum values. More elegant than comparing with zero, I think.
> 
>     m_resolvedInitialPosition = std::max(-kGridMaxTracks,
> std::min(resolvedInitialPosition, kGridMaxTracks - 1));

Done.

[WebKit Commit Bot]

Comment on attachment 273340 [details]
Patch

Rejecting attachment 273340 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
emMac.dia -c /Volumes/Data/EWS/WebKit/Source/WebCore/history/mac/HistoryItemMac.mm -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/HistoryItemMac.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/GridResolvedPosition.o rendering/style/GridResolvedPosition.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/943725

[Manuel Rego Casasnovas]

Created attachment 273410 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 273410 [details]
Patch

Clearing flags on attachment: 273410

Committed r197850: <http://trac.webkit.org/changeset/197850>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.