SkPDF: Change handling of large-numbers in matricies.

SkPDF: Change handling of large-numbers in matricies.

re-do http://crrev.com/1438503002 right.

Also, add a unit test that asserts without this change.

BUG=skia:734
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1552263003

Committed: https://skia.googlesource.com/skia/+/53b1c09c139d2e8e04bc86e230256a23629ad7f2

[hal.canary, 2016-01-04 22:03:43]

Description was changed from

==========
SkPDF: Change handling of large-numbers in matricies.

re-do http://crrev.com/1438503002 right.

Also, add a unit test that asserts without this change.

BUG=skia:734
==========

to

==========
SkPDF: Change handling of large-numbers in matricies.

re-do http://crrev.com/1438503002 right.

Also, add a unit test that asserts without this change.

BUG=skia:734
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[hal.canary, 2016-01-04 22:04:11]

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

[commit-bot: I haz the power, 2016-01-04 22:04:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1552263003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1552263003/1

[hal.canary, 2016-01-04 22:06:30]

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

[hal.canary, 2016-01-04 22:06:30]

PTAL

[commit-bot: I haz the power, 2016-01-04 22:18:20]

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

[commit-bot: I haz the power, 2016-01-04 22:18:21]

Dry run: This issue passed the CQ dry run.

[tomhudson, 2016-01-05 15:28:15]

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

https://codereview.chromium.org/1552263003/diff/1/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:822: newMatrix = untranslate(*d.fMatrix, points[0].x(),
points[0].y());
Do we have any guarantee that this will fix the problem?
It looks more like this is our best hope, but that if points[0] was close to the
origin it won't make any difference?

https://codereview.chromium.org/1552263003/diff/1/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1334: y = 0;
So the problem we're handling is purely too-large-a-translation encoded in the
matrix, never too-large-a-translation specified in (x, y)?

https://codereview.chromium.org/1552263003/diff/1/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1410: SkASSERT(offset.isZero());
This is unintuitive, particularly given the degree to which the following code
caters to non-zero offsets. Can we make it clearer?

https://codereview.chromium.org/1552263003/diff/1/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1418: if (first.x() != 0 || first.y() != 0) {
Not first.x() + srcOffset.x() ?

[hal.canary, 2016-01-05 18:11:22]

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

https://codereview.chromium.org/1552263003/diff/1/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:822: newMatrix = untranslate(*d.fMatrix, points[0].x(),
points[0].y());
On 2016/01/05 at 15:28:15, tomhudson wrote:
> Do we have any guarantee that this will fix the problem?
> It looks more like this is our best hope, but that if points[0] was close to
the origin it won't make any difference?

No, we don't guarantee that we fix anything.  But very large (eg) positive
translations will need large negative points to stay on the page.

https://codereview.chromium.org/1552263003/diff/1/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1334: y = 0;
On 2016/01/05 at 15:28:15, tomhudson wrote:
> So the problem we're handling is purely too-large-a-translation encoded in the
matrix, never too-large-a-translation specified in (x, y)?

In practice, we don't see that.  But, in theory, we could.  If that comes up, we
can change the logic of excessive_translation()

https://codereview.chromium.org/1552263003/diff/1/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1410: SkASSERT(offset.isZero());
On 2016/01/05 at 15:28:15, tomhudson wrote:
> This is unintuitive, particularly given the degree to which the following code
caters to non-zero offsets. Can we make it clearer?

In SkCanvas.cpp SkCanvas::onDrawPosText(), does this:

    SkPoint textOffset = SkPoint::Make(0, 0);

That said, I'll remove this assert since it is unnecessary the way I wrote the
code.

https://codereview.chromium.org/1552263003/diff/1/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1418: if (first.x() != 0 || first.y() != 0) {
On 2016/01/05 at 15:28:15, tomhudson wrote:
> Not first.x() + srcOffset.x() ?

no.  I shift the matrix by first+offset.  I shift the offset by -offset.  I
shift the points by -first.  It all adds up to zero.

[tomhudson, 2016-01-06 16:40:25]

lgtm

[hal.canary, 2016-01-06 16:45:01]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2016-01-06 16:45:10]

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

[commit-bot: I haz the power, 2016-01-06 17:02:28]

Description was changed from

==========
SkPDF: Change handling of large-numbers in matricies.

re-do http://crrev.com/1438503002 right.

Also, add a unit test that asserts without this change.

BUG=skia:734
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
SkPDF: Change handling of large-numbers in matricies.

re-do http://crrev.com/1438503002 right.

Also, add a unit test that asserts without this change.

BUG=skia:734
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/53b1c09c139d2e8e04bc86e230256a23629ad7f2
==========

[commit-bot: I haz the power, 2016-01-06 17:02:28]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/53b1c09c139d2e8e04bc86e230256a23629ad7f2