[webkit-reviews] review granted: [Bug 23943] Implement WebKitPoint and APIs to map points through transforms : [Attachment 27944] Patch with test

[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 23943: Implement WebKitPoint and APIs to map points through transforms
https://bugs.webkit.org/show_bug.cgi?id=23943

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

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

>			<File
> +				RelativePath="..\page\WebKitPoint.cpp"
> +				>
> +				<FileConfiguration

Don't include the FileConfiguration hunks

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

> +		494BD51E0F53721D00747828 /* WebKitPoint.idl in Resources */ =
{isa = PBXBuildFile; fileRef = 494BD51C0F53721C00747828 /* WebKitPoint.idl */;
};

Please remove the idl from Resources.

> Index: WebCore/bindings/js/JSWebKitPointConstructor.cpp
> ===================================================================
> --- WebCore/bindings/js/JSWebKitPointConstructor.cpp	(revision 0)
> +++ WebCore/bindings/js/JSWebKitPointConstructor.cpp	(revision 0)
> @@ -0,0 +1,61 @@
> +/*
> + * Copyright (C) 2006, 2007 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.
> + */

Apple license, not LGPL please.

> Index: WebCore/bindings/js/JSWebKitPointConstructor.h
> ===================================================================
> --- WebCore/bindings/js/JSWebKitPointConstructor.h	(revision 0)
> +++ WebCore/bindings/js/JSWebKitPointConstructor.h	(revision 0)
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright (C) 2006, 2007 Apple Inc.  All rights reserved.

2009

> Index: WebCore/page/DOMWindow.cpp
> ===================================================================

> +PassRefPtr<WebKitPoint> DOMWindow::webkitConvertPointFromNodeToPage(Node*
node, const WebKitPoint* p) const
> +{
> +    if (!node || !p)
> +	   return 0;
> +	   
> +    FloatPoint pp(p->x(), p->y());
> +    pp = node->convertToPage(pp);
> +    return WebKitPoint::create(pp.x(), pp.y());
> +}

Use pagePoint, or absolutePoint, not pp, for readability.


> +    FloatPoint pp(p->x(), p->y());
> +    pp = node->convertFromPage(pp);
> +    return WebKitPoint::create(pp.x(), pp.y());

localPoint, rather than pp.

> Index: WebCore/page/WebKitPoint.h
> ===================================================================
> --- WebCore/page/WebKitPoint.h	(revision 0)
> +++ WebCore/page/WebKitPoint.h	(revision 0)
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright (C) 2007 Apple Inc.  All rights reserved.

2009

> +    private:
> +	   WebKitPoint(float x=0, float y=0)

> +	   : m_x(x)
> +	   , m_y(y)

Indent these two lines


> Index: WebCore/page/WebKitPoint.idl
> ===================================================================

> + * Copyright (C) 2006, 2007, 2008 Apple Inc. All rights reserved.

2009

> +module window {
> +
> +    interface WebKitPoint {
> +	   attribute float x;
> +	   attribute float y;
> +    };

r=me with those fixes.