[webkit-reviews] review granted: [Bug 21421] Add WebKitCSSMatrix object : [Attachment 26735] Replacement patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Chris Marrin <cmarrin at apple.com>'s
request for review:
Bug 21421: Add WebKitCSSMatrix object
https://bugs.webkit.org/show_bug.cgi?id=21421

Attachment 26735: Replacement patch
https://bugs.webkit.org/attachment.cgi?id=26735&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
> +WebKitCSSMatrix::WebKitCSSMatrix(const String& s, ExceptionCode& ec) 
> +    : StyleBase(0)
> +{
> +    setMatrixValue(s, ec);
> +}

It is kind of odd to have an ExceptionCode in a constructor, though not
necessarily wrong.

> +		   ec = SYNTAX_ERR;
> +		   return;
> +	       }
> +	   }
> +	   
> +	   // set the matrix
> +	   m_matrix = t;
> +    } else
> +	   if (!string.isEmpty()) // there is something there but parsing
failed
> +	       ec = SYNTAX_ERR;

Please add braces around this else statement.

> +}
> +
> +// this is a multRight (this = this * secondMatrix)

"This" should be capitalized.

> +PassRefPtr<WebKitCSSMatrix> WebKitCSSMatrix::scale(float scaleX, float
scaleY)
> +{
> +    if (isnan(scaleX)) scaleX = 1; 
> +    if (isnan(scaleY)) scaleY = scaleX; 

The body of these if-statements should go on their own lines.

> +module css {
> +
> +    // Introduced in DOM Level ?:
> +    interface [
> +    ] WebKitCSSMatrix {

The [] are not needed.


> ===================================================================
> --- LayoutTests/ChangeLog	(revision 39918)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,25 @@
> +2009-01-14  Chris Marrin  <cmarrin at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Implemented 2D WebKitCSSMatrix
> +	   https://bugs.webkit.org/show_bug.cgi?id=21421
> +
> +	   * animations/combo-transform-translate+scale-expected.txt:
> +	   * animations/combo-transform-translate+scale.html:
> +	   * transforms/2d/cssmatrix-interface-expected.txt: Added.
> +	   * transforms/2d/cssmatrix-interface.xhtml: Added.
> +
> +2009-01-13  Chris Marrin  <cmarrin at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Testcase for implementation of WebKitCSSMatrix
> +	   https://bugs.webkit.org/show_bug.cgi?id=21421
> +
> +	   * animations/combo-transform-translate+scale-expected.txt: Added.
> +	   * animations/combo-transform-translate+scale.html: Added.


Extra changelog entry.


r=me.