[PDF] Add helper class to manage ContentEntry set up and completion.

[PDF] Add helper class to manage ContentEntry set up and completion.

This stack object helper class calls finishContentEntry when it goes out of scope, maintains the current content entry, and manages the dst form xobject when it is needed.  This can be made cleaner by moving the guts of SkPDFDevice into a core object, which can expose setUp/finishContentEntry as public, but that is left as a todo.

Committed: http://code.google.com/p/skia/source/detail?r=1409

[Chris Guillory, 2011-05-24 00:22:14]

http://codereview.appspot.com/4515126/diff/2001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

http://codereview.appspot.com/4515126/diff/2001/include/pdf/SkPDFDevice.h#new...
include/pdf/SkPDFDevice.h:40: class ContentEntryAccessor;
Nit: Place class before struct in sort order?

http://codereview.appspot.com/4515126/diff/2001/include/pdf/SkPDFDevice.h#new...
include/pdf/SkPDFDevice.h:163: ContentEntry* fContentEntriesTail;
Nit/Optional: Does a prefixed name seem better for this. fTailContentEntry?
fLastContentEntry?

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:621: if (!content.entry()) {
Optional: Move the rest of this code into internalDrawPoints?

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1229: SkClipStack clipStack =
fContentEntries->fState.fClipStack;
Is this right to use fContentEntries where before we used fCurrentContentEntry?

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1247: ContentEntryAccessor content(this,
&fExistingClipStack, fExistingClipRegion,
Nit: Perhaps not use the general name content for this local?

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1306: SkIRect bounds = clipRegion.getBounds();
Is this a semantic change?

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1407: SkWStream* content) {
Nit: setTextTransform is only used with the stream of a content entry. Perhaps
it can take the ContentEntry as its argument instead of a stream.

[Chris Guillory, 2011-05-24 00:29:34]

Couple of additional comments.

1. Nit: In descrition helped -> helper.
2. How about changing the name of ContentEntryAccessor to something that makes
it a bit more clear that it contains more accessor type logic? AutoContentEntry?
ContentEntryManager? I actually don't have a great name.

[Steve VanDeBogart, 2011-05-24 06:41:42]

On 2011/05/24 00:29:34, Chris Guillory wrote:
> Couple of additional comments.
> 
> 1. Nit: In descrition helped -> helper.

Done.

> 2. How about changing the name of ContentEntryAccessor to something that makes
> it a bit more clear that it contains more accessor type logic?
AutoContentEntry?
> ContentEntryManager? I actually don't have a great name.

I'm not really happy with ContentEntryAccesor, but I think it's the most
descriptive name we've come up with so far.  Maaaybe AutoContentEntry, but only
if you think it is clearly better.

http://codereview.appspot.com/4515126/diff/2001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

http://codereview.appspot.com/4515126/diff/2001/include/pdf/SkPDFDevice.h#new...
include/pdf/SkPDFDevice.h:40: class ContentEntryAccessor;
On 2011/05/24 00:22:14, Chris Guillory wrote:
> Nit: Place class before struct in sort order?

Done.

http://codereview.appspot.com/4515126/diff/2001/include/pdf/SkPDFDevice.h#new...
include/pdf/SkPDFDevice.h:163: ContentEntry* fContentEntriesTail;
On 2011/05/24 00:22:14, Chris Guillory wrote:
> Nit/Optional: Does a prefixed name seem better for this. fTailContentEntry?
> fLastContentEntry?

Done.

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1229: SkClipStack clipStack =
fContentEntries->fState.fClipStack;
On 2011/05/24 00:22:14, Chris Guillory wrote:
> Is this right to use fContentEntries where before we used
fCurrentContentEntry?

In this case, they are the same - based on the xfermode, setUpContentEntry must
have taken all of the current content and put it into dst and set up a single
content entry.  I added an assert that this is the only content entry.

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1247: ContentEntryAccessor content(this,
&fExistingClipStack, fExistingClipRegion,
On 2011/05/24 00:22:14, Chris Guillory wrote:
> Nit: Perhaps not use the general name content for this local?

Sure, though I'm not sure my new name is much better.

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1306: SkIRect bounds = clipRegion.getBounds();
On 2011/05/24 00:22:14, Chris Guillory wrote:
> Is this a semantic change?

No. entry should be the same as fCurrentContentEntry->fState (though, if it's
not, entry is the right value). And we just assigned clipRegion to
entry->fClipRegion.

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1407: SkWStream* content) {
On 2011/05/24 00:22:14, Chris Guillory wrote:
> Nit: setTextTransform is only used with the stream of a content entry. Perhaps
> it can take the ContentEntry as its argument instead of a stream.

To what end?  I don't understand the motivation for your suggestion.  The
functions in SkPDFUtils take a SkWStream* as their last argument.

[Chris Guillory, 2011-05-24 09:15:14]

LGTM

> I'm not really happy with ContentEntryAccesor, but I think it's the most
> descriptive name we've come up with so far.  Maaaybe AutoContentEntry, but
only
> if you think it is clearly better.
I don't. ContentEntryAccesor it is.

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1407: SkWStream* content) {
On 2011/05/24 06:41:42, Steve VanDeBogart wrote:
> On 2011/05/24 00:22:14, Chris Guillory wrote:
> > Nit: setTextTransform is only used with the stream of a content entry.
Perhaps
> > it can take the ContentEntry as its argument instead of a stream.
> 
> To what end?  I don't understand the motivation for your suggestion.  The
> functions in SkPDFUtils take a SkWStream* as their last argument.
I was just looking at consistency. Many functions in this file now take a
ContentEntry* as the last argument. Is this a candidate for SkPDFUtils?

http://codereview.appspot.com/4515126/diff/4/src/pdf/SkPDFDevice.cpp#newcode1232
src/pdf/SkPDFDevice.cpp:1232: SkASSERT(dst);
Nit: Can we move this assert above and next to the previous assert.

http://codereview.appspot.com/4515126/diff/4/src/pdf/SkPDFDevice.cpp#newcode1249
src/pdf/SkPDFDevice.cpp:1249: if (!content.entry()) {
Nit: You're right! Feel free to name this content again if you want.

[Steve VanDeBogart, 2011-05-24 17:14:51]

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4515126/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1407: SkWStream* content) {
On 2011/05/24 09:15:14, Chris Guillory wrote:
> On 2011/05/24 06:41:42, Steve VanDeBogart wrote:
> > On 2011/05/24 00:22:14, Chris Guillory wrote:
> > > Nit: setTextTransform is only used with the stream of a content entry.
> Perhaps
> > > it can take the ContentEntry as its argument instead of a stream.
> > 
> > To what end?  I don't understand the motivation for your suggestion.  The
> > functions in SkPDFUtils take a SkWStream* as their last argument.
> I was just looking at consistency. Many functions in this file now take a
> ContentEntry* as the last argument. Is this a candidate for SkPDFUtils?

It could go into SkPDFUtils, though I doubt it would get reused.  Instead I made
it a static non-member function at the top of the file.

http://codereview.appspot.com/4515126/diff/4/src/pdf/SkPDFDevice.cpp#newcode1232
src/pdf/SkPDFDevice.cpp:1232: SkASSERT(dst);
On 2011/05/24 09:15:14, Chris Guillory wrote:
> Nit: Can we move this assert above and next to the previous assert.

Done.

[Chris Guillory, 2011-07-18 17:25:43]

Can you close this issue so it doesn't show up in the reviewable list.