Check whether a pdf has XFA fields before loading XFA fields

Check whether a pdf has XFA fields before loading XFA fields

BUG=NA
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/827a1722f70e7092270b5e0fdfba1e9c0b1ac50f

[jun_fang, 2015-03-03 00:44:26]

jun_fang@foxitsoftware.com changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org

[jun_fang, 2015-03-03 00:44:27]

Hi Tom and Lei,

Please review it.

[Tom Sepez, 2015-03-03 00:53:34]

https://codereview.chromium.org/973673002/diff/20001/fpdfsdk/src/fpdfview.cpp
File fpdfsdk/src/fpdfview.cpp (right):

https://codereview.chromium.org/973673002/diff/20001/fpdfsdk/src/fpdfview.cpp...
fpdfsdk/src/fpdfview.cpp:308: if (!document ||
!((CPDFXFA_Document*)document)->GetPDFDoc())
nit: I'd do this as a local variable assignment followed by a separate
if-statemnt just to be consistent with the flow below.  Can we use a C++-style
cast here?

https://codereview.chromium.org/973673002/diff/20001/fpdfsdk/src/fpdfview.cpp...
fpdfsdk/src/fpdfview.cpp:335: return (document &&
((CPDFXFA_Document*)document)->LoadXFADoc());
nit: outer ( ) not needed.

https://codereview.chromium.org/973673002/diff/20001/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/973673002/diff/20001/samples/pdfium_test.cc#n...
samples/pdfium_test.cc:476: if (FPDF_HasXFAField(doc, docType))
This same test probably has to go into testing/embedder_tests.cpp for
EmbedderTest::OpenDocument() which also calls LoadXFA.

[jun_fang, 2015-03-03 01:02:31]

https://codereview.chromium.org/973673002/diff/20001/fpdfsdk/src/fpdfview.cpp
File fpdfsdk/src/fpdfview.cpp (right):

https://codereview.chromium.org/973673002/diff/20001/fpdfsdk/src/fpdfview.cpp...
fpdfsdk/src/fpdfview.cpp:308: if (!document ||
!((CPDFXFA_Document*)document)->GetPDFDoc())
On 2015/03/03 00:53:34, Tom Sepez wrote:
> nit: I'd do this as a local variable assignment followed by a separate
> if-statemnt just to be consistent with the flow below.  Can we use a C++-style
> cast here?

OK.

https://codereview.chromium.org/973673002/diff/20001/fpdfsdk/src/fpdfview.cpp...
fpdfsdk/src/fpdfview.cpp:335: return (document &&
((CPDFXFA_Document*)document)->LoadXFADoc());
On 2015/03/03 00:53:34, Tom Sepez wrote:
> nit: outer ( ) not needed.

OK.

https://codereview.chromium.org/973673002/diff/20001/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/973673002/diff/20001/samples/pdfium_test.cc#n...
samples/pdfium_test.cc:476: if (FPDF_HasXFAField(doc, docType))
On 2015/03/03 00:53:34, Tom Sepez wrote:
> This same test probably has to go into testing/embedder_tests.cpp for
> EmbedderTest::OpenDocument() which also calls LoadXFA.

Shall we remove LoadXFA here?

[Tom Sepez, 2015-03-03 01:03:27]

> This same test probably has to go into testing/embedder_tests.cpp for
> > EmbedderTest::OpenDocument() which also calls LoadXFA.
> 
> Shall we remove LoadXFA here?
No, you should duplicate the check before calling it.

[Tom Sepez, 2015-03-03 01:05:49]

On 2015/03/03 01:03:27, Tom Sepez wrote:
> > This same test probably has to go into testing/embedder_tests.cpp for
> > > EmbedderTest::OpenDocument() which also calls LoadXFA.
> > 
> > Shall we remove LoadXFA here?
> No, you should duplicate the check before calling it.
Also, can we get some information in
http://code.google.com/p/chromium/issues/detail?id=452793 about what causes
this, and what conditions are required to encounter the issue.

[jun_fang, 2015-03-03 01:05:55]

On 2015/03/03 01:03:27, Tom Sepez wrote:
> > This same test probably has to go into testing/embedder_tests.cpp for
> > > EmbedderTest::OpenDocument() which also calls LoadXFA.
> > 
> > Shall we remove LoadXFA here?
> No, you should duplicate the check before calling it.

Got it. Thanks!

[jun_fang, 2015-03-05 20:40:03]

On 2015/03/03 01:05:55, jun_fang wrote:
> On 2015/03/03 01:03:27, Tom Sepez wrote:
> > > This same test probably has to go into testing/embedder_tests.cpp for
> > > > EmbedderTest::OpenDocument() which also calls LoadXFA.
> > > 
> > > Shall we remove LoadXFA here?
> > No, you should duplicate the check before calling it.
> 
> Got it. Thanks!

Hi Tom,

please review the changes in patch 3.

[Tom Sepez, 2015-03-05 21:24:33]

lgtm

[jun_fang, 2015-03-05 21:39:36]

Committed patchset #3 (id:40001) manually as
827a1722f70e7092270b5e0fdfba1e9c0b1ac50f (presubmit successful).