Merge CPDFSDK_Document into CPDFSDK_FormFillEnvironment

Merge CPDFSDK_Document into CPDFSDK_FormFillEnvironment

This CL merges the CPDFSDK_Document and CPDFSDK_FormFillEnvironment classes
as they always existed and always pointed at each other.

Committed: https://pdfium.googlesource.com/pdfium/+/7cbe68e34257b460bfa3baf0ea68fd6d50e1bc77

[dsinclair, 2016-10-11 14:07:49]

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

[dsinclair, 2016-10-11 14:07:50]

PTAL.

[Tom Sepez, 2016-10-11 16:41:09]

https://codereview.chromium.org/2410893002/diff/1/fpdfsdk/cpdfsdk_formfillenv...
File fpdfsdk/cpdfsdk_formfillenvironment.h (right):

https://codereview.chromium.org/2410893002/diff/1/fpdfsdk/cpdfsdk_formfillenv...
fpdfsdk/cpdfsdk_formfillenvironment.h:216: CPDFSDK_FormFillEnvironment*
GetSDKDocument() { return this; }
Do we need this method?

[Tom Sepez, 2016-10-11 16:48:39]

On 2016/10/11 16:41:09, Tom Sepez wrote:
>
https://codereview.chromium.org/2410893002/diff/1/fpdfsdk/cpdfsdk_formfillenv...
> File fpdfsdk/cpdfsdk_formfillenvironment.h (right):
> 
>
https://codereview.chromium.org/2410893002/diff/1/fpdfsdk/cpdfsdk_formfillenv...
> fpdfsdk/cpdfsdk_formfillenvironment.h:216: CPDFSDK_FormFillEnvironment*
> GetSDKDocument() { return this; }
> Do we need this method?

Can you put the removal of the method in this patch rather than the subsequent
one (i.e. make it just a rename).

[dsinclair, 2016-10-11 21:15:53]

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

[commit-bot: I haz the power, 2016-10-11 21:16:01]

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

[dsinclair, 2016-10-11 21:18:19]

PTAL.

[dsinclair, 2016-10-12 12:42:24]

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

[commit-bot: I haz the power, 2016-10-12 12:42:30]

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

[dsinclair, 2016-10-12 12:51:14]

ping.

[commit-bot: I haz the power, 2016-10-12 12:56:20]

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

[commit-bot: I haz the power, 2016-10-12 12:56:21]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-10-12 18:53:00]

LGTM with a bunch of nits for follow-on.

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
File fpdfsdk/cpdfsdk_formfillenvironment.cpp (right):

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
fpdfsdk/cpdfsdk_formfillenvironment.cpp:50: for (auto& it : m_pageMap)
nit: map of unique_ptrs (keep .clear() if order important)

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
fpdfsdk/cpdfsdk_formfillenvironment.cpp:587: bool ReNew) {
nit: |renew| or |bRenew|  (has bugged me for years)

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
File fpdfsdk/cpdfsdk_formfillenvironment.h (right):

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
fpdfsdk/cpdfsdk_formfillenvironment.h:37: CPDFSDK_InterForm* GetInterForm();
nit: const method?

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
fpdfsdk/cpdfsdk_formfillenvironment.h:90: FX_BOOL GetChangeMark() { return
m_bChangeMask; }
nit: const method.

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
fpdfsdk/cpdfsdk_formfillenvironment.h:236: FX_BOOL m_bChangeMask;
nit: bool (next line too)

[dsinclair, 2016-10-12 18:55:57]

On 2016/10/12 18:53:00, Tom Sepez wrote:
> LGTM with a bunch of nits for follow-on.
> 
>
https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
> File fpdfsdk/cpdfsdk_formfillenvironment.cpp (right):
> 
>
https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
> fpdfsdk/cpdfsdk_formfillenvironment.cpp:50: for (auto& it : m_pageMap)
> nit: map of unique_ptrs (keep .clear() if order important)
> 
>
https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
> fpdfsdk/cpdfsdk_formfillenvironment.cpp:587: bool ReNew) {
> nit: |renew| or |bRenew|  (has bugged me for years)
> 
>
https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
> File fpdfsdk/cpdfsdk_formfillenvironment.h (right):
> 
>
https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
> fpdfsdk/cpdfsdk_formfillenvironment.h:37: CPDFSDK_InterForm* GetInterForm();
> nit: const method?
> 
>
https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
> fpdfsdk/cpdfsdk_formfillenvironment.h:90: FX_BOOL GetChangeMark() { return
> m_bChangeMask; }
> nit: const method.
> 
>
https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
> fpdfsdk/cpdfsdk_formfillenvironment.h:236: FX_BOOL m_bChangeMask;
> nit: bool (next line too)


Will do a followup CL for these. I'd like to keep this mostly focused on merging
the two classes.

[dsinclair, 2016-10-12 18:56:03]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-10-12 18:56:06]

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-10-12 18:56:26]

Description was changed from

==========
Merge CPDFSDK_Document into CPDFSDK_FormFillEnvironment

This CL merges the CPDFSDK_Document and CPDFSDK_FormFillEnvironment classes
as they always existed and always pointed at each other.
==========

to

==========
Merge CPDFSDK_Document into CPDFSDK_FormFillEnvironment

This CL merges the CPDFSDK_Document and CPDFSDK_FormFillEnvironment classes
as they always existed and always pointed at each other.

Committed:
https://pdfium.googlesource.com/pdfium/+/7cbe68e34257b460bfa3baf0ea68fd6d50e1...
==========

[commit-bot: I haz the power, 2016-10-12 18:56:27]

Committed patchset #4 (id:60001) as
https://pdfium.googlesource.com/pdfium/+/7cbe68e34257b460bfa3baf0ea68fd6d50e1...

[dsinclair, 2016-10-12 19:44:29]

Nits fixed in https://codereview.chromium.org/2417633002/

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
File fpdfsdk/cpdfsdk_formfillenvironment.cpp (right):

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
fpdfsdk/cpdfsdk_formfillenvironment.cpp:50: for (auto& it : m_pageMap)
On 2016/10/12 18:52:59, Tom Sepez wrote:
> nit: map of unique_ptrs (keep .clear() if order important)

Done.

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
fpdfsdk/cpdfsdk_formfillenvironment.cpp:587: bool ReNew) {
On 2016/10/12 18:52:59, Tom Sepez wrote:
> nit: |renew| or |bRenew|  (has bugged me for years)

Done.

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
File fpdfsdk/cpdfsdk_formfillenvironment.h (right):

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
fpdfsdk/cpdfsdk_formfillenvironment.h:37: CPDFSDK_InterForm* GetInterForm();
On 2016/10/12 18:52:59, Tom Sepez wrote:
> nit: const method?

Can't, creates if doesn't exist. Moved down with other similar methods and added
a comment.

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
fpdfsdk/cpdfsdk_formfillenvironment.h:90: FX_BOOL GetChangeMark() { return
m_bChangeMask; }
On 2016/10/12 18:52:59, Tom Sepez wrote:
> nit: const method.

Done.

https://codereview.chromium.org/2410893002/diff/60001/fpdfsdk/cpdfsdk_formfil...
fpdfsdk/cpdfsdk_formfillenvironment.h:236: FX_BOOL m_bChangeMask;
On 2016/10/12 18:53:00, Tom Sepez wrote:
> nit: bool (next line too)

Done.