Use EXPECT_EQ when possible.

Use EXPECT_EQ when possible.

EXPECT_RECT_EQ, EXPECT_SIZE_EQ, EXPECT_POINT_EQ, EXPECT_POINT3F_EQ
could be replaced by EXPECT_EQ.

Follow up of https://codereview.chromium.org/751453005/

Committed: https://crrev.com/6b61421f5b702f90617ffeff3ed83ab044c55ad9
Cr-Commit-Position: refs/heads/master@{#306913}

[hush (inactive), 2014-12-04 19:30:03]

hush@chromium.org changed reviewers:
+ danakj@chromium.org

[hush (inactive), 2014-12-04 19:30:04]

PTAL

If I understand correctly, EXPECT_EQ is templated and will call the operator==
of the compared objects eventually.
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

These are the low hanging fruits for now I think.

I can't replace EXPECT_FLOAT_RECT_EQ with EXPECT_EQ because float eq tolerates
float point imprecision.
I can't replace EXPECT_VECTOR_EQ with EXPECT_EQ because we use it to compare
ScrollOffset with Vector2D.

[danakj, 2014-12-04 19:34:29]

LGTM ! !

if there's no bug, remove the BUG= line please

https://codereview.chromium.org/774763004/diff/1/cc/test/geometry_test_utils.h
File cc/test/geometry_test_utils.h (left):

https://codereview.chromium.org/774763004/diff/1/cc/test/geometry_test_utils....
cc/test/geometry_test_utils.h:59: #define EXPECT_VECTOR_EQ(expected, actual) \
we could do these vector things too right?

[hush (inactive), 2014-12-04 20:10:18]

https://codereview.chromium.org/774763004/diff/1/cc/test/geometry_test_utils.h
File cc/test/geometry_test_utils.h (left):

https://codereview.chromium.org/774763004/diff/1/cc/test/geometry_test_utils....
cc/test/geometry_test_utils.h:52: #define EXPECT_POINT3F_EQ(expected, actual) \
On second thought, I need to keep EXPECT_POINT3F_EQ here as well. Using
EXPECT_EQ to compare POINT3F might fail due to floating point imprecision. We're
just lucky that the existing POINT3F comparisons are exactly equal.

https://codereview.chromium.org/774763004/diff/1/cc/test/geometry_test_utils....
cc/test/geometry_test_utils.h:59: #define EXPECT_VECTOR_EQ(expected, actual) \
On 2014/12/04 19:34:29, danakj wrote:
> we could do these vector things too right?

no... Right now EXPECT_VECTOR_EQ is treating |expected| and |actual| as
duck-typed objects. So we're using EXPECT_VECTOR_EQ quite liberally to compare
ScrollOffset against Vector2d, ScrollOffset against Size, ScrollOffset against
Rect, etc. This is pretty convenient.

If I replace it with EXPECT_EQ, I need to fix all the call sites to compare with
the same type of object, which is more hassle I think..

[danakj, 2014-12-04 20:13:19]

https://codereview.chromium.org/774763004/diff/1/cc/test/geometry_test_utils.h
File cc/test/geometry_test_utils.h (left):

https://codereview.chromium.org/774763004/diff/1/cc/test/geometry_test_utils....
cc/test/geometry_test_utils.h:52: #define EXPECT_POINT3F_EQ(expected, actual) \
On 2014/12/04 20:10:18, hush wrote:
> On second thought, I need to keep EXPECT_POINT3F_EQ here as well. Using
> EXPECT_EQ to compare POINT3F might fail due to floating point imprecision.
We're
> just lucky that the existing POINT3F comparisons are exactly equal.

I think it's fine to use EXPECT_EQ, and if a test really needs that inprecision
it can do EXPECT_FLOAT_EQ or something.

https://codereview.chromium.org/774763004/diff/1/cc/test/geometry_test_utils....
cc/test/geometry_test_utils.h:59: #define EXPECT_VECTOR_EQ(expected, actual) \
On 2014/12/04 20:10:18, hush wrote:
> On 2014/12/04 19:34:29, danakj wrote:
> > we could do these vector things too right?
> 
> no... Right now EXPECT_VECTOR_EQ is treating |expected| and |actual| as
> duck-typed objects. So we're using EXPECT_VECTOR_EQ quite liberally to compare
> ScrollOffset against Vector2d, ScrollOffset against Size, ScrollOffset against
> Rect, etc. This is pretty convenient.
> 
> If I replace it with EXPECT_EQ, I need to fix all the call sites to compare
with
> the same type of object, which is more hassle I think..

That sounds.. a bit weird. I'm guessing some places just got missed in the
conversion from Vector2d to ScrollOffset, but let's not do anything with that in
this CL.

[jamesr, 2014-12-04 20:21:50]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-12-04 20:21:50]

What does the failure output look like?

[hush (inactive), 2014-12-04 20:24:51]

On 2014/12/04 20:21:50, jamesr wrote:
> What does the failure output look like?

You mean after replacing EXPECT_VECTOR_EQ with EXPECT_EQ?

It is like this:
../../testing/gtest/include/gtest/gtest.h:1481:16: error: invalid operands to
binary expression ('const gfx::ScrollOffset' and 'const gfx::Vector2d')
  if (expected == actual) {
      ~~~~~~~~ ^  ~~~~~~
../../testing/gtest/include/gtest/gtest.h:1511:12: note: in instantiation of
function template specialization
'testing::internal::CmpHelperEQ<gfx::ScrollOffset, gfx::Vector2d>' requested
here
    return CmpHelperEQ(expected_expression, actual_expression, expected,
           ^
../../cc/trees/layer_tree_host_unittest_scroll.cc:427:9: note: in instantiation
of function template specialization
'testing::internal::EqHelper<false>::Compare<gfx::ScrollOffset, gfx::Vector2d>'
requested here
        EXPECT_EQ(scroll_layer->scroll_offset(), gfx::Vector2d(0, 0));
        ^
../../testing/gtest/include/gtest/gtest.h:2017:67: note: expanded from macro
'EXPECT_EQ'
                      EqHelper<GTEST_IS_NULL_LITERAL_(expected)>::Compare, \
                                                                  ^
../../testing/gtest/include/gtest/gtest_pred_impl.h:162:23: note: expanded from
macro 'EXPECT_PRED_FORMAT2'
  GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_)
                      ^
../../testing/gtest/include/gtest/gtest_pred_impl.h:147:17: note: expanded from
macro 'GTEST_PRED_FORMAT2_'
  GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \
                ^
../../testing/gtest/include/gtest/gtest_pred_impl.h:77:52: note: expanded from
macro 'GTEST_ASSERT_'
  if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                   ^
../../ui/gfx/geometry/vector2d.h:72:13: note: candidate function not viable: no
known conversion from 'const gfx::ScrollOffset' to 'const gfx::Vector2d' for 1st
argument
inline bool operator==(const Vector2d& lhs, const Vector2d& rhs) {
            ^
../../ui/gfx/geometry/vector2d_f.h:70:13: note: candidate function not viable:
no known conversion from 'const gfx::ScrollOffset' to 'const gfx::Vector2dF' for
1st argument
inline bool operator==(const Vector2dF& lhs, const Vector2dF& rhs) {
            ^
../../ui/gfx/geometry/scroll_offset.h:78:13: note: candidate function not
viable: no known conversion from 'const gfx::Vector2d' to 'const
gfx::ScrollOffset' for 2nd argument
inline bool operator==(const ScrollOffset& lhs, const ScrollOffset& rhs) {
            ^
../../ui/gfx/geometry/point_f.h:81:13: note: candidate function not viable: no
known conversion from 'const gfx::ScrollOffset' to 'const gfx::PointF' for 1st
argument
inline bool operator==(const PointF& lhs, const PointF& rhs) {
            ^
../../ui/gfx/geometry/point.h:104:13: note: candidate function not viable: no
known conversion from 'const gfx::ScrollOffset' to 'const gfx::Point' for 1st
argument
inline bool operator==(const Point& lhs, const Point& rhs) {
            ^
../../ui/gfx/geometry/size_f.h:60:13: note: candidate function not viable: no
known conversion from 'const gfx::ScrollOffset' to 'const gfx::SizeF' for 1st
argument
inline bool operator==(const SizeF& lhs, const SizeF& rhs) {
            ^
../../ui/gfx/geometry/rect_f.h:176:13: note: candidate function not viable: no
known conversion from 'const gfx::ScrollOffset' to 'const gfx::RectF' for 1st
argument
inline bool operator==(const RectF& lhs, const RectF& rhs) {
            ^
../../ui/gfx/geometry/size.h:80:13: note: candidate function not viable: no
known conversion from 'const gfx::ScrollOffset' to 'const gfx::Size' for 1st
argument
inline bool operator==(const Size& lhs, const Size& rhs) {
            ^
../../ui/gfx/geometry/rect.h:195:13: note: candidate function not viable: no
known conversion from 'const gfx::ScrollOffset' to 'const gfx::Rect' for 1st
argument
inline bool operator==(const Rect& lhs, const Rect& rhs) {
            ^
../../ui/gfx/geometry/vector3d_f.h:81:13: note: candidate function not viable:
no known conversion from 'const gfx::ScrollOffset' to 'const gfx::Vector3dF' for
1st argument
inline bool operator==(const Vector3dF& lhs, const Vector3dF& rhs) {
            ^
../../ui/gfx/geometry/point3_f.h:85:13: note: candidate function not viable: no
known conversion from 'const gfx::ScrollOffset' to 'const gfx::Point3F' for 1st
argument
inline bool operator==(const Point3F& lhs, const Point3F& rhs) {
            ^
../../testing/gtest/include/gtest/internal/gtest-linked_ptr.h:213:6: note:
candidate template ignored: could not match 'T *' against 'gfx::ScrollOffset'

[jamesr, 2014-12-04 20:44:38]

No, I mean if the rect doesn't actually match what does the failure output of

  EXPECT_EQ(gfx::Rect(...), actual_value)

look like?  Do you get the correct strinigification of the gfx::Rect or not? 
Iirc that was the motivation for adding the extra macros

[jamesr, 2014-12-04 21:12:37]

Ah, Dana says this'll work thanks to the ToString() functions in gfx (which I
added forever ago but forgot the details of).  Cool :)

[hush (inactive), 2014-12-04 21:43:09]

On 2014/12/04 21:12:37, jamesr wrote:
> Ah, Dana says this'll work thanks to the ToString() functions in gfx (which I
> added forever ago but forgot the details of).  Cool :)

Yeah, if the object supports toString(), then fine.
It looks like this:
Actual: 0,0 0x0
Expected: expected
Which is: 0,0 1x1

[hush (inactive), 2014-12-04 21:43:17]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2014-12-04 21:44:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/774763004/40001

[jamesr, 2014-12-04 21:49:46]

On 2014/12/04 21:43:09, hush wrote:
> On 2014/12/04 21:12:37, jamesr wrote:
> > Ah, Dana says this'll work thanks to the ToString() functions in gfx (which
I
> > added forever ago but forgot the details of).  Cool :)
> 
> Yeah, if the object supports toString(), then fine.
> It looks like this:
> Actual: 0,0 0x0
> Expected: expected
> Which is: 0,0 1x1

Yup.  I added the ToString()s for these types a while back and then forgot about
it.

[commit-bot: I haz the power, 2014-12-04 22:37:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-04 22:38:34]

Patchset 3 (id:??) landed as
https://crrev.com/6b61421f5b702f90617ffeff3ed83ab044c55ad9
Cr-Commit-Position: refs/heads/master@{#306913}