Bug 21421 – Add WebKitCSSMatrix object

• Component: CSS
• Status: RESOLVED FIXED

[Chris Marrin]

This Javascript object takes the output of getComputedStyle().webkitTransform and makes it into a 2D matrix, with several operations. It gives access to the a-f components of the matrix and has methods to perform multiply(), inverse(), translate(), rotate(), and scale().

[Chris Marrin]

Created attachment 24136 [details]
Patch, including LayoutTest file

[Eric Seidel (no email)]

Comment on attachment 24136 [details]
Patch, including LayoutTest file

Oh yay.  Yet another matrix class. :(

I see some c-style casts in here (float) that should be static_cast or reinterpret_cast

I didn't look very closely at the patch except to notice that sadly we have yet another matrix class in JavaScript.

[Sam Weinig]

Comment on attachment 24136 [details]
Patch, including LayoutTest file

The license for JSWebKitCSSMatrixConstructor.cpp should be the two clause BSD license.  It can be found in ScrollbarThemeComposite.h for example.

+#include "Text.h"
Why is this included?

+    if (args.size() >= 1) {
+        s = args.at(exec, 0)->toString(exec);
+    }
Braces are not needed.

Why does the JSWebKitCSSMatrixConstructor store a JSDocument if it never seems to use it?  Did you intend to use it when constructing the WebKitCSSMatrix?  Perhaps to enforce strict parsing rules?

In WebKitCSSMatrix.cpp:
+ * This file is part of the DOM implementation for KDE.
+ *
+ * (C) 1999-2003 Lars Knoll ([e-mail])
+ * (C) 2002-2003 Dirk Mueller ([e-mail])
+ * Copyright (C) 2002, 2005, 2006 Apple Computer, Inc.
Why is this new file giving copyright to these individuals?

+WebKitCSSMatrix::WebKitCSSMatrix() : StyleBase(0)
The initialization list should have on item per line.

+WebKitCSSMatrix::WebKitCSSMatrix(const WebKitCSSMatrix& m) : StyleBase(0)
+{
+    m_matrix = m.m_matrix;
Same issue, and m_matrix can be initialized using initializer list syntax.

+WebKitCSSMatrix::WebKitCSSMatrix(const AffineTransform& m) : StyleBase(0)
+{
+    m_matrix = m;
Same here.

+    CSSParser p(useStrictParsing());
Since a parent is never passed to StyleBase, I think this is always the same.

+WebKitCSSMatrix* WebKitCSSMatrix::multiply(WebKitCSSMatrix* secondMatrix)
+{
+    AffineTransform tmp(secondMatrix->m_matrix);
+    tmp.multiply(m_matrix);
+    return new WebKitCSSMatrix(tmp);
+}
This should probably return a PassRefPtr and use WebKitCSSMatrix::create instead of new.

+WebKitCSSMatrix* WebKitCSSMatrix::inverse()
+{
+    return new WebKitCSSMatrix(m_matrix.inverse());
+}
Same here.

+WebKitCSSMatrix* WebKitCSSMatrix::translate(float x, float y)
And here.

+{
+    if (isnan(x)) x = 0; 
The x = 0 should be on its own line.

+    if (isnan(y)) y = 0; 
Here too.

+    return new WebKitCSSMatrix(AffineTransform(m_matrix).translate(x,y));
m_matrix is already an AffineTransform so I see no reason to call the constructor.  There is also a missing space after the x,.

+WebKitCSSMatrix* WebKitCSSMatrix::scale(float scaleX, float scaleY)
+{
+    if (isnan(scaleX)) scaleX = 1; 
+    if (isnan(scaleY)) scaleY = scaleX; 
+    return new WebKitCSSMatrix(AffineTransform(m_matrix).scale(scaleX,scaleY));
+}
Same issues as WebKitCSSMatrix::translate


+WebKitCSSMatrix* WebKitCSSMatrix::rotate(float rot)
+{
+    if (isnan(rot))
+        rot = 0;
+    return new WebKitCSSMatrix(AffineTransform(m_matrix).rotate(rot));
+}
No need to call the constructor and it should use PassRefPtr/create.

+typedef int ExceptionCode;
This typedef is not used.

+    float a() const { return (float) m_matrix.a(); }
+    float b() const { return (float) m_matrix.b(); }
+    float c() const { return (float) m_matrix.c(); }
+    float d() const { return (float) m_matrix.d(); }
+    float e() const { return (float) m_matrix.e(); }
+    float f() const { return (float) m_matrix.f(); }
Please use static_cast<float>.

+    void setA(float f)   { m_matrix.setA(f); }
+    void setB(float f)   { m_matrix.setB(f); }
+    void setC(float f)   { m_matrix.setC(f); }
+    void setD(float f)   { m_matrix.setD(f); }
+    void setE(float f)   { m_matrix.setE(f); }
+    void setF(float f)   { m_matrix.setF(f); }
The extra white space is not needed.

+    void                setMatrixValue(const String& string);
+    
+    // this = this * secondMatrix
+    WebKitCSSMatrix*    multiply(WebKitCSSMatrix* secondMatrix);
+    
+    // FIXME: we really should have an exception here, for when matrix is not invertible
+    WebKitCSSMatrix*    inverse();
+    WebKitCSSMatrix*    translate(float x, float y);
+    WebKitCSSMatrix*    scale(float scaleX, float scaleY);
+    WebKitCSSMatrix*    rotate(float rot);
+    
+    const AffineTransform&    transform() { return m_matrix; }
We usually don't put this kind of whitespace in.

+    WebKitCSSMatrix(const WebKitCSSMatrix& m);
+    WebKitCSSMatrix(const AffineTransform& m);
+    WebKitCSSMatrix(const String& s);
The parameter names are not needed here.

+    interface [
+        InterfaceUUID=349c989e-877f-416e-9cce-ca811e52d9e8,
+        ImplementationUUID=5feb16e6-f441-4b46-9b10-9e833a4dcd1b
+    ] WebKitCSSMatrix {
How were these UUIDs generated.  I think they are probably not necessary as we are not using this class in COM yet.

+        WebKitCSSMatrix multiply(in WebKitCSSMatrix secondMatrix) raises CSSException;
Since the c++ implementation does not raise an exception, this should not be here.  The reason this is probably not showing up is that the idl syntax we use for exceptions is             raises(CSSException); and the parser is lax enough to ignore this.

+        DOMString toString();
This should probably be [DontEnum]

Your test does not test the methods or attributes of the WebKitCSSMatrix object.  I am also curious why you chose floats instead of doubles for the type of the matrix values as it seems like this will cause a lot of unneeded casting as the AffineTransform is in terms of doubles as are JS numbers

r-

[Chris Marrin]

Created attachment 26735 [details]
Replacement patch

This addresses all the issues raised in the review. But I've left the types as floats since this matches some future functionality.

[Sam Weinig]

Comment on attachment 26735 [details]
Replacement patch

> +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 @@
> [phone]  Chris Marrin  <[e-mail]>
> +
> +        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.
> +
> [phone]  Chris Marrin  <[e-mail]>
> +
> +        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.

[Chris Marrin]

I made all the requested changes. But I removed the SVGMatrix test from the test case. Simon made the comment that this would cause problems when you build without SVG.

Sending        LayoutTests/ChangeLog
Sending        LayoutTests/animations/combo-transform-translate+scale-expected.txt
Sending        LayoutTests/animations/combo-transform-translate+scale.html
Adding         LayoutTests/transforms/2d/cssmatrix-interface-expected.txt
Adding         LayoutTests/transforms/2d/cssmatrix-interface.xhtml
Sending        WebCore/ChangeLog
Sending        WebCore/DerivedSources.make
Sending        WebCore/WebCore.xcodeproj/project.pbxproj
Sending        WebCore/bindings/js/JSDOMWindowBase.cpp
Adding         WebCore/bindings/js/JSWebKitCSSMatrixConstructor.cpp
Adding         WebCore/bindings/js/JSWebKitCSSMatrixConstructor.h
Sending        WebCore/bindings/objc/DOMInternal.h
Adding         WebCore/css/WebKitCSSMatrix.cpp
Adding         WebCore/css/WebKitCSSMatrix.h
Adding         WebCore/css/WebKitCSSMatrix.idl
Transmitting file data ...............
Committed revision 39922.

[David Kilzer (:ddkilzer)]

Fixed layout test results:

$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/dom/Window/window-properties-expected.txt
	M	LayoutTests/fast/js/global-constructors-expected.txt
Committed r39927

http://trac.webkit.org/changeset/39927

[Simon Fraser (smfr)]

Thanks Dave. Others also fixed:
wx build: http://trac.webkit.org/changeset/39929
debug mac build: http://trac.webkit.org/changeset/39926
gtk build: http://trac.webkit.org/changeset/39923

What about Windows?

[Chris Marrin]

Need to add Windows project updates

[Chris Marrin]

Created attachment 26763 [details]
Patch to update Windows project file

[Darin Adler]

Comment on attachment 26763 [details]
Patch to update Windows project file

It looks like this is a build fix, so doesn't require patch review; and we should get it in right away. r=me!

[Chris Marrin]

Sending        WebCore/ChangeLog
Sending        WebCore/WebCore.vcproj/WebCore.vcproj
Transmitting file data ..
Committed revision 39939.

[Chris Marrin]

Created attachment 26779 [details]
patch to fix test case on ppc and get rid of idl file in Resources

[Chris Marrin]

Sending        LayoutTests/ChangeLog
Sending        LayoutTests/transforms/2d/cssmatrix-interface.xhtml
Sending        WebCore/ChangeLog
Sending        WebCore/WebCore.xcodeproj/project.pbxproj
Transmitting file data ....
Committed revision 39968.