Remove CFX_PtrArray usage in fpdfsdk.

Remove CFX_PtrArray usage in fpdfsdk.

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/8deeacd23872ecc132a0a678e344a018b5114a3a

[Lei Zhang, 2015-11-09 17:46:29]

thestig@chromium.org changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2015-11-09 21:25:32]

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
File core/include/fpdfdoc/fpdf_doc.h (left):

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:582: FX_BOOL IsValidFormField(const void*
pField);
Is dead?

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
File core/include/fpdfdoc/fpdf_doc.h (right):

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:47: explicit CPDF_NameTree(CPDF_Dictionary*
pRoot) { m_pRoot = pRoot; }
nit: prefer initialization list, can m_pRoot be const member?

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:69: explicit CPDF_BookmarkTree(CPDF_Document*
pDoc) : m_pDocument(pDoc) {}
nitto for m_pDocument.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:169: CPDF_ActionFields(const CPDF_Action*
pAction) {
nit: init list, const m_pAction member?

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:173: operator CPDF_Action*() const { return
m_pAction; }
nit: also todo, replace with an explicit GetAction() method.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:268: CPDF_AAction(CPDF_Dictionary* pDict = NULL)
{ m_pDict = pDict; }
nit: prefer two methods and no default param, also init list and const member if
possible (yeah yeah yeah).

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:321: CPDF_Document* m_pDocument;
nit: maybe const?

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:327: explicit CPDF_FileSpec(CPDF_Object* pObj) {
m_pObj = pObj; }
nitto.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:479: CPDF_DefaultAppearance(const
CFX_ByteString& csDA = "") { m_csDA = csDA; }
nit: two methods, no default args.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:649: const std::vector<CPDF_FormField*>* fields,
nit: const reference.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:650: FX_BOOL bIncludeOrExclude) const;
nit: bool.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:891: CPDF_IconFit(CPDF_Dictionary* pDict = NULL)
{ m_pDict = pDict; }
nitto: two methods, const member, no default args, etc.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:1064: explicit CPDF_PageLabel(CPDF_Document*
pDocument) { m_pDocument = pDocument; }
nitto

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:1104: CPDF_Document* m_pDoc;
nit: const member?

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:1109: CPDF_ApSettings(CPDF_Dictionary* pDict =
NULL) { m_pDict = pDict; }
nitto

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_action...
File core/src/fpdfdoc/doc_action.cpp (right):

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_action...
core/src/fpdfdoc/doc_action.cpp:115: std::vector<CPDF_Object*>
CPDF_ActionFields::GetAllFields() const {
Is this always small, or are we just counting on c++11 move semantics?

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.cpp
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:942: int iRet =
m_pFormNotify->BeforeFormReset(this);
nit: local not needed.  This returns an int, right?

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:964: int iRet =
m_pFormNotify->BeforeFormReset(this);
ditto

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:1126: if (dwFlags & 0x04)
nit: magic number.

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:1149: CPDF_FormField* pField =
m_pFieldTree->m_Root.GetField(i);
nit: local not needed.

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:1155: // TODO(thestig): Fix this.
nit: by fix this, you mean "move to header"?

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:1197: if ((dwFlags & 0x02) != 0 &&
pField->m_pDict->GetString("V").IsEmpty()) {
nit: magic number.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/include/fsdk_basefo...
File fpdfsdk/include/fsdk_baseform.h (right):

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/include/fsdk_basefo...
fpdfsdk/include/fsdk_baseform.h:188: void GetWidgets(const CFX_WideString&
sFieldName,
nit: as before, do we just want to return this and count on c++11 move
semantics?  We should be consistent.  I think we may have to go this way for
now.  Hmmm.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/include/fsdk_basefo...
fpdfsdk/include/fsdk_baseform.h:222: std::vector<CPDF_FormField*>
GetFieldFromObjects(
And here too.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.cpp
File fpdfsdk/src/fsdk_baseform.cpp (right):

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:1604: std::vector<CPDFSDK_Widget*>* widgets) const
{
I guess we don't want to first clear |widgets| here, but it seems like we
should.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:1939: ASSERT(pControl != NULL);
nit: just ASSERT(pControl)

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:1944: nFlags &= (~ANNOTFLAG_INVISIBLE);
nit: overparenthesization.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:1949: nFlags &= (~ANNOTFLAG_HIDDEN);
nit: two of these occur in both branches.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:1955: ASSERT(pPageView != NULL);
nit: not needed, segv on 1957.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:1998: CPDFDoc_Environment* pEnv =
m_pDocument->GetEnv();
nit:  move to 2012, lose assert.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:2007: if (bUrlEncoded) {
nit: combine.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:2148: return m_pInterForm->ResetForm(fields,
!(dwFlags & 0x01), true);
nit: magic numbers again for dwFlags.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Docu...
File fpdfsdk/src/javascript/Document.cpp (right):

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Docu...
fpdfsdk/src/javascript/Document.cpp:531: rcAnnot.left -= 1;
nit: some would say --.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Docu...
fpdfsdk/src/javascript/Document.cpp:540: ASSERT(pPage != NULL);
nit: ASSERT(pPage)

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Docu...
fpdfsdk/src/javascript/Document.cpp:544: 
nit: blank line probably not needed here.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Docu...
fpdfsdk/src/javascript/Document.cpp:595: pPDFForm->ResetForm(TRUE);
nit: maybe early return instead of else.  (Someone just mentioned "else
considered harmful" over the weekend in another context to me).

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
File fpdfsdk/src/javascript/Field.cpp (right):

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:117: }
Good catch.  This code is littered with stray semicolons.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:219: ASSERT(pDocument != NULL);
nit: segv on 222 otherwise.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:223: ASSERT(pInterForm != NULL);
nit: segv on 226 otherwise.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:572: CPDF_FormField* pFormField =
FieldArray[0];
I think I've seen this pattern somewhere before.  Maybe consolidate?

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:1932: } else {
nit: else considered harmful? Maybe return TRUE.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:2869: if (bDot)
huh?

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:3199: } else {
nit: vRet = (FormField->GetFieldType() == FIELDTYPE_CHECKBOX) ||
             pFormField->GetFieldType() == FIELDTYPE_RADIOBUTTON)

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Field.h
File fpdfsdk/src/javascript/Field.h (right):

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.h:453: static std::vector<CPDF_FormField*>
GetFormFields(
nit: again, the $64 question is to return these or not.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cpp
File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.c...
fpdfsdk/src/jsapi/fxjs_v8.cpp:36: int m_ObjDefID;
nit: just noticed this could be const?

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.c...
fpdfsdk/src/jsapi/fxjs_v8.cpp:46: static CFXJS_ObjDefinition* ForID(v8::Isolate*
pIsolate, size_t id) {
some places ID is an int, other places it's a size_t.  Can we make it
consistent?

[Lei Zhang, 2015-11-09 22:45:29]

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
File core/include/fpdfdoc/fpdf_doc.h (left):

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:582: FX_BOOL IsValidFormField(const void*
pField);
On 2015/11/09 21:25:31, Tom Sepez wrote:
> Is dead?

It cannot ever return true AFAICT. Please double check.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
File core/include/fpdfdoc/fpdf_doc.h (right):

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:47: explicit CPDF_NameTree(CPDF_Dictionary*
pRoot) { m_pRoot = pRoot; }
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: prefer initialization list, can m_pRoot be const member?

Noted, but this CL is plenty big as is. I'll make another pass later.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:69: explicit CPDF_BookmarkTree(CPDF_Document*
pDoc) : m_pDocument(pDoc) {}
On 2015/11/09 21:25:30, Tom Sepez wrote:
> nitto for m_pDocument.

Acknowledged.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:169: CPDF_ActionFields(const CPDF_Action*
pAction) {
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: init list, const m_pAction member?

Acknowledged.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:173: operator CPDF_Action*() const { return
m_pAction; }
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: also todo, replace with an explicit GetAction() method.

Done.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:268: CPDF_AAction(CPDF_Dictionary* pDict = NULL)
{ m_pDict = pDict; }
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: prefer two methods and no default param, also init list and const member
if
> possible (yeah yeah yeah).

Acknowledged.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:321: CPDF_Document* m_pDocument;
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: maybe const?

Done.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:327: explicit CPDF_FileSpec(CPDF_Object* pObj) {
m_pObj = pObj; }
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nitto.

Acknowledged.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:479: CPDF_DefaultAppearance(const
CFX_ByteString& csDA = "") { m_csDA = csDA; }
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: two methods, no default args.

Acknowledged.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:649: const std::vector<CPDF_FormField*>* fields,
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: const reference.

Some callers pass in a nullptr.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:650: FX_BOOL bIncludeOrExclude) const;
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: bool.

Done.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:891: CPDF_IconFit(CPDF_Dictionary* pDict = NULL)
{ m_pDict = pDict; }
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nitto: two methods, const member, no default args, etc.

Acknowledged.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:1064: explicit CPDF_PageLabel(CPDF_Document*
pDocument) { m_pDocument = pDocument; }
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nitto

Acknowledged.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:1104: CPDF_Document* m_pDoc;
On 2015/11/09 21:25:30, Tom Sepez wrote:
> nit: const member?

Done.

https://codereview.chromium.org/1430213002/diff/1/core/include/fpdfdoc/fpdf_d...
core/include/fpdfdoc/fpdf_doc.h:1109: CPDF_ApSettings(CPDF_Dictionary* pDict =
NULL) { m_pDict = pDict; }
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nitto

Acknowledged.

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_action...
File core/src/fpdfdoc/doc_action.cpp (right):

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_action...
core/src/fpdfdoc/doc_action.cpp:115: std::vector<CPDF_Object*>
CPDF_ActionFields::GetAllFields() const {
On 2015/11/09 21:25:31, Tom Sepez wrote:
> Is this always small, or are we just counting on c++11 move semantics?

We do this in Chromium's base/ too. It's RVO.

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.cpp
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:942: int iRet =
m_pFormNotify->BeforeFormReset(this);
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: local not needed.  This returns an int, right?

Done.

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:964: int iRet =
m_pFormNotify->BeforeFormReset(this);
On 2015/11/09 21:25:31, Tom Sepez wrote:
> ditto

Done. return false, even.

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:1126: if (dwFlags & 0x04)
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: magic number.

Added a TODO.

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:1149: CPDF_FormField* pField =
m_pFieldTree->m_Root.GetField(i);
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: local not needed.

Done.

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:1155: // TODO(thestig): Fix this.
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: by fix this, you mean "move to header"?

Yes, another day.

https://codereview.chromium.org/1430213002/diff/1/core/src/fpdfdoc/doc_form.c...
core/src/fpdfdoc/doc_form.cpp:1197: if ((dwFlags & 0x02) != 0 &&
pField->m_pDict->GetString("V").IsEmpty()) {
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: magic number.

Acknowledged.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/include/fsdk_basefo...
File fpdfsdk/include/fsdk_baseform.h (right):

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/include/fsdk_basefo...
fpdfsdk/include/fsdk_baseform.h:188: void GetWidgets(const CFX_WideString&
sFieldName,
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: as before, do we just want to return this and count on c++11 move
> semantics?  We should be consistent.  I think we may have to go this way for
> now.  Hmmm.

I vaguely remember these may take an existing list and append to them. The ones
that return a std::vector starts with an empty vector for sure.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/include/fsdk_basefo...
fpdfsdk/include/fsdk_baseform.h:222: std::vector<CPDF_FormField*>
GetFieldFromObjects(
On 2015/11/09 21:25:31, Tom Sepez wrote:
> And here too.

Acknowledged.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.cpp
File fpdfsdk/src/fsdk_baseform.cpp (right):

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:1604: std::vector<CPDFSDK_Widget*>* widgets) const
{
On 2015/11/09 21:25:32, Tom Sepez wrote:
> I guess we don't want to first clear |widgets| here, but it seems like we
> should.

No, I think some caller are appending to |widgets|, so we do not want to clear
it.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:1939: ASSERT(pControl != NULL);
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: just ASSERT(pControl)

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:1944: nFlags &= (~ANNOTFLAG_INVISIBLE);
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: overparenthesization.

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:1949: nFlags &= (~ANNOTFLAG_HIDDEN);
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: two of these occur in both branches.

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:1955: ASSERT(pPageView != NULL);
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit: not needed, segv on 1957.

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:1998: CPDFDoc_Environment* pEnv =
m_pDocument->GetEnv();
On 2015/11/09 21:25:31, Tom Sepez wrote:
> nit:  move to 2012, lose assert.

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:2007: if (bUrlEncoded) {
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: combine.

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/fsdk_baseform.c...
fpdfsdk/src/fsdk_baseform.cpp:2148: return m_pInterForm->ResetForm(fields,
!(dwFlags & 0x01), true);
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: magic numbers again for dwFlags.

Acknowledged.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Docu...
File fpdfsdk/src/javascript/Document.cpp (right):

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Docu...
fpdfsdk/src/javascript/Document.cpp:531: rcAnnot.left -= 1;
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: some would say --.

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Docu...
fpdfsdk/src/javascript/Document.cpp:540: ASSERT(pPage != NULL);
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: ASSERT(pPage)

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Docu...
fpdfsdk/src/javascript/Document.cpp:544: 
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: blank line probably not needed here.

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Docu...
fpdfsdk/src/javascript/Document.cpp:595: pPDFForm->ResetForm(TRUE);
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: maybe early return instead of else.  (Someone just mentioned "else
> considered harmful" over the weekend in another context to me).

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
File fpdfsdk/src/javascript/Field.cpp (right):

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:117: }
On 2015/11/09 21:25:32, Tom Sepez wrote:
> Good catch.  This code is littered with stray semicolons.

Acknowledged.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:219: ASSERT(pDocument != NULL);
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: segv on 222 otherwise.

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:223: ASSERT(pInterForm != NULL);
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: segv on 226 otherwise.

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:572: CPDF_FormField* pFormField =
FieldArray[0];
On 2015/11/09 21:25:32, Tom Sepez wrote:
> I think I've seen this pattern somewhere before.  Maybe consolidate?

Me too. Maybe another day though.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:1932: } else {
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: else considered harmful? Maybe return TRUE.

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:2869: if (bDot)
On 2015/11/09 21:25:32, Tom Sepez wrote:
> huh?

Huh! (Copy + pasted)

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.cpp:3199: } else {
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: vRet = (FormField->GetFieldType() == FIELDTYPE_CHECKBOX) ||
>              pFormField->GetFieldType() == FIELDTYPE_RADIOBUTTON)

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Field.h
File fpdfsdk/src/javascript/Field.h (right):

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/javascript/Fiel...
fpdfsdk/src/javascript/Field.h:453: static std::vector<CPDF_FormField*>
GetFormFields(
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: again, the $64 question is to return these or not.

Answer is RVO yet again.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cpp
File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.c...
fpdfsdk/src/jsapi/fxjs_v8.cpp:36: int m_ObjDefID;
On 2015/11/09 21:25:32, Tom Sepez wrote:
> nit: just noticed this could be const?

Done.

https://codereview.chromium.org/1430213002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.c...
fpdfsdk/src/jsapi/fxjs_v8.cpp:46: static CFXJS_ObjDefinition* ForID(v8::Isolate*
pIsolate, size_t id) {
On 2015/11/09 21:25:32, Tom Sepez wrote:
> some places ID is an int, other places it's a size_t.  Can we make it
> consistent?

I went back to int, since we use -1 in some places.

[Tom Sepez, 2015-11-09 23:02:20]

lgtm

https://codereview.chromium.org/1430213002/diff/20001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1430213002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:941: if (bNotify && m_pFormNotify &&
(m_pFormNotify->BeforeFormReset(this) < 0))
nit: overparenthesized

https://codereview.chromium.org/1430213002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:960: if (bNotify && m_pFormNotify &&
(m_pFormNotify->BeforeFormReset(this) < 0))
ditto

[Lei Zhang, 2015-11-10 06:39:54]

https://codereview.chromium.org/1430213002/diff/20001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_form.cpp (right):

https://codereview.chromium.org/1430213002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:941: if (bNotify && m_pFormNotify &&
(m_pFormNotify->BeforeFormReset(this) < 0))
On 2015/11/09 23:02:20, Tom Sepez wrote:
> nit: overparenthesized

Done.

https://codereview.chromium.org/1430213002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:960: if (bNotify && m_pFormNotify &&
(m_pFormNotify->BeforeFormReset(this) < 0))
On 2015/11/09 23:02:20, Tom Sepez wrote:
> ditto

Done.

[Lei Zhang, 2015-11-10 06:41:29]

Committed patchset #4 (id:60001) manually as
8deeacd23872ecc132a0a678e344a018b5114a3a (presubmit successful).