[PDF] Make color shaders work correctly.

[PDF] Make color shaders work correctly.

Make SkPDFShader correctly bail out for color shaders.
Fix the bail out handling code.

Committed: http://code.google.com/p/skia/source/detail?r=1886

[Steve VanDeBogart, 2011-07-18 15:11:29]

RE: http://codereview.appspot.com/4674048
I guess this path was never tested...

Chris for review unless Brian wants to do it.

[reed1, 2011-07-18 15:24:25]

Does bail out mean we now correctly handle colorshaders, or that we ignore them,
or that we draw nothing?

Do we have a good gm test for this now?

[Steve VanDeBogart, 2011-07-18 15:34:21]

On 2011/07/18 15:24:25, reed1 wrote:
> Does bail out mean we now correctly handle colorshaders, or that we ignore
them,
> or that we draw nothing?
> 
> Do we have a good gm test for this now?

The PDF code handles a color shader by not using a PDF shader and just
effectively changing the paint color, hence bail out.  With this change, color
shaders should be correctly handled in the PDF code.  Brian sent out a gm for a
color shader (blocked on the PDF fix), which looks sufficient.

[reed1, 2011-07-18 15:46:46]

LGTM

Some general questions/comments:

1. canonicalShaders and canonicalShadersMutex should be initial capitalized,
since they are static methods. Alternatively, they could just be static/local
functions inside the cpp, since that is the only place they are used. That is a
little nicer, since they are basically an implementation detail.

2. I don't understand how a set of global objects can be used and mutated, given
that they're shared between threads. If thread A finds one and uses it, and
thread B does the same, are we not at risk of corrupting the object? I realize
that the lookup functionality is mutex protected, but I can't see that accessing
the found instance is, though I may just be missing that.

[Chris Guillory, 2011-07-18 17:13:21]

LGTM

http://codereview.appspot.com/4750045/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4750045/diff/1/src/pdf/SkPDFDevice.cpp#newcode1412
src/pdf/SkPDFDevice.cpp:1412: if (pdfShader) {
Optional: pdfShader.get()? That's the usage pattern you have in your change to
SkPDFCatalog.cpp.

[bsalomon, 2011-07-18 17:22:06]

I defer to the PDF code experts :)

On Mon, Jul 18, 2011 at 1:13 PM, <ctguil@chromium.org> wrote:

> LGTM
>
>
>
http://codereview.appspot.com/**4750045/diff/1/src/pdf/**SkPDFDevice.cpp<http...
> File src/pdf/SkPDFDevice.cpp (right):
>
> http://codereview.appspot.com/**4750045/diff/1/src/pdf/**
>
SkPDFDevice.cpp#newcode1412<http://codereview.appspot.com/4750045/diff/1/src/pdf/SkPDFDevice.cpp#newcode1412>
> src/pdf/SkPDFDevice.cpp:1412: if (pdfShader) {
> Optional: pdfShader.get()? That's the usage pattern you have in your
> change to SkPDFCatalog.cpp.
>
>
>
http://codereview.appspot.com/**4750045/<http://codereview.appspot.com/4750045/>
>

[Steve VanDeBogart, 2011-07-18 18:39:41]

On 2011/07/18 15:46:46, reed1 wrote:
> LGTM
> 
> Some general questions/comments:
> 
> 1. canonicalShaders and canonicalShadersMutex should be initial capitalized,

Done.

> since they are static methods. Alternatively, they could just be static/local
> functions inside the cpp, since that is the only place they are used. That is
a
> little nicer, since they are basically an implementation detail.

While, I think that's feasible, I think they make more sense where they are. 
Logically, these are class static fields, but Chrome doesn't like static
initializers, so I made them static methods with static variables.

> 2. I don't understand how a set of global objects can be used and mutated,
given

They can't - they aren't mutated. 

> that they're shared between threads. If thread A finds one and uses it, and
> thread B does the same, are we not at risk of corrupting the object? I realize
> that the lookup functionality is mutex protected, but I can't see that
accessing
> the found instance is, though I may just be missing that.

http://codereview.appspot.com/4750045/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4750045/diff/1/src/pdf/SkPDFDevice.cpp#newcode1412
src/pdf/SkPDFDevice.cpp:1412: if (pdfShader) {
On 2011/07/18 17:13:22, Chris Guillory wrote:
> Optional: pdfShader.get()? That's the usage pattern you have in your change to
> SkPDFCatalog.cpp.

Done.