Remove IXFA_Parser, cleanup XFA parser code.

Remove IXFA_Parser, cleanup XFA parser code.

The IXFA_Parser only created a CXFA_SimpleParser, the CXFA_DocumentParser is
only created in one spot and doesn't need all the IXFA_Parser methods.

This CL removes IXFA_Parser, instantiates the CXFA_SimpleParser where needed
and cleans up surrounding code.

Committed: https://pdfium.googlesource.com/pdfium/+/a1b0772321e9b839073b9b312bac22143f2d4011

[dsinclair, 2016-07-06 19:19:10]

dsinclair@chromium.org changed reviewers:
+ thestig@chromium.org, weili@chromium.org

[dsinclair, 2016-07-06 19:19:11]

PTAL.

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp
File xfa/fxfa/app/xfa_ffdoc.cpp (left):

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:168: CXFA_DocumentParser* pDocParser = new
CXFA_DocumentParser(m_pNotify);
This ownership was messed up. The DocumentParser owns the Document and will
cleanup the document when it's done with it. The Document has a pointer to the
DocumentParser.

In this case, we'd leak the document parser here and then later depend on the
Document getting its parser and deleting it, which is really wacky as that would
then delete itself.

Instead store the parser, get it when we needed it and clean it up correctly.

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffwidgetha...
File xfa/fxfa/app/xfa_ffwidgethandler.cpp (left):

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffwidgetha...
xfa/fxfa/app/xfa_ffwidgethandler.cpp:508: CXFA_Node* pNewTemplateNode =
pXFADoc->GetParser()->GetFactory()->CreateNode(
The only time we call SetFactory is to set the Document as the factory. Instead,
skip the calls as the answer should just be the same Document.

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (left):

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:4919:
pDoc->GetParser()->GetNotify()->GetAppProvider();
Document has a GetNotify() method already which proxies to the parser.

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/parser/xfa_document.h
File xfa/fxfa/parser/xfa_document.h (right):

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/parser/xfa_documen...
xfa/fxfa/parser/xfa_document.h:69: CFDE_XMLDoc* GetXMLDoc() const;
This is added so we no longer have to expose GetParser() from Document.

[Lei Zhang, 2016-07-06 23:02:24]

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp
File xfa/fxfa/app/xfa_ffdoc.cpp (right):

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:152: m_pDocumentParser(nullptr),
No need to initialize a std::unique_ptr.

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:171: int32_t iStatus =
m_pDocumentParser->StartParse(m_pStream);
no need for |iStatus|.

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:240: if (iStatus == XFA_PARSESTATUS_Done &&
!m_pPDFDoc) {
Early return here, and then unindent everything else below?

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:242:
m_pDocumentParser->GetDocument()->GetXFAObject(XFA_HASHCODE_Pdf));
Use GetXFADoc() here?

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:271: CXFA_Document* doc =
m_pDocumentParser->GetDocument();
GetXFADoc() here as well? More potential uses of GetXFADoc() below.

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:273: new CXFA_SimpleParser(doc, TRUE));
Can we do bool here?

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:274: if (!pParser)
This can't happen.

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:388: m_pDocumentParser ?
m_pDocumentParser->GetDocument() : nullptr;
This makes me worry that GetXFADoc() calls can crash, since it does not check if
|m_pDocumentParser| contains a nullptr or not.

https://codereview.chromium.org/2123133004/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:394: m_pNotify.reset(nullptr);
Just reset()

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_obj...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_obj...
xfa/fxfa/parser/xfa_object_imp.cpp:938: new CXFA_SimpleParser(m_pDocument,
FALSE));
Can we do bool here?

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
File xfa/fxfa/parser/xfa_parser_imp.cpp (left):

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
xfa/fxfa/parser/xfa_parser_imp.cpp:1359: CloseParser();
No longer needed?

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
File xfa/fxfa/parser/xfa_parser_imp.cpp (right):

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
xfa/fxfa/parser/xfa_parser_imp.cpp:1386: m_pDocument.reset(nullptr);
Just reset()

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
File xfa/fxfa/parser/xfa_parser_imp.h (right):

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
xfa/fxfa/parser/xfa_parser_imp.h:20: CXFA_SimpleParser(CXFA_Document* pFactory,
FX_BOOL bDocumentParser = FALSE);
Can we remove the default argument to |bDocumentParser| while we are here?

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
xfa/fxfa/parser/xfa_parser_imp.h:87: CXFA_DocumentParser(CXFA_FFNotify*
pNotify);
explicit

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_scr...
File xfa/fxfa/parser/xfa_script_resolveprocessor.cpp (left):

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_scr...
xfa/fxfa/parser/xfa_script_resolveprocessor.cpp:14: #include
"xfa/fxfa/parser/xfa_parser.h"
xfa/ can definitely use a few rounds of IWYU cleanup.

[dsinclair, 2016-07-07 13:54:44]

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_obj...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_obj...
xfa/fxfa/parser/xfa_object_imp.cpp:938: new CXFA_SimpleParser(m_pDocument,
FALSE));
On 2016/07/06 23:02:23, Lei Zhang wrote:
> Can we do bool here?

Done.

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
File xfa/fxfa/parser/xfa_parser_imp.cpp (left):

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
xfa/fxfa/parser/xfa_parser_imp.cpp:1359: CloseParser();
On 2016/07/06 23:02:24, Lei Zhang wrote:
> No longer needed?

I thought it wasn't, but looking again it needs to trigger the
m_nodeParser.CloseParser(). Once those resources are changed from Release() to
unique_ptr it won't be needed.

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
File xfa/fxfa/parser/xfa_parser_imp.cpp (right):

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
xfa/fxfa/parser/xfa_parser_imp.cpp:1386: m_pDocument.reset(nullptr);
On 2016/07/06 23:02:24, Lei Zhang wrote:
> Just reset()

Done.

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
File xfa/fxfa/parser/xfa_parser_imp.h (right):

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
xfa/fxfa/parser/xfa_parser_imp.h:20: CXFA_SimpleParser(CXFA_Document* pFactory,
FX_BOOL bDocumentParser = FALSE);
On 2016/07/06 23:02:24, Lei Zhang wrote:
> Can we remove the default argument to |bDocumentParser| while we are here?

Done.

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_par...
xfa/fxfa/parser/xfa_parser_imp.h:87: CXFA_DocumentParser(CXFA_FFNotify*
pNotify);
On 2016/07/06 23:02:24, Lei Zhang wrote:
> explicit

Done.

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_scr...
File xfa/fxfa/parser/xfa_script_resolveprocessor.cpp (left):

https://codereview.chromium.org/2123133004/diff/20001/xfa/fxfa/parser/xfa_scr...
xfa/fxfa/parser/xfa_script_resolveprocessor.cpp:14: #include
"xfa/fxfa/parser/xfa_parser.h"
On 2016/07/06 23:02:24, Lei Zhang wrote:
> xfa/ can definitely use a few rounds of IWYU cleanup.

Acknowledged.

[dsinclair, 2016-07-07 14:54:22]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-07-07 14:54:23]

The CQ bit was unchecked by dsinclair@chromium.org

[Lei Zhang, 2016-07-07 20:36:59]

lgtm

[dsinclair, 2016-07-07 23:32:55]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-07-07 23:33:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 15:32:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 15:32:19]

Try jobs failed on following builders:
  linux_no_v8 on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_no_v8/bui...)
  linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)

[dsinclair, 2016-07-11 13:27:06]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-07-11 13:27:06]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2123133004/#ps80001
(title: "Rebase to master")

[commit-bot: I haz the power, 2016-07-11 13:27:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 13:54:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-11 13:54:37]

Try jobs failed on following builders:
  win_no_v8 on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_no_v8/build...)

[dsinclair, 2016-07-11 14:03:22]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-07-11 14:03:23]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2123133004/#ps100001
(title: "Rebase to master")

[commit-bot: I haz the power, 2016-07-11 14:03:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 15:21:03]

Description was changed from

==========
Remove IXFA_Parser, cleanup XFA parser code.

The IXFA_Parser only created a CXFA_SimpleParser, the CXFA_DocumentParser is
only created in one spot and doesn't need all the IXFA_Parser methods.

This CL removes IXFA_Parser, instantiates the CXFA_SimpleParser where needed
and cleans up surrounding code.
==========

to

==========
Remove IXFA_Parser, cleanup XFA parser code.

The IXFA_Parser only created a CXFA_SimpleParser, the CXFA_DocumentParser is
only created in one spot and doesn't need all the IXFA_Parser methods.

This CL removes IXFA_Parser, instantiates the CXFA_SimpleParser where needed
and cleans up surrounding code.

Committed:
https://pdfium.googlesource.com/pdfium/+/a1b0772321e9b839073b9b312bac22143f2d...
==========

[commit-bot: I haz the power, 2016-07-11 15:21:04]

Committed patchset #6 (id:100001) as
https://pdfium.googlesource.com/pdfium/+/a1b0772321e9b839073b9b312bac22143f2d...