Split core/include/fpdfdoc/fpdf_ap.h

Split core/include/fpdfdoc/fpdf_ap.h

This CL splits the fpdf_ap.h file apart into individual pieces and moves the
implementations to their own CPP files.

Committed: https://pdfium.googlesource.com/pdfium/+/777b333109db7fceb5f2ca1fc521ced7f863fa18

[dsinclair, 2016-03-30 15:24:12]

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

[dsinclair, 2016-03-30 15:24:13]

PTAL.

[Tom Sepez, 2016-03-30 17:19:53]

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.cpp
File core/fpdfdoc/cpvt_fontmap.cpp (right):

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.c...
core/fpdfdoc/cpvt_fontmap.cpp:31: if (pDoc && pResDict) {
nit: prefer early return

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.c...
core/fpdfdoc/cpvt_fontmap.cpp:58: return nullptr;
nit: maybe this becomes default: and there's no need for any code after the
switch

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.c...
core/fpdfdoc/cpvt_fontmap.cpp:70: return m_sSysFontAlias;
ditto

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.h
File core/fpdfdoc/cpvt_fontmap.h (right):

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.h...
core/fpdfdoc/cpvt_fontmap.h:27: // IPVT_FontMap
nit: // IPVT_FontMap:

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.h...
core/fpdfdoc/cpvt_fontmap.h:37: CPDF_Document* m_pDocument;
can any of these be * const ?

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_provider.h
File core/fpdfdoc/cpvt_provider.h (right):

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_provider....
core/fpdfdoc/cpvt_provider.h:15: public:
kinda wish it was nested as IPDF_VariableText::Provider

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/include/ipdf_v...
File core/fpdfdoc/include/ipdf_variabletext_provider.h (right):

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/include/ipdf_v...
core/fpdfdoc/include/ipdf_variabletext_provider.h:12: class
IPDF_VariableText_Provider {
this all could go in ipdf_variabletext.h if it were nested as
IPDF_VariableText::Provider per previous comment.

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/include/ipdf_v...
core/fpdfdoc/include/ipdf_variabletext_provider.h:25: 
nit: no blank line here.

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/ipdf_variablet...
File core/fpdfdoc/ipdf_variabletext_provider.cpp (right):

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/ipdf_variablet...
core/fpdfdoc/ipdf_variabletext_provider.cpp:9:
IPDF_VariableText_Provider::~IPDF_VariableText_Provider() {}
ditto

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/ipvt_fontmap.h
File core/fpdfdoc/ipvt_fontmap.h (right):

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/ipvt_fontmap.h...
core/fpdfdoc/ipvt_fontmap.h:19: virtual CPDF_Font* GetPDFFont(int32_t
nFontIndex) = 0;
nit: maybe blank line before here, no matter.

https://codereview.chromium.org/1840413002/diff/1/core/include/fpdfdoc/fpdf_vt.h
File core/include/fpdfdoc/fpdf_vt.h (right):

https://codereview.chromium.org/1840413002/diff/1/core/include/fpdfdoc/fpdf_v...
core/include/fpdfdoc/fpdf_vt.h:265: class IPDF_VariableText_Iterator {
Maybe this too gets nested as IPDF_VariableTest::Iterator

[dsinclair, 2016-03-30 18:41:20]

PTAL.

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.cpp
File core/fpdfdoc/cpvt_fontmap.cpp (right):

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.c...
core/fpdfdoc/cpvt_fontmap.cpp:31: if (pDoc && pResDict) {
On 2016/03/30 17:19:52, Tom Sepez wrote:
> nit: prefer early return

Done.

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.c...
core/fpdfdoc/cpvt_fontmap.cpp:58: return nullptr;
On 2016/03/30 17:19:52, Tom Sepez wrote:
> nit: maybe this becomes default: and there's no need for any code after the
> switch

Done.

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.c...
core/fpdfdoc/cpvt_fontmap.cpp:70: return m_sSysFontAlias;
On 2016/03/30 17:19:52, Tom Sepez wrote:
> ditto

Done.

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.h
File core/fpdfdoc/cpvt_fontmap.h (right):

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.h...
core/fpdfdoc/cpvt_fontmap.h:27: // IPVT_FontMap
On 2016/03/30 17:19:52, Tom Sepez wrote:
> nit: // IPVT_FontMap:

Done.

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_fontmap.h...
core/fpdfdoc/cpvt_fontmap.h:37: CPDF_Document* m_pDocument;
On 2016/03/30 17:19:52, Tom Sepez wrote:
> can any of these be * const ?

Done.

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_provider.h
File core/fpdfdoc/cpvt_provider.h (right):

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/cpvt_provider....
core/fpdfdoc/cpvt_provider.h:15: public:
On 2016/03/30 17:19:52, Tom Sepez wrote:
> kinda wish it was nested as IPDF_VariableText::Provider

pdfium:455

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/include/ipdf_v...
File core/fpdfdoc/include/ipdf_variabletext_provider.h (right):

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/include/ipdf_v...
core/fpdfdoc/include/ipdf_variabletext_provider.h:12: class
IPDF_VariableText_Provider {
On 2016/03/30 17:19:52, Tom Sepez wrote:
> this all could go in ipdf_variabletext.h if it were nested as
> IPDF_VariableText::Provider per previous comment.

Acknowledged.

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/include/ipdf_v...
core/fpdfdoc/include/ipdf_variabletext_provider.h:25: 
On 2016/03/30 17:19:52, Tom Sepez wrote:
> nit: no blank line here.

Done.

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/ipdf_variablet...
File core/fpdfdoc/ipdf_variabletext_provider.cpp (right):

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/ipdf_variablet...
core/fpdfdoc/ipdf_variabletext_provider.cpp:9:
IPDF_VariableText_Provider::~IPDF_VariableText_Provider() {}
On 2016/03/30 17:19:52, Tom Sepez wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/ipvt_fontmap.h
File core/fpdfdoc/ipvt_fontmap.h (right):

https://codereview.chromium.org/1840413002/diff/1/core/fpdfdoc/ipvt_fontmap.h...
core/fpdfdoc/ipvt_fontmap.h:19: virtual CPDF_Font* GetPDFFont(int32_t
nFontIndex) = 0;
On 2016/03/30 17:19:52, Tom Sepez wrote:
> nit: maybe blank line before here, no matter.

Done.

https://codereview.chromium.org/1840413002/diff/1/core/include/fpdfdoc/fpdf_vt.h
File core/include/fpdfdoc/fpdf_vt.h (right):

https://codereview.chromium.org/1840413002/diff/1/core/include/fpdfdoc/fpdf_v...
core/include/fpdfdoc/fpdf_vt.h:265: class IPDF_VariableText_Iterator {
On 2016/03/30 17:19:52, Tom Sepez wrote:
> Maybe this too gets nested as IPDF_VariableTest::Iterator 

Acknowledged.

[Tom Sepez, 2016-03-30 20:11:52]

https://codereview.chromium.org/1840413002/diff/2/core/fpdfdoc/cpvt_fontmap.h
File core/fpdfdoc/cpvt_fontmap.h (right):

https://codereview.chromium.org/1840413002/diff/2/core/fpdfdoc/cpvt_fontmap.h...
core/fpdfdoc/cpvt_fontmap.h:38: const CPDF_Dictionary* m_pResDict;
Actually, I mean CPDF_Dictionary* const m_pResDict and such, indicating that the
dictionary must outlive the CPVT_FontMap

[dsinclair, 2016-03-31 13:43:13]

https://codereview.chromium.org/1840413002/diff/2/core/fpdfdoc/cpvt_fontmap.h
File core/fpdfdoc/cpvt_fontmap.h (right):

https://codereview.chromium.org/1840413002/diff/2/core/fpdfdoc/cpvt_fontmap.h...
core/fpdfdoc/cpvt_fontmap.h:38: const CPDF_Dictionary* m_pResDict;
On 2016/03/30 20:11:52, Tom Sepez wrote:
> Actually, I mean CPDF_Dictionary* const m_pResDict and such, indicating that
the
> dictionary must outlive the CPVT_FontMap

Done.

[Tom Sepez, 2016-03-31 22:02:04]

lgtm

[dsinclair, 2016-04-01 03:02:38]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-04-01 03:02:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1840413002/30001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1840413002/30001

[commit-bot: I haz the power, 2016-04-01 03:03:12]

Description was changed from

==========
Split core/include/fpdfdoc/fpdf_ap.h

This CL splits the fpdf_ap.h file apart into individual pieces and moves the
implementations to their own CPP files.
==========

to

==========
Split core/include/fpdfdoc/fpdf_ap.h

This CL splits the fpdf_ap.h file apart into individual pieces and moves the
implementations to their own CPP files.

Committed:
https://pdfium.googlesource.com/pdfium/+/777b333109db7fceb5f2ca1fc521ced7f863...
==========

[commit-bot: I haz the power, 2016-04-01 03:03:13]

Committed patchset #3 (id:30001) as
https://pdfium.googlesource.com/pdfium/+/777b333109db7fceb5f2ca1fc521ced7f863...