Bug 89926 – [chromium] Create WebKit::WebRegion wrapper for WebCore::Region

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Shawn Singh]

Patch coming soon, including (a) the new WebRegion class *AND* (b) the actual migration itself.   I'm happy to split them into separate patches, but I had to implement both parts to test it fully, anyway.  Just let me know if you prefer to split it.

There are some unit tests covering WebCore::Region already; personally I feel comfortable enough that this layer of testing is enough for migrating.   But when we get around to implementing the non-webkit underlying implementation, we will want to make sure we have more complete testing in place.

[Shawn Singh]

Created attachment 149408 [details]
Patch

Tested in both debug and release: manual, unit tests, and layout tests, on OS X, no obvious regressions.  (Technically I'm still running Debug layout tests, but it's going well so far.)

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Comment on attachment 149408 [details]
Patch

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

Nice!  I like this a lot and agree that doing it in one shot is fine. Left a lot of stylistic comments on the shape of the WebRegion API itself.

> Source/Platform/chromium/public/WebRegion.h:40
> +#if WEBKIT_IMPLEMENTATION
> +#include "FloatPoint.h"
> +#include "FloatPoint3D.h"
> +#include "FloatQuad.h"
> +#include "Region.h"
> +#include "TransformationMatrix.h"
> +#endif

I don't think you need any of these - just forward declare WebCore::Region (and IntRect, and IntPoint if you need them)

> Source/Platform/chromium/public/WebRegion.h:49
> +    WebRegion();

it's more typical in WebKit API to define the c'tor inline and defer any initialization work to a WEBKIT_EXPORT'd function

> Source/Platform/chromium/public/WebRegion.h:50
> +    WebRegion(const WebRegion&);

for these, we often have an assign() to do the copy and define the actual c'tor inline

> Source/Platform/chromium/public/WebRegion.h:51
> +    WebRegion(const WebCore::IntRect&);

this needs to be guarded, or even better be defined in terms of WebRect

> Source/Platform/chromium/public/WebRegion.h:54
> +    void reset();

WEBKIT_EXPORT on exported functions (i.e. everything that's not defined inline)

> Source/Platform/chromium/public/WebRegion.h:58
> +    WebCore::IntRect bounds() const;

could this be WebRect?

> Source/Platform/chromium/public/WebRegion.h:61
> +    Vector<WebCore::IntRect> rects() const;

what Vector type is this? could this be WebVector<WebRect> ?

> Source/Platform/chromium/public/WebRegion.h:68
> +    bool contains(const WebCore::IntPoint&) const;

this version needs to be compile guarded or take a WebPoint

> Source/Platform/chromium/public/WebRegion.h:75
> +protected:
> +    friend bool operator==(const WebRegion&, const WebRegion&);

hm, why isn't this just a public function - do we not need to compare two regions for equality?

it's a bit unusual to see operator== without operator!=. operator!= is often implemented as just !(a == b)

> Source/Platform/chromium/public/WebRegion.h:80
> +    // While migrating this code: Code that is external to WebKit should have no knowledge
> +    // of WebCore::WebRegion. Because of the underlying implementation of WebCore::Region
> +    // (which dynamically allocates things) we have to use a pointer here to support
> +    // different implementations between WebKit and non-WebKit code.

It's OK for non-WEBKIT_IMPLEMENTATION code to see a WebPrivateOwnPtr<WebCore::Region> in this header, it won't be able to dereference it since it only has a forward decl of the type but that's OK

> Source/Platform/chromium/public/WebRegion.h:82
> +    WebPrivateOwnPtr<WebCore::Region> m_private;

OwnPtr, or do we want to be able to share refs to an underlying region (threadsafe refcount if we pass across threads)? how often do we copy these guys?

> Source/Platform/chromium/public/WebRegion.h:89
> +COMPILE_ASSERT(sizeof(WebPrivateOwnPtr<WebCore::Region>) == sizeof(WebPrivateOwnPtr<int>), WebRegion_has_unexpected_size);

I don't think this is needed - we rely on WebPrivateOwnPtr<T> to be the same size for all types T in many places in the code

> Source/Platform/chromium/public/WebRegion.h:106
> +bool operator==(const WebRegion&, const WebRegion&);

normally in WebKit API we define and export an equals() function and define this operator inline in the header that defers to the equals()

> Source/WebCore/platform/chromium/support/WebRegion.cpp:50
> +#if WEBKIT_IMPLEMENTATION

this whole .cpp is WEBKIT_IMPLEMENTATION, no need for the #if guard

[James Robinson]

Comment on attachment 149408 [details]
Patch

R- for the API stuff, but the implementation code all looks good far as I can tell.

[Shawn Singh]

Cool thanks  - I'll submit a new patch by tomorrow at the latest.

[Dana Jansens]

Comment on attachment 149408 [details]
Patch

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

nice one

>> Source/Platform/chromium/public/WebRegion.h:61
>> +    Vector<WebCore::IntRect> rects() const;
> 
> what Vector type is this? could this be WebVector<WebRect> ?

probably need to expand WebVector to hold a Vector in order to implement that without a copy? (please don't introduce a copy :))

[Shawn Singh]

(In reply to comment #6)
> (From update of attachment 149408 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149408&action=review
> 
> nice one
> 
> >> Source/Platform/chromium/public/WebRegion.h:61
> >> +    Vector<WebCore::IntRect> rects() const;
> > 
> > what Vector type is this? could this be WebVector<WebRect> ?
> 
> probably need to expand WebVector to hold a Vector in order to implement that without a copy? (please don't introduce a copy :))

The original code is already doing a copy when accessing rects().

grepping for "rects\(" shows:

./cc/CCOcclusionTracker.cpp:154:    Vector<IntRect> rects = region.rects();
./cc/CCOcclusionTracker.cpp:171:    Vector<IntRect> affectedOcclusionRects = affectedOcclusion.rects();
./cc/CCOcclusionTracker.cpp:322:    Vector<IntRect> contentRects = opaqueContents.rects();
./cc/CCRenderPass.cpp:98:    Vector<IntRect> fillRects = fillRegion.rects();
./TiledLayerChromium.cpp:160:    Vector<IntRect> rects = newRegion.rects();
... and a bunch of simple uses in unit tests.

It looks to me like all cases that use the rects() accessor only look at the Vector read-only.  Ideally we could return by reference, but since we're wrapping a different data type, perhaps its best to do the following instead:

(1) make WebCore::Region::rects() return by const reference

(2) use the following wrappers on WebRegion:
  WebRect WebRegion::getRect(int index) { return m_private vector [index]; }
  int WebRegion::numRects() { return m_private vector size; }

What do you guys think?

[Dana Jansens]

Comment on attachment 149408 [details]
Patch

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

>>>> Source/Platform/chromium/public/WebRegion.h:61
>>>> +    Vector<WebCore::IntRect> rects() const;
>>> 
>>> what Vector type is this? could this be WebVector<WebRect> ?
>> 
>> probably need to expand WebVector to hold a Vector in order to implement that without a copy? (please don't introduce a copy :))
> 
> The original code is already doing a copy when accessing rects().
> 
> grepping for "rects\(" shows:
> 
> ./cc/CCOcclusionTracker.cpp:154:    Vector<IntRect> rects = region.rects();
> ./cc/CCOcclusionTracker.cpp:171:    Vector<IntRect> affectedOcclusionRects = affectedOcclusion.rects();
> ./cc/CCOcclusionTracker.cpp:322:    Vector<IntRect> contentRects = opaqueContents.rects();
> ./cc/CCRenderPass.cpp:98:    Vector<IntRect> fillRects = fillRegion.rects();
> ./TiledLayerChromium.cpp:160:    Vector<IntRect> rects = newRegion.rects();
> ... and a bunch of simple uses in unit tests.
> 
> It looks to me like all cases that use the rects() accessor only look at the Vector read-only.  Ideally we could return by reference, but since we're wrapping a different data type, perhaps its best to do the following instead:
> 
> (1) make WebCore::Region::rects() return by const reference
> 
> (2) use the following wrappers on WebRegion:
>   WebRect WebRegion::getRect(int index) { return m_private vector [index]; }
>   int WebRegion::numRects() { return m_private vector size; }
> 
> What do you guys think?

Ya the returned rect now is created by the function, just voting for no more intermediate copies. I do think this is a good idea, however..

SkRegion actually has an iterator over the rects within it, instead of creating a Vector. Perhaps making an iterator returned by rectsBegin() and rectsEnd() would be better, as it could wrap the SkRegion iterator also in future? The numRects() might be problematic in the SkRegion case, I don't see a way to query this number in its API.

[Shawn Singh]

(In reply to comment #8)
> (From update of attachment 149408 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149408&action=review
> 
> >>>> Source/Platform/chromium/public/WebRegion.h:61
> >>>> +    Vector<WebCore::IntRect> rects() const;
> >>> 
> >>> what Vector type is this? could this be WebVector<WebRect> ?
> >> 
> >> probably need to expand WebVector to hold a Vector in order to implement that without a copy? (please don't introduce a copy :))
> > 
> > The original code is already doing a copy when accessing rects().
> > 
> > grepping for "rects\(" shows:
> > 
> > ./cc/CCOcclusionTracker.cpp:154:    Vector<IntRect> rects = region.rects();
> > ./cc/CCOcclusionTracker.cpp:171:    Vector<IntRect> affectedOcclusionRects = affectedOcclusion.rects();
> > ./cc/CCOcclusionTracker.cpp:322:    Vector<IntRect> contentRects = opaqueContents.rects();
> > ./cc/CCRenderPass.cpp:98:    Vector<IntRect> fillRects = fillRegion.rects();
> > ./TiledLayerChromium.cpp:160:    Vector<IntRect> rects = newRegion.rects();
> > ... and a bunch of simple uses in unit tests.
> > 
> > It looks to me like all cases that use the rects() accessor only look at the Vector read-only.  Ideally we could return by reference, but since we're wrapping a different data type, perhaps its best to do the following instead:
> > 
> > (1) make WebCore::Region::rects() return by const reference
> > 
> > (2) use the following wrappers on WebRegion:
> >   WebRect WebRegion::getRect(int index) { return m_private vector [index]; }
> >   int WebRegion::numRects() { return m_private vector size; }
> > 
> > What do you guys think?
> 
> Ya the returned rect now is created by the function, just voting for no more intermediate copies. I do think this is a good idea, however..
> 
> SkRegion actually has an iterator over the rects within it, instead of creating a Vector. Perhaps making an iterator returned by rectsBegin() and rectsEnd() would be better, as it could wrap the SkRegion iterator also in future? The numRects() might be problematic in the SkRegion case, I don't see a way to query this number in its API.


Sounds good - would it be OK if we do the iterator part in a separate patch, once we really do implement an SkRegion back-end?

[Dana Jansens]

Comment on attachment 149408 [details]
Patch

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

>>>>>> Source/Platform/chromium/public/WebRegion.h:61
>>>>>> +    Vector<WebCore::IntRect> rects() const;
>>>>> 
>>>>> what Vector type is this? could this be WebVector<WebRect> ?
>>>> 
>>>> probably need to expand WebVector to hold a Vector in order to implement that without a copy? (please don't introduce a copy :))
>>> 
>>> The original code is already doing a copy when accessing rects().
>>> 
>>> grepping for "rects\(" shows:
>>> 
>>> ./cc/CCOcclusionTracker.cpp:154:    Vector<IntRect> rects = region.rects();
>>> ./cc/CCOcclusionTracker.cpp:171:    Vector<IntRect> affectedOcclusionRects = affectedOcclusion.rects();
>>> ./cc/CCOcclusionTracker.cpp:322:    Vector<IntRect> contentRects = opaqueContents.rects();
>>> ./cc/CCRenderPass.cpp:98:    Vector<IntRect> fillRects = fillRegion.rects();
>>> ./TiledLayerChromium.cpp:160:    Vector<IntRect> rects = newRegion.rects();
>>> ... and a bunch of simple uses in unit tests.
>>> 
>>> It looks to me like all cases that use the rects() accessor only look at the Vector read-only.  Ideally we could return by reference, but since we're wrapping a different data type, perhaps its best to do the following instead:
>>> 
>>> (1) make WebCore::Region::rects() return by const reference
>>> 
>>> (2) use the following wrappers on WebRegion:
>>>   WebRect WebRegion::getRect(int index) { return m_private vector [index]; }
>>>   int WebRegion::numRects() { return m_private vector size; }
>>> 
>>> What do you guys think?
>> 
>> Ya the returned rect now is created by the function, just voting for no more intermediate copies. I do think this is a good idea, however..
>> 
>> SkRegion actually has an iterator over the rects within it, instead of creating a Vector. Perhaps making an iterator returned by rectsBegin() and rectsEnd() would be better, as it could wrap the SkRegion iterator also in future? The numRects() might be problematic in the SkRegion case, I don't see a way to query this number in its API.
> 
> Sounds good - would it be OK if we do the iterator part in a separate patch, once we really do implement an SkRegion back-end?

As long as changing/breaking this API at that time is fine, I don't mind.

[Shawn Singh]

Update:

(1) WebCore::Region procedurally computes the vector of IntRects, so we can't make a const reference return value.   oh well....

(2) Also we need to define WEBKIT_IMPLEMENTATION to make the WebRect / WebPoint conversions clean and automatic, but it turns out that webkit_unit_tests does not compile successfully yet with that define.  I think its cleaner to address that in a separate patch before coming back to this WebRegion migration here.

[Shawn Singh]

This bug is obsolete now... marking as wontfix.