Add SkPDFResourceDict class, refactor existing code

Adds SkPDFResourceDict class, refactor existing code to use it.

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

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

CL still busted. Superceded by https://codereview.chromium.org/19954011

[ducky, 2013-07-10 03:15:51]

This is a Pre-CL for transparency gradients (18585002), which uses the
SkPDFResourceDict class to more cleanly manage resources.

[vandebo (ex-Chrome), 2013-07-10 18:24:31]

https://codereview.chromium.org/18977002/diff/1/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/18977002/diff/1/include/pdf/SkPDFDevice.h#new...
include/pdf/SkPDFDevice.h:211: SkPDFResourceDict* fResourceDict;
As is this doesn't include the proc set.  I think you just always want to
include the proc set though.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.cpp
File src/pdf/SkPDFResourceDict.cpp (right):

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.cpp...
src/pdf/SkPDFResourceDict.cpp:1: 
Extra line

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.cpp...
src/pdf/SkPDFResourceDict.cpp:30: case kExtGState_ResourceType:
Instead of this verbose switch statement, use compile asserts to ensure that the
enums have particular values and then use an array of chars in this case and and
an array of c-strings for the getResourceTypeName method.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.cpp...
src/pdf/SkPDFResourceDict.cpp:132: if (*(fTypes[i].key) == *type) {
Instead of doing a string comparison for every insert, why not just store
pointers to the internal dicts in an array indexed by type?

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h
File src/pdf/SkPDFResourceDict.h (right):

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:1: 
nit: remove blank line.  Other files have it because of a copyright update that
inserted it by mistake.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:9: 
nit: remove extra blank line.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:13: #include "SkRefCnt.h"
omit

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:26: public:
nit: no indent.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:31: kColorSpace_ResourceType,
omit, since not used.  A comment with the unused types would be fine.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:41: SkPDFResourceDict() : SkPDFDict() {};
non-POD member variable means that the constructor should be defined in the .cpp
file.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:41: SkPDFResourceDict() : SkPDFDict() {};
Why both a no argument constructor and a constructor with a bool parameter that
when false has the same effect as the no argument constructor?

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:47: SkPDFResourceDict(bool createProcSets);
Is this ever set to false?  If not, omit the argument and just always create the
proc set.  Also, constructors with one argument should always have the explicit
keyword.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:59: SkPDFObject* insertResource(SkPDFResourceType
type, int key,
Not used, omit.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:94: const char*
getResourceTypePrefix(SkPDFResourceType type);
return a char, instead of a c-string

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:94: const char*
getResourceTypePrefix(SkPDFResourceType type);
Not used publicly, make private.  Actually it doesn't use any instance
information so it can be a static function in the implementation file.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:98: const char*
getResourceTypeName(SkPDFResourceType type);
Same as above.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:99: private:
nit: no indent.

[ducky, 2013-07-10 22:54:34]

Addresses code review comments. Major stylistic changes, especially with the
enum to string mapping. Also changes internals so that the type->SkPDFDict
(sub-dictionary) lookup is done using a preallocated array.

https://codereview.chromium.org/18977002/diff/1/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/18977002/diff/1/include/pdf/SkPDFDevice.h#new...
include/pdf/SkPDFDevice.h:211: SkPDFResourceDict* fResourceDict;
On 2013/07/10 18:24:31, vandebo wrote:
> As is this doesn't include the proc set.  I think you just always want to
> include the proc set though.

Wha? It doesn't?
The object isn't even being constructed here?

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.cpp
File src/pdf/SkPDFResourceDict.cpp (right):

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.cpp...
src/pdf/SkPDFResourceDict.cpp:1: 
On 2013/07/10 18:24:31, vandebo wrote:
> Extra line

Done.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.cpp...
src/pdf/SkPDFResourceDict.cpp:30: case kExtGState_ResourceType:
On 2013/07/10 18:24:31, vandebo wrote:
> Instead of this verbose switch statement, use compile asserts to ensure that
the
> enums have particular values and then use an array of chars in this case and
and
> an array of c-strings for the getResourceTypeName method.

Done.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.cpp...
src/pdf/SkPDFResourceDict.cpp:132: if (*(fTypes[i].key) == *type) {
On 2013/07/10 18:24:31, vandebo wrote:
> Instead of doing a string comparison for every insert, why not just store
> pointers to the internal dicts in an array indexed by type?

Done. Good point.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h
File src/pdf/SkPDFResourceDict.h (right):

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:1: 
On 2013/07/10 18:24:31, vandebo wrote:
> nit: remove blank line.  Other files have it because of a copyright update
that
> inserted it by mistake.

Done.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:9: 
On 2013/07/10 18:24:31, vandebo wrote:
> nit: remove extra blank line.

Done.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:13: #include "SkRefCnt.h"
On 2013/07/10 18:24:31, vandebo wrote:
> omit

Done.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:26: public:
On 2013/07/10 18:24:31, vandebo wrote:
> nit: no indent.

Done. Not really sure where that came from...

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:31: kColorSpace_ResourceType,
On 2013/07/10 18:24:31, vandebo wrote:
> omit, since not used.  A comment with the unused types would be fine.

Done.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:41: SkPDFResourceDict() : SkPDFDict() {};
On 2013/07/10 18:24:31, vandebo wrote:
> non-POD member variable means that the constructor should be defined in the
.cpp
> file.

Done.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:41: SkPDFResourceDict() : SkPDFDict() {};
On 2013/07/10 18:24:31, vandebo wrote:
> Why both a no argument constructor and a constructor with a bool parameter
that
> when false has the same effect as the no argument constructor?

Done. It looked useful at once point in time, but not anymore...

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:47: SkPDFResourceDict(bool createProcSets);
On 2013/07/10 18:24:31, vandebo wrote:
> Is this ever set to false?  If not, omit the argument and just always create
the
> proc set.  Also, constructors with one argument should always have the
explicit
> keyword.

No, it's never set to false. The original idea was to add an explicit argument
so there's less possible confusion about the magic happening with ProcSets
automatically being constructed. I've just moved that into a comment now.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:59: SkPDFObject* insertResource(SkPDFResourceType
type, int key,
On 2013/07/10 18:24:31, vandebo wrote:
> Not used, omit.

It's used by insertResourceAsRef, and it's possible for a resource dict to
directly reference an object (though I don't think Skia does that currently).

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:94: const char*
getResourceTypePrefix(SkPDFResourceType type);
On 2013/07/10 18:24:31, vandebo wrote:
> Not used publicly, make private.  Actually it doesn't use any instance
> information so it can be a static function in the implementation file.

Now moved into a static function in the implementation file.
I see the logic behind making it a char, but that also seems to make the code
uglier, since SkString can't construct from a char, but rather needs a char[].

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:98: const char*
getResourceTypeName(SkPDFResourceType type);
On 2013/07/10 18:24:31, vandebo wrote:
> Same as above.

Done.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:99: private:
On 2013/07/10 18:24:31, vandebo wrote:
> nit: no indent.

Done.

[vandebo (ex-Chrome), 2013-07-11 17:50:23]

https://codereview.chromium.org/18977002/diff/1/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/18977002/diff/1/include/pdf/SkPDFDevice.h#new...
include/pdf/SkPDFDevice.h:211: SkPDFResourceDict* fResourceDict;
On 2013/07/10 22:54:34, ducky wrote:
> On 2013/07/10 18:24:31, vandebo wrote:
> > As is this doesn't include the proc set.  I think you just always want to
> > include the proc set though.
> 
> Wha? It doesn't?
> The object isn't even being constructed here?

Oops, right.

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h
File src/pdf/SkPDFResourceDict.h (right):

https://codereview.chromium.org/18977002/diff/1/src/pdf/SkPDFResourceDict.h#n...
src/pdf/SkPDFResourceDict.h:59: SkPDFObject* insertResource(SkPDFResourceType
type, int key,
On 2013/07/10 22:54:34, ducky wrote:
> On 2013/07/10 18:24:31, vandebo wrote:
> > Not used, omit.
> 
> It's used by insertResourceAsRef, and it's possible for a resource dict to
> directly reference an object (though I don't think Skia does that currently).

If we're not using it, don't make it part of the public API.  It's always easier
to add things to a public API than it is to take them away.  You could leave it
as a separate function and just make it private for now.  Or you could inline
the code at the caller.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict.cpp
File src/pdf/SkPDFResourceDict.cpp (right):

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.cpp:22: const char resource_type_prefixes[][2] = {
static

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.cpp:29: const char resource_type_names[][11] = {
static

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.cpp:36: static const char*
getResourceTypePrefix(SkPDFResourceDict::SkPDFResourceType type) {
get_resource_type_prefix

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.cpp:54: for (size_t i=0; i <
SK_ARRAY_COUNT(resource_type_names); i++) {
fTypes should be populated lazily - we don't want to include an empty dict for a
resource type that's not used.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.cpp:64: SkPDFDict* typeDict = fTypes[type];
Lazy population can go here:
if (!typeDict) ...

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.cpp:67: keyString.appendS32(key);
You could use keyString.printf("%c%d", getResourceTypePrefix(type), key);

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict.h
File src/pdf/SkPDFResourceDict.h (right):

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.h:31: // These additional types are defined by the
spec, but not
You may want to add KResouceTypeCount to help with your asserts.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.h:33: //  ColorSpace, Shading, Properties
nit, will fit on the previous line.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.h:61: *  @param type  The type of resource being
entered, like Pattern or ExtGState.
Skia allows 100 column lines, but it looks like you're shooting for 80 column
lines.  Please do one or the other.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.h:61: *  @param type  The type of resource being
entered, like Pattern or ExtGState.
comment out of date, i.e. Pattern -> kPattern_ResourceType

[ducky, 2013-07-11 23:09:50]

Sub-dicts now lazily initialized, and addresses more code review comments.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict.cpp
File src/pdf/SkPDFResourceDict.cpp (right):

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.cpp:22: const char resource_type_prefixes[][2] = {
On 2013/07/11 17:50:23, vandebo wrote:
> static

Done.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.cpp:29: const char resource_type_names[][11] = {
On 2013/07/11 17:50:23, vandebo wrote:
> static

Done.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.cpp:36: static const char*
getResourceTypePrefix(SkPDFResourceDict::SkPDFResourceType type) {
On 2013/07/11 17:50:23, vandebo wrote:
> get_resource_type_prefix

Done.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.cpp:54: for (size_t i=0; i <
SK_ARRAY_COUNT(resource_type_names); i++) {
On 2013/07/11 17:50:23, vandebo wrote:
> fTypes should be populated lazily - we don't want to include an empty dict for
a
> resource type that's not used.

Yep. Just realized that after I left yesterday. =(

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.cpp:64: SkPDFDict* typeDict = fTypes[type];
On 2013/07/11 17:50:23, vandebo wrote:
> Lazy population can go here:
> if (!typeDict) ...

Done.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.cpp:67: keyString.appendS32(key);
On 2013/07/11 17:50:23, vandebo wrote:
> You could use keyString.printf("%c%d", getResourceTypePrefix(type), key);

Done.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict.h
File src/pdf/SkPDFResourceDict.h (right):

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.h:31: // These additional types are defined by the
spec, but not
On 2013/07/11 17:50:23, vandebo wrote:
> You may want to add KResouceTypeCount to help with your asserts.

Done.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.h:33: //  ColorSpace, Shading, Properties
On 2013/07/11 17:50:23, vandebo wrote:
> nit, will fit on the previous line.

Done.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.h:61: *  @param type  The type of resource being
entered, like Pattern or ExtGState.
On 2013/07/11 17:50:23, vandebo wrote:
> Skia allows 100 column lines, but it looks like you're shooting for 80 column
> lines.  Please do one or the other.

Adhering to 80 lines.

https://codereview.chromium.org/18977002/diff/7001/src/pdf/SkPDFResourceDict....
src/pdf/SkPDFResourceDict.h:61: *  @param type  The type of resource being
entered, like Pattern or ExtGState.
On 2013/07/11 17:50:23, vandebo wrote:
> comment out of date, i.e. Pattern -> kPattern_ResourceType

Done.

[vandebo (ex-Chrome), 2013-07-12 19:14:59]

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict...
File src/pdf/SkPDFResourceDict.cpp (right):

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict...
src/pdf/SkPDFResourceDict.cpp:26: static char resource_type_prefixes[] = {
sorry, this should be const as well.

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict...
src/pdf/SkPDFResourceDict.cpp:27: 'G',
These constants are currently being used directly in SkPDFDevice.  Since we're
making them actual constants here, they should probably be publicly exposed as
constants and used in SkPDFDevice instead of the literal values.

Actually, probably a better alternative is to have a static method on this class
that given a resource type and index, returns/appends the thing "i.e. G0".

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict...
src/pdf/SkPDFResourceDict.cpp:43: SkASSERT(type <=
SkPDFResourceDict::kFont_ResourceType);
Your asserts here should use 0 and Count

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict...
src/pdf/SkPDFResourceDict.cpp:107: typeDict = SkNEW(SkPDFDict());
It'll be the same number of lines, but less fragile to assign to an SkAutoTUnref
and also assign to typeDict

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict.h
File src/pdf/SkPDFResourceDict.h (right):

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict...
src/pdf/SkPDFResourceDict.h:32: // These additional types are defined by the
spec, but not
nit: I would put Count below this comment.

[ducky, 2013-07-12 21:18:17]

Another code review iteration!
This adds SkPDFResourceDict::getResourceName and does associated refactoring.

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict...
File src/pdf/SkPDFResourceDict.cpp (right):

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict...
src/pdf/SkPDFResourceDict.cpp:26: static char resource_type_prefixes[] = {
On 2013/07/12 19:14:59, vandebo wrote:
> sorry, this should be const as well.

Done.

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict...
src/pdf/SkPDFResourceDict.cpp:27: 'G',
On 2013/07/12 19:14:59, vandebo wrote:
> These constants are currently being used directly in SkPDFDevice.  Since we're
> making them actual constants here, they should probably be publicly exposed as
> constants and used in SkPDFDevice instead of the literal values.
> 
> Actually, probably a better alternative is to have a static method on this
class
> that given a resource type and index, returns/appends the thing "i.e. G0".

Done. Also refactored all usage cases.

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict...
src/pdf/SkPDFResourceDict.cpp:43: SkASSERT(type <=
SkPDFResourceDict::kFont_ResourceType);
On 2013/07/12 19:14:59, vandebo wrote:
> Your asserts here should use 0 and Count

Done.

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict...
src/pdf/SkPDFResourceDict.cpp:107: typeDict = SkNEW(SkPDFDict());
On 2013/07/12 19:14:59, vandebo wrote:
> It'll be the same number of lines, but less fragile to assign to an
SkAutoTUnref
> and also assign to typeDict

Done.

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict.h
File src/pdf/SkPDFResourceDict.h (right):

https://codereview.chromium.org/18977002/diff/25001/src/pdf/SkPDFResourceDict...
src/pdf/SkPDFResourceDict.h:32: // These additional types are defined by the
spec, but not
On 2013/07/12 19:14:59, vandebo wrote:
> nit: I would put Count below this comment.

Done.

[vandebo (ex-Chrome), 2013-07-12 21:58:58]

A couple nits then just waiting for the pre-cls to land.

https://codereview.chromium.org/18977002/diff/30001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/18977002/diff/30001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:440:
fContentStream->writeText(SkPDFResourceDict::getResourceName(
Since this is called twice with the same index, you probably want to store the
result and use it twice.

https://codereview.chromium.org/18977002/diff/30001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1714:
contentEntry->fContent.writeText(SkPDFResourceDict::getResourceName(
This statement looks kind of ugly.  Do you think it would look nicer if the
function appended directly to the stream?

[ducky, 2013-07-12 23:49:31]

Now, just a matter of waiting for the Great SkData Debate...

https://codereview.chromium.org/18977002/diff/30001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/18977002/diff/30001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:440:
fContentStream->writeText(SkPDFResourceDict::getResourceName(
On 2013/07/12 21:58:58, vandebo wrote:
> Since this is called twice with the same index, you probably want to store the
> result and use it twice.

Done.

https://codereview.chromium.org/18977002/diff/30001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1714:
contentEntry->fContent.writeText(SkPDFResourceDict::getResourceName(
On 2013/07/12 21:58:58, vandebo wrote:
> This statement looks kind of ugly.  Do you think it would look nicer if the
> function appended directly to the stream?

I think it's going to look ugly either way, because you need the two
SkPDFResourceDict:: qualifiers and each takes up a huge chunk of space. I like
how right now it's at least contentEntry->fContent.writeText all the way down,
so it's consistent.
Adding a stream method also seems to be a bit specialized, even though it may
remove a few characters of code and would be heavily used.

It might make sense to allow a SkWStream to write a SkString directly (instead
of needing a c_str() in the caller), but it's probably only syntactic sugar. And
my last CL messing with core classes didn't go over so well either...

In this function on line 1718 (new), it also might make more sense to do
contentEntry->fContent.writeScalarAsText, rather than SkPDFScalar::Append. Do
you know if there is any difference between the two, and why this version was
preferred?

[edisonn, 2013-07-17 18:38:22]

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:442: fContentStream->writeText("/Pattern CS /Pattern cs
/");
I have seen this code in another cl, consider re factor it so we minimize
duplicate code. same question with /Pattern being undefined since it is found
twice

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1713:
contentEntry->fContent.writeText(SkPDFResourceDict::getResourceName(
could there be no font at all?

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFResourceDict.h
File src/pdf/SkPDFResourceDict.h (right):

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFResourceDict...
src/pdf/SkPDFResourceDict.h:16: /** \class SkPDFResourceDict
\c has any meaning?

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFUtils.cpp
File src/pdf/SkPDFUtils.cpp (right):

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFUtils.cpp#ne...
src/pdf/SkPDFUtils.cpp:211:
content->writeText(SkPDFResourceDict::getResourceName(
can we have here a null returned?

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFUtils.cpp#ne...
src/pdf/SkPDFUtils.cpp:220:
content->writeText(SkPDFResourceDict::getResourceName(
can we have a null returned here?

[ducky, 2013-07-17 19:51:25]

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:442: fContentStream->writeText("/Pattern CS /Pattern cs
/");
On 2013/07/17 18:38:22, edisonn wrote:
> I have seen this code in another cl, consider re factor it so we minimize
> duplicate code. same question with /Pattern being undefined since it is found
> twice

Yes, this is in the transparency gradients CL, which is supposed to be after
this one. That one does the refactor, so there's no code duplication.

As for Pattern, it's actually using the postfix notation operator CS/cs, so the
more conventional way to read it would be CS(Pattern), cs(Pattern).

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:1713:
contentEntry->fContent.writeText(SkPDFResourceDict::getResourceName(
On 2013/07/17 18:38:22, edisonn wrote:
> could there be no font at all?

SkPDFResourceDict::getResourceName essentially encapsulates the same
functionality as the original, so this will never return null. However, if the
internal data structures were inconsistent, you could pass in a garbage
fontIndex, in which case it would happily give you a garbage resource name out
(just like the original).

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFResourceDict.h
File src/pdf/SkPDFResourceDict.h (right):

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFResourceDict...
src/pdf/SkPDFResourceDict.h:16: /** \class SkPDFResourceDict
On 2013/07/17 18:38:22, edisonn wrote:
> \c has any meaning?

Not really sure. It's a Doxygen command, so it's probably used to generate shiny
documentation. Otherwise, this is essentially the product of a copied and
repurposed SkPDFDict code.

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFUtils.cpp
File src/pdf/SkPDFUtils.cpp (right):

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFUtils.cpp#ne...
src/pdf/SkPDFUtils.cpp:211:
content->writeText(SkPDFResourceDict::getResourceName(
On 2013/07/17 18:38:22, edisonn wrote:
> can we have here a null returned?

The current implementation won't return NULL, as it is essentially just a string
concatenation.

It might make sense to return NULL for objects not in the dict, but that check
isn't done (and it would be a lot of overhead for little gain). If it ever did
return NULL, it means that your output PDF isn't going to be sane, either.

https://codereview.chromium.org/18977002/diff/46001/src/pdf/SkPDFUtils.cpp#ne...
src/pdf/SkPDFUtils.cpp:220:
content->writeText(SkPDFResourceDict::getResourceName(
On 2013/07/17 18:38:22, edisonn wrote:
> can we have a null returned here?

Same response as above.

[edisonn, 2013-07-17 20:07:30]

LGTM from me - Steve, do you have any comments?

[vandebo (ex-Chrome), 2013-07-17 20:54:02]

LGTM

[commit-bot: I haz the power, 2013-07-19 18:36:00]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/18977002/56001

[commit-bot: I haz the power, 2013-07-19 18:58:36]

Change committed as 10202

[commit-bot: I haz the power, 2013-07-22 18:20:37]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/18977002/56001

[commit-bot: I haz the power, 2013-07-22 18:20:54]

Change committed as 10245

[fmalita_google_do_not_use, 2013-07-22 18:36:06]

On 2013/07/22 18:20:54, I haz the power (commit-bot) wrote:
> Change committed as 10245

Looks like this broke the clang builds, will revert.


[18:25:43.684607] In file included from ../src/pdf/SkPDFDevice.cpp:23:
[18:25:43.684661] ../src/pdf/SkPDFResourceDict.h:66:10: error:
'SkPDFResourceDict::getResources' hides overloaded virtual function
[-Werror,-Woverloaded-virtual]
[18:25:43.684725]     void getResources(
[18:25:43.684798]          ^
[18:25:43.684857] ../src/pdf/SkPDFTypes.h:50:18: note: hidden overloaded virtual
function 'SkPDFObject::getResources' declared here
[18:25:43.684918]     virtual void getResources(const SkTSet<SkPDFObject*>&
knownResourceObjects,
[18:25:43.684970]                  ^
[18:25:43.685022]   CXX(target) Debug/obj.target/pdf/src/pdf/SkPDFFont.o
[18:25:43.685074] 1 error generated.

[ducky, 2013-07-22 18:49:49]

Renamed SkPDFResourceDict::getResources to getRefResources to fix the Clang
build error.