Bug 224433 – Refactor parameters to blending functions

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

[Antoine Quint]

Refactor parameters to blending functions

[Antoine Quint]

Created attachment 425746 [details]
Patch

[Antti Koivisto]

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.

[Antoine Quint]

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

[Antoine Quint]

Created attachment 425778 [details]
Patch

[Antoine Quint]

*** Bug 223173 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

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.

[Antoine Quint]

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

[Antoine Quint]

Created attachment 425843 [details]
Patch

[Antoine Quint]

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

[Antoine Quint]

Created attachment 425846 [details]
Patch

[Dean Jackson]

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?

[Darin Adler]

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?

[Antti Koivisto]

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?

[Antoine Quint]

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

[Antoine Quint]

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

[Antoine Quint]

Committed r276141 (236634@main): <https://commits.webkit.org/236634@main>

[Darin Adler]

Need to roll this out; build is broken on multiple platforms.

[Chris Dumez]

Did this cause this? 
./animation/CSSPropertyAnimation.cpp:78:11: error: no matching constructor for initialization of 'WebCore::BlendingContext'

[Chris Dumez]

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

[Antoine Quint]

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

[Chris Dumez]

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

[Chris Dumez]

(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&’

[Darin Adler]

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

[Chris Dumez]

Trying this for GTK / Win's build: <https://commits.webkit.org/r276143>

[Chris Dumez]

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.

[Ryan Haddad]

rdar://76765208