Changes to SkTDStackNester.

SkTDStackNester is a class used by PdfViewer to assist in saving
and restoring the PDF state. Clean up and test this class.

Add some documentation.

Add FIXME's where I have questions to resolve.

Fix a bug where fNestingLevel was not initialized.

Remove a commented out line of code copied over from
SkTDStack.

Rename SkTDStackNester::nests() to nestingLevel() and make it const.

Remove unnecessary predeclaration and friend declaration.

Remove index() (both const and non-const versions). They were
unused, return something that may not be expected (index from
the top, rather than from the bottom), and don't work to get any
elements in earlier Recs once the first one is full.

Report a warning if the nesting level goes above the maximum level,
or if we attempt to bring it below zero.

Prevent fNestingLevel from dropping below zero.

Add kUnusedObject_SkPdfIssue, and use it where appropriate.

Depends on https://codereview.chromium.org/64093009/

R=mtklein@google.com

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

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

5 files changed, 99 insertions, 32 deletions

diff --git a/experimental/PdfViewer/SkPdfReporter.cpp b/experimental/PdfViewer/SkPdfReporter.cpp
index f4e8ea4963..7aebc431bf 100644
--- a/experimental/PdfViewer/SkPdfReporter.cpp
+++ b/experimental/PdfViewer/SkPdfReporter.cpp
@@ -47,7 +47,7 @@ void SkPdfReportIf(bool report,
                    SkPdfIssueSeverity sev, SkPdfIssue issue,
                    const char* context,
                    const SkPdfNativeObject* obj,
-SkPdfContext* pdfContext) {
+                   SkPdfContext* pdfContext) {
     if (!report) {
         return;
     }
diff --git a/experimental/PdfViewer/SkPdfReporter.h b/experimental/PdfViewer/SkPdfReporter.h
index be93959864..3718154d84 100644
--- a/experimental/PdfViewer/SkPdfReporter.h
+++ b/experimental/PdfViewer/SkPdfReporter.h
@@ -34,6 +34,7 @@ enum SkPdfIssue {
     kNoIssue_SkPdfIssue,
 
     kNullObject_SkPdfIssue,
+    kUnusedObject_SkPdfIssue,
     kUnexpectedArraySize_SkPdfIssue,
     kMissingEncoding_SkPdfIssue,
     kNYI_SkPdfIssue,
diff --git a/experimental/PdfViewer/pdfparser/native/SkPdfNativeObject.cpp b/experimental/PdfViewer/pdfparser/native/SkPdfNativeObject.cpp
index a1d5df049f..1682fb92b2 100644
--- a/experimental/PdfViewer/pdfparser/native/SkPdfNativeObject.cpp
+++ b/experimental/PdfViewer/pdfparser/native/SkPdfNativeObject.cpp
@@ -110,7 +110,7 @@ bool SkPdfNativeObject::filterStream() {
 
 void SkPdfNativeObject::releaseData() {
 #ifdef PDF_TRACK_OBJECT_USAGE
-    SkPdfReportIf(!fUsed, kInfo_SkPdfIssueSeverity, kNoIssue_SkPdfIssue,
+    SkPdfReportIf(!fUsed, kInfo_SkPdfIssueSeverity, kUnusedObject_SkPdfIssue,
                   "Unused object in rendering", this, NULL);
 #endif  // PDF_TRACK_OBJECT_USAGE
 
diff --git a/experimental/PdfViewer/src/SkPdfRenderer.cpp b/experimental/PdfViewer/src/SkPdfRenderer.cpp
index 25fc41df88..9747aad402 100644
--- a/experimental/PdfViewer/src/SkPdfRenderer.cpp
+++ b/experimental/PdfViewer/src/SkPdfRenderer.cpp
@@ -1133,7 +1133,7 @@ SkPdfResult PdfOp_Q(SkPdfContext* pdfContext, SkCanvas* canvas, PdfTokenLooper**
         pdfContext->fStateStack.pop();
         canvas->restore();
 
-        if (pdfContext->fObjectStack.nests() == 0) {
+        if (pdfContext->fObjectStack.nestingLevel() == 0) {
             SkPdfReport(kIgnoreError_SkPdfIssueSeverity, kStackNestingOverflow_SkPdfIssue,
                         "stack nesting overflow (q/Q)", NULL, pdfContext);
             return kIgnoreError_SkPdfResult;
diff --git a/experimental/PdfViewer/src/SkTDStackNester.h b/experimental/PdfViewer/src/SkTDStackNester.h
index 4910ad1a3d..dd936baaed 100644
--- a/experimental/PdfViewer/src/SkTDStackNester.h
+++ b/experimental/PdfViewer/src/SkTDStackNester.h
@@ -9,25 +9,34 @@
 #define SkTDStackNester_DEFINED
 
 #include "SkTypes.h"
+#include "SkPdfReporter.h"
 
-// Adobe limits it to 28, so 256 should be more than enough
+// Adobe limits it to 28. Allow deeper nesting in case a file does not quite meet the
+// spec. 256 should be more than enough.
 #define MAX_NESTING 256
 
 /** \class SkTDStackNester
  *
+ * Specialized version of SkTDStack which allows a stack of stacks.
+ * FIXME (scroggo): Could this be a subclass of SkTDStack? Could it have-a SkTDStack?
  * The difference between SkTDStackNester and SkTDStack is that:
  *   - SkTDStackNester uses new/delete to manage initializations
+ *     FIXME (scroggo): Why use new rather than malloc?
  *   - Supports nest/unnest which simulates a stack of stack. unnest will pop all the
  *     objects pushed since the last nest
+ *   - kSlotCount is 64, instead of 8.
+ *     FIXME (scroggo): How did we arrive at this number?
  */
 
 template <typename T> class SkTDStackNester : SkNoncopyable {
 public:
-    SkTDStackNester() : fCount(0), fTotalCount(0), fLocalCount(0) {
+    SkTDStackNester()
+        : fCount(0)
+        , fLocalCount(0)
+        , fNestingLevel(0) {
         fInitialRec.fNext = NULL;
         fRec = &fInitialRec;
-
-    //  fCount = kSlotCount;
+        SkDEBUGCODE(fTotalCount = 0;)
     }
 
     ~SkTDStackNester() {
@@ -39,36 +48,73 @@ public:
         }
     }
 
+    /**
+     * Return the number of objects in the current nesting level.
+     */
     int count() const { return fLocalCount; }
+
+    /**
+     * Whether the current nesting level is empty.
+     */
     bool empty() const { return fLocalCount == 0; }
 
-    int nests() {
+    /**
+     * The current nesting level.
+     */
+    int nestingLevel() const {
         return fNestingLevel;
     }
 
+    /**
+     * Analogous to an SkCanvas::save(). When unnest() is called, the state of this SkTDStackNester
+     * will return to its state when nest() was called.
+     *
+     * After a call to nest(), fLocalCount is reset to 0, since the stack is on a new nesting
+     * level.
+     */
     void nest() {
-        // We are are past max nesting levels, we will still continue to work, but we might fail
-        // to properly ignore errors. Ideally it should only mean poor rendering in exceptional
-        // cases
-        if (fNestingLevel >= 0 && fNestingLevel < MAX_NESTING) {
+        SkASSERT(fNestingLevel >= 0);
+        if (fNestingLevel < MAX_NESTING) {
             fNestings[fNestingLevel] = fLocalCount;
             fLocalCount = 0;
+        } else {
+            // We are are past max nesting levels. We will still continue to work, but we might fail
+            // to properly ignore errors. Ideally it should only mean poor rendering in exceptional
+            // cases.
+            SkPdfReport(kWarning_SkPdfIssueSeverity, kStackNestingOverflow_SkPdfIssue,
+                        "Past maximum nesting level", NULL, NULL);
         }
         fNestingLevel++;
     }
 
+    /**
+     * Analagous to an SkCanvas::restore(). Will revert this stack to the state it was in the last
+     * time nest() was called. It is an error to call unnest() more times than nest() has been
+     * called.
+     */
     void unnest() {
-        SkASSERT(fNestingLevel > 0);
+        SkASSERT(fNestingLevel >= 0);
+        if (0 == fNestingLevel) {
+            SkPdfReport(kWarning_SkPdfIssueSeverity, kStackNestingOverflow_SkPdfIssue,
+                        "Nesting underflow", NULL, NULL);
+            return;
+        }
+
         fNestingLevel--;
-        if (fNestingLevel >= 0 && fNestingLevel < MAX_NESTING) {
-            // TODO(edisonn): warn if fLocal > 0
+        if (fNestingLevel < MAX_NESTING) {
             while (fLocalCount > 0) {
-                pop();
+                // FIXME (scroggo): Pass the object?
+                SkPdfReport(kInfo_SkPdfIssueSeverity, kUnusedObject_SkPdfIssue,
+                            "Unused object when calling unnest!", NULL, NULL);
+                this->pop();
             }
             fLocalCount = fNestings[fNestingLevel];
         }
     }
 
+    /**
+     * Add an object to the stack, and return a pointer to it for modification.
+     */
     T* push() {
         SkASSERT(fCount <= kSlotCount);
         if (fCount == kSlotCount) {
@@ -77,33 +123,35 @@ public:
             fRec = rec;
             fCount = 0;
         }
-        ++fTotalCount;
+        SkDEBUGCODE(++fTotalCount;)
         ++fLocalCount;
         return &fRec->fSlots[fCount++];
     }
 
+    /**
+     * Add an object to the stack, copied from elem.
+     */
     void push(const T& elem) { *this->push() = elem; }
 
-    const T& index(int idx) const {
-        SkASSERT(fRec && fCount > idx);
-        return fRec->fSlots[fCount - idx - 1];
-    }
-
-    T& index(int idx) {
-        SkASSERT(fRec && fCount > idx);
-        return fRec->fSlots[fCount - idx - 1];
-    }
-
+    /**
+     * Return the top element.
+     */
     const T& top() const {
         SkASSERT(fRec && fCount > 0);
         return fRec->fSlots[fCount - 1];
     }
 
+    /**
+     * Return the top element.
+     */
     T& top() {
         SkASSERT(fRec && fCount > 0);
         return fRec->fSlots[fCount - 1];
     }
 
+    /**
+     * Pop an object off the stack (via pop()), and copy its members into elem.
+     */
     void pop(T* elem) {
         if (elem) {
             *elem = fRec->fSlots[fCount - 1];
@@ -111,10 +159,15 @@ public:
         this->pop();
     }
 
+    /**
+     * Pop an object off the stack. It is an error to call pop() more times
+     * than push() has been called in total or since the last call to nest().
+     */
     void pop() {
         SkASSERT(fCount > 0 && fRec);
+        SkASSERT(fLocalCount > 0);
         --fLocalCount;
-        --fTotalCount;
+        SkDEBUGCODE(--fTotalCount;)
         if (--fCount == 0) {
             if (fRec != &fInitialRec) {
                 Rec* rec = fRec->fNext;
@@ -129,20 +182,33 @@ public:
 
 private:
     enum {
+        // Number of objects held per Rec. Storing multiple objects in one Rec
+        // means that we call new less often.
         kSlotCount  = 64
     };
 
-    struct Rec;
-    friend struct Rec;
-
     struct Rec {
         Rec* fNext;
         T    fSlots[kSlotCount];
     };
+
+    // First Rec, requiring no allocation.
     Rec     fInitialRec;
+    // The Rec on top of the stack.
     Rec*    fRec;
-    int     fCount, fTotalCount, fLocalCount;
+    // Number of objects in fRec.
+    int     fCount;
+    // Number of objects in the current nesting level.
+    int     fLocalCount;
+    // Array of counts of objects per nesting level.
+    // Only valid for fNestings[0] through fNestings[fNestingLevel-1].
     int     fNestings[MAX_NESTING];
+    // Current nesting level.
     int     fNestingLevel;
+    // Total number of objects in this SkTDStackNester.
+    SkDEBUGCODE(int     fTotalCount;)
+
+    // For testing.
+    friend class SkTDStackNesterTester;
 };
 #endif // SkTDStackNester_DEFINED