Bug 28188 – WINCE PORT: Path, PlatformPath

• Component: Platform
• Status: RESOLVED FIXED

[Yong Li]

taken out from 27511

[Yong Li]

Created attachment 34598 [details]
the patch (modified a little)

modified a little bit. added some comments to contains()

[Adam Treat]

Comment on attachment 34598 [details]
the patch (modified a little)

> +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.

[Yong Li]

(In reply to comment #2)
> (From update of attachment 34598 [details])
> > +Path& Path::operator=(const Path& other)
> > +{
> > +    m_path = new PlatformPath(*other.m_path);
> > +    return *this;
> > +}
> 
> Delete the possibly existing m_path unless &other == this?
> 
I think you've found a bug here. Thanks! :)

> > +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?

No. There's at least one port that does differently. That's why this method was introduced. And I'm not sure !isEmpty() is absolutely correct. There have been some discussions on this thing. I forgot the bug number. At the meantime, I just do this as most ports do, because I haven't got a better idea. Keeping an eye on other ports... :)

> 
> 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.

I could do that. But then I would have to add a lot of members into Path.h with #if PLATFORM(WINCE), which may be harder to get r+ :)

> 
> > +// 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...

Yeah... This stableRound is defined in many files. Not sure if it's a good idea to add it into MathExtra.h.

The differences between stableRound() and lround() is:

1) stableRound() rounds -0.5 to 0, where lround() round -0.5 to -1. Consider the case that the transformation has a shift -1, and 0.5 changes to -0.5. With lround, the rounded value 1 changes to -1, so that the rounded shift is -2. Probably the name should be changed to lowerRound() or leftRound(). 

2) stableRound() doesn't call other function like ceil() where lround() does

Can we do this later? Because some landed file like GraphicsContextWince.cpp needs to be changed, too. This patch just contains new source files, and doesn't include the changes to any existing file.

> 
> > +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.

Agree with this. but can we do this later?

> 
> > +#define PI        3.14159265358979323846
> 
> This is a part of MathExtras.h.  Please use that instead of duplicating.

Agree.

> 
> > +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?

Don't know if an angle bigger than 2*PI is valid or invalid. I think it should still be handled.

> > +
> 
> 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...
>

Yeah, I'll add some header files like FloatPoint.h

[Yong Li]

Created attachment 34662 [details]
refactored

modified according to Adam's suggestions.

stableRound() is separated out to WinceGraphicsExtras.h

Afterward, we'll let other files that define stableRound() by themself use this header file.

[Adam Treat]

Comment on attachment 34662 [details]
refactored

Ok, please file a bug and patch for moving 'containsPoint' into FloatRect after this lands.  Thanks!

Otherwise, LGTM. r+ :)

[Yong Li]

Created attachment 34666 [details]
fix a bug in last patch

[Adam Treat]

Landed with r47118.