WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Replacement patch
(43.69 KB, patch)
2009-01-14 16:00 PST
,
Chris Marrin
sam
: review+
Details
Formatted Diff
Diff
Patch to update Windows project file
(1.95 KB, patch)
2009-01-15 11:17 PST
,
Chris Marrin
mitz: review+
Details
Formatted Diff
Diff
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+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug