[webkit-reviews] review denied: [Bug 120381] [CSS Shapes] Redefine the ShapeIntervals class as a template : [Attachment 209980] Patch

[bugzilla-daemon at webkit.org]

Alexandru Chiculita <achicu at adobe.com> has denied Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 120381: [CSS Shapes] Redefine the ShapeIntervals class as a template
https://bugs.webkit.org/show_bug.cgi?id=120381

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

------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=209980&action=review


Thanks. There are a couple of changes that need to be done.

> Source/WebCore/rendering/shapes/PolygonShape.cpp:228
> +static inline bool appendIntervalX(float x, bool inside,
Vector<FloatShapeInterval>& result)

nit: might look better if you typedef Vector<FloatShapeInterval> to
FloatShapeIntervals. Maybe next patch as there were already a couple of changes
in this patch.

> Source/WebCore/rendering/shapes/PolygonShape.cpp:233
> +	   result.last().set(result.last().x1(), x);

nit: Why not just rersult.last().setX2(x) ?

> Source/WebCore/rendering/shapes/PolygonShape.cpp:400
> +	   FloatShapeInterval interval = includedIntervals[i];

nit: you may want to use const FloatShapeInterval& inteval =
includedIntervals[i]; to avoid the copy.

> Source/WebCore/rendering/shapes/ShapeInterval.h:62
> +    bool intersect(const ShapeInterval<T>& i, ShapeInterval<T>& rv) const

Use full names for the parameters.

I would also convert it to something like this:
if (a.overalps(b)) {
    ShapeInterval<T> intersection = a.intersect(b); // the assert inside the
constructor will prevent the usage when they don't overlap.
}

> Source/WebCore/rendering/shapes/ShapeInterval.h:64
> +	   if (x2() < i.x1() || x1() > i.x2())

it looks like this is just if (!overlaps(i)) return false. That would be less
code to read.

> Source/WebCore/rendering/shapes/ShapeInterval.h:72
> +    typedef Vector<ShapeInterval<T> > ShapeIntervalVector;
> +    typedef typename Vector<ShapeInterval<T> >::const_iterator
const_iterator;
> +    typedef typename Vector<ShapeInterval<T> >::iterator iterator;

You can typedef ShapeInterval<T> to avoid moving T all over the place.

> Source/WebCore/rendering/shapes/ShapeInterval.h:104
> +	       }

nit: This } is missplaced.

> Source/WebCore/rendering/shapes/ShapeInterval.h:173
> +		       rv.insert(i1, ShapeInterval(interval1.x1(),
interval2.x1()));

nit: You may want to use the appropriate type here. C++ matched it for you, but
I would rather use the typedef for ShapeInterval<T> to make it look the same
way.

> Source/WebCore/rendering/shapes/ShapeInterval.h:197
> +template<typename T> inline bool operator==(const ShapeInterval<T>& lhs,
const ShapeInterval<T>& rhs) { return lhs.x1() == rhs.x1() && lhs.x2() ==
rhs.x2(); }
> +template<typename T> inline bool operator!=(const ShapeInterval<T>& lhs,
const ShapeInterval<T>& rhs) { return !operator==(lhs, rhs); }
> +template<typename T> inline bool operator< (const ShapeInterval<T>& lhs,
const ShapeInterval<T>& rhs) { return lhs.x1() < rhs.x1(); }
> +template<typename T> inline bool operator> (const ShapeInterval<T>& lhs,
const ShapeInterval<T>& rhs) { return operator< (rhs, lhs); }
> +template<typename T> inline bool operator<=(const ShapeInterval<T>& lhs,
const ShapeInterval<T>& rhs) { return !operator> (lhs, rhs); }
> +template<typename T> inline bool operator>=(const ShapeInterval<T>& lhs,
const ShapeInterval<T>& rhs) { return !operator< (lhs, rhs); }

You can keep these inside the class definition to avoid all the template<>
stuff. Just compare "this" and "other".