[webkit-reviews] review denied: [Bug 72294] Add contains() test to Region : [Attachment 115172] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 72294: Add contains() test to Region
https://bugs.webkit.org/show_bug.cgi?id=72294

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

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=115172&action=review


This is looking much better! It looks like it will break the Windows build
though.

> Source/WebCore/platform/graphics/Region.cpp:72
> +    Region test(subset);
> +    test.intersect(*this);
> +    return test == subset;

I think this can be written more as:

return intersect(*this, subset) == subset;

(And I'd rename subset to region).

> Source/WebCore/platform/graphics/Region.cpp:103
> +    Shape a = m_shape;
> +    Shape b = other.m_shape;
> +
> +    Shape::SpanIterator aSpan = a.spans_begin();
> +    Shape::SpanIterator bSpan = b.spans_begin();
> +    const Shape::SpanIterator aSpanEnd = a.spans_end();
> +    const Shape::SpanIterator bSpanEnd = b.spans_end();
> +
> +    if (aSpanEnd - aSpan != bSpanEnd - bSpan) // Test for equal number of
spans
> +	   return false;
> +
> +    for (; aSpan != aSpanEnd; ++aSpan, ++bSpan) {
> +	   if (aSpan->y != bSpan->y) // Test if spans are at same position
> +	       return false;
> +
> +	   Shape::SegmentIterator aSegment = a.segments_begin(aSpan);
> +	   Shape::SegmentIterator bSegment = b.segments_begin(bSpan);
> +	   const Shape::SegmentIterator aSegmentEnd = a.segments_end(aSpan);
> +	   const Shape::SegmentIterator bSegmentEnd = b.segments_end(bSpan);
> +
> +	   if (aSegmentEnd - aSegment != bSegmentEnd - bSegment) // Test for
equal number of segments
> +	       return false;
> +	   if (!std::equal(aSegment, aSegmentEnd, bSegment)) // Test if
segments are at same positions
> +	       return false;
> +    }
> +
> +    return true;

If you write an operator== for Shape, I think this can become much simpler.