pdf: report NYI features, and fail gracefully when something is not supported in pdf.

pdf: report NYI features, and fail gracefully when something is not supported in pdf.

R=vandebo@chromium.org

Committed: https://code.google.com/p/skia/source/detail?r=11388

[vandebo (ex-Chrome), 2013-09-12 21:37:05]

https://codereview.chromium.org/23654036/diff/5001/gm/gradients.cpp
File gm/gradients.cpp (right):

https://codereview.chromium.org/23654036/diff/5001/gm/gradients.cpp#newcode111
gm/gradients.cpp:111: virtual SkISize onISize() { return make_isize(840, 815); }
Of course, this will require a rebaseline across all configs...

https://codereview.chromium.org/23654036/diff/5001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/23654036/diff/5001/src/pdf/SkPDFDevice.cpp#ne...
src/pdf/SkPDFDevice.cpp:682: SkASSERT(!initialTransform.hasPerspective());
If you want to assert, just pass true for the second arg to NOT_IMPLEMENTED...

https://codereview.chromium.org/23654036/diff/5001/src/pdf/SkPDFDevice.cpp#ne...
src/pdf/SkPDFDevice.cpp:907: if (d.fMatrix->hasPerspective()) {
If you put this in ScopedContentEntry, you'll cover most of the cases with a
single test.

https://codereview.chromium.org/23654036/diff/5001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/23654036/diff/5001/src/pdf/SkPDFShader.cpp#ne...
src/pdf/SkPDFShader.cpp:58: if (range == SkIntToScalar(0)) {
This should go to a different CL.

You can't just return (that violates the interface of this function).  Based on
the comment, I think you need to result->append("pop").

[edisonn, 2013-09-17 15:14:23]

https://codereview.chromium.org/23654036/diff/5001/gm/gradients.cpp
File gm/gradients.cpp (right):

https://codereview.chromium.org/23654036/diff/5001/gm/gradients.cpp#newcode111
gm/gradients.cpp:111: virtual SkISize onISize() { return make_isize(840, 815); }
On 2013/09/12 21:37:05, vandebo wrote:
> Of course, this will require a rebaseline across all configs...

yep - the view needs to be extended

https://codereview.chromium.org/23654036/diff/5001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/23654036/diff/5001/src/pdf/SkPDFDevice.cpp#ne...
src/pdf/SkPDFDevice.cpp:682: SkASSERT(!initialTransform.hasPerspective());
On 2013/09/12 21:37:05, vandebo wrote:
> If you want to assert, just pass true for the second arg to NOT_IMPLEMENTED...

Done.

https://codereview.chromium.org/23654036/diff/5001/src/pdf/SkPDFDevice.cpp#ne...
src/pdf/SkPDFDevice.cpp:907: if (d.fMatrix->hasPerspective()) {
On 2013/09/12 21:37:05, vandebo wrote:
> If you put this in ScopedContentEntry, you'll cover most of the cases with a
> single test.

Done.

https://codereview.chromium.org/23654036/diff/5001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/23654036/diff/5001/src/pdf/SkPDFShader.cpp#ne...
src/pdf/SkPDFShader.cpp:58: if (range == SkIntToScalar(0)) {
On 2013/09/12 21:37:05, vandebo wrote:
> This should go to a different CL.
> 
> You can't just return (that violates the interface of this function).  Based
on
> the comment, I think you need to result->append("pop").

Done.

[vandebo (ex-Chrome), 2013-09-17 16:35:09]

https://codereview.chromium.org/23654036/diff/14001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/23654036/diff/14001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:635: if (matrix.hasPerspective() ||
The predominate style of this file is 80col.  The way I read the style guide
suggests that you should honor that style here unless it would be ugly.

https://codereview.chromium.org/23654036/diff/14001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:794: if (totalTransform.hasPerspective()) {
This isn't needed.  totalTransform is formed from fInitialTransform and
contentENtry->fState.fMatrix, which have both already been checked for
perspective. Matrix inversion is an affine transform, so if the input was not
perspective, the output won't be either.

https://codereview.chromium.org/23654036/diff/14001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:939: // TODO(edisonn): report somewhow why we failed?
Log statement?

https://codereview.chromium.org/23654036/diff/14001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:945: if (matrix.hasPerspective()) {
Not necessary, the matrix will eventually get checked in the ScopedContentEntry
init method.

https://codereview.chromium.org/23654036/diff/14001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1935: if (scaled.hasPerspective()) {
Not needed - scaled in passed into ScopedContentEntry...

[edisonn, 2013-09-17 17:32:03]

https://codereview.chromium.org/23654036/diff/14001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/23654036/diff/14001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:635: if (matrix.hasPerspective() ||
On 2013/09/17 16:35:09, vandebo wrote:
> The predominate style of this file is 80col.  The way I read the style guide
> suggests that you should honor that style here unless it would be ugly.

Done.

https://codereview.chromium.org/23654036/diff/14001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:794: if (totalTransform.hasPerspective()) {
On 2013/09/17 16:35:09, vandebo wrote:
> This isn't needed.  totalTransform is formed from fInitialTransform and
> contentENtry->fState.fMatrix, which have both already been checked for
> perspective. Matrix inversion is an affine transform, so if the input was not
> perspective, the output won't be either.

Done.

https://codereview.chromium.org/23654036/diff/14001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:939: // TODO(edisonn): report somewhow why we failed?
On 2013/09/17 16:35:09, vandebo wrote:
> Log statement?

it is a todo, not looking for an answer in this cl. I am sure there are more
instances like this. i just want to mark it, if I see it.

https://codereview.chromium.org/23654036/diff/14001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:945: if (matrix.hasPerspective()) {
On 2013/09/17 16:35:09, vandebo wrote:
> Not necessary, the matrix will eventually get checked in the
ScopedContentEntry
> init method.

Done.

https://codereview.chromium.org/23654036/diff/14001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1935: if (scaled.hasPerspective()) {
On 2013/09/17 16:35:09, vandebo wrote:
> Not needed - scaled in passed into ScopedContentEntry...

Done.

[vandebo (ex-Chrome), 2013-09-17 17:46:17]

LGTM

https://codereview.chromium.org/23654036/diff/20001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/23654036/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:637:
paint.getShader()->getLocalMatrix().hasPerspective())) {
nit: indent one more space.

[edisonn, 2013-09-17 18:01:50]

https://codereview.chromium.org/23654036/diff/20001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/23654036/diff/20001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:637:
paint.getShader()->getLocalMatrix().hasPerspective())) {
On 2013/09/17 17:46:17, vandebo wrote:
> nit: indent one more space.

Done.

[edisonn, 2013-09-19 17:36:50]

Committed patchset #8 manually as r11388 (presubmit successful).

[epoger, 2013-09-19 18:13:34]

Edi- 

Is this a case for which it would be helpful for the ignored_tests.txt to have
allowed matching by configs instead of just test names?  (You could have hidden
all PDF errors, until you rebaselined...)

It's a tough balancing act, as noted in
https://code.google.com/p/skia/issues/detail?id=1600#c4 .  Our experience with
Webkit's TestExpectations file tells us that if it's too easy to suppress broad
swaths of test failures, the number of ignored failures just keeps going up and
up.

[edisonn, 2013-09-19 18:16:20]

On 2013/09/19 18:13:34, epoger wrote:
> Edi- 
> 
> Is this a case for which it would be helpful for the ignored_tests.txt to have
> allowed matching by configs instead of just test names?  (You could have
hidden
> all PDF errors, until you rebaselined...)
> 
> It's a tough balancing act, as noted in
> https://code.google.com/p/skia/issues/detail?id=1600#c4 .  Our experience with
> Webkit's TestExpectations file tells us that if it's too easy to suppress
broad
> swaths of test failures, the number of ignored failures just keeps going up
and
> up.

I think so, I did not think of filtering like this. But I guess it would be
useful, but I don't see it useful more than once a month maybe.

If we need it again, then we can implement it

Edi

[edisonn, 2013-09-19 18:18:23]

On 2013/09/19 18:16:20, edisonn wrote:
> On 2013/09/19 18:13:34, epoger wrote:
> > Edi- 
> > 
> > Is this a case for which it would be helpful for the ignored_tests.txt to
have
> > allowed matching by configs instead of just test names?  (You could have
> hidden
> > all PDF errors, until you rebaselined...)
> > 
> > It's a tough balancing act, as noted in
> > https://code.google.com/p/skia/issues/detail?id=1600#c4 .  Our experience
with
> > Webkit's TestExpectations file tells us that if it's too easy to suppress
> broad
> > swaths of test failures, the number of ignored failures just keeps going up
> and
> > up.
> 
> I think so, I did not think of filtering like this. But I guess it would be
> useful, but I don't see it useful more than once a month maybe.
> 
> If we need it again, then we can implement it
> 
> Edi

Actually for this kind of tests, it might be useful to have a sort of version of
try, that will report back new images and new checksums, which I could apply it
to my cl, and I would get no failure at all when I check in

[epoger, 2013-09-19 18:19:40]

> Actually for this kind of tests, it might be useful to have a sort of version
of
> try, that will report back new images and new checksums, which I could apply
it
> to my cl, and I would get no failure at all when I check in

Yup, that's the long-term plan.  I should note that in bug 1600.