Pdfviewer refactoring.

Mostly superficial changes, to help me make sure I understand the
code while making modifications.

SkPdfRenderer:
First class I'm modifying. Move it into include/ and src/ directories.
Inherit from SkNoncopyable.
Replace load() with factory function which returns NULL if the load
fails.
Remove unload() and loaded(), which no longer make sense, since the
factory will return NULL on a failure to load, and unload() happens
on destruction.
Use a const char* for loading a PDF, following the convention of
SkStream::NewFromFile.
Remove unnecessary call to sqrt in SkPDFNativeRenderToBitmap.
Also in SkPDFNativeRenderToBitmap, use an appropriate SkScalar macro
to convert to an integer.
Use this-> when calling member functions.

pdf_viewer_main.cpp:
Call the new interface for SkPdfRenderer.

gyp files:
Refer to the new location of SkPdfRenderer.

R=edisonn@google.com

Review URL: https://codereview.chromium.org/59493011

git-svn-id: http://skia.googlecode.com/svn/trunk@12296 2bbb7eff-a529-9590-31e7-b0007b416f81

10 files changed, 117 insertions, 135 deletions

diff --git a/experimental/PdfViewer/SkPdfRenderer.h b/experimental/PdfViewer/inc/SkPdfRenderer.h
index 51b564973e..c978a739ec 100644
--- a/experimental/PdfViewer/SkPdfRenderer.h
+++ b/experimental/PdfViewer/inc/SkPdfRenderer.h
@@ -9,17 +9,16 @@
 #ifndef SkPdfRenderer_DEFINED
 #define SkPdfRenderer_DEFINED
 
-// TODO(edisonn): remove this dependency, and load only from a stream!
-#include "SkString.h"
+#include "SkTypes.h"
 
 class SkBitmap;
 class SkCanvas;
 class SkPdfNativeDoc;
 struct SkRect;
 class SkStream;
-class SkString;
 
 // What kind of content to render.
+// FIXME: Currently unused.
 enum SkPdfContent {
     kNoForms_SkPdfContent,
     kAll_SkPdfContent,
@@ -30,26 +29,20 @@ enum SkPdfContent {
  *  The SkPdfRenderer class is used to render a PDF into canvas.
  *
  */
-class SkPdfRenderer {
+class SkPdfRenderer : public SkNoncopyable {
 public:
-    SkPdfRenderer() : fPdfDoc(NULL) {}
-    virtual ~SkPdfRenderer() {unload();}
-
-    // Render a specific page into the canvas, in a specific rectangle.
-    bool renderPage(int page, SkCanvas* canvas, const SkRect& dst) const;
-
-    // TODO(edisonn): deprecated, should be removed!
-    bool load(const SkString inputFileName);
-
+    // Create a new renderer from a stream.
     // TODO(edisonn): replace it with a SkSmartStream which would know to to efficiently
     // deal with a HTTP stream.
-    bool load(SkStream* stream);
+    // FIXME: Untested.
+    static SkPdfRenderer* CreateFromStream(SkStream*);
+    // Create a new renderer from a file.
+    static SkPdfRenderer* CreateFromFile(const char* filename);
 
-    // Unloads the pdf document.
-    void unload();
+    ~SkPdfRenderer();
 
-    // Returns true if we succesfully loaded a document.
-    bool loaded() const {return fPdfDoc != NULL && pages() > 0;}
+    // Render a specific page into the canvas, in a specific rectangle.
+    bool renderPage(int page, SkCanvas* canvas, const SkRect& dst) const;
 
     // Returns the number of pages in the loaded pdf.
     int pages() const;
@@ -62,6 +55,8 @@ public:
     size_t bytesUsed() const;
 
 private:
+    // Takes ownership of SkPdfNativeDoc.
+    SkPdfRenderer(SkPdfNativeDoc*);
     SkPdfNativeDoc* fPdfDoc;
 };
 
diff --git a/experimental/PdfViewer/pdf_viewer_main.cpp b/experimental/PdfViewer/pdf_viewer_main.cpp
index 6f8fb538eb..5ecd2dbd97 100644
--- a/experimental/PdfViewer/pdf_viewer_main.cpp
+++ b/experimental/PdfViewer/pdf_viewer_main.cpp
@@ -214,81 +214,67 @@ static bool render_page(const SkString& outputDir,
 /** Reads an skp file, renders it to pdf and writes the output to a pdf file
  * @param inputPath The skp file to be read.
  * @param outputDir Output dir.
- * @param renderer The object responsible to render the skp object into pdf.
  */
-static bool process_pdf(const SkString& inputPath, const SkString& outputDir,
-                        SkPdfRenderer& renderer) {
+static bool process_pdf(const SkString& inputPath, const SkString& outputDir) {
     SkDebugf("Loading PDF:  %s\n", inputPath.c_str());
 
     SkString inputFilename = SkOSPath::SkBasename(inputPath.c_str());
 
-    bool success = true;
+    SkAutoTDelete<SkPdfRenderer> renderer(SkPdfRenderer::CreateFromFile(inputPath.c_str()));
+    if (NULL == renderer.get()) {
+        SkDebugf("Failure loading file %s\n", inputPath.c_str());
+        return false;
+    }
 
-    success = renderer.load(inputPath);
     if (FLAGS_showMemoryUsage) {
-        SkDebugf("Memory usage after load: %u\n", (unsigned int)renderer.bytesUsed());
+        SkDebugf("Memory usage after load: %u\n", (unsigned int) renderer->bytesUsed());
     }
 
     // TODO(edisonn): bench timers
     if (FLAGS_benchLoad > 0) {
         for (int i = 0 ; i < FLAGS_benchLoad; i++) {
-            success = renderer.load(inputPath) && success;
-            if (FLAGS_showMemoryUsage) {
+            SkAutoTDelete<SkPdfRenderer> benchRenderer(
+                    SkPdfRenderer::CreateFromFile(inputPath.c_str()));
+            if (NULL == benchRenderer.get()) {
+                SkDebugf("Failed to load on %ith attempt\n", i);
+            } else if (FLAGS_showMemoryUsage) {
                 SkDebugf("Memory usage after load %i number : %u\n", i,
-                         (unsigned int)renderer.bytesUsed());
+                         (unsigned int) benchRenderer->bytesUsed());
             }
         }
     }
 
-    if (success) {
-        if (!renderer.pages())
-        {
-            SkDebugf("ERROR: Empty PDF Document %s\n", inputPath.c_str());
-            return false;
-        } else {
-            for (int i = 0; i < FLAGS_benchRender + 1; i++) {
-                // TODO(edisonn) if (i == 1) start timer
-                if (strcmp(FLAGS_pages[0], "all") == 0) {
-                    for (int pn = 0; pn < renderer.pages(); ++pn) {
-                        success = render_page(
-                                outputDir,
-                                inputFilename,
-                                renderer,
-                                FLAGS_noExtensionForOnePagePdf && renderer.pages() == 1 ? -1 :
-                                                                                          pn) &&
-                                     success;
-                    }
-                } else if (strcmp(FLAGS_pages[0], "reverse") == 0) {
-                    for (int pn = renderer.pages() - 1; pn >= 0; --pn) {
-                        success = render_page(
-                                outputDir,
-                                inputFilename,
-                                renderer,
-                                FLAGS_noExtensionForOnePagePdf && renderer.pages() == 1 ? -1 :
-                                                                                          pn) &&
-                                   success;
-                    }
-                } else if (strcmp(FLAGS_pages[0], "first") == 0) {
-                    success = render_page(
-                            outputDir,
-                            inputFilename,
-                            renderer,
-                            FLAGS_noExtensionForOnePagePdf && renderer.pages() == 1 ? -1 : 0) &&
-                                success;
-                } else if (strcmp(FLAGS_pages[0], "last") == 0) {
-                    success = render_page(
-                            outputDir,
-                            inputFilename,
-                            renderer,
-                            FLAGS_noExtensionForOnePagePdf &&
-                                    renderer.pages() == 1 ? -1 : renderer.pages() - 1) && success;
-                } else {
-                    int pn = atoi(FLAGS_pages[0]);
-                    success = render_page(outputDir, inputFilename, renderer,
-                                          FLAGS_noExtensionForOnePagePdf &&
-                                              renderer.pages() == 1 ? -1 : pn) && success;
-                }
+    if (!renderer->pages()) {
+        // This should never happen, since CreateFromFile will return NULL if there are no pages.
+        SkASSERT(false);
+        SkDebugf("ERROR: Empty PDF Document %s\n", inputPath.c_str());
+        return false;
+    }
+
+    bool success = true;
+    for (int i = 0; i < FLAGS_benchRender + 1; i++) {
+        // TODO(edisonn) if (i == 1) start timer
+        if (strcmp(FLAGS_pages[0], "all") == 0) {
+            for (int pn = 0; pn < renderer->pages(); ++pn) {
+                success &= render_page(outputDir, inputFilename, *renderer,
+                        FLAGS_noExtensionForOnePagePdf && renderer->pages() == 1 ? -1 : pn);
+            }
+        } else if (strcmp(FLAGS_pages[0], "reverse") == 0) {
+            for (int pn = renderer->pages() - 1; pn >= 0; --pn) {
+                success &= render_page(outputDir, inputFilename, *renderer,
+                        FLAGS_noExtensionForOnePagePdf && renderer->pages() == 1 ? -1 : pn);
             }
+        } else if (strcmp(FLAGS_pages[0], "first") == 0) {
+            success &= render_page(outputDir, inputFilename, *renderer,
+                    FLAGS_noExtensionForOnePagePdf && renderer->pages() == 1 ? -1 : 0);
+        } else if (strcmp(FLAGS_pages[0], "last") == 0) {
+            success &= render_page(outputDir, inputFilename, *renderer,
+                    FLAGS_noExtensionForOnePagePdf && renderer->pages() == 1 ? -1
+                    : renderer->pages() - 1);
+        } else {
+            int pn = atoi(FLAGS_pages[0]);
+            success &= render_page(outputDir, inputFilename, *renderer,
+                    FLAGS_noExtensionForOnePagePdf && renderer->pages() == 1 ? -1 : pn);
         }
     }
 
@@ -303,23 +289,21 @@ static bool process_pdf(const SkString& inputPath, const SkString& outputDir,
  * parse_pdf.
  * @param input A directory or an pdf file.
  * @param outputDir Output dir.
- * @param renderer The object responsible to render the skp object into pdf.
  */
-static int process_input(const char* input, const SkString& outputDir,
-                         SkPdfRenderer& renderer) {
+static int process_input(const char* input, const SkString& outputDir) {
     int failures = 0;
     if (sk_isdir(input)) {
         SkOSFile::Iter iter(input, PDF_FILE_EXTENSION);
         SkString inputFilename;
         while (iter.next(&inputFilename)) {
             SkString inputPath = SkOSPath::SkPathJoin(input, inputFilename.c_str());
-            if (!process_pdf(inputPath, outputDir, renderer)) {
+            if (!process_pdf(inputPath, outputDir)) {
                 ++failures;
             }
         }
     } else {
         SkString inputPath(input);
-        if (!process_pdf(inputPath, outputDir, renderer)) {
+        if (!process_pdf(inputPath, outputDir)) {
             ++failures;
         }
     }
@@ -336,8 +320,6 @@ int tool_main(int argc, char** argv) {
         exit(-1);
     }
 
-    SkPdfRenderer renderer;
-
     SkString outputDir;
     if (FLAGS_writePath.count() == 1) {
         outputDir.set(FLAGS_writePath[0]);
@@ -345,8 +327,7 @@ int tool_main(int argc, char** argv) {
 
     int failures = 0;
     for (int i = 0; i < FLAGS_readPath.count(); i ++) {
-        failures += process_input(FLAGS_readPath[i], outputDir, renderer);
-        renderer.unload();
+        failures += process_input(FLAGS_readPath[i], outputDir);
     }
 
     reportPdfRenderStats();
diff --git a/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.cpp b/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.cpp
index 1d8f510f7f..d3fea715df 100644
--- a/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.cpp
+++ b/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.cpp
@@ -126,11 +126,11 @@ void SkPdfNativeDoc::init(const void* bytes, size_t length) {
 
     bool storeCatalog = true;
     while (xrefByteOffset >= 0) {
-        const unsigned char* trailerStart = readCrossReferenceSection(fFileContent + xrefByteOffset,
-                                                                      xrefstartKeywordLine);
+        const unsigned char* trailerStart = this->readCrossReferenceSection(fFileContent + xrefByteOffset,
+                                                                            xrefstartKeywordLine);
         xrefByteOffset = -1;
         if (trailerStart < xrefstartKeywordLine) {
-            readTrailer(trailerStart, xrefstartKeywordLine, storeCatalog, &xrefByteOffset, false);
+            this->readTrailer(trailerStart, xrefstartKeywordLine, storeCatalog, &xrefByteOffset, false);
             storeCatalog = false;
         }
     }
@@ -303,7 +303,7 @@ const unsigned char* SkPdfNativeDoc::readCrossReferenceSection(const unsigned ch
                 return current;
             }
 
-            addCrossSectionInfo(startId + i, generation, offset, *token.c_str() == 'f');
+            this->addCrossSectionInfo(startId + i, generation, offset, *token.c_str() == 'f');
         }
     }
     SkPdfReport(kInfo_SkPdfIssueSeverity, kNoIssue_SkPdfIssue,
@@ -363,7 +363,7 @@ const unsigned char* SkPdfNativeDoc::readTrailer(const unsigned char* trailerSta
 void SkPdfNativeDoc::addCrossSectionInfo(int id, int generation, int offset, bool isFreed) {
     // TODO(edisonn): security here, verify id
     while (fObjects.count() < id + 1) {
-        reset(fObjects.append());
+        this->reset(fObjects.append());
     }
 
     fObjects[id].fOffset = offset;
diff --git a/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.h b/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.h
index d8241376c1..73265cad89 100644
--- a/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.h
+++ b/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.h
@@ -68,6 +68,7 @@ public:
     SkPdfNativeDoc(const char* path);
 
     // TODO(edisonn) should be deprecated
+    // FIXME: Untested.
     SkPdfNativeDoc(SkStream* stream);
 
     ~SkPdfNativeDoc();
diff --git a/experimental/PdfViewer/SkPdfRenderer.cpp b/experimental/PdfViewer/src/SkPdfRenderer.cpp
index 2d79f13077..25fc41df88 100644
--- a/experimental/PdfViewer/SkPdfRenderer.cpp
+++ b/experimental/PdfViewer/src/SkPdfRenderer.cpp
@@ -3087,73 +3087,73 @@ bool SkPdfRenderer::renderPage(int page, SkCanvas* canvas, const SkRect& dst) co
     return true;
 }
 
-bool SkPdfRenderer::load(const SkString inputFileName) {
-    unload();
-
-    fPdfDoc = new SkPdfNativeDoc(inputFileName.c_str());
-    if (fPdfDoc->pages() == 0) {
-        delete fPdfDoc;
-        fPdfDoc = NULL;
+SkPdfRenderer* SkPdfRenderer::CreateFromFile(const char* inputFileName) {
+    // FIXME: SkPdfNativeDoc should have a similar Create function.
+    SkPdfNativeDoc* pdfDoc = SkNEW_ARGS(SkPdfNativeDoc, (inputFileName));
+    if (pdfDoc->pages() == 0) {
+        SkDELETE(pdfDoc);
+        return NULL;
     }
 
-    return fPdfDoc != NULL;
+    return SkNEW_ARGS(SkPdfRenderer, (pdfDoc));
 }
 
-bool SkPdfRenderer::load(SkStream* stream) {
-    unload();
-
+SkPdfRenderer* SkPdfRenderer::CreateFromStream(SkStream* stream) {
     // TODO(edisonn): create static function that could return NULL if there are errors
-    fPdfDoc = new SkPdfNativeDoc(stream);
-    if (fPdfDoc->pages() == 0) {
-        delete fPdfDoc;
-        fPdfDoc = NULL;
+    SkPdfNativeDoc* pdfDoc = SkNEW_ARGS(SkPdfNativeDoc, (stream));
+    if (pdfDoc->pages() == 0) {
+        SkDELETE(pdfDoc);
+        return NULL;
     }
 
-    return fPdfDoc != NULL;
+    return SkNEW_ARGS(SkPdfRenderer, (pdfDoc));
 }
 
+SkPdfRenderer::SkPdfRenderer(SkPdfNativeDoc* doc)
+    :fPdfDoc(doc) {
+}
 
-int SkPdfRenderer::pages() const {
-    return fPdfDoc != NULL ? fPdfDoc->pages() : 0;
+SkPdfRenderer::~SkPdfRenderer() {
+    SkDELETE(fPdfDoc);
 }
 
-void SkPdfRenderer::unload() {
-    delete fPdfDoc;
-    fPdfDoc = NULL;
+int SkPdfRenderer::pages() const {
+    SkASSERT(fPdfDoc != NULL);
+    return fPdfDoc->pages();
 }
 
 SkRect SkPdfRenderer::MediaBox(int page) const {
-    SkASSERT(fPdfDoc);
+    SkASSERT(fPdfDoc != NULL);
     return fPdfDoc->MediaBox(page);
 }
 
 size_t SkPdfRenderer::bytesUsed() const {
-    return fPdfDoc ? fPdfDoc->bytesUsed() : 0;
+    SkASSERT(fPdfDoc != NULL);
+    return fPdfDoc->bytesUsed();
 }
 
 bool SkPDFNativeRenderToBitmap(SkStream* stream,
                                SkBitmap* output,
                                int page,
-                               SkPdfContent content,
+                               SkPdfContent unused,
                                double dpi) {
     SkASSERT(page >= 0);
-    SkPdfRenderer renderer;
-    renderer.load(stream);
-    if (!renderer.loaded() || page >= renderer.pages() || page < 0) {
+    SkPdfRenderer* renderer = SkPdfRenderer::CreateFromStream(stream);
+    if (NULL == renderer) {
         return false;
     }
 
-    SkRect rect = renderer.MediaBox(page < 0 ? 0 :page);
+    SkRect rect = renderer->MediaBox(page < 0 ? 0 :page);
 
-    SkScalar width = SkScalarMul(rect.width(),  SkDoubleToScalar(sqrt(dpi / 72.0)));
-    SkScalar height = SkScalarMul(rect.height(),  SkDoubleToScalar(sqrt(dpi / 72.0)));
+    SkScalar width = SkScalarMul(rect.width(),  SkDoubleToScalar(dpi / 72.0));
+    SkScalar height = SkScalarMul(rect.height(),  SkDoubleToScalar(dpi / 72.0));
 
     rect = SkRect::MakeWH(width, height);
 
-    setup_bitmap(output, (int)SkScalarToDouble(width), (int)SkScalarToDouble(height));
+    setup_bitmap(output, SkScalarCeilToInt(width), SkScalarCeilToInt(height));
 
     SkAutoTUnref<SkBaseDevice> device(SkNEW_ARGS(SkBitmapDevice, (*output)));
     SkCanvas canvas(device);
 
-    return renderer.renderPage(page, &canvas, rect);
+    return renderer->renderPage(page, &canvas, rect);
 }
diff --git a/gyp/SampleApp.gyp b/gyp/SampleApp.gyp
index 3a0770296b..2708036d84 100644
--- a/gyp/SampleApp.gyp
+++ b/gyp/SampleApp.gyp
@@ -1,3 +1,4 @@
+#
 {
   'variables': {
     #manually set sample_pdf_file_viewer to 1 to have the PdfViewer in SampleApp
@@ -167,7 +168,7 @@
            'pdfviewer_lib.gyp:pdfviewer_lib',
          ],
          'include_dirs' : [
-           '../experimental/PdfViewer/',
+           '../experimental/PdfViewer/inc',
          ],
          'sources': [
            '../samplecode/SamplePdfFileViewer.cpp',
diff --git a/gyp/gm.gyp b/gyp/gm.gyp
index 504b11ec20..90c09a4f24 100644
--- a/gyp/gm.gyp
+++ b/gyp/gm.gyp
@@ -65,7 +65,7 @@
             'SK_BUILD_NATIVE_PDF_RENDERER',
           ],
           'include_dirs' : [
-            '../experimental/PdfViewer',
+            '../experimental/PdfViewer/inc',
           ],
           'dependencies': [
             'pdfviewer_lib.gyp:pdfviewer_lib',
diff --git a/gyp/pdfviewer.gyp b/gyp/pdfviewer.gyp
index 1f7877f179..763587710d 100644
--- a/gyp/pdfviewer.gyp
+++ b/gyp/pdfviewer.gyp
@@ -16,6 +16,7 @@
       ],
       'include_dirs': [
         '../experimental/PdfViewer',
+        '../experimental/PdfViewer/inc',
         '../experimental/PdfViewer/pdfparser',
         '../experimental/PdfViewer/pdfparser/native',
       ],
diff --git a/gyp/pdfviewer_lib.gyp b/gyp/pdfviewer_lib.gyp
index f9acdd6432..e25e024166 100644
--- a/gyp/pdfviewer_lib.gyp
+++ b/gyp/pdfviewer_lib.gyp
@@ -9,9 +9,13 @@
       'target_name': 'pdfviewer_lib',
       'type': 'static_library',
       'sources': [
+        # FIXME: Include directory is named "inc" (instead of "include") in
+        # order to not be considered the public API.
+        '../experimental/PdfViewer/inc/SkPdfRenderer.h',
+        '../experimental/PdfViewer/src/SkPdfRenderer.cpp',
+
         '../experimental/PdfViewer/SkPdfGraphicsState.cpp',
         '../experimental/PdfViewer/SkPdfFont.cpp',
-        '../experimental/PdfViewer/SkPdfRenderer.cpp',
         '../experimental/PdfViewer/SkPdfReporter.cpp',
         '../experimental/PdfViewer/SkPdfUtils.cpp',
         #'../experimental/PdfViewer/SkPdfNYI.cpp',
@@ -25,6 +29,7 @@
       ],
       'include_dirs': [
         '../experimental/PdfViewer',
+        '../experimental/PdfViewer/inc',
         '../experimental/PdfViewer/pdfparser',
         '../experimental/PdfViewer/pdfparser/native',
         '../experimental/PdfViewer/pdfparser/native/pdfapi',
diff --git a/samplecode/SamplePdfFileViewer.cpp b/samplecode/SamplePdfFileViewer.cpp
index fc8b0f0acd..55c7002fa8 100644
--- a/samplecode/SamplePdfFileViewer.cpp
+++ b/samplecode/SamplePdfFileViewer.cpp
@@ -37,18 +37,16 @@ private:
     SkPicture*  fPicture;  // TODO(edisonn): multiple pages, one page / picture, make it an array
 
     static SkPicture* LoadPdf(const char path[]) {
-        SkPicture* pic = NULL;
-
-        SkPdfRenderer renderer;
-        SkString skpath;
-        skpath.append(path);
-        renderer.load(skpath);
-        if (renderer.loaded()) {
-            pic = SkNEW(SkPicture);
-            SkCanvas* canvas = pic->beginRecording((int)renderer.MediaBox(0).width(), (int)renderer.MediaBox(0).height());
-            renderer.renderPage(0, canvas, renderer.MediaBox(0));
-            pic->endRecording();
+        SkAutoTDelete<SkPdfRenderer> renderer(SkPdfRenderer::CreateFromFile(path));
+        if (NULL == renderer.get()) {
+            return NULL;
         }
+
+        SkPicture* pic = SkNEW(SkPicture);
+        SkCanvas* canvas = pic->beginRecording((int) renderer->MediaBox(0).width(),
+                                               (int) renderer->MediaBox(0).height());
+        renderer->renderPage(0, canvas, renderer->MediaBox(0));
+        pic->endRecording();
         return pic;
     }