Bug 154818 – [css-grid] Get rid of GridResolvedPosition

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

[Manuel Rego Casasnovas]

Port this patch from Blink: https://codereview.chromium.org/1500433003/

[Manuel Rego Casasnovas]

Created attachment 272493 [details]
Patch

[Darin Adler]

Comment on attachment 272493 [details]
Patch

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

Biggest mistake here is the ++ operator on the iterator class.

I see a lot of +1 and -1 here and I don’t know how we are certain that overflow and underflow are not an issue in each case. I commented on the first few I saw but then stopped because there were so many.

> Source/WebCore/ChangeLog:2429
> -        No new tests, no change of behavior.
> +        no new tests, no change of behavior.

Why this change?

> Source/WebCore/rendering/RenderGrid.cpp:665
> +    for (auto resolvedPosition : tracksSpan) {
> +        unsigned trackIndex = resolvedPosition;

Seems like trackIndex and resolvedPosition are the same thing. I suggest something more like this:

    for (auto trackIndex : tracksSpan) {

> Source/WebCore/rendering/RenderGrid.cpp:868
> +    const unsigned trackPosition = span.resolvedInitialPosition();

const here is OK bu not all that helpful

> Source/WebCore/rendering/RenderGrid.cpp:1486
> +        || (positions.resolvedFinalPosition() - 1 > lastTrackIndex);

Is there something that prevents this from underflowing?

> Source/WebCore/rendering/RenderGrid.cpp:1491
> +    unsigned finalPosition = endIsAuto ? lastPosition : positions.resolvedFinalPosition() - 1;

Is there something that prevents this from underflowing?

> Source/WebCore/rendering/RenderGrid.cpp:1557
> +    LayoutUnit finalTrackPosition = linePositions[span.resolvedFinalPosition() - 1];

Does something prevent this from underflowing?

> Source/WebCore/rendering/RenderGrid.cpp:1560
> +    return finalTrackPosition - initialTrackPosition + tracks[span.resolvedFinalPosition() - 1].baseSize();

Does something prevent this from underflowing?

> Source/WebCore/rendering/style/GridCoordinate.h:90
> +        GridSpanIterator(unsigned v)
> +            : value(v)
> +        {
> +        }

Even in a case like this our style normally doesn’t embrace one-letter names in WebKit. Could either use "value" (OK to use the same name for the argument, it works in this case) or "initialValue".

> Source/WebCore/rendering/style/GridCoordinate.h:93
> +        unsigned operator++() { return value++; }

Normally we’d implement both pre-increment and post-increment, or leave out the return value. This uses the C++ syntax for implementing pre-increment, but it implements post-increment. That will be a problem. If I write:

    unsigned x = ++y;

And "y" is a GridSpanIterator, x will have the value *before* incrementing.

> Source/WebCore/rendering/style/GridCoordinate.h:94
> +        bool operator!=(GridSpanIterator other) const { return value != other.value; }

Peculiar to implement != but not ==.

> Source/WebCore/rendering/style/GridResolvedPosition.cpp:201
> +            return GridSpan::definiteGridSpan(resolvedOppositePosition -1, resolvedOppositePosition);

Missing space here after the "-".

> Source/WebCore/rendering/style/GridResolvedPosition.h:49
> +// This is a utility class with all the code related to grid items positions resolution.

I don’t know what “a utility class” is. I think we should avoid that terminology.

> Source/WebCore/rendering/style/GridResolvedPosition.h:58
> +    static GridSpan resolveGridPositionsFromAutoPlacementPosition(const RenderStyle&, const RenderBox&, GridTrackSizingDirection, unsigned);
>      static GridSpan resolveGridPositionsFromStyle(const RenderStyle&, const RenderBox&, GridTrackSizingDirection);
>      static unsigned explicitGridColumnCount(const RenderStyle&);
>      static unsigned explicitGridRowCount(const RenderStyle&);
>      static bool isNonExistentNamedLineOrArea(const String& lineName, const RenderStyle&, GridPositionSide);

I suggest omitting the blank lines before and after these functions inside the class definition.

I’m not sure these functions benefit much from being grouped in a class. Given their names, I think they would be fine as WebCore-namespace-level functions.

[Manuel Rego Casasnovas]

Created attachment 272557 [details]
Patch

[Manuel Rego Casasnovas]

Thanks for the review.

(In reply to comment #2)
> I see a lot of +1 and -1 here and I don’t know how we are certain that
> overflow and underflow are not an issue in each case. I commented on the
> first few I saw but then stopped because there were so many.

In the patch I'm adding an ASSERT in GridSpan::resolvedFinalPosition():
    ASSERT(m_resolvedFinalPosition);

And when we're doing "-1" we're always getting the final position and subtracting one. So we should be safe.

On top of that, this will be changed to "int" in next patches.
As we need to store negative values too.

> > Source/WebCore/ChangeLog:2429
> > -        No new tests, no change of behavior.
> > +        no new tests, no change of behavior.
> 
> Why this change?

This was a mistake. Removed it.

> > Source/WebCore/rendering/RenderGrid.cpp:665
> > +    for (auto resolvedPosition : tracksSpan) {
> > +        unsigned trackIndex = resolvedPosition;
> 
> Seems like trackIndex and resolvedPosition are the same thing. I suggest
> something more like this:
> 
>     for (auto trackIndex : tracksSpan) {

Ok.

> > Source/WebCore/rendering/RenderGrid.cpp:868
> > +    const unsigned trackPosition = span.resolvedInitialPosition();
> 
> const here is OK bu not all that helpful

Removed const.

> > Source/WebCore/rendering/RenderGrid.cpp:1486
> > +        || (positions.resolvedFinalPosition() - 1 > lastTrackIndex);
> 
> Is there something that prevents this from underflowing?

As explained before, the ASSERT in GridSpan::resolvedFinalPosition().

> > Source/WebCore/rendering/RenderGrid.cpp:1491
> > +    unsigned finalPosition = endIsAuto ? lastPosition : positions.resolvedFinalPosition() - 1;
> 
> Is there something that prevents this from underflowing?

Ditto.

> > Source/WebCore/rendering/RenderGrid.cpp:1557
> > +    LayoutUnit finalTrackPosition = linePositions[span.resolvedFinalPosition() - 1];
> 
> Does something prevent this from underflowing?

Ditto.

> > Source/WebCore/rendering/RenderGrid.cpp:1560
> > +    return finalTrackPosition - initialTrackPosition + tracks[span.resolvedFinalPosition() - 1].baseSize();
> 
> Does something prevent this from underflowing?

Ditto.

> > Source/WebCore/rendering/style/GridCoordinate.h:90
> > +        GridSpanIterator(unsigned v)
> > +            : value(v)
> > +        {
> > +        }
> 
> Even in a case like this our style normally doesn’t embrace one-letter names
> in WebKit. Could either use "value" (OK to use the same name for the
> argument, it works in this case) or "initialValue".

Done.

> > Source/WebCore/rendering/style/GridCoordinate.h:93
> > +        unsigned operator++() { return value++; }
> 
> Normally we’d implement both pre-increment and post-increment, or leave out
> the return value. This uses the C++ syntax for implementing pre-increment,
> but it implements post-increment. That will be a problem. If I write:
> 
>     unsigned x = ++y;
> 
> And "y" is a GridSpanIterator, x will have the value *before* incrementing.

Yes, this is wrong.

The iterator was implementing *, ++ and != because it was the only things mandatory for having it working.
Anyway, I've changed it and now it's just overriding * and &. That's enough and hopefully more clear.

> > Source/WebCore/rendering/style/GridResolvedPosition.cpp:201
> > +            return GridSpan::definiteGridSpan(resolvedOppositePosition -1, resolvedOppositePosition);
> 
> Missing space here after the "-".

Fixed.

> > Source/WebCore/rendering/style/GridResolvedPosition.h:49
> > +// This is a utility class with all the code related to grid items positions resolution.
> 
> I don’t know what “a utility class” is. I think we should avoid that
> terminology.

Removed it.

> > Source/WebCore/rendering/style/GridResolvedPosition.h:58
> > +    static GridSpan resolveGridPositionsFromAutoPlacementPosition(const RenderStyle&, const RenderBox&, GridTrackSizingDirection, unsigned);
> >      static GridSpan resolveGridPositionsFromStyle(const RenderStyle&, const RenderBox&, GridTrackSizingDirection);
> >      static unsigned explicitGridColumnCount(const RenderStyle&);
> >      static unsigned explicitGridRowCount(const RenderStyle&);
> >      static bool isNonExistentNamedLineOrArea(const String& lineName, const RenderStyle&, GridPositionSide);
> 
> I suggest omitting the blank lines before and after these functions inside
> the class definition.

Done.

> I’m not sure these functions benefit much from being grouped in a class.
> Given their names, I think they would be fine as WebCore-namespace-level
> functions.

Goo idea, maybe we can think about this in a follow-up patch.

[WebKit Commit Bot]

Comment on attachment 272557 [details]
Patch

Clearing flags on attachment: 272557

Committed r197400: <http://trac.webkit.org/changeset/197400>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.