[Webkit-unassigned] [Bug 28188] WINCE PORT: Path, PlatformPath

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=28188





--- Comment #2 from Adam Treat <treat at kde.org>  2009-08-12 06:57:52 PDT ---
(From update of attachment 34598)
> +Path& Path::operator=(const Path& other)
> +{
> +    m_path = new PlatformPath(*other.m_path);
> +    return *this;
> +}

Delete the possibly existing m_path unless &other == this?

> +bool Path::hasCurrentPoint() const
> +{
> +    return !isEmpty();
> +}

I see this is duplicated by most other ports, but not all.  Perhaps it should
be refactored and implemented in Path.cpp?  Maybe after this goes in?

About PathWince.cpp in general...  I'm curious why you don't just implement
methods here rather than breaking it out into another class: PlatformPathWince.
Any reason or just following other ports example of having a 'native'
PlatformPath type...  Not a biggy, but one extra level of indirection that
might not be necessary.

> +// Implemented in GraphicsContextWince.cpp
> +void getEllipsePointByAngle(double angle, double a, double b, float& x, float& y);
> +
...
> +static inline int stableRound(double d)
> +{
> +    if (d > 0)
> +        return static_cast<int>(d + 0.5);
> +
> +    int i = static_cast<int>(d);
> +    return i - d > 0.5 ? i - 1 : i;
> +}

This is also implemented in GraphicsContextWince.cpp.  Any reason this is
duplicated here?  I think this should probably be shared rather than
copy&paste...

> +static bool containsPoint(const FloatRect& r, const FloatPoint& p)
> +{
> +    return p.x() >= r.x() && p.y() >= r.y() && p.x() < r.right() && p.y() < r.bottom();
> +}

This should be added to FloatRect.

> +#define PI        3.14159265358979323846

This is a part of MathExtras.h.  Please use that instead of duplicating.

> +static void normalizeAngle(float& angle)
> +{
> +    while (angle < 0)
> +        angle += 2 * PI;
> +    while (angle >= 2 * PI)
> +        angle -= 2 * PI;
> +    if (angle < 0.00001)
> +        angle = 0;
> +}

This is used by addEllipse, but I'm not sure I understand.  If the angles given
are invalid, then why not just return or fix the calling site?

> +// return 0-based value: 0 - first Quadrant ( 0 - 90 degree)
> +static inline int quadrant(const PathPoint& point, const PathPoint& origPoint)
> +{
> +    return point.m_x < origPoint.m_x ?
> +        (point.m_y < origPoint.m_y ? 2 : 1)
> +        : (point.m_y < origPoint.m_y ? 3 : 0);
> +}

Heh, origin, originPoint, originalPoint?  I think all three are better than
'origPoint' depending on context :)

> +static void drawPolygons(HDC dc, const Vector<PathPolygon>& polygons, bool fill, const TransformationMatrix* tr)

tr should be matrix or transform.  Keep the variable names well named.

> +    MemoryAllocationCanFail canFail;

Will this compile?

> +void PlatformPathElement::inflateRectToContainMe(FloatRect& r, const FloatPoint& lastPoint) const

Cute :)

> +PlatformPath::PlatformPath()
> +: m_penLifted(true)

Indentation.

> +                // FIXME: magic number?
> +                quadCurve(50, m_subpaths.last(), control);

What does this mean?

> +}
> +
> +

Extra space.

> +#ifndef PlatformPathWince_h
> +#define PlatformPathWince_h

No includes?  How does this work?  Only forward declaration is GraphicsContext,
but you are using lots more WebCore classes...

General thoughts:

There are some style issues and minor nitpicks, but not very many.  I'm not
concerned about them as much as I'd like the questions above figured out.  I
would also like to know if you've run this code with the relevant LayoutTests
and what the results were.  You mention in the ChangeLog that this could become
cross-platform code for ports which do not have any native version of this
functionality.  That is possible, but only if the code is refactored and
probably broken out into one class per file as well as aggressive layout tests
confirm.  Finally, the diff looks corrupted: see top of patch file.  r- mostly
for the above questions.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.