Add tests where we split near an edge.

Add tests where we split near an edge.

This exercises some corner cases where we have an edge
along the thick plane by which we are splitting.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/b5ec6f946eafcefff83e44d5e3b051c7f0fda814
Cr-Commit-Position: refs/heads/master@{#426244}

[Peter Mayo, 2016-10-12 21:56:30]

Description was changed from

==========
Add tests where we split near an edge.

This exercises some corner casesi where we have an edge
along the thick plane by which we are splitting.

TEST=unittests, both Debug and Release.
BUG=
==========

to

==========
Add tests where we split near an edge.

This exercises some corner casesi where we have an edge
along the thick plane by which we are splitting.

TEST=unittests, both Debug and Release.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Peter Mayo, 2016-10-12 22:02:37]

petermayo@chromium.org changed reviewers:
+ flackr@chromium.org

[Peter Mayo, 2016-10-12 22:02:38]

Wait for failing debug try runs ...

[flackr, 2016-10-13 18:42:26]

Thanks for coming up with a good test. This assert has come up on my WIP fix for
https://crbug.com/626095 but I was sure that I had well formed polygons.

https://codereview.chromium.org/2408203005/diff/20001/cc/quads/draw_polygon_u...
File cc/quads/draw_polygon_unittest.cc (right):

https://codereview.chromium.org/2408203005/diff/20001/cc/quads/draw_polygon_u...
cc/quads/draw_polygon_unittest.cc:379: #if !defined(OS_WIN)
I agree this ensures that the polygons are indeed well formed - but I'm confused
how CREATE_TEST_DRAW_FORWARD_POLYGON works or is excluded on windows?

[flackr, 2016-10-13 18:49:30]

lgtm

https://codereview.chromium.org/2408203005/diff/20001/cc/quads/draw_polygon_u...
File cc/quads/draw_polygon_unittest.cc (right):

https://codereview.chromium.org/2408203005/diff/20001/cc/quads/draw_polygon_u...
cc/quads/draw_polygon_unittest.cc:379: #if !defined(OS_WIN)
On 2016/10/13 at 18:42:26, flackr wrote:
> I agree this ensures that the polygons are indeed well formed - but I'm
confused how CREATE_TEST_DRAW_FORWARD_POLYGON works or is excluded on windows?

Nevermind, thanks for pointing out we declare
DrawPolygon::RecomputeNormalForTesting in DrawPolygon on Windows. This seems
fine to not do the same here given the increased amount of code.

[Peter Mayo, 2016-10-13 22:32:32]

The CQ bit was checked by petermayo@chromium.org

[Peter Mayo, 2016-10-13 22:32:33]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org
Link to the patchset: https://codereview.chromium.org/2408203005/#ps60001
(title: "Fix Windows truncation error")

[commit-bot: I haz the power, 2016-10-13 22:32:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 22:43:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 22:43:51]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Mayo, 2016-10-13 23:02:21]

petermayo@chromium.org changed reviewers:
+ enne@chromium.org

[Peter Mayo, 2016-10-13 23:02:21]

PT An Owner's Look

[enne (OOO), 2016-10-14 17:54:54]

Description was changed from

==========
Add tests where we split near an edge.

This exercises some corner casesi where we have an edge
along the thick plane by which we are splitting.

TEST=unittests, both Debug and Release.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Add tests where we split near an edge.

This exercises some corner cases where we have an edge
along the thick plane by which we are splitting.

TEST=unittests, both Debug and Release.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[enne (OOO), 2016-10-14 17:55:01]

Description was changed from

==========
Add tests where we split near an edge.

This exercises some corner cases where we have an edge
along the thick plane by which we are splitting.

TEST=unittests, both Debug and Release.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Add tests where we split near an edge.

This exercises some corner cases where we have an edge
along the thick plane by which we are splitting.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[enne (OOO), 2016-10-14 18:04:59]

Seems reasonable, except

https://codereview.chromium.org/2408203005/diff/60001/cc/quads/draw_polygon_u...
File cc/quads/draw_polygon_unittest.cc (right):

https://codereview.chromium.org/2408203005/diff/60001/cc/quads/draw_polygon_u...
cc/quads/draw_polygon_unittest.cc:380: #if !defined(OS_WIN)
These are well-formed polygons, except on Windows.  <_<

Can you make this work everywhere?

[flackr, 2016-10-14 18:12:33]

https://codereview.chromium.org/2408203005/diff/60001/cc/quads/draw_polygon_u...
File cc/quads/draw_polygon_unittest.cc (right):

https://codereview.chromium.org/2408203005/diff/60001/cc/quads/draw_polygon_u...
cc/quads/draw_polygon_unittest.cc:380: #if !defined(OS_WIN)
On 2016/10/14 at 18:04:59, enne wrote:
> These are well-formed polygons, except on Windows.  <_<
> 
> Can you make this work everywhere?

They're well-formed on Windows too, but defining DrawPolygon member functions
which are only compiled in the unit test doesn't work on windows (see
DrawPolygon::RecomputeNormalForTesting which is separately defined in
draw_polygon.cc on Windows). This is a tradeoff to not add or duplicate these
longer functions in draw_polygon.cc.

[enne (OOO), 2016-10-14 18:29:47]

(Yes, yes, I was joking about it not being well-formed on Windows, sorry.)

Can you remind me what the Windows issue is here?

[Peter Mayo, 2016-10-14 18:50:41]

On 2016/10/14 18:29:47, enne wrote:
> (Yes, yes, I was joking about it not being well-formed on Windows, sorry.)
> 
> Can you remind me what the Windows issue is here?

I don't remember exactly,  I think it was a conflict in having two translation
units define implementations of the same class.

Let me undo it temporarily and demonstrate (or disprove)

[Peter Mayo, 2016-10-14 18:51:26]

Rebased

[Peter Mayo, 2016-10-14 19:26:27]

On 2016/10/14 18:51:26, Peter Mayo wrote:
> Rebased

From
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_dbg...

...\src\cc\quads\draw_polygon_unittest.cc(39): warning C4273:
'cc::DrawPolygon::IsPlanarForTesting': inconsistent dll linkage
...\src\cc\quads\draw_polygon.h(61): note: see previous definition of
'IsPlanarForTesting'
...\src\cc\quads\draw_polygon_unittest.cc(48): warning C4273:
'cc::DrawPolygon::IsConvexForTesting': inconsistent dll linkage
...\src\cc\quads\draw_polygon.h(62): note: see previous definition of
'IsConvexForTesting'

So, the executable can't supply methods for a class that is usually in a DLL. 
Implementing a second, expanded DLL just for testing seems like a worse option
too - it could easily acquire test-defeats, or otherwise fail to provide
realistic, transparent testing.

[Peter Mayo, 2016-10-14 19:28:22]

On 2016/10/14 19:26:27, Peter Mayo wrote:
> On 2016/10/14 18:51:26, Peter Mayo wrote:
> > Rebased
> 
> From
>
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_dbg...
> 
> ...\src\cc\quads\draw_polygon_unittest.cc(39): warning C4273:
> 'cc::DrawPolygon::IsPlanarForTesting': inconsistent dll linkage
> ...\src\cc\quads\draw_polygon.h(61): note: see previous definition of
> 'IsPlanarForTesting'
> ...\src\cc\quads\draw_polygon_unittest.cc(48): warning C4273:
> 'cc::DrawPolygon::IsConvexForTesting': inconsistent dll linkage
> ...\src\cc\quads\draw_polygon.h(62): note: see previous definition of
> 'IsConvexForTesting'
> 
> So, the executable can't supply methods for a class that is usually in a DLL. 
> Implementing a second, expanded DLL just for testing seems like a worse option
> too - it could easily acquire test-defeats, or otherwise fail to provide
> realistic, transparent testing.

I did have another implementation idea ready in case a reviewer really stuck on
this.
It's not very pretty, but keeps the dupli-code as simple as
RecomputeNormalForTesting,
want me to write & show it for comparison too?

[enne (OOO), 2016-10-14 21:46:06]

Why is ComputeForTesting in both draw_polygon.cc and draw_polygon_unittest.cc?
Is it possible to just move all the test functions to test code and friend them
as needed from non-test code? I'm not sure I understand what the complication is
here.

[Peter Mayo, 2016-10-15 03:05:59]

On 2016/10/14 21:46:06, enne wrote:
> Why is ComputeForTesting in both draw_polygon.cc and draw_polygon_unittest.cc?
> Is it possible to just move all the test functions to test code and friend
them
> as needed from non-test code? I'm not sure I understand what the complication
is
> here.

It's only in one of them on each platform, and it's only in the test code on any
platform where the straightforward solution works.

It seems windows does actually like friend functions better for linking.

[Peter Mayo, 2016-10-15 03:06:01]

On 2016/10/14 21:46:06, enne wrote:
> Why is ComputeForTesting in both draw_polygon.cc and draw_polygon_unittest.cc?
> Is it possible to just move all the test functions to test code and friend
them
> as needed from non-test code? I'm not sure I understand what the complication
is
> here.

It's only in one of them on each platform, and it's only in the test code on any
platform where the straightforward solution works.

It seems windows does actually like friend functions better for linking.

[enne (OOO), 2016-10-17 20:24:22]

Thanks for that cleanup.  lgtm

[Peter Mayo, 2016-10-17 22:54:31]

The CQ bit was checked by petermayo@chromium.org

[Peter Mayo, 2016-10-17 22:54:32]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org
Link to the patchset: https://codereview.chromium.org/2408203005/#ps150001
(title: "rebased")

[commit-bot: I haz the power, 2016-10-17 22:55:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 00:57:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 00:57:24]

Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_TIMED_OUT, no build
URL)

[Peter Mayo, 2016-10-19 16:52:26]

The CQ bit was checked by petermayo@chromium.org

[commit-bot: I haz the power, 2016-10-19 16:52:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 18:12:09]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2016-10-21 13:10:12]

Description was changed from

==========
Add tests where we split near an edge.

This exercises some corner cases where we have an edge
along the thick plane by which we are splitting.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Add tests where we split near an edge.

This exercises some corner cases where we have an edge
along the thick plane by which we are splitting.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/b5ec6f946eafcefff83e44d5e3b051c7f0fda814
Cr-Commit-Position: refs/heads/master@{#426244}
==========

[commit-bot: I haz the power, 2016-10-21 13:10:13]

Patchset 9 (id:??) landed as
https://crrev.com/b5ec6f946eafcefff83e44d5e3b051c7f0fda814
Cr-Commit-Position: refs/heads/master@{#426244}