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

dm/DMSrcSink.cpp

 #include "DMSrcSink.h"
 #include "SamplePipeControllers.h"
 #include "SkCommonFlags.h"
 #include "SkDocument.h"
 #include "SkMultiPictureDraw.h"
 #include "SkOSFile.h"
 #include "SkPictureRecorder.h"
 #include "SkRandom.h"
 #include "SkStream.h"
 
@@ skipping 83 matching lines @@
 Name ImageSrc::name() const {
     Name name = SkOSPath::Basename(fPath.c_str());
     if (fSubsets > 0) {
         name.appendf("-%d-subsets", fSubsets);
     }
     return name;
 }
 
 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
 
 static const SkRect kSKPViewport = {0,0, 1000,1000};

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

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.

 
 SKPSrc::SKPSrc(SkString path) : fPath(path) {}
 
 Error SKPSrc::draw(SkCanvas* canvas) const {
     SkAutoTDelete<SkStream> stream(SkStream::NewFromFile(fPath.c_str()));
     if (!stream) {
         return SkStringPrintf("Couldn't read %s.", fPath.c_str());
     }
     SkAutoTUnref<SkPicture> pic(SkPicture::CreateFromStream(stream));
     if (!pic) {
@@ skipping 51 matching lines @@
         factory.abandonContexts();
     }
     return "";
 }
 
 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
 
 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 "";
 }
 
 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
 
 RasterSink::RasterSink(SkColorType colorType) : fColorType(colorType) {}
 
 Error RasterSink::draw(const Src& src, SkBitmap* dst, SkWStream*) const {
     const SkISize size = src.size();
     // If there's an appropriate alpha type for this color type, use it, otherwise use premul.
@@ skipping 147 matching lines @@
             surfaces.unrefAll();
             return "";
         }
         SkISize size() const SK_OVERRIDE { return fSize; }
         Name name() const SK_OVERRIDE { sk_throw(); return ""; }  // No one should be calling this.
     } proxy(fW, fH, pic, src.size());
     return fSink->draw(proxy, bitmap, stream);
 }
 
 }  // namespace DM