[webkit-reviews] review denied: [Bug 78155] Implement hardware animation of CSS filters : [Attachment 126206] Patch fixing Windows build

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Chris Marrin <cmarrin at apple.com>'s
request for review:
Bug 78155: Implement hardware animation of CSS filters
https://bugs.webkit.org/show_bug.cgi?id=78155

Attachment 126206: Patch fixing Windows build
https://bugs.webkit.org/attachment.cgi?id=126206&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=126206&action=review


very minor comments. one confusion/error about the return type of the validate
function.

> Source/WebCore/page/animation/AnimationBase.cpp:1055
>	   gPropertyWrappers->append(new PropertyWrapper<const
FilterOperations&>(CSSPropertyWebkitFilter, &RenderStyle::filter,
&RenderStyle::setFilter));
>  #endif
> +#endif
> +

Seems like you added a blank line here by accident.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:468
>	   size_t numAnimations = propertyAnimations.size();
>	   for (size_t i = 0; i < numAnimations; ++i) {
>	       const LayerPropertyAnimation& currAnimation =
propertyAnimations[i];
> -	       if (currAnimation.m_property == property)
> +	       
> +	       if (currAnimation.m_property == AnimatedPropertyWebkitTransform
|| currAnimation.m_property == AnimatedPropertyOpacity
> +		       || currAnimation.m_property ==
AnimatedPropertyBackgroundColor
> +#if ENABLE(CSS_FILTERS)
> +		       || currAnimation.m_property ==
AnimatedPropertyWebkitFilter
> +#endif
> +		   )

question: this is now copying those four properties for the layers, but the
calling sites previously didn't copy all four. Will there be a case where we're
now copying something we didn't before, and will it cause a problem?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1398
> +    moveOrCopyAnimations(Move,  m_layer.get(), m_structuralLayer.get());

extra space

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1863
> +bool GraphicsLayerCA::appendToUncommittedAnimations(const KeyframeValueList&
valueList, const FilterOperations* operations, const Animation* animation,
const String& animationName, int animationIndex, double timeOffset)

Why not pass in the FilterOperation directly rather than FilterOperations and
index? I see you need the index to generate the keypath, but maybe just that.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1865
> +    bool additive = animationIndex > 0;

Do these ever need to be additive? Aren't we always animating a different
keypath?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1898
> +
> +// get list of properties to animate for this operation (from
PlatformCALayer)
> +// for each property:
> +//	   create kf or basic animation
> +//	       animation name is propertyIdToString(valueList.property()) + "."
+ "filter_" + animationIndex + property
> +//	       maybe get this from PlatformCALayer
> +//	   setFilterAnimationKeyframes or setFilterAnimationEndpoints
> +//	       need valueList for values, operations and animationIndex (or
operation), property index
> +//	       maybe need to ask PlatformCALayer to return values given an
operation and index?
> +

I think you meant to remove this. If not, indentation is off.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1906
> +    int listIndex = validateFilterOperations(valueList);

validateFilterOperations returns a boolean - not an int. I guess this means you
were always getting the 0th entry in the KeyframeValueList.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1918
> +	   if (!appendToUncommittedAnimations(valueList, operations, animation,
animationName, animationIndex, timeOffset))

As above, why not pass in operations->operations().at(animationIndex) (and
maybe the index)?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:187
> +    PassRefPtr<PlatformCAAnimation> createBasicAnimation(const Animation*,
const String& keyPath, bool additive);

I guess you don't need keyPath here.

> Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:451
> +	   if (propertyIndex < 3) {
> +	       double multiplier = 1 - op->amount() * 2;
> +	       value.adoptNS([[NSArray arrayWithObjects:
> +		   [NSNumber numberWithDouble:invertConstants[propertyIndex][0]
* multiplier],

I'm confused here. What's with this *2 thing? And why do this when
propertyIndex < 3?

I'm also slightly confused about propertyIndex. I'm not sure what it means.

OK. I've looked elsewhere and worked it out. It's the index into the number of
animations needed for each effect. So it isn't anything to do with CSS
properties. Maybe we could think of a better name?

> Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:715
> +	   default: return "";
> +	   case 0: return "inputRVector";
> +	   case 1: return "inputGVector";
> +	   case 2: return "inputBVector";
> +	   }
> +    case FilterOperation::SATURATE: return "inputSaturation";
> +    case FilterOperation::HUE_ROTATE: return "inputAngle";
> +    case FilterOperation::INVERT:
> +	   switch(propertyIndex) {
> +	   default: return "";
> +	   case 0: return "inputRVector";
> +	   case 1: return "inputGVector";
> +	   case 2: return "inputBVector";
> +	   case 3: return "inputBiasVector";

Shouldn't default be last?

> Source/WebCore/rendering/RenderLayer.h:527
>  #if ENABLE(CSS_FILTERS)
> +    bool hasFilter() const { return renderer()->hasFilter(); }
>      virtual void filterNeedsRepaint();

hasFilter() might be able to go outside the ENABLE. I did the same for
style->hasFilter. That saves the ugly code below.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1208
> +    if (!hasOpacity && !hasTransform
> +#if ENABLE(CSS_FILTERS)
> +	   && !hasFilter
> +#endif
> +    )

This is the ugly bit. I think we can do it outside the ENABLE.