Bug 226174 – [css-grid] Add discrete animation support for grid-template-columns|rows

• Component: CSS
• Status: RESOLVED FIXED

We need to add discrete animation support for grid-template-columns|rows. This is the first step for Bug 204580.

Created attachment 429527 [details]
Patch

[Oriol Brufau]

Comment on attachment 429527 [details]
Patch

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

> Source/WebCore/animation/CSSPropertyAnimation.cpp:700
> +class GridTemplateColumnsWrapper final : public AnimationPropertyWrapperBase {

These 2 classes are almost identical. Maybe you could merge them into e.g. GridTemplateTracksWrapper? And then use something like

  new GridTemplateTracksWrapper(CSSPropertyGridTemplateColumns, &RenderStyle::gridColumns, &RenderStyle::setGridColumns, &RenderStyle::gridAutoRepeatColumns, &RenderStyle::setGridAutoRepeatColumns),
  new GridTemplateTracksWrapper(CSSPropertyGridTemplateRows, &RenderStyle::gridRows, &RenderStyle::setGridRows, &RenderStyle::gridAutoRepeatRows, &RenderStyle::setGridAutoRepeatRows),

> Source/WebCore/animation/CSSPropertyAnimation.cpp:707
> +    {        

Nit: remove trailing spaces

> Source/WebCore/animation/CSSPropertyAnimation.cpp:743
> +    {        

Nit: remove trailing spaces

Created attachment 429648 [details]
Patch

Created attachment 429650 [details]
Patch

[Oriol Brufau]

Comment on attachment 429650 [details]
Patch

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

> Source/WebCore/animation/CSSPropertyAnimation.cpp:709
> +        , m_orderedNamedGridColumnLinesWrapper(DiscretePropertyWrapper<const OrderedNamedGridLinesMap&>(CSSPropertyGridTemplateColumns, &RenderStyle::orderedNamedGridColumnLines, &RenderStyle::setOrderedNamedGridColumnLines))

This may also need the named grid lines inside an auto-repeat: autoRepeatOrderedNamedGridColumnLines(), setAutoRepeatOrderedNamedGridColumnLines()

> Source/cmake/OptionsGTK.cmake:73
> +#WEBKIT_OPTION_DEFINE(USE_AVIF "Whether to enable support for AVIF images." PUBLIC ${ENABLE_EXPERIMENTAL_FEATURES})

The changes in this file seem unrelated

Created attachment 429749 [details]
Patch

[Antoine Quint]

Oriol commented earlier that the two classes should be refactored into a single class and I second that comment.

(In reply to Antoine Quint from comment #7)
> Oriol commented earlier that the two classes should be refactored into a
> single class and I second that comment.

Thanks for the comments! Oriol and I had a private discussion on this. If we unify them into a single class, We will have to pass a lot arguments over to the the unified class, for the moment, around 13. We thought maybe just leave them separated?

[Oriol Brufau]

(In reply to zsun from comment #8)
> Thanks for the comments! Oriol and I had a private discussion on this. If we
> unify them into a single class, We will have to pass a lot arguments over to
> the the unified class, for the moment, around 13. We thought maybe just
> leave them separated?

Thinking more about this, rather than passing all the functions as arguments to the constructor, I guess you could define a template class, and then specialize with different constructors. Something like

template <GridTrackSizingDirection>
class GridTemplateTracksWrapper : public AnimationPropertyWrapperBase {
    WTF_MAKE_FAST_ALLOCATED;
public:
    GridTemplateTracksWrapper();

private:
    // ... common logic
};

template <>
GridTemplateTracksWrapper<ForColumns>::GridTemplateTracksWrapper()
    : AnimationPropertyWrapperBase(CSSPropertyGridTemplateColumns)
    , m_gridTracksWrapper(DiscretePropertyWrapper<const Vector<GridTrackSize>&>(CSSPropertyGridTemplateColumns, &RenderStyle::gridColumns, &RenderStyle::setGridColumns))
    , m_autoRepeatTracksWrapper(DiscretePropertyWrapper<const Vector<GridTrackSize>&>(CSSPropertyGridTemplateColumns, &RenderStyle::gridAutoRepeatColumns, &RenderStyle::setGridAutoRepeatColumns))
    // ...
    {
    }

template <>
GridTemplateTracksWrapper<ForRows>::GridTemplateTracksWrapper()
    : AnimationPropertyWrapperBase(CSSPropertyGridTemplateRows)
    , m_gridTracksWrapper(DiscretePropertyWrapper<const Vector<GridTrackSize>&>(CSSPropertyGridTemplateRows, &RenderStyle::gridRows, &RenderStyle::setGridRows))
    , m_autoRepeatTracksWrapper(DiscretePropertyWrapper<const Vector<GridTrackSize>&>(CSSPropertyGridTemplateRows, &RenderStyle::gridAutoRepeatRows, &RenderStyle::setGridAutoRepeatRows))
    // ...
    {
    }

GridTrackSizingDirection is defined in GridPositionsResolver.h, you could also create another enum here I guess.

Created attachment 429861 [details]
Patch

(In reply to Oriol Brufau from comment #9)
> (In reply to zsun from comment #8)
> > Thanks for the comments! Oriol and I had a private discussion on this. If we
> > unify them into a single class, We will have to pass a lot arguments over to
> > the the unified class, for the moment, around 13. We thought maybe just
> > leave them separated?
> 
> Thinking more about this, rather than passing all the functions as arguments
> to the constructor, I guess you could define a template class, and then
> specialize with different constructors. Something like
> 
> template <GridTrackSizingDirection>
> class GridTemplateTracksWrapper : public AnimationPropertyWrapperBase {
>     WTF_MAKE_FAST_ALLOCATED;
> public:
>     GridTemplateTracksWrapper();
> 
> private:
>     // ... common logic
> };
> 
> template <>
> GridTemplateTracksWrapper<ForColumns>::GridTemplateTracksWrapper()
>     : AnimationPropertyWrapperBase(CSSPropertyGridTemplateColumns)
>     , m_gridTracksWrapper(DiscretePropertyWrapper<const
> Vector<GridTrackSize>&>(CSSPropertyGridTemplateColumns,
> &RenderStyle::gridColumns, &RenderStyle::setGridColumns))
>     , m_autoRepeatTracksWrapper(DiscretePropertyWrapper<const
> Vector<GridTrackSize>&>(CSSPropertyGridTemplateColumns,
> &RenderStyle::gridAutoRepeatColumns, &RenderStyle::setGridAutoRepeatColumns))
>     // ...
>     {
>     }
> 
> template <>
> GridTemplateTracksWrapper<ForRows>::GridTemplateTracksWrapper()
>     : AnimationPropertyWrapperBase(CSSPropertyGridTemplateRows)
>     , m_gridTracksWrapper(DiscretePropertyWrapper<const
> Vector<GridTrackSize>&>(CSSPropertyGridTemplateRows, &RenderStyle::gridRows,
> &RenderStyle::setGridRows))
>     , m_autoRepeatTracksWrapper(DiscretePropertyWrapper<const
> Vector<GridTrackSize>&>(CSSPropertyGridTemplateRows,
> &RenderStyle::gridAutoRepeatRows, &RenderStyle::setGridAutoRepeatRows))
>     // ...
>     {
>     }
> 
> GridTrackSizingDirection is defined in GridPositionsResolver.h, you could
> also create another enum here I guess.

Thanks very much for the suggestions! Code has been updated.

[Oriol Brufau]

Comment on attachment 429861 [details]
Patch

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

Looks good, thanks! Just some nits

> Source/WebCore/ChangeLog:10
> +        Reviewed by  Antoine Quint.

Nit: double space in "by  Antoine"

> Source/WebCore/animation/CSSPropertyAnimation.cpp:754
> +    {

Nit: remove the indentation. I think the curly brackets should be aligned with GridTemplateTracksWrapper, not with the commas.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:766
> +    // ...

Nit: remove this comment

> Source/WebCore/animation/CSSPropertyAnimation.cpp:767
> +    {

Nit: ditto (indentation)

> LayoutTests/imported/w3c/ChangeLog:6
> +        Reviewed by  Antoine Quint.

Nit: ditto (spaces)

Created attachment 429876 [details]
Patch

[EWS]

Committed r278173 (238216@main): <https://commits.webkit.org/238216@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429876 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78582540>