Run CanvasState test across a library boundary.

Run CanvasState test across a library boundary.

Refactor CanvasStateTest to pull out functions which can
either be called directly or from a shared library. Add a
command line flag to pass in the path to a library to open
to call those functions from.

Separate different CanvasTest tests into separate DEF_TEST
tests. This allows them to be run in parallel.

Move the ifdefs outside of function declarations to skip
running CanvasTest tests which do nothing.

Add the canvas_state_lib target. It is a shared library that
exports some functions to be called by another version of
Skia.

BUG=b/15693384

Committed: https://skia.googlesource.com/skia/+/24519371cb541db6f59d903d21878ed9c45eb549

[scroggo, 2014-07-17 21:32:38]

https://codereview.chromium.org/400043003/diff/1/tests/CanvasStateTest.cpp
File tests/CanvasStateTest.cpp (right):

https://codereview.chromium.org/400043003/diff/1/tests/CanvasStateTest.cpp#ne...
tests/CanvasStateTest.cpp:198: &localRegion);
Note that I'm passing a pointer to an SkRegion across the library boundary. I'd
like to avoid that. (I don't expect SkRegion to change anytime soon, but it
still seems incorrect.) I don't see a good way to decompose it though. I suppose
I could just recreate it from various parameters, though that will be messy.

Thoughts?

[reed1, 2014-07-18 12:52:38]

You can decompose any region into an array of SkIRects, and then reconstruct it
on the other side.

[djsollen, 2014-07-18 14:11:54]

https://codereview.chromium.org/400043003/diff/1/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/400043003/diff/1/gyp/common_conditions.gypi#n...
gyp/common_conditions.gypi:189: # For now. Perhaps I should do what mtklein
suggested and add skia_pic
that seems like a good idea with a default to true (for at least android).

https://codereview.chromium.org/400043003/diff/1/gyp/most.gyp
File gyp/most.gyp (right):

https://codereview.chromium.org/400043003/diff/1/gyp/most.gyp#newcode27
gyp/most.gyp:27: 'canvas_state_lib.gyp:canvas_state_lib',
why not add the dependency to tests.gypi

https://codereview.chromium.org/400043003/diff/1/tests/CanvasStateTest.cpp
File tests/CanvasStateTest.cpp (right):

https://codereview.chromium.org/400043003/diff/1/tests/CanvasStateTest.cpp#ne...
tests/CanvasStateTest.cpp:159: void* handle;
move this duplicate setup code into a function if possible.

[scroggo, 2014-07-18 19:31:02]

ptal

https://codereview.chromium.org/400043003/diff/1/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/400043003/diff/1/gyp/common_conditions.gypi#n...
gyp/common_conditions.gypi:189: # For now. Perhaps I should do what mtklein
suggested and add skia_pic
On 2014/07/18 14:11:54, djsollen wrote:
> that seems like a good idea with a default to true (for at least android).

Uploaded a separate CL for that: https://codereview.chromium.org/406613003/

https://codereview.chromium.org/400043003/diff/1/gyp/most.gyp
File gyp/most.gyp (right):

https://codereview.chromium.org/400043003/diff/1/gyp/most.gyp#newcode27
gyp/most.gyp:27: 'canvas_state_lib.gyp:canvas_state_lib',
On 2014/07/18 14:11:54, djsollen wrote:
> why not add the dependency to tests.gypi

I don't think that's what I want. The 'tests' target wants to use a separately
created .so of canvas_state_lib (ideally from a different version of Skia so the
test is a little more meaningful).

Also, canvas_state_lib requires skia_lib to have been built with '-fPIC'. (Is
there a way to enforce that?) It seems like this would automatically build
something that is not valid by default (since -fPIC is not default for
skia_lib).

https://codereview.chromium.org/400043003/diff/1/tests/CanvasStateTest.cpp
File tests/CanvasStateTest.cpp (right):

https://codereview.chromium.org/400043003/diff/1/tests/CanvasStateTest.cpp#ne...
tests/CanvasStateTest.cpp:159: void* handle;
On 2014/07/18 14:11:54, djsollen wrote:
> move this duplicate setup code into a function if possible.

I've moved the call to dlopen into its own function. Do you mean the whole
if/else block? I'm not sure how to factor out the rest, even though the two
callers are similar. I need to return handle, but in the 'if' case it's an error
for it to be null, and in the 'else' it's expected. Also, the two callers of
this function would need to assign functions to function pointers with different
signatures. Is there a clean way to do that?

https://codereview.chromium.org/400043003/diff/1/tests/CanvasStateTest.cpp#ne...
tests/CanvasStateTest.cpp:198: &localRegion);
On 2014/07/17 21:32:37, scroggo wrote:
> Note that I'm passing a pointer to an SkRegion across the library boundary.
I'd
> like to avoid that. (I don't expect SkRegion to change anytime soon, but it
> still seems incorrect.) I don't see a good way to decompose it though. I
suppose
> I could just recreate it from various parameters, though that will be messy.
> 
> Thoughts?

On 2014/07/18 12:52:38, reed1 wrote:
> You can decompose any region into an array of SkIRects, and then reconstruct
it
> on the other side.

Done. Thanks!

https://codereview.chromium.org/400043003/diff/20001/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/400043003/diff/20001/gyp/common_conditions.gy...
gyp/common_conditions.gypi:13: ['skia_pic', {
Ignore these; I've pulled them into a separate CL
(https://codereview.chromium.org/406613003/ - please comment there if you have
some input).

https://codereview.chromium.org/400043003/diff/20001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/400043003/diff/20001/gyp/common_variables.gyp...
gyp/common_variables.gypi:183: [ 'skia_shared_lib == 1 or skia_sanitizer ==
"thread" '
Also pulled out into https://codereview.chromium.org/406613003/

[djsollen, 2014-07-22 15:06:34]

https://codereview.chromium.org/400043003/diff/20001/gyp/canvas_state_lib.gyp
File gyp/canvas_state_lib.gyp (right):

https://codereview.chromium.org/400043003/diff/20001/gyp/canvas_state_lib.gyp...
gyp/canvas_state_lib.gyp:18: # Dummy 'most' target, since gyp_skia sets 'most'
to be the default.
Can you remove this as well?

How does canvas_state_lib get picked up in a normal build?  Shouldn't it be part
of most.gyp, perhaps just for Android?

https://codereview.chromium.org/400043003/diff/20001/tests/CanvasStateTest.cpp
File tests/CanvasStateTest.cpp (right):

https://codereview.chromium.org/400043003/diff/20001/tests/CanvasStateTest.cp...
tests/CanvasStateTest.cpp:33: return dlopen(library, RTLD_LAZY | RTLD_LOCAL);
I expected this function to check the library flag and open the appropriate
library.  It can return 0 both if there was no library and if the library failed
to load, but in the failure case we would log an assert.  If 0 we can just do
the local function.

https://codereview.chromium.org/400043003/diff/20001/tests/CanvasStateTest.cp...
tests/CanvasStateTest.cpp:35: #endif
why #endif followed by identical #ifdef?  Remove both.

[scroggo, 2014-07-22 17:10:11]

https://codereview.chromium.org/400043003/diff/20001/gyp/canvas_state_lib.gyp
File gyp/canvas_state_lib.gyp (right):

https://codereview.chromium.org/400043003/diff/20001/gyp/canvas_state_lib.gyp...
gyp/canvas_state_lib.gyp:18: # Dummy 'most' target, since gyp_skia sets 'most'
to be the default.
On 2014/07/22 15:06:34, djsollen wrote:
> Can you remove this as well?
> 
> How does canvas_state_lib get picked up in a normal build? 

Currently, it does not. In order to build it, you use:

GYP_DEFINES=skia_pic=1 ./gyp_skia gyp/canvas_state_lib.gyp && ninja -C out/Debug
canvas_state_lib

(Which I need to document somewhere if we leave the current behavior.)
> Shouldn't it be part
> of most.gyp, perhaps just for Android?

Possibly. I'm not sure though. It won't actually be used with the rest of the
build (it's intended to run with a *different* version of Skia, after all), so
I'm not sure it makes sense to build it with the rest of the build. (Also it's
nice to be able to run the test on platforms other than Android. In that case,
it would need to be included as part of most no matter what. That leaves the
problem that it won't build unless you use skia_pic, which is not the default on
all platforms.)

https://codereview.chromium.org/400043003/diff/20001/tests/CanvasStateTest.cpp
File tests/CanvasStateTest.cpp (right):

https://codereview.chromium.org/400043003/diff/20001/tests/CanvasStateTest.cp...
tests/CanvasStateTest.cpp:33: return dlopen(library, RTLD_LAZY | RTLD_LOCAL);
On 2014/07/22 15:06:34, djsollen wrote:
> I expected this function to check the library flag and open the appropriate
> library.  It can return 0 both if there was no library and if the library
failed
> to load, but in the failure case we would log an assert.  If 0 we can just do
> the local function.

Done.

https://codereview.chromium.org/400043003/diff/20001/tests/CanvasStateTest.cp...
tests/CanvasStateTest.cpp:35: #endif
On 2014/07/22 15:06:34, djsollen wrote:
> why #endif followed by identical #ifdef?  Remove both.

Just for future proofing. I can imagine someone adding a new function in
between, not noticing that the code was ifdef'ed out. I'm fine with removing
though.

https://codereview.chromium.org/400043003/diff/20001/tests/CanvasStateTest.cp...
tests/CanvasStateTest.cpp:99: // And draw to it in the second Skia.
Originally I tried to write this test outside of Skia. It would load two
versions of Skia and use both. That seemed to be messy and unnecessary, but I
think I may have put the wrong calls in the external library. On Android, we
pass an SkCanvas to a newer version of Skia. In order to test that behavior with
this test, we need to build canvas_state_lib with a newer version of Skia, and
then run the test inside the old version of Skia, passing in canvas_state_lib.
Maybe a better way to do this would be to have the library do the creation of
the SkCanvas/SkCanvasState. That way, to automate it, we just need to keep old
versions of canvas_state_lib around. Thoughts?

[djsollen, 2014-07-22 17:24:42]

https://codereview.chromium.org/400043003/diff/20001/tests/CanvasStateTest.cpp
File tests/CanvasStateTest.cpp (right):

https://codereview.chromium.org/400043003/diff/20001/tests/CanvasStateTest.cp...
tests/CanvasStateTest.cpp:99: // And draw to it in the second Skia.
On 2014/07/22 17:10:10, scroggo wrote:
> Originally I tried to write this test outside of Skia. It would load two
> versions of Skia and use both. That seemed to be messy and unnecessary, but I
> think I may have put the wrong calls in the external library. On Android, we
> pass an SkCanvas to a newer version of Skia. In order to test that behavior
with
> this test, we need to build canvas_state_lib with a newer version of Skia, and
> then run the test inside the old version of Skia, passing in canvas_state_lib.
> Maybe a better way to do this would be to have the library do the creation of
> the SkCanvas/SkCanvasState. That way, to automate it, we just need to keep old
> versions of canvas_state_lib around. Thoughts?

Lets stick with the current version as the other way around gets really messy. 
However document that if you change the SkCanvasState you need to generate the
library with the new content and play it back on the existing code to make sure
everything works.

https://codereview.chromium.org/400043003/diff/40001/tests/CanvasStateTest.cpp
File tests/CanvasStateTest.cpp (right):

https://codereview.chromium.org/400043003/diff/40001/tests/CanvasStateTest.cp...
tests/CanvasStateTest.cpp:97: if (openLibResult.libRequired()) {
I assumed that in this case we could still test the internal method and not need
to return. That would save you tracking that variable and checking it here, but
I'm fine either way.

[scroggo, 2014-07-22 18:12:01]

https://codereview.chromium.org/400043003/diff/20001/gyp/canvas_state_lib.gyp
File gyp/canvas_state_lib.gyp (right):

https://codereview.chromium.org/400043003/diff/20001/gyp/canvas_state_lib.gyp...
gyp/canvas_state_lib.gyp:18: # Dummy 'most' target, since gyp_skia sets 'most'
to be the default.
On 2014/07/22 17:10:10, scroggo wrote:
> In order to build it, you use:
> 
> GYP_DEFINES=skia_pic=1 ./gyp_skia gyp/canvas_state_lib.gyp && ninja -C
out/Debug
> canvas_state_lib
> 
> (Which I need to document somewhere if we leave the current behavior.)

Done.

https://codereview.chromium.org/400043003/diff/40001/tests/CanvasStateTest.cpp
File tests/CanvasStateTest.cpp (right):

https://codereview.chromium.org/400043003/diff/40001/tests/CanvasStateTest.cp...
tests/CanvasStateTest.cpp:97: if (openLibResult.libRequired()) {
On 2014/07/22 17:24:42, djsollen wrote:
> I assumed that in this case we could still test the internal method and not
need
> to return. That would save you tracking that variable and checking it here,
but
> I'm fine either way.

Done.

[djsollen, 2014-07-22 18:22:01]

lgtm

[scroggo, 2014-07-22 19:25:36]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-07-22 19:26:43]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/scroggo@google.com/400043003/80001

[commit-bot: I haz the power, 2014-07-22 19:39:01]

Change committed as 24519371cb541db6f59d903d21878ed9c45eb549