[webkit-reviews] review denied: [Bug 120381] [CSS Shapes] Redefine the ShapeIntervals class as a template : [Attachment 209980] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Aug 30 07:19:32 PDT 2013
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".
More information about the webkit-reviews
mailing list