[webkit-reviews] review denied: [Bug 64640] Add optimizations to TransformationMatrix for matrices that are identity or only have translation values : [Attachment 101071] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 64640: Add optimizations to TransformationMatrix for matrices that are
identity or only have translation values
https://bugs.webkit.org/show_bug.cgi?id=64640

Attachment 101071: Patch
https://bugs.webkit.org/attachment.cgi?id=101071&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=101071&action=review


I think we could get more optimizations here.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:650
> +    m_identityType = IdentityUnknown;

If the angle is 0, the type shouldn't change, right?

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:739
> +    m_identityType = IdentityUnknown;

Optimize for no-op rotation?

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:814
> +    m_identityType = IdentityUnknown;

Seems like you should be able to deduce the new identity type easily.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:825
> +    m_identityType = IdentityUnknown;

Ditto.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:836
> +    m_identityType = IdentityUnknown;

Ditto.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:907
> +    m_identityType = IdentityUnknown;

Test the identity type of the incoming matrix?

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:68
> +    enum IdentityType { IdentityUnknown, IdentityYes, IdentityTranslation,
IdentityNo };

I'm not sure "Identity" is the right prefix. IdentityTranslation is
nonsensical, and IdentityNo is hard to parse. Maybe

MatrixType { Unknown, Identity, Translation, Other }

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:133
> +	   return isIdentity() || m_identityType == IdentityTranslation;

It's slightly subtle that the call to isIdentity() calls
determineIdentityType(). Maybe have a determineIdentityTypeIfNecessary()
method?

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:376
> +    IdentityType m_identityType;

I think this could be mutable to avoid the nasty const casts.