Use skset to fix issue when pdf generates an exp number of resources

Use SkSet to fix issue when pdf generates an exp number of resources.

The problem fixed - http://code.google.com/p/skia/issues/detail?id=940 - is that getResources will recursively obtain all child resource recursively without checking for duplicates.

If we have lots of duplicates, then we try to build a very large vector (exponential with the number of nodes usually) and sooner or later we end up using too much memory and crash.

A possible solution could have been to make sure resources do not have duplicates, but that requirement is impractical, and it this leaves the solution fragile, if there is any issue in the tree,  we crash.

When we emit the pdf, the large number of duplicates is not an issue, because SkPDFCatalog::addObject will deal with duplicates.

I have run the gm with --config pdf, and the images are 100% same bits, while the pdfs have the same size but some very small changes, the order of some objects.
Committed: https://code.google.com/p/skia/source/detail?r=7883

[edisonn, 2012-10-21 03:56:18]

updated description, some rewording

[Steve VanDeBogart, 2012-10-21 07:05:03]

https://codereview.appspot.com/6744050/diff/2001/src/pdf/SkPDFDocument.cpp
File src/pdf/SkPDFDocument.cpp (right):

https://codereview.appspot.com/6744050/diff/2001/src/pdf/SkPDFDocument.cpp#ne...
src/pdf/SkPDFDocument.cpp:21: for (int i = firstIndex; i <
resourceList->count(); i++) {
I see that you made the set maintain order so that this still works.  It seems
bad to rely on that.  It's easy enough to fix by just passing the new resources
here.

https://codereview.appspot.com/6744050/diff/2001/src/pdf/SkPDFDocument.cpp#ne...
src/pdf/SkPDFDocument.cpp:106: fPages[i]->finalizePage(fCatalog.get(),
firstPage, &fPageResources);
Fetch to s new set each time.

https://codereview.appspot.com/6744050/diff/2001/src/pdf/SkPDFDocument.cpp#ne...
src/pdf/SkPDFDocument.cpp:111: fSecondPageFirstResourceIndex =
fPageResources.count();
Instead of storing the index, use two sets, one for the first page, the other
for the rest.

https://codereview.appspot.com/6744050/diff/2001/src/pdf/SkPDFFormXObject.cpp
File src/pdf/SkPDFFormXObject.cpp (right):

https://codereview.appspot.com/6744050/diff/2001/src/pdf/SkPDFFormXObject.cpp...
src/pdf/SkPDFFormXObject.cpp:23: device->getResources(&fResources, false);
This doesn't get the resources recursively, so why is there a problem?

It'd be great to actually understand the problem before hiding it forever by
using a set.  If there's something in the code that's adding resources
recursively when it doesn't have to we can save a lot of CPU by not doing that.

I agree with your point about it being fragile.  But if the problem can be
fixed, then we can also add a debug check that when adding something to a
resource set it's not already there.  That will make it never run out of memory
but also catch any new inefficiencies that might try to creep in.

[edisonn, 2012-10-22 17:19:38]

https://codereview.appspot.com/6744050/diff/2001/src/pdf/SkPDFDocument.cpp
File src/pdf/SkPDFDocument.cpp (right):

https://codereview.appspot.com/6744050/diff/2001/src/pdf/SkPDFDocument.cpp#ne...
src/pdf/SkPDFDocument.cpp:21: for (int i = firstIndex; i <
resourceList->count(); i++) {
On 2012/10/21 07:05:03, Steve VanDeBogart wrote:
> I see that you made the set maintain order so that this still works.  It seems
> bad to rely on that.  It's easy enough to fix by just passing the new
resources
> here.

Agree. But it is going to be a two step check in, and I will make this change in
 a future CL

https://codereview.appspot.com/6744050/diff/2001/src/pdf/SkPDFDocument.cpp#ne...
src/pdf/SkPDFDocument.cpp:106: fPages[i]->finalizePage(fCatalog.get(),
firstPage, &fPageResources);
On 2012/10/21 07:05:03, Steve VanDeBogart wrote:
> Fetch to s new set each time.

Agree. But I wanted a two phase checkin. In the first one I am added a "sort of
set" where I maintain the order of the elements, so the changes to pdf src are
cosmetic.

In a future CL I will add a true set, and I will fetch to a new set each time.

https://codereview.appspot.com/6744050/diff/2001/src/pdf/SkPDFDocument.cpp#ne...
src/pdf/SkPDFDocument.cpp:111: fSecondPageFirstResourceIndex =
fPageResources.count();
On 2012/10/21 07:05:03, Steve VanDeBogart wrote:
> Instead of storing the index, use two sets, one for the first page, the other
> for the rest.

Agree. See above. it will be a two step checkin, so this change will follow
soon.

https://codereview.appspot.com/6744050/diff/2001/src/pdf/SkPDFFormXObject.cpp
File src/pdf/SkPDFFormXObject.cpp (right):

https://codereview.appspot.com/6744050/diff/2001/src/pdf/SkPDFFormXObject.cpp...
src/pdf/SkPDFFormXObject.cpp:23: device->getResources(&fResources, false);
On 2012/10/21 07:05:03, Steve VanDeBogart wrote:
> This doesn't get the resources recursively, so why is there a problem?
It does not get the resources recursively now, but it will happen later when we
will finalize the pdf. but this is the first time the problem can be exposed.
> 
> It'd be great to actually understand the problem before hiding it forever by
> using a set.  If there's something in the code that's adding resources
> recursively when it doesn't have to we can save a lot of CPU by not doing
that.
> 


Looking into. Please see this png with the tree of resources. I will spend more
time and understand why each resource is using some other resources. (I will add
for each node the resource type and the callstack)


http://skia.googlecode.com/issues/attachment?aid=9400003000&name=skia_pdf_res...

> I agree with your point about it being fragile.  But if the problem can be
> fixed, then we can also add a debug check that when adding something to a
> resource set it's not already there.  That will make it never run out of
memory
> but also catch any new inefficiencies that might try to creep in.

The code is fragile in release mode. Imagine you have two separate graphic
objects, which do not know anything about each other, they have to make sure
each of them is listing the right set of dependencies. The moment they are put
together, we will explore their common dependencies twice. Building complex
objects asks for trouble.

[edisonn, 2012-10-31 15:12:33]

ping

[reed1, 2012-10-31 17:54:23]

https://codereview.appspot.com/6744050/diff/9001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/6744050/diff/9001/include/pdf/SkPDFDevice.h#ne...
include/pdf/SkPDFDevice.h:135: */
Seems like we don't *have* to use the Set type here in the public API. It can be
documented to return unique references (what else would make sense) but it could
just return an array to hold the answers.

Just trying to reduce the public API surface area...

https://codereview.appspot.com/6744050/diff/9001/include/pdf/SkPDFDocument.h
File include/pdf/SkPDFDocument.h (right):

https://codereview.appspot.com/6744050/diff/9001/include/pdf/SkPDFDocument.h#...
include/pdf/SkPDFDocument.h:79: SkPDFDict* fDocCatalog;
If we wanted, we could change this to a ptr-to-set, and then just forward
declare the type, allowing us to move the set into src/ instead of include/

Not critical per-se.

[Steve VanDeBogart, 2012-10-31 18:07:26]

Looking through this change, it looks like the only methods of SkTPDFSet that
are used are:

operator[], count, setReserve, and insertAndRef

Since right now the PDF code requires the special property that the set stay
sorted in insert order, It's probably best to make a specialized data structure
that only supports the operations needed.  Once the code is migrated to use a
regular set (easy to do and I suggested doing it first), then a general
templatized solution would be appropriate.

https://codereview.appspot.com/6744050/diff/9001/src/pdf/SkPDFFormXObject.h
File src/pdf/SkPDFFormXObject.h (right):

https://codereview.appspot.com/6744050/diff/9001/src/pdf/SkPDFFormXObject.h#n...
src/pdf/SkPDFFormXObject.h:16: #include "SkTPDFSet.h"
nit: no need to include this, it's in SkPDFTypes.h

[edisonn, 2012-11-02 20:22:02]

I updated the code to use a true Set.

But the code is a little bit ugly. We need to pass two sets, not one.

The reason is that if I pass only one set, then I need to remember what was the
set of known objects before I process a page, then I need to diff the sets. This
would have worse complexity and also I need to complicate the code that keeps
track of ref_count

If I pass two sets, then the code is simplified and faster.

The complexity of the code (either passing one set or two sets) is at least
O(PAGES*OBJECTS), but in the form I wrote the code with two sets, I could get
rid of the mergeInto(...) and I could reduce the complexity.  I need to think
more about that.

https://codereview.appspot.com/6744050/diff/9001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/6744050/diff/9001/include/pdf/SkPDFDevice.h#ne...
include/pdf/SkPDFDevice.h:135: */
On 2012/10/31 17:54:24, reed1 wrote:
> Seems like we don't *have* to use the Set type here in the public API. It can
be
> documented to return unique references (what else would make sense) but it
could
> just return an array to hold the answers.
> 
> Just trying to reduce the public API surface area...

Done.

https://codereview.appspot.com/6744050/diff/9001/include/pdf/SkPDFDocument.h
File include/pdf/SkPDFDocument.h (right):

https://codereview.appspot.com/6744050/diff/9001/include/pdf/SkPDFDocument.h#...
include/pdf/SkPDFDocument.h:79: SkPDFDict* fDocCatalog;
I would leave it like that. It won't be long lived anyway, and if anyone wants
to use it, we can speak up in code review. 

On 2012/10/31 17:54:24, reed1 wrote:
> If we wanted, we could change this to a ptr-to-set, and then just forward
> declare the type, allowing us to move the set into src/ instead of include/
> 
> Not critical per-se.

[edisonn, 2012-11-02 20:23:40]

https://codereview.appspot.com/6744050/diff/9001/src/pdf/SkPDFFormXObject.h
File src/pdf/SkPDFFormXObject.h (right):

https://codereview.appspot.com/6744050/diff/9001/src/pdf/SkPDFFormXObject.h#n...
src/pdf/SkPDFFormXObject.h:16: #include "SkTPDFSet.h"
On 2012/10/31 18:07:26, Steve VanDeBogart wrote:
> nit: no need to include this, it's in SkPDFTypes.h

I would rather have it here, so it is clear what is used.

[Steve VanDeBogart, 2012-11-05 23:45:38]

https://codereview.appspot.com/6744050/diff/9001/src/pdf/SkPDFFormXObject.h
File src/pdf/SkPDFFormXObject.h (right):

https://codereview.appspot.com/6744050/diff/9001/src/pdf/SkPDFFormXObject.h#n...
src/pdf/SkPDFFormXObject.h:16: #include "SkTPDFSet.h"
On 2012/11/02 20:23:40, edisonn wrote:
> On 2012/10/31 18:07:26, Steve VanDeBogart wrote:
> > nit: no need to include this, it's in SkPDFTypes.h
> 
> I would rather have it here, so it is clear what is used.

This is the only file you've included it in though.  It should either be in all
of them or none of them.  I vote for none since it's part of the interface
defined in SkPDFTypes and the only reason it's in this file is because of that
interface.

https://codereview.appspot.com/6744050/diff/19003/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/6744050/diff/19003/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:131: *  @param resourceList A list to append the
resources to.
param list needs to be updated.

https://codereview.appspot.com/6744050/diff/19003/include/pdf/SkPDFDocument.h
File include/pdf/SkPDFDocument.h (right):

https://codereview.appspot.com/6744050/diff/19003/include/pdf/SkPDFDocument.h...
include/pdf/SkPDFDocument.h:80: SkTSet<SkPDFObject*>* fFirstPageResources;
Why are these pointers instead of direct members?

https://codereview.appspot.com/6744050/diff/19003/include/pdf/SkPDFDocument.h...
include/pdf/SkPDFDocument.h:81: SkTSet<SkPDFObject*>*
fAllExceptFirstPageResources;
fOtherPageResources ?

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1071: void SkPDFDevice::getResources(const
SkTSet<SkPDFObject*>* knownResourceObjects,
const & instead of const * ?

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp
File src/pdf/SkPDFDocument.cpp (right):

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:112:
SkASSERT(knownResources.mergeInto(*fFirstPageResources) == 0);
Shouldn't this be empty at this point?

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:117: *fAllExceptFirstPageResources) == 0);
Similarly, shouldn't this be empty at this point?

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFPage.h
File src/pdf/SkPDFPage.h (right):

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFPage.h#newcode43
src/pdf/SkPDFPage.h:43: *  @param resourceObjects All the resource objects
(recursively) used on
Update param list

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFTypes.h
File src/pdf/SkPDFTypes.h (right):

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFTypes.h#newcode46
src/pdf/SkPDFTypes.h:46: *  @param resourceList  The list to append dependant
resources to.
Update param list

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFTypes.h#newcode79
src/pdf/SkPDFTypes.h:79: * @param result    The list to add to.
Update param list

[edisonn, 2013-02-06 17:25:07]

https://codereview.appspot.com/6744050/diff/9001/src/pdf/SkPDFFormXObject.h
File src/pdf/SkPDFFormXObject.h (right):

https://codereview.appspot.com/6744050/diff/9001/src/pdf/SkPDFFormXObject.h#n...
src/pdf/SkPDFFormXObject.h:16: #include "SkTPDFSet.h"
On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> On 2012/11/02 20:23:40, edisonn wrote:
> > On 2012/10/31 18:07:26, Steve VanDeBogart wrote:
> > > nit: no need to include this, it's in SkPDFTypes.h
> > 
> > I would rather have it here, so it is clear what is used.
> 
> This is the only file you've included it in though.  It should either be in
all
> of them or none of them.  I vote for none since it's part of the interface
> defined in SkPDFTypes and the only reason it's in this file is because of that
> interface.

Done.

https://codereview.appspot.com/6744050/diff/19003/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/6744050/diff/19003/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:131: *  @param resourceList A list to append the
resources to.
On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> param list needs to be updated.

Done.

https://codereview.appspot.com/6744050/diff/19003/include/pdf/SkPDFDocument.h
File include/pdf/SkPDFDocument.h (right):

https://codereview.appspot.com/6744050/diff/19003/include/pdf/SkPDFDocument.h...
include/pdf/SkPDFDocument.h:80: SkTSet<SkPDFObject*>* fFirstPageResources;
On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> Why are these pointers instead of direct members?

SkTSet.h is in src/pdf/ so we don't expose publicly in /include, wehre
SkPDFDocument is located, hence we need forward declaration and we must use then
a pointer

https://codereview.appspot.com/6744050/diff/19003/include/pdf/SkPDFDocument.h...
include/pdf/SkPDFDocument.h:81: SkTSet<SkPDFObject*>*
fAllExceptFirstPageResources;
On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> fOtherPageResources ?

Done.

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1071: void SkPDFDevice::getResources(const
SkTSet<SkPDFObject*>* knownResourceObjects,
On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> const & instead of const * ?

Done. replaced everywhere

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp
File src/pdf/SkPDFDocument.cpp (right):

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:112:
SkASSERT(knownResources.mergeInto(*fFirstPageResources) == 0);
On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> Shouldn't this be empty at this point?

yep, and I should not be using SkASSERT, fixed lines below

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:117: *fAllExceptFirstPageResources) == 0);
On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> Similarly, shouldn't this be empty at this point?

one can call emitPdf twice, so this one might not be empty

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFPage.h
File src/pdf/SkPDFPage.h (right):

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFPage.h#newcode43
src/pdf/SkPDFPage.h:43: *  @param resourceObjects All the resource objects
(recursively) used on
On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> Update param list

Done.

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFTypes.h
File src/pdf/SkPDFTypes.h (right):

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFTypes.h#newcode46
src/pdf/SkPDFTypes.h:46: *  @param resourceList  The list to append dependant
resources to.
On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> Update param list

Done.

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFTypes.h#newcode79
src/pdf/SkPDFTypes.h:79: * @param result    The list to add to.
On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> Update param list

Done.

[Steve VanDeBogart, 2013-02-06 19:28:55]

Just a few nits/clean ups.

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp
File src/pdf/SkPDFDocument.cpp (right):

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:112:
SkASSERT(knownResources.mergeInto(*fFirstPageResources) == 0);
On 2013/02/06 17:25:07, edisonn wrote:
> On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> > Shouldn't this be empty at this point?
> 
> yep, and I should not be using SkASSERT, fixed lines below

From flow control we can prove that both knownResources and fFirstPageResoruces
are empty at this point, so you can just remove this line.

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:117: *fAllExceptFirstPageResources) == 0);
On 2013/02/06 17:25:07, edisonn wrote:
> On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> > Similarly, shouldn't this be empty at this point?
> 
> one can call emitPdf twice, so this one might not be empty

But this code path is only called the first time emitPDF is called, so you can
remove this line.

https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFDocument.cpp
File src/pdf/SkPDFDocument.cpp (right):

https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:56: fFirstPageResources = SkNEW(SkTSet<SkPDFObject*>);
Allocate these in emitPDF() to make it clearer that they are not used until
emitPDF() is called.

https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:117: SkASSERT(duplicates == 0);           }
Fix formatting here.

https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:119: fPages[i]->finalizePage(
The ownership of the resources returned is a bit muddled here.  I agree with
your choice that it's not worth referencing them for knownResources only
unreference them within emitPDF().  But please add a comment here or maybe a bit
lower that the references returned in newResources are transfered to
fFirstPageResources or fOtherPageResources depending on firstPage and
knownResources doesn't have a reference but just relies on the other two sets to
maintain a reference.

https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFTypes.h
File src/pdf/SkPDFTypes.h (right):

https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFTypes.h#newcode47
src/pdf/SkPDFTypes.h:47: *  @param knownResourceObjects  The set of resources to
be ignored.
nit: The set of already known resources. (multiple places)

[thakis, 2013-06-03 18:22:43]

https://codereview.appspot.com/6744050/diff/32001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/6744050/diff/32001/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:32: template <typename T> class SK_API SkTSet;
Do you need the SK_API here? Usually these defines are put only on type
definitions, not on declarations. If it's here, gcc 4.4 (and maybe others) warn:

../../third_party/skia/include/pdf/SkPDFDevice.h:33: warning: type attributes
ignored after type is already defined

[edisonn, 2013-06-03 18:23:51]

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp
File src/pdf/SkPDFDocument.cpp (right):

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:112:
SkASSERT(knownResources.mergeInto(*fFirstPageResources) == 0);
On 2013/02/06 19:28:55, Steve VanDeBogart wrote:
> On 2013/02/06 17:25:07, edisonn wrote:
> > On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> > > Shouldn't this be empty at this point?
> > 
> > yep, and I should not be using SkASSERT, fixed lines below
> 
> From flow control we can prove that both knownResources and
fFirstPageResoruces
> are empty at this point, so you can just remove this line.

Done.

https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:117: *fAllExceptFirstPageResources) == 0);
On 2013/02/06 19:28:55, Steve VanDeBogart wrote:
> On 2013/02/06 17:25:07, edisonn wrote:
> > On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
> > > Similarly, shouldn't this be empty at this point?
> > 
> > one can call emitPdf twice, so this one might not be empty
> 
> But this code path is only called the first time emitPDF is called, so you can
> remove this line.

Done.

https://codereview.appspot.com/6744050/diff/32001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/6744050/diff/32001/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:32: template <typename T> class SK_API SkTSet;
Already fixed. Thanks for feedback.

On 2013/06/03 18:22:43, thakis wrote:
> Do you need the SK_API here? Usually these defines are put only on type
> definitions, not on declarations. If it's here, gcc 4.4 (and maybe others)
warn:
> 
> ../../third_party/skia/include/pdf/SkPDFDevice.h:33: warning: type attributes
> ignored after type is already defined

https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFDocument.cpp
File src/pdf/SkPDFDocument.cpp (right):

https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:56: fFirstPageResources = SkNEW(SkTSet<SkPDFObject*>);
On 2013/02/06 19:28:55, Steve VanDeBogart wrote:
> Allocate these in emitPDF() to make it clearer that they are not used until
> emitPDF() is called.

Done.

https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:117: SkASSERT(duplicates == 0);           }
On 2013/02/06 19:28:55, Steve VanDeBogart wrote:
> Fix formatting here.

Done.

https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:119: fPages[i]->finalizePage(
On 2013/02/06 19:28:55, Steve VanDeBogart wrote:
> The ownership of the resources returned is a bit muddled here.  I agree with
> your choice that it's not worth referencing them for knownResources only
> unreference them within emitPDF().  But please add a comment here or maybe a
bit
> lower that the references returned in newResources are transfered to
> fFirstPageResources or fOtherPageResources depending on firstPage and
> knownResources doesn't have a reference but just relies on the other two sets
to
> maintain a reference.

Done.

[thakis, 2013-06-03 18:24:54]

On Mon, Jun 3, 2013 at 11:23 AM, <edisonn@google.com> wrote:

>
> https://codereview.appspot.**com/6744050/diff/19003/src/**
>
pdf/SkPDFDocument.cpp<https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp>
> File src/pdf/SkPDFDocument.cpp (right):
>
> https://codereview.appspot.**com/6744050/diff/19003/src/**
>
pdf/SkPDFDocument.cpp#**newcode112<https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp#newcode112>
> src/pdf/SkPDFDocument.cpp:112:
> SkASSERT(knownResources.**mergeInto(***fFirstPageResources) == 0);
> On 2013/02/06 19:28:55, Steve VanDeBogart wrote:
>
>> On 2013/02/06 17:25:07, edisonn wrote:
>> > On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
>> > > Shouldn't this be empty at this point?
>> >
>> > yep, and I should not be using SkASSERT, fixed lines below
>>
>
>   From flow control we can prove that both knownResources and
>>
> fFirstPageResoruces
>
>> are empty at this point, so you can just remove this line.
>>
>
> Done.
>
> https://codereview.appspot.**com/6744050/diff/19003/src/**
>
pdf/SkPDFDocument.cpp#**newcode117<https://codereview.appspot.com/6744050/diff/19003/src/pdf/SkPDFDocument.cpp#newcode117>
> src/pdf/SkPDFDocument.cpp:117: *fAllExceptFirstPageResources) == 0);
> On 2013/02/06 19:28:55, Steve VanDeBogart wrote:
>
>> On 2013/02/06 17:25:07, edisonn wrote:
>> > On 2012/11/05 23:45:38, Steve VanDeBogart wrote:
>> > > Similarly, shouldn't this be empty at this point?
>> >
>> > one can call emitPdf twice, so this one might not be empty
>>
>
>  But this code path is only called the first time emitPDF is called, so
>>
> you can
>
>> remove this line.
>>
>
> Done.
>
>
> https://codereview.appspot.**com/6744050/diff/32001/**
>
include/pdf/SkPDFDevice.h<https://codereview.appspot.com/6744050/diff/32001/include/pdf/SkPDFDevice.h>
> File include/pdf/SkPDFDevice.h (right):
>
> https://codereview.appspot.**com/6744050/diff/32001/**
>
include/pdf/SkPDFDevice.h#**newcode32<https://codereview.appspot.com/6744050/diff/32001/include/pdf/SkPDFDevice.h#newcode32>
> include/pdf/SkPDFDevice.h:32: template <typename T> class SK_API SkTSet;
> Already fixed. Thanks for feedback.


Ah, sorry. The version of skia that chromium uses doesn't have the fix yet.

Thanks for fixing!


>
>
> On 2013/06/03 18:22:43, thakis wrote:
>
>> Do you need the SK_API here? Usually these defines are put only on
>>
> type
>
>> definitions, not on declarations. If it's here, gcc 4.4 (and maybe
>>
> others) warn:
>
>  ../../third_party/skia/**include/pdf/SkPDFDevice.h:33: warning: type
>>
> attributes
>
>> ignored after type is already defined
>>
>
> https://codereview.appspot.**com/6744050/diff/32001/src/**
>
pdf/SkPDFDocument.cpp<https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFDocument.cpp>
> File src/pdf/SkPDFDocument.cpp (right):
>
> https://codereview.appspot.**com/6744050/diff/32001/src/**
>
pdf/SkPDFDocument.cpp#**newcode56<https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFDocument.cpp#newcode56>
> src/pdf/SkPDFDocument.cpp:56: fFirstPageResources =
> SkNEW(SkTSet<SkPDFObject*>);
> On 2013/02/06 19:28:55, Steve VanDeBogart wrote:
>
>> Allocate these in emitPDF() to make it clearer that they are not used
>>
> until
>
>> emitPDF() is called.
>>
>
> Done.
>
> https://codereview.appspot.**com/6744050/diff/32001/src/**
>
pdf/SkPDFDocument.cpp#**newcode117<https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFDocument.cpp#newcode117>
> src/pdf/SkPDFDocument.cpp:117: SkASSERT(duplicates == 0);           }
> On 2013/02/06 19:28:55, Steve VanDeBogart wrote:
>
>> Fix formatting here.
>>
>
> Done.
>
> https://codereview.appspot.**com/6744050/diff/32001/src/**
>
pdf/SkPDFDocument.cpp#**newcode119<https://codereview.appspot.com/6744050/diff/32001/src/pdf/SkPDFDocument.cpp#newcode119>
> src/pdf/SkPDFDocument.cpp:119: fPages[i]->finalizePage(
> On 2013/02/06 19:28:55, Steve VanDeBogart wrote:
>
>> The ownership of the resources returned is a bit muddled here.  I
>>
> agree with
>
>> your choice that it's not worth referencing them for knownResources
>>
> only
>
>> unreference them within emitPDF().  But please add a comment here or
>>
> maybe a bit
>
>> lower that the references returned in newResources are transfered to
>> fFirstPageResources or fOtherPageResources depending on firstPage and
>> knownResources doesn't have a reference but just relies on the other
>>
> two sets to
>
>> maintain a reference.
>>
>
> Done.
>
>
https://codereview.appspot.**com/6744050/<https://codereview.appspot.com/6744...
>