Simplify SkPdfTokenLooper behavior.

Simplify SkPdfTokenLooper behavior.

Instead of passing a pointer to a pointer to a NULL looper, which can
then be set to point to a new looper (which then must be looped and
deleted), pass a pointer to the current looper. Each function can then
create a looper based on the parent (on the stack) and call loop()
itself.

Remove setUp(). Now that there is a pointer to the parent at creation
time, there is no need for this function.

Modify the constructors to only provide ones that are needed.

Add documentation.

Remove PdfInlineImageLooper::done(), which is never used.

R=mtklein@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12447

[mtklein, 2013-11-22 18:04:37]

Nits only.  Big improvement!  lgtm

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/i...
File experimental/PdfViewer/inc/SkPdfTokenLooper.h (right):

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/i...
experimental/PdfViewer/inc/SkPdfTokenLooper.h:41:
SkPdfTokenLooper(SkPdfTokenLooper* parent)
explicit?

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
File experimental/PdfViewer/src/SkPdfRenderer.cpp (right):

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
experimental/PdfViewer/src/SkPdfRenderer.cpp:211:
PdfInlineImageLooper(SkPdfTokenLooper* parent)
explicit?

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
experimental/PdfViewer/src/SkPdfRenderer.cpp:923: SkPdfResult
PdfOp_q(SkPdfContext* pdfContext, SkCanvas* canvas, SkPdfTokenLooper*
parentLooper) {
I like what you've got below, dropping the name when unused.  Another
alternative I like is

  ..., SkPdfTokenLooper* /*parentLooper*/)

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
experimental/PdfViewer/src/SkPdfRenderer.cpp:930: SkPdfResult
PdfOp_Q(SkPdfContext* pdfContext, SkCanvas* canvas, SkPdfTokenLooper*
parentLooper) {
Same

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
experimental/PdfViewer/src/SkPdfRenderer.cpp:1784: static SkPdfResult
PdfOp_G(SkPdfContext* pdfContext, SkCanvas* canvas, SkPdfTokenLooper*
parentLooper) {
same

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
experimental/PdfViewer/src/SkPdfRenderer.cpp:1788: static SkPdfResult
PdfOp_g(SkPdfContext* pdfContext, SkCanvas* canvas, SkPdfTokenLooper*
parentLooper) {
same

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
experimental/PdfViewer/src/SkPdfRenderer.cpp:1849: SkPdfTokenLooper*
parentLooper) {
same

[scroggo, 2013-12-02 21:19:53]

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/i...
File experimental/PdfViewer/inc/SkPdfTokenLooper.h (right):

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/i...
experimental/PdfViewer/inc/SkPdfTokenLooper.h:41:
SkPdfTokenLooper(SkPdfTokenLooper* parent)
On 2013/11/22 18:04:37, mtklein wrote:
> explicit?

Done.

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
File experimental/PdfViewer/src/SkPdfRenderer.cpp (right):

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
experimental/PdfViewer/src/SkPdfRenderer.cpp:211:
PdfInlineImageLooper(SkPdfTokenLooper* parent)
On 2013/11/22 18:04:37, mtklein wrote:
> explicit?

Done.

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
experimental/PdfViewer/src/SkPdfRenderer.cpp:923: SkPdfResult
PdfOp_q(SkPdfContext* pdfContext, SkCanvas* canvas, SkPdfTokenLooper*
parentLooper) {
On 2013/11/22 18:04:37, mtklein wrote:
> I like what you've got below, dropping the name when unused.  Another
> alternative I like is
> 
>   ..., SkPdfTokenLooper* /*parentLooper*/)

Done.

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
experimental/PdfViewer/src/SkPdfRenderer.cpp:930: SkPdfResult
PdfOp_Q(SkPdfContext* pdfContext, SkCanvas* canvas, SkPdfTokenLooper*
parentLooper) {
On 2013/11/22 18:04:37, mtklein wrote:
> Same

Done.

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
experimental/PdfViewer/src/SkPdfRenderer.cpp:1784: static SkPdfResult
PdfOp_G(SkPdfContext* pdfContext, SkCanvas* canvas, SkPdfTokenLooper*
parentLooper) {
On 2013/11/22 18:04:37, mtklein wrote:
> same

Done.

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
experimental/PdfViewer/src/SkPdfRenderer.cpp:1788: static SkPdfResult
PdfOp_g(SkPdfContext* pdfContext, SkCanvas* canvas, SkPdfTokenLooper*
parentLooper) {
On 2013/11/22 18:04:37, mtklein wrote:
> same

Done.

https://codereview.chromium.org/83183004/diff/110001/experimental/PdfViewer/s...
experimental/PdfViewer/src/SkPdfRenderer.cpp:1849: SkPdfTokenLooper*
parentLooper) {
On 2013/11/22 18:04:37, mtklein wrote:
> same

Done.

[mtklein, 2013-12-02 21:28:08]

lgtm, and as seems to be usual now, a suggestion for the next round of cleanup.
:)

https://codereview.chromium.org/83183004/diff/190001/experimental/PdfViewer/i...
File experimental/PdfViewer/inc/SkPdfTokenLooper.h (right):

https://codereview.chromium.org/83183004/diff/190001/experimental/PdfViewer/i...
experimental/PdfViewer/inc/SkPdfTokenLooper.h:29:
SkPdfTokenLooper(SkPdfNativeTokenizer* tokenizer,
This has just occurred to me (I think, maybe I've asked this before...): why
does SkPdfTokenLooper have anything but

struct SkPdfTokenLooper {
  virtual ~SkPdfTokenLooper() {}

  virtual SkPdfResult consumeToken(PdfToken& token) = 0;
  virtual void loop() = 0;
};

I.e. can't it be an interface?  The members are just there for convenience
right?  Do all subclasses need all the members?

[scroggo, 2013-12-02 22:34:28]

https://codereview.chromium.org/83183004/diff/190001/experimental/PdfViewer/i...
File experimental/PdfViewer/inc/SkPdfTokenLooper.h (right):

https://codereview.chromium.org/83183004/diff/190001/experimental/PdfViewer/i...
experimental/PdfViewer/inc/SkPdfTokenLooper.h:29:
SkPdfTokenLooper(SkPdfNativeTokenizer* tokenizer,
On 2013/12/02 21:28:08, mtklein wrote:
> This has just occurred to me (I think, maybe I've asked this before...): why
> does SkPdfTokenLooper have anything but
> 
> struct SkPdfTokenLooper {
>   virtual ~SkPdfTokenLooper() {}
> 
>   virtual SkPdfResult consumeToken(PdfToken& token) = 0;
>   virtual void loop() = 0;
> };
> 
> I.e. can't it be an interface?  The members are just there for convenience
> right?  Do all subclasses need all the members?

All subclasses need fTokenizer, fPdfContext, and fCanvas. Only
PdfCompatibilitySectionLooper uses fParent (though PdfInlineImageLooper uses it
to set the other fields). As noted in a FIXME elsewhere, PdfInlineImageLooper
ONLY exists so it can have access to the protected fields of SkPdfTokenLooper.

Added a FIXME to continue investigating.

[scroggo, 2013-12-02 22:34:51]

Committed patchset #7 manually as r12447 (presubmit successful).