[webkit-reviews] review denied: [Bug 21421] Add WebKitCSSMatrix object : [Attachment 24136] Patch, including LayoutTest file

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied 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 24136: Patch, including LayoutTest file
https://bugs.webkit.org/attachment.cgi?id=24136&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>
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 (knoll at kde.org)
+ * (C) 2002-2003 Dirk Mueller (mueller at kde.org)
+ * 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-