Split core/include/fpdfapi/fpdf_resource.h

Split core/include/fpdfapi/fpdf_resource.h

This CL splits apart the core/include/fpdfapi/fpdf_resource.h file and places
the classes into individual files.

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/61b2fc718910a5ab2a75ec5026b239ff33bccfdc

[dsinclair, 2016-03-22 16:00:16]

dsinclair@chromium.org changed reviewers:
+ tsepez@chromium.org

[dsinclair, 2016-03-22 16:00:18]

PTAL.

Turns out, fpdf_resources.h was really big, heh.

[Tom Sepez, 2016-03-22 16:55:53]

LGTM with comments, few of which are about this CLs changes.

https://codereview.chromium.org/1824033002/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1824033002/diff/20001/BUILD.gn#newcode313
BUILD.gn:313: "core/fpdfapi/fpdf_page/cpdf_countedobject.h",
nit: alphabetize this and the sections at 325 amd 457.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
File core/fpdfapi/fpdf_font/cpdf_cidfont.cpp (right):

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_cidfont.cpp:11: #include
"core/fpdfapi/fpdf_font/font_int.h"
duplicated.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_cidfont.cpp:25: uint8_t a, b, c, d, e, f;
nit: one per line while were at it if you like.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_cidfont.cpp:305: FX_WCHAR this_unicode =
m_pCID2UnicodeMap->UnicodeFromCID((uint16_t)CID);
nit: 80 cols.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_cidfont.cpp:811: int first_code = 0, last_code;
nit: one per line like before?

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
File core/fpdfapi/fpdf_font/cpdf_fontencoding.cpp (right):

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_fontencoding.cpp:23: 8707,   37,     38,     8715,  
40,     41,     8727,   43,     44,
nit: These are all represented as hex in the next N tables.  File a follow-on to
write some code to spit this out as all hex.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_fontencoding.cpp:237: const FX_CHAR* const
StandardEncodingNames[224] = {
nit: as part of that follow-up, maybe the tables above can become size 224,
omitting the first 32 0's as does this table. Save a few bytes of binary size.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_fontencoding.cpp:1145: const FX_CHAR* const
PDFDocEncodingNames[232] = {
nit: but this is 232, since it starts 8 characters before the others.  Maybe all
of these become size 232 with an "==0" assumption for the first 24.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_fontencoding.cpp:1634: FX_DWORD PDF_FindCode(const
uint16_t* pCodes, uint16_t unicode) {
nit: mind-crushingly slow if we need to go in this direction. Add to bug?

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_fontencoding.cpp:1769: if (encoding ==
PDFFONT_ENCODING_PDFDOC) {
This gets saner if they were all offset by 24.

https://codereview.chromium.org/1824033002/diff/20001/public/fpdf_edit.h
File public/fpdf_edit.h (right):

https://codereview.chromium.org/1824033002/diff/20001/public/fpdf_edit.h#newc...
public/fpdf_edit.h:19: ((((uint32_t)(((uint8_t)(b) | ((uint16_t)((uint8_t)(g))
<< 8)) | \
Formatting, also this looks wrong.  We want to cast all of these to FX_DWORD or
uint32_t to avoid silent promotion to int for smaller types.

[dsinclair, 2016-03-23 00:00:26]

PTAL.

https://codereview.chromium.org/1824033002/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1824033002/diff/20001/BUILD.gn#newcode313
BUILD.gn:313: "core/fpdfapi/fpdf_page/cpdf_countedobject.h",
On 2016/03/22 16:55:52, Tom Sepez wrote:
> nit: alphabetize this and the sections at 325 amd 457.

Did a 'git cl format' pass and fixed this up. Strange it didn't warn me on
upload.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
File core/fpdfapi/fpdf_font/cpdf_cidfont.cpp (right):

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_cidfont.cpp:11: #include
"core/fpdfapi/fpdf_font/font_int.h"
On 2016/03/22 16:55:52, Tom Sepez wrote:
> duplicated.

Done.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_cidfont.cpp:25: uint8_t a, b, c, d, e, f;
On 2016/03/22 16:55:53, Tom Sepez wrote:
> nit: one per line while were at it if you like.

Done.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_cidfont.cpp:305: FX_WCHAR this_unicode =
m_pCID2UnicodeMap->UnicodeFromCID((uint16_t)CID);
On 2016/03/22 16:55:52, Tom Sepez wrote:
> nit: 80 cols.

Done.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_cidfont.cpp:811: int first_code = 0, last_code;
On 2016/03/22 16:55:52, Tom Sepez wrote:
> nit: one per line like before?

Sigh, bad merge.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
File core/fpdfapi/fpdf_font/cpdf_fontencoding.cpp (right):

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_fontencoding.cpp:23: 8707,   37,     38,     8715,  
40,     41,     8727,   43,     44,
On 2016/03/22 16:55:53, Tom Sepez wrote:
> nit: These are all represented as hex in the next N tables.  File a follow-on
to
> write some code to spit this out as all hex.

pdfium:443

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_fontencoding.cpp:237: const FX_CHAR* const
StandardEncodingNames[224] = {
On 2016/03/22 16:55:53, Tom Sepez wrote:
> nit: as part of that follow-up, maybe the tables above can become size 224,
> omitting the first 32 0's as does this table. Save a few bytes of binary size.

Acknowledged.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_fontencoding.cpp:1145: const FX_CHAR* const
PDFDocEncodingNames[232] = {
On 2016/03/22 16:55:53, Tom Sepez wrote:
> nit: but this is 232, since it starts 8 characters before the others.  Maybe
all
> of these become size 232 with an "==0" assumption for the first 24.

Acknowledged.

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_fontencoding.cpp:1634: FX_DWORD PDF_FindCode(const
uint16_t* pCodes, uint16_t unicode) {
On 2016/03/22 16:55:53, Tom Sepez wrote:
> nit: mind-crushingly slow if we need to go in this direction. Add to bug?

Seemed distinct enough that I made a separate bug: pdfium:444

https://codereview.chromium.org/1824033002/diff/20001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_fontencoding.cpp:1769: if (encoding ==
PDFFONT_ENCODING_PDFDOC) {
On 2016/03/22 16:55:53, Tom Sepez wrote:
> This gets saner if they were all offset by 24.

Acknowledged.

https://codereview.chromium.org/1824033002/diff/20001/public/fpdf_edit.h
File public/fpdf_edit.h (right):

https://codereview.chromium.org/1824033002/diff/20001/public/fpdf_edit.h#newc...
public/fpdf_edit.h:19: ((((uint32_t)(((uint8_t)(b) | ((uint16_t)((uint8_t)(g))
<< 8)) | \
On 2016/03/22 16:55:53, Tom Sepez wrote:
> Formatting, also this looks wrong.  We want to cast all of these to FX_DWORD
or
> uint32_t to avoid silent promotion to int for smaller types.

Done. You may want to verify my bracketing.

[Tom Sepez, 2016-03-23 19:57:02]

https://codereview.chromium.org/1824033002/diff/60001/public/fpdf_edit.h
File public/fpdf_edit.h (right):

https://codereview.chromium.org/1824033002/diff/60001/public/fpdf_edit.h#newc...
public/fpdf_edit.h:19: ((uint32_t)((uint32_t)(b) | ((uint32_t)(g) << 8) |
((uint32_t)(r) << 16) | \
not quite.  They are counting on the cast to uint8_t to truncate values above
256 to occupy bottom byte only.  I'd prefer an explicit (b & 0xff) rather than
the cast, so
(static_cast<uint32_t>(b) & 0xFF) |
((static_cast<uint32_t>(g) & 0xFF) << 8) | ...

[dsinclair, 2016-03-23 20:09:11]

https://codereview.chromium.org/1824033002/diff/60001/public/fpdf_edit.h
File public/fpdf_edit.h (right):

https://codereview.chromium.org/1824033002/diff/60001/public/fpdf_edit.h#newc...
public/fpdf_edit.h:19: ((uint32_t)((uint32_t)(b) | ((uint32_t)(g) << 8) |
((uint32_t)(r) << 16) | \
On 2016/03/23 19:57:01, Tom Sepez wrote:
> not quite.  They are counting on the cast to uint8_t to truncate values above
> 256 to occupy bottom byte only.  I'd prefer an explicit (b & 0xff) rather than
> the cast, so
> (static_cast<uint32_t>(b) & 0xFF) |
> ((static_cast<uint32_t>(g) & 0xFF) << 8) | ...
>  

Can't static_cast as this is public/ but made the other changes.

[Tom Sepez, 2016-03-23 21:47:43]

lgtm

[dsinclair, 2016-03-23 23:21:30]

Description was changed from

==========
Split core/include/fpdfapi/fpdf_resource.h

This CL splits apart the core/include/fpdfapi/fpdf_resource.h file and places
the classes into individual files.
==========

to

==========
Split core/include/fpdfapi/fpdf_resource.h

This CL splits apart the core/include/fpdfapi/fpdf_resource.h file and places
the classes into individual files.

R=tsepez@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/61b2fc718910a5ab2a75ec5026b239ff33bc...
==========

[dsinclair, 2016-03-23 23:21:33]

Committed patchset #6 (id:100001) manually as
61b2fc718910a5ab2a75ec5026b239ff33bccfdc (presubmit successful).