Summary: | [css-grid] Add discrete animation support for grid-template-columns|rows | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zsun | ||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | annulen, ews-watchlist, graouts, gyuyoung.kim, jfernandez, obrufau, rego, ryuan.choi, sergio, svillar, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 204580 | ||||||||||||||||
Attachments: |
|
Description
zsun
2021-05-24 05:24:01 PDT
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]. |