Improve efficiency of glyph id collection during font subsetting.

Improve efficiency of glyph id collection during font subsetting.

BUG=none
TEST=none

From Mike's comments in http://codereview.appspot.com/4827041/

[reed1, 2011-07-27 14:21:52]

I think <uint32_t> might be a clearer template parameter, but LGTM

[Steve VanDeBogart, 2011-07-27 18:07:20]

http://codereview.appspot.com/4828044/diff/1/include/pdf/SkBitSet.h
File include/pdf/SkBitSet.h (right):

http://codereview.appspot.com/4828044/diff/1/include/pdf/SkBitSet.h#newcode53
include/pdf/SkBitSet.h:53: void exportTo(SkTDArray<unsigned int>* array);
I agree uint32_t would be cleaner

http://codereview.appspot.com/4828044/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4828044/diff/1/src/pdf/SkPDFFont.cpp#newcode453
src/pdf/SkPDFFont.cpp:453:
const_cast<SkPDFGlyphSet*>(subset)->exportTo(&glyphIDs);
why the const cast?  You should just mark exportTo as const.

[reed1, 2011-07-27 18:25:10]

+1 for making exportTo() const

[arthurhsu, 2011-07-27 18:37:09]

http://codereview.appspot.com/4828044/diff/1/include/pdf/SkBitSet.h
File include/pdf/SkBitSet.h (right):

http://codereview.appspot.com/4828044/diff/1/include/pdf/SkBitSet.h#newcode53
include/pdf/SkBitSet.h:53: void exportTo(SkTDArray<unsigned int>* array);
On 2011/07/27 18:07:20, Steve VanDeBogart wrote:
> I agree uint32_t would be cleaner

It is used to pass over to sfntly which takes unsigned int for concerns about
not having stdint.h under VC 2008.

http://codereview.appspot.com/4828044/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4828044/diff/1/src/pdf/SkPDFFont.cpp#newcode453
src/pdf/SkPDFFont.cpp:453:
const_cast<SkPDFGlyphSet*>(subset)->exportTo(&glyphIDs);
On 2011/07/27 18:07:20, Steve VanDeBogart wrote:
> why the const cast?  You should just mark exportTo as const.

Done.

[reed1, 2011-07-27 18:38:57]

Skia ensures that you'll have uint32_t, as it uses it all over the place.

[Steve VanDeBogart, 2011-07-27 18:41:46]

On 2011/07/27 18:38:57, reed1 wrote:
> Skia ensures that you'll have uint32_t, as it uses it all over the place.

I think the concern is that if unsigned int isn't actually the same size as
uint32_t on some platform (though I don't know of a platform where that's the
case), you'd end up with a wrong result....  Maybe the thing is to make the
interface uint32_t and at the call site where the weirdness could crop up, use a
static assert that unsigned int is the right size.

[Steve VanDeBogart, 2011-07-27 18:42:14]

http://codereview.appspot.com/4828044/diff/6001/include/pdf/SkPDFFont.h
File include/pdf/SkPDFFont.h (right):

http://codereview.appspot.com/4828044/diff/6001/include/pdf/SkPDFFont.h#newco...
include/pdf/SkPDFFont.h:38: void exportTo(SkTDArray<unsigned int>* glyphIDs);
This can be const as well.

[arthurhsu, 2011-07-27 18:57:51]

On 2011/07/27 18:41:46, Steve VanDeBogart wrote:
> On 2011/07/27 18:38:57, reed1 wrote:
> > Skia ensures that you'll have uint32_t, as it uses it all over the place.
> 
> I think the concern is that if unsigned int isn't actually the same size as
> uint32_t on some platform (though I don't know of a platform where that's the
> case), you'd end up with a wrong result....  Maybe the thing is to make the
> interface uint32_t and at the call site where the weirdness could crop up, use
a
> static assert that unsigned int is the right size.

Done.

[arthurhsu, 2011-07-27 18:57:58]

http://codereview.appspot.com/4828044/diff/6001/include/pdf/SkPDFFont.h
File include/pdf/SkPDFFont.h (right):

http://codereview.appspot.com/4828044/diff/6001/include/pdf/SkPDFFont.h#newco...
include/pdf/SkPDFFont.h:38: void exportTo(SkTDArray<unsigned int>* glyphIDs);
On 2011/07/27 18:42:14, Steve VanDeBogart wrote:
> This can be const as well.

Done.

[Steve VanDeBogart, 2011-07-27 19:54:26]

http://codereview.appspot.com/4828044/diff/10001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4828044/diff/10001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:451: SkASSERT(sizeof(unsigned int) == sizeof(uint32_t));
There's SK_COMPILE_ASSERT, which is resolved at compile time.  A comment would
be good.

http://codereview.appspot.com/4828044/diff/10001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:452: SkTDArray<unsigned int> glyphIDs;
This can be uint32_t as well.

http://codereview.appspot.com/4828044/diff/10001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:461: unsigned char* subsetFont = NULL;
Put the assert here - next to where it's important.

[arthurhsu, 2011-07-27 20:39:31]

http://codereview.appspot.com/4828044/diff/10001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/4828044/diff/10001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:451: SkASSERT(sizeof(unsigned int) == sizeof(uint32_t));
On 2011/07/27 19:54:27, Steve VanDeBogart wrote:
> There's SK_COMPILE_ASSERT, which is resolved at compile time.  A comment would
> be good.

Done.

http://codereview.appspot.com/4828044/diff/10001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:452: SkTDArray<unsigned int> glyphIDs;
On 2011/07/27 19:54:27, Steve VanDeBogart wrote:
> This can be uint32_t as well.

Done.

http://codereview.appspot.com/4828044/diff/10001/src/pdf/SkPDFFont.cpp#newcod...
src/pdf/SkPDFFont.cpp:461: unsigned char* subsetFont = NULL;
On 2011/07/27 19:54:27, Steve VanDeBogart wrote:
> Put the assert here - next to where it's important.

Done.

[Steve VanDeBogart, 2011-07-27 21:00:26]

LGTM - committed as r1978