Bug 86421 – [chromium] Add unit testing to WebTransformationMatrix.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

WebTransformationMatrix is going to become the compositor's data type for transforms.  It currently wraps WebCore::TransformationMatrix, and in the future there will be additional implementation of the same interface. However, transformation math is a high profile, high risk black hole for errors... there is a lot of "almost-redundant" code that can have tiny mistakes that are very difficult to find.

So it is probably a good idea to be very aggressive with testing here, going for very tight, specific, and complete test coverage.

This patch will provide all of those tests, except for the mapRect/mapQuad/projectQuad routines that will eventually be removed.  The new mapRect/mapQuad/projectQuad functionality would be in CCMathUtil, and testing does need to be added to that code soon.  Then, after the dust settles from GTFO, WebTransformationMatrix and CCMathUtil could probably be merged.

[Shawn Singh]

Created attachment 141843 [details]
Patch

Note: I also skipped the blend() function for now. Will talk to vollick@ about what coverge he feels exists or is needed before spending time on that.

[Shawn Singh]

(In reply to comment #1)
> Created an attachment (id=141843) [details]
> Patch
> 
> Note: I also skipped the blend() function for now. Will talk to vollick@ about what coverge he feels exists or is needed before spending time on that.

Spoke with him, and we will need to add test coverage for blend(), I would like to do it in a separate patch.  Thanks!

[Adrienne Walker]

Comment on attachment 141843 [details]
Patch

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

> Source/WebKit/chromium/tests/WebTransformationMatrixTest.cpp:234
> +    WebTransformationMatrix B = A;

This isn't assignment, it's copy-initialization via the copy constructor.  The assignment needs to be in a separate statement.

Also, maybe you should test the return value from the assignment operator, e.g. A = B = C.

[Shawn Singh]

Created attachment 142048 [details]
Addressed reviewer comment

[Adrienne Walker]

Comment on attachment 142048 [details]
Addressed reviewer comment

R=me.  I don't quite follow why blend needs to be in a separate patch, but this set of tests looks fine to me.

[Shawn Singh]

Comment on attachment 142048 [details]
Addressed reviewer comment

There was no strong reason, please feel free to cancel the cq if you want them to be together for any reason.

blend() will add another 7-8 tests.  I'm running into challenges trying to figure out why the blending is not as "linear" as I expect for skewY; and testing rotation blending will not be ideal either since they use quaternions; I'll probably just resort to testing the status quo in a few different configurations.

[WebKit Review Bot]

Comment on attachment 142048 [details]
Addressed reviewer comment

Clearing flags on attachment: 142048

Committed r117199: <http://trac.webkit.org/changeset/117199>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Singh]

(In reply to comment #6)
> (From update of attachment 142048 [details])
> There was no strong reason, please feel free to cancel the cq if you want them to be together for any reason.
> 
> blend() will add another 7-8 tests.  I'm running into challenges trying to figure out why the blending is not as "linear" as I expect for skewY; and testing rotation blending will not be ideal either since they use quaternions; I'll probably just resort to testing the status quo in a few different configurations.

https://bugs.webkit.org/show_bug.cgi?id=87066 will add unit tests for blend().