Inflict PPDF_ENABLE_XFA ifdefs on XFA core/

Inflict PPDF_ENABLE_XFA ifdefs on XFA core/

R=thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/5c4c193fd4b6dd0657abf5e74125f9887f91d720

[Tom Sepez, 2015-11-24 22:15:10]

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

[Tom Sepez, 2015-11-24 22:15:11]

Lei, for review.

[Tom Sepez, 2015-11-24 22:19:32]

Description was changed from

==========
Inflict PPDF_ENABLE_XFA ifdefs on master core/
==========

to

==========
Inflict PPDF_ENABLE_XFA ifdefs on XFA core/
==========

[Lei Zhang, 2015-11-24 23:36:04]

Still looking...

https://codereview.chromium.org/1471323004/diff/1/core/include/fpdfdoc/fpdf_d...
File core/include/fpdfdoc/fpdf_doc.h (right):

https://codereview.chromium.org/1471323004/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:835: #endif
Blank line below rather than above? Ditto in other files.

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
File core/include/fxcodec/fx_codec.h (right):

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
core/include/fxcodec/fx_codec.h:11: #include <map>
Can we have a single #ifdef PDF_ENABLE_XFA block for #includes? i.e. mvoe this
down to line 20.

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
core/include/fxcodec/fx_codec.h:27: #ifndef PDF_ENABLE_XFA
Ditto, move these to the separate block below.

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
core/include/fxcodec/fx_codec.h:103: #ifdef PDF_ENABLE_XFA
blink line above, not below.

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
core/include/fxcodec/fx_codec.h:253: #ifndef PDF_ENABLE_XFA
#ifdef PDF_ENABLE_XFA
... nComps,\n...pAttribute...
#else
... nComps,\n
#endif

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
core/include/fxcodec/fx_codec.h:416: #endif
How about adding // PDF_ENABLE_XFA if the block is more than, say, 50 lines
long?

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcrt/fx_ucd.h
File core/include/fxcrt/fx_ucd.h (right):

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcrt/fx_ucd.h...
core/include/fxcrt/fx_ucd.h:10: #ifndef PDF_ENABLE_XFA
Maybe just fx_basic.h, since that #include fx_system.h already?

[Lei Zhang, 2015-11-24 23:38:10]

https://codereview.chromium.org/1471323004/diff/1/core/src/fpdfdoc/doc_form.cpp
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1471323004/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:389: #endif
These 2 needs to be labeled.

https://codereview.chromium.org/1471323004/diff/1/core/src/fxcodec/codec/fx_c...
File core/src/fxcodec/codec/fx_codec.cpp (right):

https://codereview.chromium.org/1471323004/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec.cpp:32: m_pTiffModule(new CCodec_TiffModule) {
If the ordering does not matter, you can put m_pFlateModule at the end.

[Tom Sepez, 2015-11-24 23:39:50]

This is a fully-automated patch.  I restrained myself from doing any of these
cleanups.  I will add the corresponding #endif // PDF_ENABLE_XFA comments along
with fixing whitespace and putting things together in a subsequent patch.

[Tom Sepez, 2015-11-25 00:12:06]

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
File core/include/fxcodec/fx_codec.h (right):

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
core/include/fxcodec/fx_codec.h:253: #ifndef PDF_ENABLE_XFA
Making this purely "additive" (by that I mean no "#else"), followed by git
format gives:

  virtual int ReadHeader(void* pContext,
                         int* width,
                         int* height,
                         int* nComps
#ifdef PDF_ENABLE_XFA
                         ,
                         CFX_DIBAttribute* pAttribute
#endif
                         ) = 0;

which i think is pretty readable.  What do you think?

[Lei Zhang, 2015-11-25 00:18:25]

On 2015/11/25 00:12:06, Tom Sepez wrote:
>
https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
> File core/include/fxcodec/fx_codec.h (right):
> 
>
https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
> core/include/fxcodec/fx_codec.h:253: #ifndef PDF_ENABLE_XFA
> Making this purely "additive" (by that I mean no "#else"), followed by git
> format gives:
> 
>   virtual int ReadHeader(void* pContext,
>                          int* width,
>                          int* height,
>                          int* nComps
> #ifdef PDF_ENABLE_XFA
>                          ,
>                          CFX_DIBAttribute* pAttribute
> #endif
>                          ) = 0;
> 
> which i think is pretty readable.  What do you think?

It's kind of meh. I wonder if we can move |pAttribute| to after pContext ? That
would be prettier but then you'd have to fix all the overrides and callers.

[Tom Sepez, 2015-11-25 00:22:32]

> It's kind of meh. I wonder if we can move |pAttribute| to after pContext ?
That
> would be prettier but then you'd have to fix all the overrides and callers.
The other alternative is:

#ifdef PDF_ENABLE_XFA
   virtual int ReadHeader(void* pContext,
                          int* width,
                          int* height,
                          int* nComps) = 0;
#else
   virtual int ReadHeader(void* pContext,
                          int* width,
                          int* height,
                          int* nComps,   
                          CFX_DIBAttribute* pAttribute) = 0;
#endif


Any of these can be done by hand once the automatic stuff hits.

[Lei Zhang, 2015-11-25 00:24:02]

On 2015/11/25 00:22:32, Tom Sepez wrote:
> > It's kind of meh. I wonder if we can move |pAttribute| to after pContext ?
> That
> > would be prettier but then you'd have to fix all the overrides and callers.
> The other alternative is:
> 
> #ifdef PDF_ENABLE_XFA
>    virtual int ReadHeader(void* pContext,
>                           int* width,
>                           int* height,
>                           int* nComps) = 0;
> #else
>    virtual int ReadHeader(void* pContext,
>                           int* width,
>                           int* height,
>                           int* nComps,   
>                           CFX_DIBAttribute* pAttribute) = 0;
> #endif
> 
> 
> Any of these can be done by hand once the automatic stuff hits.

I guess that's ok too.

[Tom Sepez, 2015-11-25 18:51:46]

I'd like to land this as-is and tidy up in the next CL, without making manual
changes to this generated CL.

https://codereview.chromium.org/1471323004/diff/1/core/include/fpdfdoc/fpdf_d...
File core/include/fpdfdoc/fpdf_doc.h (right):

https://codereview.chromium.org/1471323004/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:835: #endif
On 2015/11/24 23:36:04, Lei Zhang wrote:
> Blank line below rather than above? Ditto in other files.

Yep, that's just a result of the way automated diff works. Will fix manually in
next CL.

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
File core/include/fxcodec/fx_codec.h (right):

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
core/include/fxcodec/fx_codec.h:11: #include <map>
On 2015/11/24 23:36:04, Lei Zhang wrote:
> Can we have a single #ifdef PDF_ENABLE_XFA block for #includes? i.e. mvoe this
> down to line 20.

Yep, will fix in next CL.

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcodec/fx_cod...
core/include/fxcodec/fx_codec.h:27: #ifndef PDF_ENABLE_XFA
On 2015/11/24 23:36:04, Lei Zhang wrote:
> Ditto, move these to the separate block below.

Acknowledged.

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcrt/fx_ucd.h
File core/include/fxcrt/fx_ucd.h (right):

https://codereview.chromium.org/1471323004/diff/1/core/include/fxcrt/fx_ucd.h...
core/include/fxcrt/fx_ucd.h:10: #ifndef PDF_ENABLE_XFA
On 2015/11/24 23:36:04, Lei Zhang wrote:
> Maybe just fx_basic.h, since that #include fx_system.h already?

Yes, will do in next CL.

https://codereview.chromium.org/1471323004/diff/1/core/src/fpdfdoc/doc_form.cpp
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1471323004/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:389: #endif
On 2015/11/24 23:38:10, Lei Zhang wrote:
> These 2 needs to be labeled.
I will label all in next CL.

https://codereview.chromium.org/1471323004/diff/1/core/src/fxcodec/codec/fx_c...
File core/src/fxcodec/codec/fx_codec.cpp (right):

https://codereview.chromium.org/1471323004/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec.cpp:32: m_pTiffModule(new CCodec_TiffModule) {
On 2015/11/24 23:38:10, Lei Zhang wrote:
> If the ordering does not matter, you can put m_pFlateModule at the end.

That's a great idea.

[Lei Zhang, 2015-11-25 20:01:21]

ok, lgtm

[Tom Sepez, 2015-11-25 20:15:42]

Description was changed from

==========
Inflict PPDF_ENABLE_XFA ifdefs on XFA core/
==========

to

==========
Inflict PPDF_ENABLE_XFA ifdefs on XFA core/

R=thestig@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/5c4c193fd4b6dd0657abf5e74125f9887f91...
==========

[Tom Sepez, 2015-11-25 20:15:43]

Committed patchset #1 (id:1) manually as
5c4c193fd4b6dd0657abf5e74125f9887f91d720 (presubmit successful).

[Tom Sepez, 2015-11-25 21:01:39]

https://codereview.chromium.org/1471323004/diff/1/core/src/fpdfdoc/doc_form.cpp
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1471323004/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:389: #endif
Am I blind?  I see no matching ifdef!

[Lei Zhang, 2015-11-25 21:20:21]

https://codereview.chromium.org/1471323004/diff/1/core/src/fpdfdoc/doc_form.cpp
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1471323004/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:389: #endif
On 2015/11/25 21:01:39, Tom Sepez wrote:
> Am I blind?  I see no matching ifdef!

#if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_ on line 333, thus the need for
#endif  // FOO