[webkit-reviews] review granted: [Bug 52255] Add a Region class which represents a graphical region : [Attachment 78607] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 11 15:35:07 PST 2011
Sam Weinig <sam at webkit.org> has granted Anders Carlsson <andersca at apple.com>'s
request for review:
Bug 52255: Add a Region class which represents a graphical region
https://bugs.webkit.org/show_bug.cgi?id=52255
Attachment 78607: Patch
https://bugs.webkit.org/attachment.cgi?id=78607&action=review
------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78607&action=review
We should consider moving this to WebCore at some point.
> WebKit2/Platform/Region.cpp:131
> + return m_spans.data() + m_spans.size();
This could be m_spans.end(). Same with begin.
> WebKit2/Platform/Region.cpp:137
> + assert(it >= m_spans.data());
> + assert(it < m_spans.data() + m_spans.size());
Wrong assert style.
> WebKit2/Platform/Region.cpp:149
Also wrong assert.
> WebKit2/Platform/Region.cpp:155
> + assert(it + 1 < m_spans.data() + m_spans.size());
Wrong assert.
> WebKit2/Platform/Region.cpp:170
> +void Region::Shape::dump() const
> +{
> + for (Shape::SpanIterator span = spans_begin(), end = spans_end(); span
!= end; ++span) {
> + printf("%6d: (", span->y);
> + for (Shape::SegmentIterator segment = segments_begin(span), end =
segments_end(span); segment != end; ++segment)
> + printf("%d ", *segment);
> + printf(")\n");
> + }
> +
> + printf("\n");
> +}
This should be debug only.
> WebKit2/Platform/Region.cpp:191
> + assert(firstSegment != lastSegment);
Wrong assert style.
> WebKit2/Platform/Region.cpp:204
> + assert(minX <= maxX);
> + assert(minY <= maxY);
Wrong assert style.
> WebKit2/Platform/Region.cpp:226
> +enum {
> + Shape1,
> + Shape2,
> +};
These are dead code.
> WebKit2/Platform/Region.cpp:231
> + // FIXME: These should be static asserts.
THEY ARE!
> WebKit2/Platform/Region.cpp:251
> + SegmentIterator segments2 = 0;
> + SegmentIterator segments2End = 0;
> +
> +
Extra new line.
> WebKit2/Platform/Region.cpp:278
> + std::vector<int> segments;
Please use WTF::Vector instead.
> WebKit2/Platform/Region.cpp:392
> + static const int opCode = 1;
This makes me sad. Name it!
More information about the webkit-reviews
mailing list