Bug 120381 – [CSS Shapes] Redefine the ShapeIntervals class as a template

• Component: CSS
• Status: RESOLVED FIXED

[Hans Muller]

To enable PolygonShape and RasterShape to use the ShapeInterval set operations we need to support ShapeInterval<float> and ShapeInterval<int>.

See https://bugs.webkit.org/show_bug.cgi?id=120211 for more information .

[Hans Muller]

Created attachment 209924 [details]
Patch

The existing ShapeIntervals class has been converted into a template whose type specifies the type of the interval's x1 and x2 horizontal endpoints (formerly float). There were several other minor changes, all in the realm of refactoring:

- The original type was a struct with public x1 and x2 fields. It's now a class with x1 and x2 accessors. ASSERTS are now used to maintain the x2 >= x1 invariant.  In the original code the invariant was not checked.

- The logical comparison operators have been overloaded for ShapeInterval. This obviates the IntervalX1Comparator class which has been removed.

- The names of the global ShapeInterval Vector set operation methods have been changed to reflect the fact that they're now members of the template class, rather than globals.

PolygonShape.cpp depended on the ShapeInterval class. In the one or two places where an interval's x1 or x1 fields had been set explicitly, ShapeInterval::set() is now used to set both fields. This also enables the invariant check mentioned earlier. The other changes to this class are syntatic, to account for the ShapeInterval class's changes.

[Early Warning System Bot]

Comment on attachment 209924 [details]
Patch

Attachment 209924 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1630015

[Hans Muller]

Created attachment 209926 [details]
Patch

Updated Target.pri for Qt build.

[Build Bot]

Comment on attachment 209926 [details]
Patch

Attachment 209926 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1628408

[Hans Muller]

Created attachment 209933 [details]
Patch.

Hopefully correct the win build problem by removing the ShapeInterval.cpp entry from Source/WebCore/WebCore.vcxproj.filters.

[Hans Muller]

Created attachment 209980 [details]
Patch

Restored the three mysterious bytes at the top of WebCore.vcxproj.filters.

[Alexandru Chiculita]

Comment on attachment 209980 [details]
Patch

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

[Alexandru Chiculita]

Comment on attachment 209980 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209980&action=review

> Source/WebCore/rendering/shapes/ShapeInterval.h:102
> +            if (interval)

I wonder when is the case when you don't have an interval != 0 here. "v" has the size of v1 + v2, which are both non-zero, so it should always have one.

Maybe you could rewrite the loop to start from 1 instead and make sure that interval always starts at v[0].

[Hans Muller]

Created attachment 210132 [details]
Patch

I've made all of the suggested changes, except the ones that affect code which is just going to be replaced by https://bugs.webkit.org/show_bug.cgi?id=120439.

[Hans Muller]

Created attachment 210134 [details]
Patch

Added one more requested change that I'd missed in the previous patch.

[Alexandru Chiculita]

Comment on attachment 210134 [details]
Patch

Looks good.

[WebKit Commit Bot]

Comment on attachment 210134 [details]
Patch

Clearing flags on attachment: 210134

Committed r154904: <http://trac.webkit.org/changeset/154904>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.