SkPDF: fix large-number bug

SkPDF: fix large-number bug

Example:
        https://fiddle.skia.org/c/db95a3f6c928017635c4f4ddf25f2a7b

BUG=skia:257
Committed: https://skia.googlesource.com/skia/+/05b48e2a24ae78312c80ed94195f0b705fdb0547

[hal.canary, 2015-11-10 16:54:59]

Patchset #1 (id:1) has been deleted

[hal.canary, 2015-11-10 16:56:09]

Description was changed from

==========
SkPDF: fix large-number bug

BUG=skia:257
==========

to

==========
SkPDF: fix large-number bug

Example:
        https://fiddle.skia.org/c/db95a3f6c928017635c4f4ddf25f2a7b

BUG=skia:257
==========

[hal.canary, 2015-11-10 16:56:09]

halcanary@google.com changed reviewers:
+ tomhudson@google.com

[hal.canary, 2015-11-16 16:10:46]

PTAL 

Before and after:
http://i.imgur.com/2VUB4aS.png

[tomhudson, 2015-11-16 16:35:30]

https://codereview.chromium.org/1438503002/diff/60001/gm/skbug_257.cpp
File gm/skbug_257.cpp (right):

https://codereview.chromium.org/1438503002/diff/60001/gm/skbug_257.cpp#newcode30
gm/skbug_257.cpp:30: static void excercise_draw_pos_text(SkCanvas* canvas,
Nit: spelling?

https://codereview.chromium.org/1438503002/diff/60001/gm/skbug_257.cpp#newcode46
gm/skbug_257.cpp:46: static void excercise_draw_pos_text_h(SkCanvas* canvas,
Nit: spelling?

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

https://codereview.chromium.org/1438503002/diff/60001/src/pdf/SkPDFDevice.cpp...
src/pdf/SkPDFDevice.cpp:55: SkASSERT(translate.getTranslateX() <= 1.0);
Do we want an fabs() here for paranoia?

https://codereview.chromium.org/1438503002/diff/60001/src/pdf/SkPDFDevice.cpp...
src/pdf/SkPDFDevice.cpp:57: drawCopy.fMatrix = &translate;
Uh-oh; returning reference to a local static - this is non-reentrant.
If I'm reading the docs right, it's entirely unsafe, because a static function
is only saying something about the linkage; you're returning a pointer to space
up above the stack.
Even if it were safe (static SkMatrix translate), I'd want a warning about
reentrancy issues, since the current uses of the function look compatible but
it's a nasty thing to hide.

I wish SkDraw.h told us clearly that the structure doesn't own any of the things
it points to, that it's guaranteed to have a default destructor, ...

https://codereview.chromium.org/1438503002/diff/60001/src/pdf/SkPDFDevice.cpp...
src/pdf/SkPDFDevice.cpp:826: // PDF can't exactly represent the float values as
numbers.
Nit: this comment is cut-and-pasted four times. Do we have somewhere we could
put it once, around excessive_translation() or untranslate(), which would still
be sufficiently descriptive? (It does help motivate the 8192.0f magic number.)

These conditionals are all similar, but templating it would be two or three
steps too far.

https://codereview.chromium.org/1438503002/diff/60001/src/pdf/SkPDFDevice.cpp...
src/pdf/SkPDFDevice.cpp:1408: return;  // NOTE: shader behavior will be off.
Can we quantify and document how much of a shader problem we'll have? Do we
sense whether it'll be seriously breaking? Are you proposing to just accept that
smaller bug now and fix it later if it becomes a problem?

https://codereview.chromium.org/1438503002/diff/60001/src/pdf/SkPDFDevice.cpp...
src/pdf/SkPDFDevice.cpp:1425: matrix.setTranslate(offset);
This gets merged into d.fMatrix in drawPath(), *after* the excessive translation
check; looks like a failure case?

[hal.canary, 2015-11-16 16:50:18]

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

https://codereview.chromium.org/1438503002/diff/60001/src/pdf/SkPDFDevice.cpp...
src/pdf/SkPDFDevice.cpp:55: SkASSERT(translate.getTranslateX() <= 1.0);
On 2015/11/16 at 16:35:30, tomhudson wrote:
> Do we want an fabs() here for paranoia?

done

https://codereview.chromium.org/1438503002/diff/60001/src/pdf/SkPDFDevice.cpp...
src/pdf/SkPDFDevice.cpp:57: drawCopy.fMatrix = &translate;
On 2015/11/16 at 16:35:30, tomhudson wrote:
> Uh-oh; returning reference to a local static - this is non-reentrant.
> If I'm reading the docs right, it's entirely unsafe, because a static function
is only saying something about the linkage; you're returning a pointer to space
up above the stack.
> Even if it were safe (static SkMatrix translate), I'd want a warning about
reentrancy issues, since the current uses of the function look compatible but
it's a nasty thing to hide.
> 
> I wish SkDraw.h told us clearly that the structure doesn't own any of the
things it points to, that it's guaranteed to have a default destructor, ...

Argh!

This is what I get from copy-and-pasting into a function!  It was right, and
then it wasn't/

Fixed.

https://codereview.chromium.org/1438503002/diff/60001/src/pdf/SkPDFDevice.cpp...
src/pdf/SkPDFDevice.cpp:826: // PDF can't exactly represent the float values as
numbers.
On 2015/11/16 at 16:35:30, tomhudson wrote:
> Nit: this comment is cut-and-pasted four times. Do we have somewhere we could
put it once, around excessive_translation() or untranslate(), which would still
be sufficiently descriptive? (It does help motivate the 8192.0f magic number.)
> 
> These conditionals are all similar, but templating it would be two or three
steps too far.

done

https://codereview.chromium.org/1438503002/diff/60001/src/pdf/SkPDFDevice.cpp...
src/pdf/SkPDFDevice.cpp:1408: return;  // NOTE: shader behavior will be off.
On 2015/11/16 at 16:35:30, tomhudson wrote:
> Can we quantify and document how much of a shader problem we'll have? Do we
sense whether it'll be seriously breaking?

It's already off (comparing PDF to 8888 in that GM will show you).

> Are you proposing to just accept that smaller bug now and fix it later if it
becomes a problem?

Yes.

[hal.canary, 2015-11-16 16:52:11]

https://codereview.chromium.org/1438503002/diff/60001/gm/skbug_257.cpp
File gm/skbug_257.cpp (right):

https://codereview.chromium.org/1438503002/diff/60001/gm/skbug_257.cpp#newcode30
gm/skbug_257.cpp:30: static void excercise_draw_pos_text(SkCanvas* canvas,
On 2015/11/16 at 16:35:30, tomhudson wrote:
> Nit: spelling?

done

https://codereview.chromium.org/1438503002/diff/60001/gm/skbug_257.cpp#newcode46
gm/skbug_257.cpp:46: static void excercise_draw_pos_text_h(SkCanvas* canvas,
On 2015/11/16 at 16:35:30, tomhudson wrote:
> Nit: spelling?

done

[tomhudson, 2015-11-16 17:37:18]

lgtm

[hal.canary, 2015-11-16 18:23:46]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-11-16 18:23:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438503002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438503002/140001

[commit-bot: I haz the power, 2015-11-16 18:26:15]

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

[commit-bot: I haz the power, 2015-11-16 18:26:16]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[hal.canary, 2015-11-16 18:29:15]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-11-16 18:29:15]

The patchset sent to the CQ was uploaded after l-g-t-m from tomhudson@google.com
Link to the patchset: https://codereview.chromium.org/1438503002/#ps160001
(title: "2015-11-16 (Monday) 13:28:57 EST")

[commit-bot: I haz the power, 2015-11-16 18:29:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438503002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438503002/160001

[commit-bot: I haz the power, 2015-11-16 18:31:22]

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

[commit-bot: I haz the power, 2015-11-16 18:31:23]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[hal.canary, 2015-11-16 18:37:59]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-11-16 18:37:59]

The patchset sent to the CQ was uploaded after l-g-t-m from tomhudson@google.com
Link to the patchset: https://codereview.chromium.org/1438503002/#ps180001
(title: "2015-11-16 (Monday) 13:37:23 EST")

[commit-bot: I haz the power, 2015-11-16 18:38:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438503002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438503002/180001

[commit-bot: I haz the power, 2015-11-16 18:51:25]

Committed patchset #9 (id:180001) as
https://skia.googlesource.com/skia/+/05b48e2a24ae78312c80ed94195f0b705fdb0547