Bug 236480

Created attachment 451633 [details] Patch

Created attachment 451642 [details] Patch

Created attachment 451646 [details] Patch

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.

Created attachment 451733 [details] Patch

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

Created attachment 451753 [details] Patch

Created attachment 451934 [details] Patch

Created attachment 451945 [details] Patch

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.

Created attachment 452068 [details] Patch

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

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

<rdar://problem/88994181>

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