[webkit-reviews] review denied: [Bug 50633] Make WebKitCSSMatrix more efficient for use in WebGL : [Attachment 96968] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 13 14:24:46 PDT 2011
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.
More information about the webkit-reviews
mailing list