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

dm/DMSrcSink.cpp

Index: dm/DMSrcSink.cpp
diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index affb844bc6aa39f76d5c2b5150f6c8954ea5fe94..dd4878257d722cb8cdb23e5909a799f94e13444a 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -173,18 +173,38 @@ Error GPUSink::draw(const Src& src, SkBitmap* dst, SkWStream*) const {
 PDFSink::PDFSink() {}
 
 Error PDFSink::draw(const Src& src, SkBitmap*, SkWStream* dst) const {
-    SkSize size;
-    size = src.size();
+    // Print the given DM:Src to a PDF, breaking on 8.5x11 pages.
     SkAutoTUnref<SkDocument> doc(SkDocument::CreatePDF(dst));
-    SkCanvas* canvas = doc->beginPage(size.width(), size.height());
 
-    Error err = src.draw(canvas);
-    if (!err.isEmpty()) {
-        return err;
+    int width = src.size().width();
+    int height = src.size().height();
+
+    const int kLetterWidth = 612;   // 8.5 * 72
+    const int kLetterHeight = 792;  // 11 * 72
+    const SkScalar kLetterWidthScalar = SkIntToScalar(kLetterWidth);

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

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.

+    const SkScalar kLetterHeightScalar = SkIntToScalar(kLetterHeight);
+    SkRect letterRect = SkRect::MakeWH(kLetterWidthScalar, kLetterHeightScalar);
+
+    int xPages = ((width - 1) / kLetterWidth) + 1;
+    int yPages = ((height - 1) / kLetterHeight) + 1;
+
+    for (int y = 0; y < yPages; ++y) {
+        for (int x = 0; x < xPages; ++x) {
+            int w = SkTMin(kLetterWidth, width - (x * kLetterWidth));
+            int h = SkTMin(kLetterHeight, height - (y * kLetterHeight));
+            SkCanvas* canvas = doc->beginPage(w, h);
+            canvas->clipRect(letterRect);
+            canvas->translate(-kLetterWidthScalar * x,
+                              -kLetterHeightScalar * y);
+            Error err = src.draw(canvas);
+            if (!err.isEmpty()) {
+                return err;
+            }
+            doc->endPage();
+        }
     }
-    canvas->flush();
-    doc->endPage();
     doc->close();
+    dst->flush();
     return "";
 }