[webkit-reviews] review granted: [Bug 23883] Add 3D functions and new TransformOperations to platform/transforms : [Attachment 27547] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 23883: Add 3D functions and new TransformOperations to platform/transforms
https://bugs.webkit.org/show_bug.cgi?id=23883

Attachment 27547: Patch
https://bugs.webkit.org/attachment.cgi?id=27547&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================

> Index: WebCore/WebCore.xcodeproj/project.pbxproj
> ===================================================================

Please run the WebCore.xcodeproj/project.pbxproj file through
WebKitTools/Scripts/sort-Xcode-project-file
before committing.

> Index: WebCore/platform/graphics/transforms/IdentityTransformOperation.h
> ===================================================================

>      virtual bool isIdentity() const { return true; }
> +    virtual bool isAffine() const { return true; }

We don't need isAffine(). You should remove it.

> Index: WebCore/platform/graphics/transforms/Matrix3DTransformOperation.cpp
> ===================================================================
> --- WebCore/platform/graphics/transforms/Matrix3DTransformOperation.cpp      
(revision 0)
> +++ WebCore/platform/graphics/transforms/Matrix3DTransformOperation.cpp      
(revision 0)
> @@ -0,0 +1,54 @@
> +/*
> + * Copyright (C) 1999 Antti Koivisto (koivisto at kde.org)
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights
reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License

> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + *
> + */

This is a new file; it should have the 2-clause apple license, not LGPL.

> +    if (blendToIdentity)
> +	   std::swap(fromT, toT);

I think the preferred way is to put a
using namespace std
near the top of the file, and skip the namespace here.

> Index: WebCore/platform/graphics/transforms/Matrix3DTransformOperation.h
> ===================================================================
> --- WebCore/platform/graphics/transforms/Matrix3DTransformOperation.h
(revision 0)
> +++ WebCore/platform/graphics/transforms/Matrix3DTransformOperation.h
(revision 0)
> @@ -0,0 +1,76 @@
> +/*
> + * Copyright (C) 2000 Lars Knoll (knoll at kde.org)
> + *		(C) 2000 Antti Koivisto (koivisto at kde.org)
> + *		(C) 2000 Dirk Mueller (mueller at kde.org)
> + * Copyright (C) 2003, 2005, 2006, 2007, 2008 Apple Inc. All rights
reserved.
> + * Copyright (C) 2006 Graham Dennis (graham.dennis at gmail.com)
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License

> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + *
> + */

Wrong license.


> +    virtual bool isAffine() const { return m_matrix.isAffine(); }

Remove.

> Index: WebCore/platform/graphics/transforms/MatrixTransformOperation.h

> +    virtual bool isAffine() const { return true; }

Remove

>      virtual bool apply(TransformationMatrix& transform, const IntSize&)
const
>      {
>	   TransformationMatrix matrix(m_a, m_b, m_c, m_d, m_e, m_f);
> -	   transform = matrix * transform;
> +	   transform.multLeft(TransformationMatrix(matrix));
>	   return false;

I wish it was easier to know what the return value meant.

> Index: WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp

> ===================================================================
> --- WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp   
(revision 0)
> +++ WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp   
(revision 0)
> @@ -0,0 +1,59 @@
> +/*
> + * Copyright (C) 1999 Antti Koivisto (koivisto at kde.org)
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights
reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License

> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + *
> + */

Wrong license.


> +PassRefPtr<TransformOperation> PerspectiveTransformOperation::blend(const
TransformOperation* from, double progress, bool blendToIdentity)
> +{

> +    if (blendToIdentity)
> +	   std::swap(fromP, toP);

Ditto re: swap().

> +    
> +    TransformationMatrix fromT;
> +    TransformationMatrix toT;
> +    fromT.applyPerspective((float) fromP);
> +    toT.applyPerspective((float) toP);

Don't cast to float.

> Index: WebCore/platform/graphics/transforms/PerspectiveTransformOperation.h
> ===================================================================
> --- WebCore/platform/graphics/transforms/PerspectiveTransformOperation.h     
(revision 0)
> +++ WebCore/platform/graphics/transforms/PerspectiveTransformOperation.h     
(revision 0)
> @@ -0,0 +1,75 @@
> +/*
> + * Copyright (C) 2000 Lars Knoll (knoll at kde.org)
> + *		(C) 2000 Antti Koivisto (koivisto at kde.org)
> + *		(C) 2000 Dirk Mueller (mueller at kde.org)
> + * Copyright (C) 2003, 2005, 2006, 2007, 2008 Apple Inc. All rights
reserved.
> + * Copyright (C) 2006 Graham Dennis (graham.dennis at gmail.com)
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License

> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + *
> + */

Wrong license.

> +private:
> +    virtual bool isIdentity() const { return m_p == 0; }
> +    virtual bool isAffine() const { return m_p == 0; }

Remove

> +    virtual bool apply(TransformationMatrix& transform, const IntSize&)
const
> +    {
> +	   transform.applyPerspective((float)m_p);
> +	   return false;

Remove cast.

> Index: WebCore/platform/graphics/transforms/RotateTransformOperation.cpp
> ===================================================================

> +    if (blendToIdentity)
> +	   std::swap(fromOp, toOp);

Ditto.

> +    
> +    // Convert that to Axis/Angle form
> +    float x = (float) -decomp.quaternionX;
> +    float y = (float) -decomp.quaternionY;
> +    float z = (float) -decomp.quaternionZ;
> +    float length = sqrtf(x * x + y * y + z * z);
> +    float angle = 0.0f;

Should this math be in doubles?

> Index: WebCore/platform/graphics/transforms/RotateTransformOperation.h
> ===================================================================

> +    virtual bool isAffine() const { return m_x == 0.0 && m_y == 0.0; }

Remove.

>      virtual bool apply(TransformationMatrix& transform, const IntSize&
/*borderBoxSize*/) const
>      {
> -	   transform.rotate(m_angle);
> +	   transform.rotate3d((float)m_x, (float)m_y, (float)m_z,
(float)m_angle);
>	   return false;

No need for casts.

> Index: WebCore/platform/graphics/transforms/ScaleTransformOperation.h
> ===================================================================

> +    virtual bool isAffine() const { return m_z == 1; }

Remove

> Index: WebCore/platform/graphics/transforms/SkewTransformOperation.h
> ===================================================================

> +    virtual bool isAffine() const { return true; }

Remove

> Index: WebCore/platform/graphics/transforms/TransformOperation.h
> ===================================================================

> +    virtual bool isAffine() const = 0;

Remove

> Index: WebCore/platform/graphics/transforms/TransformOperations.h
> ===================================================================

> +    // Returns true if the operations can all be represented as affine
transforms (even if
> +    // they use 3d operations)
> +    bool isAffine() const
> +    {
> +	   for (unsigned i = 0; i < m_operations.size(); ++i)
> +	       if (!m_operations[i]->isAffine())
> +		   return false;
> +	   return true;

Remove

> Index: WebCore/platform/graphics/transforms/TranslateTransformOperation.h
> ===================================================================

> +    virtual bool isAffine() const { return m_z.calcFloatValue(1) == 0; }
Remove

>      virtual bool apply(TransformationMatrix& transform, const IntSize&
borderBoxSize) const
>      {
> -	   transform.translate(m_x.calcFloatValue(borderBoxSize.width()),
m_y.calcFloatValue(borderBoxSize.height()));
> +	   transform.translate3d((float) x(borderBoxSize), (float)
y(borderBoxSize), (float) z(borderBoxSize));

No need for casting.


r=me with comments addressed.