[webkit-reviews] review denied: [Bug 50633] Make WebKitCSSMatrix more efficient for use in WebGL : [Attachment 96968] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Igor Trindade Oliveira
<itrindade.oliveira at gmail.com>'s request for review:
Bug 50633: Make WebKitCSSMatrix more efficient for use in WebGL
https://bugs.webkit.org/show_bug.cgi?id=50633

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=96968&action=review

This mostly looks okay, but the naming of the copy operations should be made
consistent. Couple of other minor issues.

> LayoutTests/transforms/cssmatrix-3d-interface.xhtml:244
> +shouldBe('parseFloat(m.m11)', '538');

I'm not sure this is the best way to write this. I've seen problems in other
tests where floating point values like "1.0e12" weren't handled correctly. For
situations where you're doing a simple truncation to integer I would suggest
using Math.floor(). For example: shouldBe('Math.floor(m.m11)', '538');

> Source/WebCore/css/WebKitCSSMatrix.h:106
>      // The following math function return a new matrix with the 
>      // specified operation applied. The this value is not modified.

This comment isn't accurate any more. Either group the in-place operations
together in their own section or adjust this comment. Also, the new entry
points need documentation.

> Source/WebCore/css/WebKitCSSMatrix.idl:63
> +	   void copy(in Float32Array array);
> +	   void copy64(in Float64Array array);

These methods can and should be named identically; WebKit's code generators
(both JSC and V8) support overloaded methods. Personally I would probably name
these "copyTo" or "toArray".

> Source/WebCore/css/WebKitCSSMatrix.idl:67
> +	   void multiplyMatrix(in WebKitCSSMatrix secondMatrix);

This and all of the other new entry points need documentation.