Make creation of CPDFSDK_Document clearer

Make creation of CPDFSDK_Document clearer

Move the creation of the CPDFSDK_Document into FPDFDOC_InitFormFillEnvironment
instead of hidden inside a Get method in CDPFXFA_Document.

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

[dsinclair, 2016-09-22 14:47:43]

Patchset #1 (id:1) has been deleted

[dsinclair, 2016-09-22 14:50:09]

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

[dsinclair, 2016-09-22 14:50:10]

PTAL.

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

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfxfa/fpdfxfa...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:194: void
CPDFXFA_Document::SetSDKDoc(std::unique_ptr<CPDFSDK_Document> pSDKDoc) {
The one difference between this and the old code, in the old world if
FPDFDOC_InitFormFillEnvironment was called twice, we'd have the same
CPDFSDK_Document. In this world, we'll have a new one.

[npm, 2016-09-22 15:04:14]

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

[dsinclair, 2016-09-22 15:04:17]

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

[commit-bot: I haz the power, 2016-09-22 15:04:18]

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-22 15:16:08]

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

[commit-bot: I haz the power, 2016-09-22 15:16:09]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-22 17:02:23]

Please double-check my conclusions.  Thanks!

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfformfill.cpp
File fpdfsdk/fpdfformfill.cpp (right):

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfformfill.cp...
fpdfsdk/fpdfformfill.cpp:246: CPDFSDK_Document* pSDKDoc = new
CPDFSDK_Document(pDocument, pEnv);
I like how this adds credibility to the theory that these two should merge.  I
think this the only place we create these two classes.  If so, we can get rid of
setsdkdocument and make ctor do the new'ing of the document itself.

Otherwise, nit: local not needed.

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfformfill.cp...
fpdfsdk/fpdfformfill.cpp:268: pEnv->SetSDKDocument(nullptr);
Similarly, this is the only place where this gets deleted.  The env dtor can
take care of this cleanup.  Maybe a unique_ptr even.

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

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfxfa/fpdfxfa...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:194: void
CPDFXFA_Document::SetSDKDoc(std::unique_ptr<CPDFSDK_Document> pSDKDoc) {
On 2016/09/22 14:50:10, dsinclair wrote:
> The one difference between this and the old code, in the old world if
> FPDFDOC_InitFormFillEnvironment was called twice, we'd have the same
> CPDFSDK_Document. In this world, we'll have a new one.

Acknowledged.

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfxfa/include...
File fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h (right):

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfxfa/include...
fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h:43: CPDFSDK_Document* GetSDKDoc() { return
m_pSDKDoc.get(); }
nit: const method.

[Tom Sepez, 2016-09-22 17:04:42]

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

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfxfa/fpdfxfa...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:194: void
CPDFXFA_Document::SetSDKDoc(std::unique_ptr<CPDFSDK_Document> pSDKDoc) {
> FPDFDOC_InitFormFillEnvironment was called twice, we'd have the same
> > CPDFSDK_Document. In this world, we'll have a new one.
> 
> Acknowledged.
Actually, this scares me.  who own the CPDFSKD_Document?  The XFA document or th
CPDFSDK_Environment?

[dsinclair, 2016-09-22 17:32:13]

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfformfill.cpp
File fpdfsdk/fpdfformfill.cpp (right):

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfformfill.cp...
fpdfsdk/fpdfformfill.cpp:246: CPDFSDK_Document* pSDKDoc = new
CPDFSDK_Document(pDocument, pEnv);
On 2016/09/22 17:02:22, Tom Sepez wrote:
> I like how this adds credibility to the theory that these two should merge.  I
> think this the only place we create these two classes.  If so, we can get rid
of
> setsdkdocument and make ctor do the new'ing of the document itself.
> 
> Otherwise, nit: local not needed.  

Done.

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfformfill.cp...
fpdfsdk/fpdfformfill.cpp:268: pEnv->SetSDKDocument(nullptr);
On 2016/09/22 17:02:22, Tom Sepez wrote:
> Similarly, this is the only place where this gets deleted.  The env dtor can
> take care of this cleanup.  Maybe a unique_ptr even.


That we can't do. The environment only owns the SDKDocument in non-XFA.
CPDFXFA_Document owns it if we're in XFA.

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

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfxfa/fpdfxfa...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:194: void
CPDFXFA_Document::SetSDKDoc(std::unique_ptr<CPDFSDK_Document> pSDKDoc) {
On 2016/09/22 17:04:42, Tom Sepez wrote:
> > FPDFDOC_InitFormFillEnvironment was called twice, we'd have the same
> > > CPDFSDK_Document. In this world, we'll have a new one.
> > 
> > Acknowledged.
> Actually, this scares me.  who own the CPDFSKD_Document?  The XFA document or
th
> CPDFSDK_Environment?


Depends. In XFA it's owned by CPDFXFA_Document. In non-XFA, it's owned by the
CPDFSDK_Environment.  I changed the Init code so that this shouldn't happen,
basically if the CPDFXFA_Document has been init'd already then we just return
the previously created Environment.

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfxfa/include...
File fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h (right):

https://codereview.chromium.org/2353303004/diff/20001/fpdfsdk/fpdfxfa/include...
fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h:43: CPDFSDK_Document* GetSDKDoc() { return
m_pSDKDoc.get(); }
On 2016/09/22 17:02:23, Tom Sepez wrote:
> nit: const method.

Done.

[dsinclair, 2016-09-22 17:33:19]

On 2016/09/22 17:32:13, dsinclair wrote:
> Depends. In XFA it's owned by CPDFXFA_Document. In non-XFA, it's owned by the
> CPDFSDK_Environment.  I changed the Init code so that this shouldn't happen,
> basically if the CPDFXFA_Document has been init'd already then we just return
> the previously created Environment.
> 

I'm going to take a look at this in a followup and see if I can break the
CPDFXFA_Document requirement to have the CPDFSDK_Document passed into it and
instead give it a pointer to the CPDFSDK_Environment.

[Tom Sepez, 2016-09-22 20:03:36]

lgtm

[dsinclair, 2016-09-22 20:06:36]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-09-22 20:06:46]

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-22 20:18:47]

Description was changed from

==========
Make creation of CPDFSDK_Document clearer

Move the creation of the CPDFSDK_Document into FPDFDOC_InitFormFillEnvironment
instead of hidden inside a Get method in CDPFXFA_Document.
==========

to

==========
Make creation of CPDFSDK_Document clearer

Move the creation of the CPDFSDK_Document into FPDFDOC_InitFormFillEnvironment
instead of hidden inside a Get method in CDPFXFA_Document.

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

[commit-bot: I haz the power, 2016-09-22 20:18:48]

Committed patchset #2 (id:40001) as
https://pdfium.googlesource.com/pdfium/+/a939bfe3e102bfb28b4e8a5d951333d16bad...