[webkit-reviews] review denied: [Bug 72294] Add occludes() test to Region : [Attachment 115062] 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 occludes() test to Region
https://bugs.webkit.org/show_bug.cgi?id=72294

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

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


This is looking better, but there are still two major problems with this:

1. You should just make Region::contains take another Region - There's a Region
constructor that takes a rect.
2. Please implement contains in terms of intersects() instead of writing 60
lines of code. Like I said, the region code is very complex and we should
re-use the already existing region operations (union, intersection and
subtraction). I'd give m_segments an inline size of 32 and m_spans a size of 16
- that should cover most types of regions. Thanks for working on this!

> Source/WebCore/platform/graphics/Region.h:44
> +    // Returns true if the rect is contained fully in the areas covered by
the Region.
> +    bool contains(const IntRect&) const;

Please add a newline here.

> Source/WebKit/chromium/tests/RegionContainsTest.cpp:31
> +#include "Region.h"
> +

Extra newlines.