add runtime option to provide data->imagegenerator factory

add runtime option to provide data->imagegenerator factory

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/1c84634454aa78fb26f23875b86a243aa4596c59

[reed1, 2015-07-09 17:41:39]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-09 17:41:53]

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

[reed1, 2015-07-09 17:41:59]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-07-09 17:49:36]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-09 17:49:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1229933003/60001

[commit-bot: I haz the power, 2015-07-09 17:50:39]

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

[commit-bot: I haz the power, 2015-07-09 17:50:40]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
  skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

(exceeded global retry quota)

[reed1, 2015-07-09 18:10:03]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-09 18:10:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1229933003/80001

[commit-bot: I haz the power, 2015-07-09 18:24:14]

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

[commit-bot: I haz the power, 2015-07-09 18:24:16]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-07-09 18:27:02]

reed@google.com changed reviewers:
+ fmalita@chromium.org, mtklein@google.com, scroggo@google.com

[reed1, 2015-07-09 18:27:02]

ptal

This was inspired by the desire to have skia get access to blink's
imagegenerator, but it can't directly link against it.

I presume after this lands, Chrome would call the SkGraphics::Set...() and pass
in a factory to blink, since Chrome should have access to it.

[mtklein, 2015-07-09 18:46:15]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-07-09 18:46:16]

lgtm

[commit-bot: I haz the power, 2015-07-09 18:46:37]

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

[commit-bot: I haz the power, 2015-07-09 18:47:40]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/1c84634454aa78fb26f23875b86a243aa4596c59

[scroggo, 2015-07-09 19:01:15]

https://codereview.chromium.org/1229933003/diff/80001/include/core/SkGraphics.h
File include/core/SkGraphics.h (right):

https://codereview.chromium.org/1229933003/diff/80001/include/core/SkGraphics...
include/core/SkGraphics.h:154: static ImageGeneratorFromEncodedFactory
GetImageGeneratorFromEncodedFactory();
I do not understand the need for this accessor. Why not just call
NewFromEncoded?

(Ah, it appears the the test uses it to set it back. Is this useful outside of
tests? Even inside the tests, it seems less than ideal, as other tests might
change or use it, as you acknowledge when you mention it's racy.)

https://codereview.chromium.org/1229933003/diff/80001/include/core/SkImageGen...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/1229933003/diff/80001/include/core/SkImageGen...
include/core/SkImageGenerator.h:224: static SkImageGenerator*
NewFromEncodedImpl(SkData*);
Maybe add a comment that the implementation does not need to check for NULL?

https://codereview.chromium.org/1229933003/diff/80001/tests/ImageGeneratorTes...
File tests/ImageGeneratorTest.cpp (right):

https://codereview.chromium.org/1229933003/diff/80001/tests/ImageGeneratorTes...
tests/ImageGeneratorTest.cpp:19: static void
test_imagegenerator_factory(skiatest::Reporter* reporter) {
Why not make this its own test, with DEF_TEST?

[mtklein, 2015-07-09 19:04:57]

https://codereview.chromium.org/1229933003/diff/80001/include/core/SkGraphics.h
File include/core/SkGraphics.h (right):

https://codereview.chromium.org/1229933003/diff/80001/include/core/SkGraphics...
include/core/SkGraphics.h:154: static ImageGeneratorFromEncodedFactory
GetImageGeneratorFromEncodedFactory();
On 2015/07/09 19:01:15, scroggo wrote:
> I do not understand the need for this accessor. Why not just call
> NewFromEncoded?
> 
> (Ah, it appears the the test uses it to set it back. Is this useful outside of
> tests? Even inside the tests, it seems less than ideal, as other tests might
> change or use it, as you acknowledge when you mention it's racy.)

I guess we could merge the two as

static ImagGeneratorFromEncodedFactory
SetImageGeneratorFromEncodedFactory(ImageGeneratorFromEncodedFactory), where the
first ImageGeneratorFromEncodedFactory is the previous value.  (Yes I typed all
that out.)

[reed1, 2015-07-09 19:05:46]

https://codereview.chromium.org/1229933003/diff/80001/include/core/SkGraphics.h
File include/core/SkGraphics.h (right):

https://codereview.chromium.org/1229933003/diff/80001/include/core/SkGraphics...
include/core/SkGraphics.h:154: static ImageGeneratorFromEncodedFactory
GetImageGeneratorFromEncodedFactory();
On 2015/07/09 19:01:15, scroggo wrote:
> I do not understand the need for this accessor. Why not just call
> NewFromEncoded?
> 
> (Ah, it appears the the test uses it to set it back. Is this useful outside of
> tests? Even inside the tests, it seems less than ideal, as other tests might
> change or use it, as you acknowledge when you mention it's racy.)

Not sure what to do here. Having a global is just a pain no matter how you spin
it.

https://codereview.chromium.org/1229933003/diff/80001/include/core/SkImageGen...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/1229933003/diff/80001/include/core/SkImageGen...
include/core/SkImageGenerator.h:224: static SkImageGenerator*
NewFromEncodedImpl(SkData*);
On 2015/07/09 19:01:15, scroggo wrote:
> Maybe add a comment that the implementation does not need to check for NULL?

Will do

https://codereview.chromium.org/1229933003/diff/80001/tests/ImageGeneratorTes...
File tests/ImageGeneratorTest.cpp (right):

https://codereview.chromium.org/1229933003/diff/80001/tests/ImageGeneratorTes...
tests/ImageGeneratorTest.cpp:19: static void
test_imagegenerator_factory(skiatest::Reporter* reporter) {
On 2015/07/09 19:01:15, scroggo wrote:
> Why not make this its own test, with DEF_TEST?

Just style difference. When I see the output from running the tool, I'm ok with
fewer lines, so just "ImageGenerator" instead of N lines for N different ideas,
seems ok.