Remove unused code in CPDFXFA_Document

Remove unused code in CPDFXFA_Document

Remove methods that always just return without doing any work. Clean up the
IXFA_DocProvider interface calls for those methods and cleanup the callers
that were calling the methods.

Committed: https://pdfium.googlesource.com/pdfium/+/89f8fa8694bbf209412845c250af48bbc539962b

[dsinclair, 2016-09-07 21:15:56]

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

[commit-bot: I haz the power, 2016-09-07 21:16:03]

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-07 21:20:55]

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

[dsinclair, 2016-09-07 21:20:56]

PTAL.

I pulled a string and a bunch of stuff fell out .... Please give a good look to
this one to make sure I didn't do something silly.

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

https://codereview.chromium.org/2323523002/diff/1/xfa/fxfa/app/xfa_ffdoc.cpp#...
xfa/fxfa/app/xfa_ffdoc.cpp:266: GetDocProvider()->OpenPDF(this, pXFAReader,
TRUE);
OpenPDF always returns nullptr. So, OpenDoc() will always return FALSE. So, as
far as I can tell this will always trigger a SyntaxError.

https://codereview.chromium.org/2323523002/diff/1/xfa/fxfa/parser/cscript_sig...
File xfa/fxfa/parser/cscript_signaturepseudomodel.cpp (left):

https://codereview.chromium.org/2323523002/diff/1/xfa/fxfa/parser/cscript_sig...
xfa/fxfa/parser/cscript_signaturepseudomodel.cpp:90: CXFA_NodeList* pList =
pNotify->GetDocProvider()->Enumerate(hDoc);
Enumerate() returns nullptr, so we just return.

[commit-bot: I haz the power, 2016-09-07 21:27:54]

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

[commit-bot: I haz the power, 2016-09-07 21:27:55]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-08 15:52:18]

(Got part way thru but didn't finish.  Will look at next week)

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

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:25]

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 14:13:05]

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

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

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-12 18:15:13]

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

https://codereview.chromium.org/2323523002/diff/20001/fpdfsdk/fpdfxfa/include...
fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h:154: FX_BOOL NotifySubmit(FX_BOOL
bPrevOrPost);
nit: Can we make these |bool| while we're at it.

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

https://codereview.chromium.org/2323523002/diff/20001/xfa/fxfa/app/xfa_ffdoc....
xfa/fxfa/app/xfa_ffdoc.cpp:242:
m_pDocumentParser->GetDocument()->GetXFAObject(XFA_HASHCODE_Pdf));
I'm concerned that this all goes away. Does GetXFAObject always return null? 
That seems strange.

[dsinclair, 2016-09-12 18:23:20]

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

https://codereview.chromium.org/2323523002/diff/20001/xfa/fxfa/app/xfa_ffdoc....
xfa/fxfa/app/xfa_ffdoc.cpp:242:
m_pDocumentParser->GetDocument()->GetXFAObject(XFA_HASHCODE_Pdf));
On 2016/09/12 18:15:13, Tom Sepez wrote:
> I'm concerned that this all goes away. Does GetXFAObject always return null? 
> That seems strange.


OpenPDF (line 266) always returns nullptr. So, OpenDoc() will always return
FALSE. So, as
far as I can tell this will always trigger a SyntaxError. (Unless I'm missing
something)?

[dsinclair, 2016-09-12 18:34:50]

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

https://codereview.chromium.org/2323523002/diff/20001/fpdfsdk/fpdfxfa/include...
fpdfsdk/fpdfxfa/include/fpdfxfa_doc.h:154: FX_BOOL NotifySubmit(FX_BOOL
bPrevOrPost);
On 2016/09/12 18:15:13, Tom Sepez wrote:
> nit: Can we make these |bool| while we're at it.

Followup, this code is shuffled around in subsequent patches, will do a followup
cl to change.

[Tom Sepez, 2016-09-12 19:04:55]

Ok, other than that I think we're good but let's get one more pair of eyes on
this.

[dsinclair, 2016-09-13 13:19:48]

weili@ would you be able to give this a second set of eyes?

[Wei Li, 2016-09-13 17:45:53]

have a more general question first

https://codereview.chromium.org/2323523002/diff/20001/xfa/fxfa/app/xfa_fftext...
File xfa/fxfa/app/xfa_fftextedit.cpp (left):

https://codereview.chromium.org/2323523002/diff/20001/xfa/fxfa/app/xfa_fftext...
xfa/fxfa/app/xfa_fftextedit.cpp:393: return
GetDoc()->GetDocProvider()->CheckWord(GetDoc(), sWord);
I am a bit concerned with changes like this. The underlying problem is the
corresponding implementations CheckWord()/>GetSuggestWords() were not
implemented (simply returns false). I would prefer adding TODO or comments to
state "not implemented" fact. Otherwise, we might escalate removing more
functions as GetSuggestWords() seems always return false.

[dsinclair, 2016-09-13 17:51:20]

https://codereview.chromium.org/2323523002/diff/20001/xfa/fxfa/app/xfa_fftext...
File xfa/fxfa/app/xfa_fftextedit.cpp (left):

https://codereview.chromium.org/2323523002/diff/20001/xfa/fxfa/app/xfa_fftext...
xfa/fxfa/app/xfa_fftextedit.cpp:393: return
GetDoc()->GetDocProvider()->CheckWord(GetDoc(), sWord);
On 2016/09/13 17:45:52, Wei Li wrote:
> I am a bit concerned with changes like this. The underlying problem is the
> corresponding implementations CheckWord()/>GetSuggestWords() were not
> implemented (simply returns false). I would prefer adding TODO or comments to
> state "not implemented" fact. Otherwise, we might escalate removing more
> functions as GetSuggestWords() seems always return false.


I guess the question is, do we know if we'll ever implement these methods? If we
have no plans on implementing them, I think we should remove them (this includes
removing the GetSuggestedWords below since it's always FALSE if it isn't in the
API somewhere).

We can always resurrect the code if we want to bring it back in the future, and
that has the added benefit that we can do a proper code review on it at that
point.

[Wei Li, 2016-09-13 18:09:07]

https://codereview.chromium.org/2323523002/diff/20001/xfa/fxfa/app/xfa_fftext...
File xfa/fxfa/app/xfa_fftextedit.cpp (left):

https://codereview.chromium.org/2323523002/diff/20001/xfa/fxfa/app/xfa_fftext...
xfa/fxfa/app/xfa_fftextedit.cpp:393: return
GetDoc()->GetDocProvider()->CheckWord(GetDoc(), sWord);
On 2016/09/13 17:51:19, dsinclair wrote:
> On 2016/09/13 17:45:52, Wei Li wrote:
> > I am a bit concerned with changes like this. The underlying problem is the
> > corresponding implementations CheckWord()/>GetSuggestWords() were not
> > implemented (simply returns false). I would prefer adding TODO or comments
to
> > state "not implemented" fact. Otherwise, we might escalate removing more
> > functions as GetSuggestWords() seems always return false.
> 
> 
> I guess the question is, do we know if we'll ever implement these methods? If
we
> have no plans on implementing them, I think we should remove them (this
includes
> removing the GetSuggestedWords below since it's always FALSE if it isn't in
the
> API somewhere).
> 
> We can always resurrect the code if we want to bring it back in the future,
and
> that has the added benefit that we can do a proper code review on it at that
> point.

I am ok with removing unimplemented code. Should we either remove them
completely or correctly mark where they are not implemented (correctly)?
Returning some default values 0 or true/false from some functions in the middle
layer may make code less readable.

[dsinclair, 2016-09-13 18:12:12]

https://codereview.chromium.org/2323523002/diff/20001/xfa/fxfa/app/xfa_fftext...
File xfa/fxfa/app/xfa_fftextedit.cpp (left):

https://codereview.chromium.org/2323523002/diff/20001/xfa/fxfa/app/xfa_fftext...
xfa/fxfa/app/xfa_fftextedit.cpp:393: return
GetDoc()->GetDocProvider()->CheckWord(GetDoc(), sWord);
On 2016/09/13 18:09:07, Wei Li wrote:
> On 2016/09/13 17:51:19, dsinclair wrote:
> > On 2016/09/13 17:45:52, Wei Li wrote:
> > > I am a bit concerned with changes like this. The underlying problem is the
> > > corresponding implementations CheckWord()/>GetSuggestWords() were not
> > > implemented (simply returns false). I would prefer adding TODO or comments
> to
> > > state "not implemented" fact. Otherwise, we might escalate removing more
> > > functions as GetSuggestWords() seems always return false.
> > 
> > 
> > I guess the question is, do we know if we'll ever implement these methods?
If
> we
> > have no plans on implementing them, I think we should remove them (this
> includes
> > removing the GetSuggestedWords below since it's always FALSE if it isn't in
> the
> > API somewhere).
> > 
> > We can always resurrect the code if we want to bring it back in the future,
> and
> > that has the added benefit that we can do a proper code review on it at that
> > point.
> 
> I am ok with removing unimplemented code. Should we either remove them
> completely or correctly mark where they are not implemented (correctly)?
> Returning some default values 0 or true/false from some functions in the
middle
> layer may make code less readable.


Yea, they should be removed completely. If they're part of the API we should
remove them back to the API entry point and mark that as Not Implemented.

I'll cleanup GetSuggestWords as it's obviously FALSE always, I'm not sure what
to do with CheckWord as it can return both, I'll check the use of the return
value and see what I can figure out.

Will ping when ready.

[dsinclair, 2016-09-13 19:37:19]

PTAL.

I've removed GetSuggestWords. I left CheckWord() because we take the return
value and stuff it back into an event. I don't want to change how the event
works so I think we should keep it.

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

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

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

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

Dry run: This issue passed the CQ dry run.

[Wei Li, 2016-09-13 21:31:20]

lgtm

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

https://codereview.chromium.org/2323523002/diff/60001/fpdfsdk/fpdfxfa/fpdfxfa...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:1120: ExportSubmitFile(pFileHandler,
FXFA_SAVEAS_XML, 0, 0x01111111);
Why change this value? passing in 0 as the default, the flag will be set
properly in ExportSubmitFile(). If you really want to make this explicit, how
about define a constant?

[dsinclair, 2016-09-14 01:06:08]

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

https://codereview.chromium.org/2323523002/diff/60001/fpdfsdk/fpdfxfa/fpdfxfa...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:1120: ExportSubmitFile(pFileHandler,
FXFA_SAVEAS_XML, 0, 0x01111111);
On 2016/09/13 21:31:20, Wei Li wrote:
> Why change this value? passing in 0 as the default, the flag will be set
> properly in ExportSubmitFile(). If you really want to make this explicit, how
> about define a constant?


This was the value of the default parameter, I just removed the default from the
header:

    FX_BOOL _ExportSubmitFile(FPDF_FILEHANDLER* ppFileHandler,	
                              int fileType,	
                              FPDF_DWORD encodeType,	
                              FPDF_DWORD flag = 0x01111111);

I'll see if I can figure out what the flags represent and make constants for
them.

[dsinclair, 2016-09-14 12:52:00]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-09-14 12:52:01]

The patchset sent to the CQ was uploaded after l-g-t-m from weili@chromium.org
Link to the patchset: https://codereview.chromium.org/2323523002/#ps100001
(title: "Review feedback")

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

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 13:11:11]

Description was changed from

==========
Remove unused code in CPDFXFA_Document

Remove methods that always just return without doing any work. Clean up the
IXFA_DocProvider interface calls for those methods and cleanup the callers
that were calling the methods.
==========

to

==========
Remove unused code in CPDFXFA_Document

Remove methods that always just return without doing any work. Clean up the
IXFA_DocProvider interface calls for those methods and cleanup the callers
that were calling the methods.

Committed:
https://pdfium.googlesource.com/pdfium/+/89f8fa8694bbf209412845c250af48bbc539...
==========

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

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