DM::PDFSink::draw excercises multi-page pdf

DM::PDFSink::draw excercises multi-page pdf

BUG=skia:3365
Committed: https://skia.googlesource.com/skia/+/fd4a993f8b162f6ca541514234a81759b67dc64d

[hal.canary, 2015-01-28 16:01:06]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-01-28 16:01:45]

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

[commit-bot: I haz the power, 2015-01-28 16:01:45]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-01-28 22:01 UTC

[commit-bot: I haz the power, 2015-01-28 16:02:56]

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

[commit-bot: I haz the power, 2015-01-28 16:02:56]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[hal.canary, 2015-01-28 16:04:20]

Patchset #1 (id:1) has been deleted

[hal.canary, 2015-01-28 16:04:23]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-01-28 16:04:46]

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

[commit-bot: I haz the power, 2015-01-28 16:04:47]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-01-28 22:04 UTC

[hal.canary, 2015-01-28 16:05:45]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2015-01-28 16:08:28]

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

[hal.canary, 2015-01-28 16:08:29]

PTAL

[mtklein, 2015-01-28 16:08:30]

Are you seeing anything span multiple pages?

https://codereview.chromium.org/881343002/diff/20001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/881343002/diff/20001/dm/DMSrcSink.cpp#newcode104
dm/DMSrcSink.cpp:104: static const SkRect kSKPViewport = {0,0, 1000,1000};
I suspect a change like this will require us to rethink always clipping SKPs on
the Src side.  Seems to me that PDF canvases want unclipped SKPs, and everyone
else wants clipped SKPs (but not clipped GMs).  Interesting problem to think
through and see if we can sort out without some sort of hack of Srcs and Sinks
identifying themselves to each other.

[hal.canary, 2015-01-28 16:12:50]

On 2015/01/28 16:08:30, mtklein wrote:
> Are you seeing anything span multiple pages?

Many GMs are wider than 612 pixels.

> https://codereview.chromium.org/881343002/diff/20001/dm/DMSrcSink.cpp
> File dm/DMSrcSink.cpp (right):
> 
>
https://codereview.chromium.org/881343002/diff/20001/dm/DMSrcSink.cpp#newcode104
> dm/DMSrcSink.cpp:104: static const SkRect kSKPViewport = {0,0, 1000,1000};
> I suspect a change like this will require us to rethink always clipping SKPs
on
> the Src side.  Seems to me that PDF canvases want unclipped SKPs, and everyone
> else wants clipped SKPs (but not clipped GMs).  Interesting problem to think
> through and see if we can sort out without some sort of hack of Srcs and Sinks
> identifying themselves to each other.

We want to clip for bitmap output, but not for stream output.  Let me take a
look.

[mtklein, 2015-01-28 16:28:19]

> We want to clip for bitmap output, but not for stream output.  Let me take a
> look.

That's a bit that discriminates this choice correctly today, but it's not clear
to me that's really the fundamental distinction here.  Might be though.

Srcs don't think about bitmaps and streams today, and I'd like to keep it that
way if possible.

[mtklein, 2015-01-28 16:28:26]

https://codereview.chromium.org/881343002/diff/20001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/881343002/diff/20001/dm/DMSrcSink.cpp#newcode184
dm/DMSrcSink.cpp:184: const SkScalar kLetterWidthScalar =
SkIntToScalar(kLetterWidth);
I think this would be more readable with fewer constants and more consistent
naming.

Let's fold these kLetter*Scalar into letterRect (and just call it 'letter'?),
and translate like this:
  canvas->translate(-letter.width()*x, -letter.height()*y);

I also find it's more readable to initialize related values of the same type in
one multi-line statement, aligning both on the = and where semantically
appropriate in the names:

    int width = src.size().width(),
       height = src.size().height();

   const int kLetterWidth  = 612,  // 8.5in * 72 dpi
             kLetterHeight = 792;  //  11in * 72 dpi

It's confusing to see some things be 'const T kFoo' and others be 'T foo' when
neither changes.  E.g., why is it not const SkRect kLetterRect?  Or const int
kWidth?  This code is very local, so there's no real need to go nuts with
kPrefixes... 'const int width', 'const int letterWidth' is plenty reminder. 
With code as small as this is, you might find it even more readable to drop
const altogether.  The noise of long names, repeated types in declarations,
const and kThis are coming together to overpower the meat of the code here.

[hal.canary, 2015-01-28 19:37:53]

Patchset #4 (id:80001) has been deleted

[hal.canary, 2015-01-28 19:37:59]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-01-28 19:38:25]

some changes.  let's land and iterate.

[commit-bot: I haz the power, 2015-01-28 19:38:54]

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

[commit-bot: I haz the power, 2015-01-28 19:38:56]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-01-29 01:38 UTC

[mtklein, 2015-01-28 19:43:51]

lgtm

[commit-bot: I haz the power, 2015-01-28 19:46:01]

Committed patchset #3 (id:60001) as
https://skia.googlesource.com/skia/+/fd4a993f8b162f6ca541514234a81759b67dc64d