[css-transforms] properly handle interpolation of non-invertible matrices

[Nikos Mouchtaris]

Web animations- properly handle interpolation of non-invertible matrices

[Nikos Mouchtaris]

Created attachment 451633 [details]
Patch

[Nikos Mouchtaris]

Created attachment 451642 [details]
Patch

[Nikos Mouchtaris]

Created attachment 451646 [details]
Patch

[Martin Robinson]

Comment on attachment 451646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451646&action=review

This looks pretty good to me. I had a couple comments, but I really like the WPT progression!

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:85
> +    if ((from.hasMatrixOperation() || hasMatrixOperation()) && (!from.isInvertible(boxSize) || !isInvertible(boxSize))) {

There are some non-matrix operations that lead to uninvertible transformation matrices. Do they also need to follow this code path? The specification mentions "scale(0)."

> Source/WebCore/platform/graphics/transforms/TransformOperations.h:66
> +        for (const auto& operation : m_operations) {

I think this might be a good candidate for using `any_of` from the standard library.

[Nikos Mouchtaris]

Created attachment 451733 [details]
Patch

[Nikos Mouchtaris]

(In reply to Martin Robinson from comment #4)
> Comment on attachment 451646 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451646&action=review
> 
> This looks pretty good to me. I had a couple comments, but I really like the
> WPT progression!
> 
> > Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:85
> > +    if ((from.hasMatrixOperation() || hasMatrixOperation()) && (!from.isInvertible(boxSize) || !isInvertible(boxSize))) {
> 
> There are some non-matrix operations that lead to uninvertible
> transformation matrices. Do they also need to follow this code path? The
> specification mentions "scale(0)."
> 
From my understanding of this line in the spec I don't think so: 
Note: Animations to or from the neutral element of additions <matrix()>, matrix3d() and perspective() fall back to discrete animations

> > Source/WebCore/platform/graphics/transforms/TransformOperations.h:66
> > +        for (const auto& operation : m_operations) {
> 
> I think this might be a good candidate for using `any_of` from the standard
> library.
Fixed.

[Nikos Mouchtaris]

Created attachment 451753 [details]
Patch

[Nikos Mouchtaris]

Created attachment 451934 [details]
Patch

[Nikos Mouchtaris]

Created attachment 451945 [details]
Patch

[Martin Robinson]

Comment on attachment 451945 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451945&action=review

Looks good to me, with a couple nits.

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:99
> +bool TransformOperations::shouldFallbackToDiscreteAnimation(const TransformOperations& from, const LayoutSize& boxSize) const

Super nit: Fallback => FallBack because "fall back" is the verb form and "fallback" is the noun.

> Source/WebCore/platform/graphics/transforms/TransformOperations.h:66
> +        return std::any_of(m_operations.begin(), m_operations.end(), [](RefPtr<TransformOperation> op) {

Hrm. Can RefPtr<TransformOperation> be replaced with auto? I would use "operation" instead of "op" here, because "op" can also mean operand.

[Nikos Mouchtaris]

Created attachment 452068 [details]
Patch

[Nikos Mouchtaris]

(In reply to Martin Robinson from comment #10)
> Comment on attachment 451945 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451945&action=review
> 
> Looks good to me, with a couple nits.
> 
> > Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:99
> > +bool TransformOperations::shouldFallbackToDiscreteAnimation(const TransformOperations& from, const LayoutSize& boxSize) const
> 
> Super nit: Fallback => FallBack because "fall back" is the verb form and
> "fallback" is the noun.
Fixed.
> 
> > Source/WebCore/platform/graphics/transforms/TransformOperations.h:66
> > +        return std::any_of(m_operations.begin(), m_operations.end(), [](RefPtr<TransformOperation> op) {
> 
> Hrm. Can RefPtr<TransformOperation> be replaced with auto? I would use
> "operation" instead of "op" here, because "op" can also mean operand.
Fixed.

[EWS]

Committed r289862 (247302@main): <https://commits.webkit.org/247302@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452068 [details].

[Radar WebKit Bug Importer]

<rdar://problem/88994181>

[Antoine Quint]

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