Bug 157260

Summary: [css-grid] Add support for position resolution with auto-repeat tracks
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, glenn, jfernandez, kling, koivisto, kondapallykalyan, rego, svillar
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60731    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews116 for mac-yosemite
none
Patch none

Description Sergio Villar Senin 2016-05-02 02:15:31 PDT
[css-grid] Add support for position resolution with auto-repeat tracks
Comment 1 Sergio Villar Senin 2016-05-02 02:35:40 PDT
Created attachment 277900 [details]
Patch
Comment 2 Build Bot 2016-05-02 03:54:06 PDT
Comment on attachment 277900 [details]
Patch

Attachment 277900 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1255028

New failing tests:
fast/css-grid-layout/grid-item-end-after-get-set.html
fast/css-grid-layout/named-grid-lines-with-named-grid-areas-resolution.html
fast/css-grid-layout/grid-container-change-named-grid-lines-recompute-child.html
fast/css-grid-layout/grid-item-named-grid-line-resolution.html
fast/css-grid-layout/grid-item-unknown-named-grid-line-resolution.html
fast/css-grid-layout/grid-item-area-get-set.html
fast/css-grid-layout/grid-item-start-before-get-set.html
fast/css-grid-layout/grid-item-bad-named-area-auto-placement.html
fast/css-grid-layout/grid-item-named-grid-area-resolution.html
fast/css-grid-layout/named-grid-lines-with-named-grid-areas-get-set.html
Comment 3 Build Bot 2016-05-02 03:54:10 PDT
Created attachment 277904 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 4 Darin Adler 2016-05-02 08:39:00 PDT
Comment on attachment 277900 [details]
Patch

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

Multiple tests crashing. Presumably hitting assertions since the failures are only on the debug build. Please do not land until that is resolved.

> Source/WebCore/rendering/RenderGrid.cpp:1190
> +    // TODO(svillar): implement the algorithm to compute the number of auto repeat tracks.

Even for comments like this, we normally use FIXME, not TODO, and we don’t put our names in these kinds of comments. We also use sentence capitalization.

> Source/WebCore/rendering/RenderGrid.cpp:1191
> +    UNUSED_PARAM(direction);

Even in the short term line this, we normally like to omit or comment out the argument names rather than using UNUSED_PARAM.

> Source/WebCore/rendering/RenderGrid.h:69
> +    size_t autoRepeatCountForDirection(GridTrackSizingDirection direction) const
> +    {
> +        return direction == ForColumns ? m_autoRepeatColumns : m_autoRepeatRows;
> +    }

When function reaches there point where it’s multiple lines like this, we almost always put the inline function outside and after the class definition with the inline keyword so the class definition itself is easier to read.

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:88
> +    if (!gridLineNames.isEmpty()) {
> +        auto it = gridLineNames.find(namedLine);
> +        m_namedLinesIndexes = it == gridLineNames.end() ? nullptr : &it->value;
> +    }
> +
> +    if (!autoRepeatGridLineNames.isEmpty()) {
> +        auto it = autoRepeatGridLineNames.find(namedLine);
> +        m_autoRepeatNamedLinesIndexes = it == autoRepeatGridLineNames.end() ? nullptr : &it->value;
> +    }

Why the isEmpty checks? The find function should be fast enough without a special case for empty.

I think this would probably be cleaner if it was done with helper functions and member initialization rather than with a function body.

    , m_namedLinesIndexes(findNamedLinesIndices(direction, gridContainerStyle))
    , m_autoRepeatNamedLinesIndexes(findAutoRepeatNamedLinesIndices(direction, gridContainerStyle))
    , m_insertionPoint(insertionPoint(direction, gridContainerStyle))

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:93
> +bool NamedLineCollection::isValidNamedLineOrArea(const String& namedLine, const RenderStyle& gridContainerStyle, GridPositionSide side)

Should be const.

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:100
> +    if (gridLineNames.contains(namedLine) || autoRepeatGridLineNames.contains(namedLine))
> +        return true;

Do we have a guarantee that namedLine is not a null string?

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:103
> +    String implicitName = implicitNamedGridLineForSide(namedLine, side);
> +    return gridLineNames.contains(implicitName) || autoRepeatGridLineNames.contains(implicitName);

Do we have a guarantee that implicitName is not a null string?

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:106
> +bool NamedLineCollection::hasNamedLines()

Should be const.

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:111
> +size_t NamedLineCollection::find(unsigned line)

Should be const.

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:127
> +            return m_autoRepeatNamedLinesIndexes->find(static_cast<unsigned>(1));

1u is probably better than static_cast<unsigned>(1). Not sure why just 1 doesn’t work.

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:128
> +        size_t position = m_autoRepeatNamedLinesIndexes->find(static_cast<unsigned>(0));

0u is probably better than static_cast<unsigned>(0). Not sure why just 1 doesn’t work.

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:131
> +        return localIndex ? m_autoRepeatNamedLinesIndexes->find(static_cast<unsigned>(1)) : notFound;

1u is probably better than static_cast<unsigned>(1). Not sure why just 1 doesn’t work.

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:137
> +bool NamedLineCollection::contains(unsigned line)

Should be const.

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:139
> +    RELEASE_ASSERT(hasNamedLines());

Why does this need to be a RELEASE_ASSERT? Normally we do not use those.

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:145
> +    RELEASE_ASSERT(hasNamedLines());

Why does this need to be a RELEASE_ASSERT? Normally we do not use those.

> Source/WebCore/rendering/style/GridPositionsResolver.h:59
> +    bool hasNamedLines();
> +    unsigned firstPosition();
> +
> +    bool contains(unsigned line);

These members should all be const.

> Source/WebCore/rendering/style/GridPositionsResolver.h:60
> +private:

Should have a blank line before this "private".

> Source/WebCore/rendering/style/GridPositionsResolver.h:63
> +    size_t find(unsigned line);
> +    const Vector<unsigned>* m_namedLinesIndexes = nullptr;
> +    const Vector<unsigned>* m_autoRepeatNamedLinesIndexes = nullptr;

I suggest a blank line between the function and the data members.

We’ve been using the { nullptr } syntax rather than the = nullptr syntax. Not sure why.
Comment 5 Sergio Villar Senin 2016-05-02 11:49:12 PDT
(In reply to comment #4)
> Comment on attachment 277900 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277900&action=review
> 
> Multiple tests crashing. Presumably hitting assertions since the failures
> are only on the debug build. Please do not land until that is resolved.

Sure, I'll take a look

> > Source/WebCore/rendering/RenderGrid.cpp:1190
> > +    // TODO(svillar): implement the algorithm to compute the number of auto repeat tracks.
> 
> Even for comments like this, we normally use FIXME, not TODO, and we don’t
> put our names in these kinds of comments. We also use sentence
> capitalization.

Yeah sorry about that, living in between chromium and webkit makes me commit this kind of mistakes from time to time.

> > Source/WebCore/rendering/style/GridPositionsResolver.cpp:100
> > +    if (gridLineNames.contains(namedLine) || autoRepeatGridLineNames.contains(namedLine))
> > +        return true;
> 
> Do we have a guarantee that namedLine is not a null string?

namedLine is a reference so it can't be null.
 
> > Source/WebCore/rendering/style/GridPositionsResolver.cpp:103
> > +    String implicitName = implicitNamedGridLineForSide(namedLine, side);
> > +    return gridLineNames.contains(implicitName) || autoRepeatGridLineNames.contains(implicitName);
> 
> Do we have a guarantee that implicitName is not a null string?

Yes, the function used to construct it always returns a non-null non-empty string

> > Source/WebCore/rendering/style/GridPositionsResolver.cpp:127
> > +            return m_autoRepeatNamedLinesIndexes->find(static_cast<unsigned>(1));
> 
> 1u is probably better than static_cast<unsigned>(1). Not sure why just 1
> doesn’t work.
> 
> > Source/WebCore/rendering/style/GridPositionsResolver.cpp:128
> > +        size_t position = m_autoRepeatNamedLinesIndexes->find(static_cast<unsigned>(0));
> 
> 0u is probably better than static_cast<unsigned>(0). Not sure why just 1
> doesn’t work.
> 
> > Source/WebCore/rendering/style/GridPositionsResolver.cpp:131
> > +        return localIndex ? m_autoRepeatNamedLinesIndexes->find(static_cast<unsigned>(1)) : notFound;
> 
> 1u is probably better than static_cast<unsigned>(1). Not sure why just 1
> doesn’t work.

At least clang is very picky with this kind of unsigned vs signed comparisons. I'll use the 'u' suffix as the representation is indeed more compact and clean.
 
> > Source/WebCore/rendering/style/GridPositionsResolver.cpp:137
> > +bool NamedLineCollection::contains(unsigned line)
> 
> Should be const.
> 
> > Source/WebCore/rendering/style/GridPositionsResolver.cpp:139
> > +    RELEASE_ASSERT(hasNamedLines());
> 
> Why does this need to be a RELEASE_ASSERT? Normally we do not use those.
> 
> > Source/WebCore/rendering/style/GridPositionsResolver.cpp:145
> > +    RELEASE_ASSERT(hasNamedLines());
> 
> Why does this need to be a RELEASE_ASSERT? Normally we do not use those.

No special reason, I'll change both to ASSERT.
Comment 6 Sergio Villar Senin 2016-05-02 12:24:05 PDT
(In reply to comment #4)
> Comment on attachment 277900 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277900&action=review
> 
> Multiple tests crashing. Presumably hitting assertions since the failures
> are only on the debug build. Please do not land until that is resolved.

It was a stupid mistake cause by a rename. We moved from a method called

isNonExistentNamedLineXXX()

to

isValidNamedLineXXX()

There is an ASSERT() using that method where I renamed it properly but forgot to prepend the ! before the method call (because it's now returning false when it used to return true and viceversa).
Comment 7 Sergio Villar Senin 2016-05-02 12:30:51 PDT
Created attachment 277922 [details]
Patch

Patch for landing
Comment 8 Sergio Villar Senin 2016-05-03 04:30:01 PDT
Committed r200368: <http://trac.webkit.org/changeset/200368>