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

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Yong Li <yong.li at torchmobile.com>  2009-08-12 07:30:45 PDT ---
(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

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