Bug 167988 – [css-grid] Move the track sizing algorithm to its own class

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[css-grid] Move the track sizing algorithm to its own class

[Sergio Villar Senin]

Created attachment 300905 [details]
Patch

[Javier Fernandez]

Comment on attachment 300905 [details]
Patch

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

I like how the algorithm is implemented now. Please, consider my comment about the Strategy pattern for future refactoring efforts.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:65
> +    return (m_growthLimit == infinity) ? m_baseSize : m_growthLimit;

Do we need this parenthesis ?

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:115
> +static GridTrackSizingDirection flowAwareDirectionForChild(const RenderGrid* grid, const RenderBox& child, GridTrackSizingDirection direction)

Either grid is defined as a reference of we need a nullcheck before using it.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:186
> +        index += index ? autoTrackStylesSize : 0;

Can index be a negative value ? If that's the case, wouldn't make more sense as follows ?

  if (std::abs(index) > 0)
     index += autoTrackStylesSize;

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:187
> +        return autoTrackStyles[index];

I guess that autoTrackStylesSize ensures index is always a positive value, however, should we add an ASSERT here ?

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:280
> +    GridItemWithSpan(RenderBox& gridItem, GridSpan span)

Shouldn't be const RenderBox& gridItem ?

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:822
> +        : GridTrackSizingAlgorithmStrategy(algorithm) { }

Have you considered using Templates for defining the different strategies instead of class inheritance ?

GridTrackSizingAlgorithmTemplate<Strategy>::operationXXX() {
   Strategy::operationXXX();
}

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:864
> +        // TODO(svillar): we pass TrackSizing to gridTrackSize() because it does not really matter

This comment does not follow our style.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:916
> +class DefiniteSizeStrategy final : public GridTrackSizingAlgorithmStrategy {

Either using templates or class inheritance, perhaps it'd be clearer to define the different strategies in different files.

[Sergio Villar Senin]

(In reply to comment #2)
> Comment on attachment 300905 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300905&action=review
> 
> I like how the algorithm is implemented now. Please, consider my comment
> about the Strategy pattern for future refactoring efforts.
> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:65
> > +    return (m_growthLimit == infinity) ? m_baseSize : m_growthLimit;
> 
> Do we need this parenthesis ?

Nop, although I left that code unchanged, just moved it around.

> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:115
> > +static GridTrackSizingDirection flowAwareDirectionForChild(const RenderGrid* grid, const RenderBox& child, GridTrackSizingDirection direction)
> 
> Either grid is defined as a reference of we need a nullcheck before using it.

I'll add an ASSERT

> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:186
> > +        index += index ? autoTrackStylesSize : 0;
> 
> Can index be a negative value ? If that's the case, wouldn't make more sense
> as follows ?
> 
>   if (std::abs(index) > 0)
>      index += autoTrackStylesSize;

Haven't changed that code. And the answer is yes, it might be negative, that's why it's defined as int and not as unsigned as some other variables in that method.

> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:187
> > +        return autoTrackStyles[index];
> 
> I guess that autoTrackStylesSize ensures index is always a positive value,
> however, should we add an ASSERT here ?

Again I haven't changed that code. But I can add another assert if you want.

> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:280
> > +    GridItemWithSpan(RenderBox& gridItem, GridSpan span)
> 
> Shouldn't be const RenderBox& gridItem ?

I haven't checked all the usages of that call, but again unchanged code.
 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:822
> > +        : GridTrackSizingAlgorithmStrategy(algorithm) { }
> 
> Have you considered using Templates for defining the different strategies
> instead of class inheritance ?
> 
> GridTrackSizingAlgorithmTemplate<Strategy>::operationXXX() {
>    Strategy::operationXXX();
> }

Nice suggestion. I haven't, not sure what would be the benefit though.
 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:864
> > +        // TODO(svillar): we pass TrackSizing to gridTrackSize() because it does not really matter
> 
> This comment does not follow our style.

Ack.

> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:916
> > +class DefiniteSizeStrategy final : public GridTrackSizingAlgorithmStrategy {
> 
> Either using templates or class inheritance, perhaps it'd be clearer to
> define the different strategies in different files.

Not sure it pays off the burden. I'd say that we could consider that if they became large, but from the time being I think they're well located right next to the algorithm mainly because the algorithm and the strategies are very coupled.

[Sergio Villar Senin]

Created attachment 301353 [details]
Patch

[Sergio Villar Senin]

Created attachment 301356 [details]
Patch

[Sergio Villar Senin]

Created attachment 301500 [details]
Patch

Build fix

[Build Bot]

Comment on attachment 301500 [details]
Patch

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

New failing tests:
fast/css-grid-layout/grid-item-end-after-get-set.html
fast/css-grid-layout/grid-item-negative-integer-explicit-grid-resolution.html
fast/css-grid-layout/grid-item-named-grid-line-resolution.html
fast/css-grid-layout/grid-item-position-changed-dynamic.html
fast/css-grid-layout/implicit-tracks-before-explicit.html
fast/css-grid-layout/grid-item-negative-position-resolution.html
fast/css-grid-layout/grid-item-area-get-set.html
fast/css-grid-layout/grid-auto-columns-rows-get-set.html
fast/css-grid-layout/grid-auto-flow-resolution.html
fast/css-grid-layout/grid-item-start-before-get-set.html
fast/css-grid-layout/grid-item-negative-indexes.html
fast/css-grid-layout/grid-item-spanning-resolution.html
fast/css-grid-layout/grid-item-unknown-named-grid-line-resolution.html
fast/css-grid-layout/grid-positioned-items-within-grid-implicit-track.html

[Build Bot]

Created attachment 301513 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sergio Villar Senin]

Created attachment 301637 [details]
Patch

Fixed an error in a newly added ASSERT

[Sergio Villar Senin]

Ping reviewers.

I know that the size of the patch makes it a bit scary but it's mostly moving code out of the RenderGrid to the new file plus some renames.

[Manuel Rego Casasnovas]

Comment on attachment 301637 [details]
Patch

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

Awesome job, just added a few nits here and there.

> Source/WebCore/ChangeLog:9
> +        GridTrackSizingAlgorithm, making the RenderGrid more compact and easy to maintain. A nice

Nit: s/the RenderGrid/RenderGrid/

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:93
> +// Static helper methods //////////////////////////////////////////////

Mmmm, is this line properly formatted?

I'm fine with having comments to separate the different parts of the class, but I'd use the standard format for comments (finishing the sentence with a "dot" too).

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:157
> +// GridTrackSizingAlgorithm private ///////////////////////////////////

Ditto.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:633
> +double GridTrackSizingAlgorithm::computeFlexFactorUnitSize(const Vector<GridTrack>& tracks, double flexFactorSum, LayoutUnit& leftOverSpace, const Vector<unsigned, 8>& flexibleTracksIndexes, std::unique_ptr<TrackIndexSet> tracksToTreatAsInflexible) const

Nit: This line seems a bit longer than usual. :-)

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:695
> +    // The function is not called if we don't have <flex> grid tracks

Nit: Missing dot after comment.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:716
> +// GridTrackSizingAlgorithmStrategy ///////////////////////////////

Ditto.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:976
> +// GridTrackSizingAlgorithm steps /////////////////////////////////

Ditto.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1109
> +// GridTrackSizingAlgorithm API ///////////////////////////////////

Ditto.

[Sergio Villar Senin]

Committed r212823: <http://trac.webkit.org/changeset/212823>