Refactor parameters to blending functions
Created attachment 425746 [details] Patch
Comment on attachment 425746 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425746&action=review > Source/WebCore/animation/CSSPropertyAnimation.h:39 > +struct BlendOptions { Maybe BlendContext or something? BlendOptions sounds like an OptionSet. > Source/WebCore/animation/CSSPropertyAnimation.h:48 > + const CSSPropertyBlendingClient* client; > + double progress; > + bool isDiscrete; > + > + BlendOptions(double progress) > + : progress(progress) > + { > + } > + Using this constructor leaves client and isDiscrete uninitialized. > Source/WebCore/animation/CSSPropertyAnimation.h:58 > + // https://drafts.csswg.org/web-animations-1/#discrete > + // The property's values cannot be meaningfully combined, thus it is not additive and > + // interpolation swaps from Va to Vb at 50% (p=0.5). > + if (isDiscrete) > + this->progress = progress < 0.5 ? 0 : 1; Nor sure this is the best place to implement spec semantics. Maybe the caller could do this? Then the struct could be just a plain struct without constructors.
(In reply to Antti Koivisto from comment #2) > Comment on attachment 425746 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425746&action=review > > > Source/WebCore/animation/CSSPropertyAnimation.h:39 > > +struct BlendOptions { > > Maybe BlendContext or something? BlendOptions sounds like an OptionSet. I'll rename it to the BlendingContext, move it to AnimationUtilities.h and make a CSSPropertyBlendingContext private to CSSPropertyAnimation.cpp with the CSSPropertyBlendingClient member. > > Source/WebCore/animation/CSSPropertyAnimation.h:58 > > + // https://drafts.csswg.org/web-animations-1/#discrete > > + // The property's values cannot be meaningfully combined, thus it is not additive and > > + // interpolation swaps from Va to Vb at 50% (p=0.5). > > + if (isDiscrete) > > + this->progress = progress < 0.5 ? 0 : 1; > > Nor sure this is the best place to implement spec semantics. Maybe the > caller could do this? Then the struct could be just a plain struct without > constructors. I'll move it back to CSSPropertyAnimation::blendProperties() where it belongs.
Created attachment 425778 [details] Patch
*** Bug 223173 has been marked as a duplicate of this bug. ***
Comment on attachment 425778 [details] Patch Do we expect BlendingContext to gain more members? I'm not sure two members justifies the struct at this point. I would not consider "progress" a part of a "context": progress is a direct input into blending.
(In reply to Simon Fraser (smfr) from comment #6) > Comment on attachment 425778 [details] > Patch > > Do we expect BlendingContext to gain more members? I'm not sure two members > justifies the struct at this point. I would not consider "progress" a part > of a "context": progress is a direct input into blending. We do: the composite operation will have to be passed through to deal with additive and accumulative animations.
Created attachment 425843 [details] Patch
(In reply to Antoine Quint from comment #7) > (In reply to Simon Fraser (smfr) from comment #6) > > Comment on attachment 425778 [details] > > Patch > > > > Do we expect BlendingContext to gain more members? I'm not sure two members > > justifies the struct at this point. I would not consider "progress" a part > > of a "context": progress is a direct input into blending. > > We do: the composite operation will have to be passed through to deal with > additive and accumulative animations. Note also that CSSPropertyBlendingContext also adds the CSSPropertyBlendingClient.
Created attachment 425846 [details] Patch
Comment on attachment 425846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425846&action=review > Source/WebCore/animation/CSSPropertyAnimation.cpp:77 > + const CSSPropertyBlendingClient* client; > + > + CSSPropertyBlendingContext(double progress, bool isDiscrete, const CSSPropertyBlendingClient* client) Don't people usually put the member variable after the constructor?
Comment on attachment 425846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425846&action=review >> Source/WebCore/animation/CSSPropertyAnimation.cpp:77 >> + CSSPropertyBlendingContext(double progress, bool isDiscrete, const CSSPropertyBlendingClient* client) > > Don't people usually put the member variable after the constructor? It’s a struct vs. class thing. When the interface to a class is public data members (most structs), then those should be at the top. When the private data members are an implementation detail, then those should be at the bottom along with other private things. This follows our traditions for structs. But I would ask: Do we really need a constructor here? Can we use aggregate initialization syntax instead and omit the constructor function?
Comment on attachment 425846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425846&action=review > Source/WebCore/animation/CSSPropertyAnimation.cpp:74 > +struct CSSPropertyBlendingContext : BlendingContext { Why do we need subtyping here? Can't we just have everything in a single BlendingContext struct? > Source/WebCore/platform/animation/AnimationUtilities.h:35 > + double progress; > + bool isDiscrete { false }; Why do some fields have defaults and some not?
(In reply to Antti Koivisto from comment #13) > Comment on attachment 425846 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425846&action=review > > > Source/WebCore/animation/CSSPropertyAnimation.cpp:74 > > +struct CSSPropertyBlendingContext : BlendingContext { > > Why do we need subtyping here? Can't we just have everything in a single > BlendingContext struct? CSSPropertyBlendingClient is really only relevant in the context of CSSPropertyAnimation.cpp, while blending of types can be done in other source files. It felt cleaner to have this member only be exposed in the context of blending work in CSSPropertyAnimation.cpp. > > Source/WebCore/platform/animation/AnimationUtilities.h:35 > > + double progress; > > + bool isDiscrete { false }; > > Why do some fields have defaults and some not?
(In reply to Darin Adler from comment #12) > Comment on attachment 425846 [details] > > But I would ask: Do we really need a constructor here? Can we use aggregate > initialization syntax instead and omit the constructor function? I'll remove the BlendingContext constructor, which is indeed useless. However, I believe we need one for CSSPropertyBlendingContext blending context since it inherits from BlendingContext if we want to also have the subtype parameter in the constructor.
Committed r276141 (236634@main): <https://commits.webkit.org/236634@main>
Need to roll this out; build is broken on multiple platforms.
Did this cause this? ./animation/CSSPropertyAnimation.cpp:78:11: error: no matching constructor for initialization of 'WebCore::BlendingContext'
(In reply to Chris Dumez from comment #18) > Did this cause this? > ./animation/CSSPropertyAnimation.cpp:78:11: error: no matching constructor > for initialization of 'WebCore::BlendingContext' I landed the following build fix: <https://commits.webkit.org/r276142>
(In reply to Chris Dumez from comment #19) > (In reply to Chris Dumez from comment #18) > > Did this cause this? > > ./animation/CSSPropertyAnimation.cpp:78:11: error: no matching constructor > > for initialization of 'WebCore::BlendingContext' > > I landed the following build fix: <https://commits.webkit.org/r276142> Thanks Chris!
(In reply to Chris Dumez from comment #19) > (In reply to Chris Dumez from comment #18) > > Did this cause this? > > ./animation/CSSPropertyAnimation.cpp:78:11: error: no matching constructor > > for initialization of 'WebCore::BlendingContext' > > I landed the following build fix: <https://commits.webkit.org/r276142> Looks like there are still some build failures on some platforms (Windows at least). But my build fix did the trick for me locally on macOS.
(In reply to Chris Dumez from comment #21) > (In reply to Chris Dumez from comment #19) > > (In reply to Chris Dumez from comment #18) > > > Did this cause this? > > > ./animation/CSSPropertyAnimation.cpp:78:11: error: no matching constructor > > > for initialization of 'WebCore::BlendingContext' > > > > I landed the following build fix: <https://commits.webkit.org/r276142> > > Looks like there are still some build failures on some platforms (Windows at > least). But my build fix did the trick for me locally on macOS. GTK too: ../../Source/WebCore/platform/graphics/nicosia/NicosiaAnimation.cpp:31:31: error: cannot convert ‘double’ to ‘const WebCore::BlendingContext&’ ../../Source/WebCore/platform/graphics/nicosia/NicosiaAnimation.cpp:141:39: error: cannot convert ‘double’ to ‘const WebCore::BlendingContext&’ ../../Source/WebCore/platform/graphics/nicosia/NicosiaAnimation.cpp:149:41: error: cannot convert ‘double’ to ‘const WebCore::BlendingContext&’ ../../Source/WebCore/platform/graphics/nicosia/NicosiaAnimation.cpp:156:52: error: cannot convert ‘double’ to ‘const WebCore::BlendingContext&’
The real problem is landing the revised patch without doing EWS first. A good fix for those NicosiaAnimation.cpp places is possibly to write "{ progress }" instead of just "progress".
Trying this for GTK / Win's build: <https://commits.webkit.org/r276143>
There were too many build failures on GTK / WinCairo. I gave up, reverted by build fixes and just added a constructor for BlendingContext in <https://commits.webkit.org/r276147> to make the bots green again. Feel free to follow-up and do better in a follow-up, relying on EWS to make sure it builds everywhere.
rdar://76765208