Bug 154818

Summary: [css-grid] Get rid of GridResolvedPosition
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: Layout and RenderingAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jfernandez, rego, simon.fraser, svillar
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 153488    
Attachments:
Description Flags
Patch
none
Patch none

Description Manuel Rego Casasnovas 2016-02-29 09:07:38 PST
Port this patch from Blink: https://codereview.chromium.org/1500433003/
Comment 1 Manuel Rego Casasnovas 2016-02-29 09:28:48 PST
Created attachment 272493 [details]
Patch
Comment 2 Darin Adler 2016-02-29 09:55:51 PST
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.
Comment 3 Manuel Rego Casasnovas 2016-03-01 03:59:12 PST
Created attachment 272557 [details]
Patch
Comment 4 Manuel Rego Casasnovas 2016-03-01 04:07:36 PST
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.
Comment 5 WebKit Commit Bot 2016-03-01 08:54:12 PST
Comment on attachment 272557 [details]
Patch

Clearing flags on attachment: 272557

Committed r197400: <http://trac.webkit.org/changeset/197400>
Comment 6 WebKit Commit Bot 2016-03-01 08:54:16 PST
All reviewed patches have been landed.  Closing bug.