RESOLVED FIXED 21421
Add WebKitCSSMatrix object
https://bugs.webkit.org/show_bug.cgi?id=21421
Summary Add WebKitCSSMatrix object
Chris Marrin
Reported 2008-10-06 17:47:17 PDT
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().
Attachments
Patch, including LayoutTest file (34.33 KB, patch)
2008-10-06 17:53 PDT, Chris Marrin
sam: review-
Replacement patch (43.69 KB, patch)
2009-01-14 16:00 PST, Chris Marrin
sam: review+
Patch to update Windows project file (1.95 KB, patch)
2009-01-15 11:17 PST, Chris Marrin
mitz: review+
patch to fix test case on ppc and get rid of idl file in Resources (3.58 KB, patch)
2009-01-15 17:30 PST, Chris Marrin
sam: review+
Chris Marrin
Comment 1 2008-10-06 17:53:25 PDT
Created attachment 24136 [details] Patch, including LayoutTest file
Eric Seidel (no email)
Comment 2 2008-10-07 01:50:25 PDT
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 3 2008-10-11 13:40:46 PDT
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 (knoll@kde.org) + * (C) 2002-2003 Dirk Mueller (mueller@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-
Chris Marrin
Comment 4 2009-01-14 16:00:35 PST
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 5 2009-01-14 16:27:22 PST
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 @@ > +2009-01-14 Chris Marrin <cmarrin@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@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.
Chris Marrin
Comment 6 2009-01-14 17:31:06 PST
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)
Comment 7 2009-01-14 18:52:40 PST
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)
Comment 8 2009-01-14 22:51:41 PST
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
Comment 9 2009-01-15 11:15:44 PST
Need to add Windows project updates
Chris Marrin
Comment 10 2009-01-15 11:17:43 PST
Created attachment 26763 [details] Patch to update Windows project file
Darin Adler
Comment 11 2009-01-15 11:19:48 PST
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
Comment 12 2009-01-15 14:45:24 PST
Sending WebCore/ChangeLog Sending WebCore/WebCore.vcproj/WebCore.vcproj Transmitting file data .. Committed revision 39939.
Chris Marrin
Comment 13 2009-01-15 17:30:42 PST
Created attachment 26779 [details] patch to fix test case on ppc and get rid of idl file in Resources
Chris Marrin
Comment 14 2009-01-16 07:09:43 PST
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.
Note You need to log in before you can comment on or make changes to this bug.