Split CPDFXFA_Document apart

Split CPDFXFA_Document apart

[DO NOT COMMIT]

This CL renames IXFA_DocProvider to IXFA_DocEnvironment to better describe the
purpose. Then, CPDFXFA_Document has all of the IXFA_DocEnvironment methods
removed and placed in CPDFXFA_DocEnvironment. The CPDFXFA_Document then
has a CPDFXFA_DocEnvironment.

This splits the code related to the document apart from the XFA callback methods
to work with that document.

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

[dsinclair, 2016-09-08 16:13:09]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 16:13:12]

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

[dsinclair, 2016-09-08 16:18:25]

Description was changed from

==========
Split CPDFXFA_Document apart

This CL renames IXFA_DocProvider to IXFA_DocEnvironment to better describe the
purpose. Then, CPDFXFA_Document has all of the IXFA_DocEnvironment methods
removed and placed in CPDFXFA_DocEnvironment. The CPDFXFA_Document then
has a CPDFXFA_DocEnvironment.

This splits the code related to the document apart from the XFA callback methods
to work with that document.
==========

to

==========
Split CPDFXFA_Document apart

[DO NOT COMMIT]

This CL renames IXFA_DocProvider to IXFA_DocEnvironment to better describe the
purpose. Then, CPDFXFA_Document has all of the IXFA_DocEnvironment methods
removed and placed in CPDFXFA_DocEnvironment. The CPDFXFA_Document then
has a CPDFXFA_DocEnvironment.

This splits the code related to the document apart from the XFA callback methods
to work with that document.
==========

[dsinclair, 2016-09-08 16:19:39]

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

[dsinclair, 2016-09-08 16:19:40]

PTAL.

This is a step towards making CPDFXFA_Document inherit from CPDF_Document. Once
all this is moved into it's own environment it makes the two classes a lot more
similar.

Does this seem like a sane split? I think it makes sense to split the 'working
with document' bits away from the document bits themselves.

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
File fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp (right):

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:67: m_pXFADoc.reset(pApp->CreateDoc(&m_DocEnv,
m_pPDFDoc.get()));
Maybe we don't need to store the doc environment, just create one at this point?

[commit-bot: I haz the power, 2016-09-08 16:26:42]

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

[commit-bot: I haz the power, 2016-09-08 16:26:43]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-08 17:12:38]

Ah, naming.

Wish we could come up with better names than environment, provider, delegate,
etc. that expressed the role played by
these classes.

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/cpdfxfa_doc...
File fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp (right):

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/cpdfxfa_doc...
fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp:16: #include
"fpdfsdk/include/fsdk_mgr.h"
nit: header order.

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/cpdfxfa_doc...
fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp:44:
CPDFXFA_DocEnvironment::CPDFXFA_DocEnvironment(CPDFXFA_Document* doc)
any code changes here?  Its just moved up from below unchanged, right?  I didn't
scrutinize it.  Let me know if I have to.

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/cpdfxfa_doc...
fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp:84: if (!hWidget || !pRtAnchor ||
!m_pDocument->GetXFADoc() ||
Do we have to worry about null m_pDocuments now that these have moved to a
different class?

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/include/fpd...
File fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h (right):

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/include/fpd...
fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h:77: // |m_pSDKDoc| has to be released
before |m_pPDFDoc| since it needs to access
nit: move this down to line 80, change has to be released to "must be
destroyed".

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/include/fpd...
fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h:88: // Must destroy before |m_pSDKDoc|.
nit: must be destroyed

https://codereview.chromium.org/2328573002/diff/1/xfa/fwl/basewidget/fwl_edit...
File xfa/fwl/basewidget/fwl_editimp.cpp (right):

https://codereview.chromium.org/2328573002/diff/1/xfa/fwl/basewidget/fwl_edit...
xfa/fwl/basewidget/fwl_editimp.cpp:1643: IXFA_DocEnvironment* pDocProvider =
pXFAWidget->GetDoc()->GetDocProvider();
Nit: GetDocProvider method name should change, too

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

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/app/xfa_ffapp.cpp#...
xfa/fxfa/app/xfa_ffapp.cpp:99: CXFA_FFDoc*
CXFA_FFApp::CreateDoc(IXFA_DocEnvironment* pProvider,
nit: call argument pProvider.

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

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:150: CXFA_FFDoc::CXFA_FFDoc(CXFA_FFApp* pApp,
IXFA_DocEnvironment* pDocProvider)
here too, probably other places.

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/app/xfa_ffnotify.h
File xfa/fxfa/app/xfa_ffnotify.h (right):

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/app/xfa_ffnotify.h...
xfa/fxfa/app/xfa_ffnotify.h:60: IXFA_DocEnvironment* GetDocProvider();
"

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/app/xfa_ffnotify.h...
xfa/fxfa/app/xfa_ffnotify.h:60: IXFA_DocEnvironment* GetDocProvider();
Can this be a const method?

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/include/xfa_ffapp.h
File xfa/fxfa/include/xfa_ffapp.h (right):

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/include/xfa_ffapp....
xfa/fxfa/include/xfa_ffapp.h:45: CXFA_FFDoc* CreateDoc(IXFA_DocEnvironment*
pProvider,
"

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/include/xfa_ffdoc.h
File xfa/fxfa/include/xfa_ffdoc.h (right):

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/include/xfa_ffdoc....
xfa/fxfa/include/xfa_ffdoc.h:30: CXFA_FFDoc(CXFA_FFApp* pApp,
IXFA_DocEnvironment* pDocProvider);
"

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/include/xfa_ffdoc....
xfa/fxfa/include/xfa_ffdoc.h:57: IXFA_DocEnvironment* m_pDocProvider;
can this be  IXFA_DocEnvironment* const m_pDocProvider; ?

[dsinclair, 2016-09-08 18:07:50]

I almost want to say CPDFXFA_DocPlatform ... but that's also horrible ...
CPDFXFA_DocAL  ... it's like a HAL but for Doc's, heh.

Suggestions?

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/cpdfxfa_doc...
File fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp (right):

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/cpdfxfa_doc...
fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp:16: #include
"fpdfsdk/include/fsdk_mgr.h"
On 2016/09/08 17:12:37, Tom Sepez (ooo 9 sept) wrote:
> nit: header order.

Sorting is hard.

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/cpdfxfa_doc...
fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp:44:
CPDFXFA_DocEnvironment::CPDFXFA_DocEnvironment(CPDFXFA_Document* doc)
On 2016/09/08 17:12:37, Tom Sepez (ooo 9 sept) wrote:
> any code changes here?  Its just moved up from below unchanged, right?  I
didn't
> scrutinize it.  Let me know if I have to.

The code changes were all to change things like m_pPDFDoc to
m_pDocument->GetPDFDoc() and other document member accesses.

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/cpdfxfa_doc...
fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp:84: if (!hWidget || !pRtAnchor ||
!m_pDocument->GetXFADoc() ||
On 2016/09/08 17:12:37, Tom Sepez (ooo 9 sept) wrote:
> Do we have to worry about null m_pDocuments now that these have moved to a
> different class?

Technically, probably. What about storing as a CPDFXFA_Document& internally
instead of a CPDFXFA_Document*const?

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/include/fpd...
File fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h (right):

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/include/fpd...
fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h:77: // |m_pSDKDoc| has to be released
before |m_pPDFDoc| since it needs to access
On 2016/09/08 17:12:37, Tom Sepez (ooo 9 sept) wrote:
> nit: move this down to line 80, change has to be released to "must be
> destroyed".

Done.

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/include/fpd...
fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h:88: // Must destroy before |m_pSDKDoc|.
On 2016/09/08 17:12:37, Tom Sepez (ooo 9 sept) wrote:
> nit: must be destroyed

Done.

https://codereview.chromium.org/2328573002/diff/1/xfa/fwl/basewidget/fwl_edit...
File xfa/fwl/basewidget/fwl_editimp.cpp (right):

https://codereview.chromium.org/2328573002/diff/1/xfa/fwl/basewidget/fwl_edit...
xfa/fwl/basewidget/fwl_editimp.cpp:1643: IXFA_DocEnvironment* pDocProvider =
pXFAWidget->GetDoc()->GetDocProvider();
On 2016/09/08 17:12:37, Tom Sepez (ooo 9 sept) wrote:
> Nit: GetDocProvider method name should change, too

Done.

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

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/app/xfa_ffapp.cpp#...
xfa/fxfa/app/xfa_ffapp.cpp:99: CXFA_FFDoc*
CXFA_FFApp::CreateDoc(IXFA_DocEnvironment* pProvider,
On 2016/09/08 17:12:37, Tom Sepez (ooo 9 sept) wrote:
> nit: call argument pProvider.

Done.

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

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:150: CXFA_FFDoc::CXFA_FFDoc(CXFA_FFApp* pApp,
IXFA_DocEnvironment* pDocProvider)
On 2016/09/08 17:12:37, Tom Sepez (ooo 9 sept) wrote:
> here too, probably other places.

Done.

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/app/xfa_ffnotify.h
File xfa/fxfa/app/xfa_ffnotify.h (right):

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/app/xfa_ffnotify.h...
xfa/fxfa/app/xfa_ffnotify.h:60: IXFA_DocEnvironment* GetDocProvider();
On 2016/09/08 17:12:37, Tom Sepez (ooo 9 sept) wrote:
> "

Done.

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/app/xfa_ffnotify.h...
xfa/fxfa/app/xfa_ffnotify.h:60: IXFA_DocEnvironment* GetDocProvider();
On 2016/09/08 17:12:37, Tom Sepez (ooo 9 sept) wrote:
> Can this be a const method?

Done.

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/include/xfa_ffapp.h
File xfa/fxfa/include/xfa_ffapp.h (right):

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/include/xfa_ffapp....
xfa/fxfa/include/xfa_ffapp.h:45: CXFA_FFDoc* CreateDoc(IXFA_DocEnvironment*
pProvider,
On 2016/09/08 17:12:37, Tom Sepez (ooo 9 sept) wrote:
> "

Done.

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/include/xfa_ffdoc.h
File xfa/fxfa/include/xfa_ffdoc.h (right):

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/include/xfa_ffdoc....
xfa/fxfa/include/xfa_ffdoc.h:30: CXFA_FFDoc(CXFA_FFApp* pApp,
IXFA_DocEnvironment* pDocProvider);
On 2016/09/08 17:12:38, Tom Sepez (ooo 9 sept) wrote:
> "

Done.

https://codereview.chromium.org/2328573002/diff/1/xfa/fxfa/include/xfa_ffdoc....
xfa/fxfa/include/xfa_ffdoc.h:57: IXFA_DocEnvironment* m_pDocProvider;
On 2016/09/08 17:12:38, Tom Sepez (ooo 9 sept) wrote:
> can this be  IXFA_DocEnvironment* const m_pDocProvider; ?

Done.

[Tom Sepez, 2016-09-08 18:22:01]

lgtm

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/cpdfxfa_doc...
File fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp (right):

https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/cpdfxfa_doc...
fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp:84: if (!hWidget || !pRtAnchor ||
!m_pDocument->GetXFADoc() ||
> Technically, probably. What about storing as a CPDFXFA_Document& internally
> instead of a CPDFXFA_Document*const?

Nah, lets just assert non-nullness in the ctor at line 45, and the m_pDocument
is const for the lifetime of the object, so we're cool.

[dsinclair, 2016-09-12 13:37:50]

On 2016/09/08 18:22:01, Tom Sepez (ooo 9 sept) wrote:
> lgtm
> 
>
https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/cpdfxfa_doc...
> File fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp (right):
> 
>
https://codereview.chromium.org/2328573002/diff/1/fpdfsdk/fpdfxfa/cpdfxfa_doc...
> fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp:84: if (!hWidget || !pRtAnchor ||
> !m_pDocument->GetXFADoc() ||
> > Technically, probably. What about storing as a CPDFXFA_Document& internally
> > instead of a CPDFXFA_Document*const?
> 
> Nah, lets just assert non-nullness in the ctor at line 45, and the m_pDocument
> is const for the lifetime of the object, so we're cool.

done.

[dsinclair, 2016-09-12 13:38:03]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 13:38:14]

Dry run: 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-09-12 13:52:25]

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

[commit-bot: I haz the power, 2016-09-12 13:52:26]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-12 17:38:41]

++LGTM

[dsinclair, 2016-09-13 19:53:08]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 19:53:16]

Dry run: 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-09-13 20:30:24]

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

[commit-bot: I haz the power, 2016-09-13 20:30:25]

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

[dsinclair, 2016-09-13 20:39:01]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 20:39:05]

Dry run: 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-09-13 21:14:19]

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

[commit-bot: I haz the power, 2016-09-13 21:14:20]

Dry run: This issue passed the CQ dry run.

[dsinclair, 2016-09-14 12:56:49]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 12:56:52]

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

[dsinclair, 2016-09-14 13:00:15]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 13:00:22]

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

[dsinclair, 2016-09-14 13:25:20]

The CQ bit was unchecked by dsinclair@chromium.org

[dsinclair, 2016-09-14 13:25:20]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-09-14 13:25:21]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2328573002/#ps140001
(title: "Fix merge")

[commit-bot: I haz the power, 2016-09-14 13:25:30]

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-09-14 14:01:57]

Description was changed from

==========
Split CPDFXFA_Document apart

[DO NOT COMMIT]

This CL renames IXFA_DocProvider to IXFA_DocEnvironment to better describe the
purpose. Then, CPDFXFA_Document has all of the IXFA_DocEnvironment methods
removed and placed in CPDFXFA_DocEnvironment. The CPDFXFA_Document then
has a CPDFXFA_DocEnvironment.

This splits the code related to the document apart from the XFA callback methods
to work with that document.
==========

to

==========
Split CPDFXFA_Document apart

[DO NOT COMMIT]

This CL renames IXFA_DocProvider to IXFA_DocEnvironment to better describe the
purpose. Then, CPDFXFA_Document has all of the IXFA_DocEnvironment methods
removed and placed in CPDFXFA_DocEnvironment. The CPDFXFA_Document then
has a CPDFXFA_DocEnvironment.

This splits the code related to the document apart from the XFA callback methods
to work with that document.

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

[commit-bot: I haz the power, 2016-09-14 14:01:58]

Committed patchset #8 (id:140001) as
https://pdfium.googlesource.com/pdfium/+/a440bb3f11f42b7a22624e9771dd8d9c5707...