Bug 226174

Created attachment 429527 [details] Patch

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

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

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?

(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.

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

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].

<rdar://problem/78582540>