Bug 95526 – [chromium] Consolidate geometry unit testing functions for cc

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Consolidate geometry unit testing functions for cc

[James Robinson]

Created attachment 161619 [details]
Patch

[James Robinson]

This is a deceptively easy to review patch.  If you review it, I'll take you out for a free lunch *!


(*offer only applies to Googlers. Must provide own transporation to/from MTV and like BigTable)

[Shawn Singh]

Are you comfortable with using EXPECT_EQ instead of EXPECT_FLOAT_EQ?   EXPECT_FLOAT_EQ has some special semantics that use ulp thresholding instead of absolute thresholding.

That's why there was originally an EXPECT_FLOAT_RECT_EQ and EXPECT_INT_RECT_EQ ...

But I do agree that CCLayerTreeHost version of this macro is redundant.

[James Robinson]

(In reply to comment #3)
> Are you comfortable with using EXPECT_EQ instead of EXPECT_FLOAT_EQ?   EXPECT_FLOAT_EQ has some special semantics that use ulp thresholding instead of absolute thresholding.
> 
> That's why there was originally an EXPECT_FLOAT_RECT_EQ and EXPECT_INT_RECT_EQ ...
> 

The tests all pass with this, so it doesn't seem that this is important.  If it does become important we could make the assertions that need to be fuzzy fuzzy.

[Shawn Singh]

OK, well aside from that issue, I looked through it and it unofficially LGTM.  =)

It makes sense from the perspective that many floating-point operations are clamp/intersect/union, which just shuffle values around by assignment without affecting precision.  But as soon as we start transforming stuff...  (and considering the possibility of migrating to a float valued matrix sometime soon...) I am surprised that exact equality works!

[James Robinson]

Created attachment 161775 [details]
Patch

[James Robinson]

This leaves the float ones alone - no reason to change those, and it makes the patch a touch smaller.

[James Robinson]

Created attachment 161776 [details]
fix includes

[Shawn Singh]

Comment on attachment 161776 [details]
fix includes

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

> Source/WebKit/chromium/tests/CCGeometryTestUtils.h:41
> +#define EXPECT_RECT_EQ(expected, actual)                            \

For me personally, the reason I liked EXPECT_INT_RECT_EQ instead of EXPECT_RECT_EQ was because its a little easier to make us aware when we accidentally use floatRects/intRects inside of the wrong EXPECT macro.

Again, the patch unofficially LGTM other than this.

[James Robinson]

(In reply to comment #9)
> (From update of attachment 161776 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161776&action=review
> 
> > Source/WebKit/chromium/tests/CCGeometryTestUtils.h:41
> > +#define EXPECT_RECT_EQ(expected, actual)                            \
> 
> For me personally, the reason I liked EXPECT_INT_RECT_EQ instead of EXPECT_RECT_EQ was because its a little easier to make us aware when we accidentally use floatRects/intRects inside of the wrong EXPECT macro.
> 
> Again, the patch unofficially LGTM other than this.

Well, without this patch we were also using EXPECT_EQ_RECT all over the place too.  I kind of like EXPECT_RECT_EQ() for where you don't really care and EXPECT_FLOAT_RECT_EQ where you explicitly want fuzzy matching.  I think this is all temporary anyway since I anticipate we'll switch to different types and just write operator== so these will all turn into EXPECT_EQ() which does the right thing on the the types provided.

The motivation for this (beyond cleanup) is moving the matrix equality operator out of CCLayerTreeHostCommonTest.cpp because currently I can't get cc_unittests and webkit_compositor_unittests linking at all downstream, so I'd like to get that resolved ASAPly and follow up on any other cleanups we want later.

[Shawn Singh]

got it - thanks for explaining.  I'm good with this, we can move on.

So, a little ways down the road, are you saying you'd like to implement operator== as a fuzzy ulp error threshold equality --- in production code?   or just in unit test code?    Is there a reason to have a fuzzy equality check in production code?

[James Robinson]

(In reply to comment #11)
> got it - thanks for explaining.  I'm good with this, we can move on.
> 
> So, a little ways down the road, are you saying you'd like to implement operator== as a fuzzy ulp error threshold equality --- in production code?   or just in unit test code?    Is there a reason to have a fuzzy equality check in production code?

In dream land, I'd define an operator== only for testing that's fuzzy.  But I don't know if that is possible or not.  We don't currently have a need for operator== in production that I know of.

[Julien Chaffraix]

Comment on attachment 161776 [details]
fix includes

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

Provided it compiles on the EWS, r=me.

>>> Source/WebKit/chromium/tests/CCGeometryTestUtils.h:41
>>> +#define EXPECT_RECT_EQ(expected, actual)                            \
>> 
>> For me personally, the reason I liked EXPECT_INT_RECT_EQ instead of EXPECT_RECT_EQ was because its a little easier to make us aware when we accidentally use floatRects/intRects inside of the wrong EXPECT macro.
>> 
>> Again, the patch unofficially LGTM other than this.
> 
> Well, without this patch we were also using EXPECT_EQ_RECT all over the place too.  I kind of like EXPECT_RECT_EQ() for where you don't really care and EXPECT_FLOAT_RECT_EQ where you explicitly want fuzzy matching.  I think this is all temporary anyway since I anticipate we'll switch to different types and just write operator== so these will all turn into EXPECT_EQ() which does the right thing on the the types provided.
> 
> The motivation for this (beyond cleanup) is moving the matrix equality operator out of CCLayerTreeHostCommonTest.cpp because currently I can't get cc_unittests and webkit_compositor_unittests linking at all downstream, so I'd like to get that resolved ASAPly and follow up on any other cleanups we want later.

Macros don't give much guarantees that the arguments are IntRects so it's probably better to use EXPECT_RECT_EQ from that perspective. This is just my 2 cents from someone not involved as strictly in the compositor code.

[James Robinson]

Committed r127340: <http://trac.webkit.org/changeset/127340>