Get rid of most instance of 'foo != NULL'

Get rid of most instance of 'foo != NULL'

R=tsepez@chromium.org

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

[Lei Zhang, 2015-12-11 09:13:17]

Patchset #1 (id:1) has been deleted

[Lei Zhang, 2015-12-12 02:11:15]

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

[Lei Zhang, 2015-12-12 02:11:15]

And after FX_BSTRC is gone, the next thing I like to get rid of '!= NULL' once
and for all.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/fsdk_basefo...
File fpdfsdk/src/fsdk_baseform.cpp (left):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/fsdk_basefo...
fpdfsdk/src/fsdk_baseform.cpp:2056: if (!m_pDocument)
Umm, this is a bit late?

[Tom Sepez, 2015-12-14 19:14:12]

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/ttgsubtable.h (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/ttgsubtable.h:280: if (SubTableCount > 0 && SubTable)
{
nit: could we write 
   if (SubTable && SubTableCount > 0)

I don't know why, but it reads much easier to me that way ...
And looking at this, I think its wrong, since we want to delete[] subtable even
if we have a subtablecout of 0?  And then the for loop guards deleting elts in
the 0 length case ... and ...  so

if (SubTable) {
  for (...)
    delete ...;
  delete ...;
}

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:164: ASSERT(this);
Uh, no.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:530: ASSERT(pObjs);
Probably segvs under next line?  Or is it special cased in CPDF_Reference()?

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:537: ASSERT(pObjs);
and here.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render.cpp:548: if (!pDocCache) {
nit: use ? oper.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_ac...
File core/src/fpdfdoc/doc_action.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_ac...
core/src/fpdfdoc/doc_action.cpp:182: if (pJS) {
nit: use ? oper.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_ba...
File core/src/fpdfdoc/doc_basic.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_ba...
core/src/fpdfdoc/doc_basic.cpp:392: ASSERT(pObj);
nit: will segv at line 399.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_ba...
core/src/fpdfdoc/doc_basic.cpp:408: ASSERT(m_pObj);
nitto

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

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:910: ASSERT(pPage);
nit: segv on next line.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:1130: ASSERT(pFieldDict);
nitto

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_formfield.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_formfield.cpp:634: if (GetType() == ComboBox) {
Nit: else if

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_oc...
File core/src/fpdfdoc/doc_ocg.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_oc...
core/src/fpdfdoc/doc_ocg.cpp:27: FXSYS_assert(pDict);
nit: segv on next line.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_oc...
core/src/fpdfdoc/doc_ocg.cpp:238: FXSYS_assert(pOCMDDict);
nitto.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec_jpeg.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpeg.cpp:285: if (icc_buf_ptr) {
Nit: assign it always, eh?

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpeg.cpp:288: if (icc_length) {
ditto.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_buffer.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_buffer.cpp:154: ASSERT(m_pBuffer);
Can expandbuf still fail?

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_buffer.cpp:173: ASSERT(m_pBuffer);
ditto

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_buffer.cpp:187: ASSERT(m_pBuffer);
ditto ditto

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_list.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_list.cpp:94: ASSERT(m_pNodeFree);
nit: segv two lines below.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_list.cpp:127: return (FX_POSITION)pNode;
nit: as long as you're fixing braces, the if doesn't need them but the for does
... as you've done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_maps.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_maps.cpp:42: if ((pAssocRet = m_pHashTable[nBucket]) !=
NULL) {
nit: {} in for statement, not if statement.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_maps.cpp:99: if (pAssoc->key == key) {
nit: no {} for if

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_maps.cpp:117: ASSERT(m_pFreeList);
nit: seg  at 119.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_maps.cpp:297: int index, key_len = key.GetLength();
nit: one per line.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_plex.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_plex.cpp:23: CFX_Plex* pNext = p->pNext;
nit: no need for pNext, just re-assign p here?, also why cast to uint8_t? maybe
just Plex* old = p; or some such.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
File fpdfsdk/src/formfiller/FFL_CheckBox.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
fpdfsdk/src/formfiller/FFL_CheckBox.cpp:94: if (CPWL_CheckBox* pWnd =
(CPWL_CheckBox*)GetPDFWindow(pPageView, FALSE)) {
nit: maybe assignment outside if() and then

return pWnd && pWjnd->IsChecked() != m_Widget->IsChecked():

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
File fpdfsdk/src/formfiller/FFL_IFormFiller.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
fpdfsdk/src/formfiller/FFL_IFormFiller.cpp:161: ASSERT(pAnnot);
nit: Assert below segvs otherwise.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
fpdfsdk/src/formfiller/FFL_IFormFiller.cpp:200:
ASSERT(pAnnot->GetPDFAnnot()->GetSubType() == "Widget");
nitto

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
fpdfsdk/src/formfiller/FFL_IFormFiller.cpp:318:
ASSERT(pAnnot->GetPDFAnnot()->GetSubType() == "Widget");
nitto, several other places.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
fpdfsdk/src/formfiller/FFL_IFormFiller.cpp:761: CFFL_PrivateData* pData =
(CFFL_PrivateData*)pPrivateData;
nit: segv if !pData in second part of assert.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
File fpdfsdk/src/formfiller/FFL_TextField.cpp (left):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
fpdfsdk/src/formfiller/FFL_TextField.cpp:272: ASSERT(m_pApp != NULL);
Might want to keep the m_pApp assert, we might return without doing anything
that would otherwise reference it.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/fsdk_annoth...
File fpdfsdk/src/fsdk_annothandler.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:63: ASSERT(pAnnot);
nit: segv on next line.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/fsdk_basean...
File fpdfsdk/src/fsdk_baseannot.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/fsdk_basean...
fpdfsdk/src/fsdk_baseannot.cpp:543: m_pAnnot->GetRect(rect);
Oh, someday to assume RVO and to have GetRect return a rect

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/pdfwindow/P...
File fpdfsdk/src/pdfwindow/PWL_ListBox.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/pdfwindow/P...
fpdfsdk/src/pdfwindow/PWL_ListBox.cpp:77: ASSERT(m_pList);
Nit: can this fail?

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/pdfwindow/P...
File fpdfsdk/src/pdfwindow/PWL_Note.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/pdfwindow/P...
fpdfsdk/src/pdfwindow/PWL_Note.cpp:1245: // TODO(thestig): Make this return an
enum.
no kidding :).

[Tom Sepez, 2015-12-14 21:10:27]

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render.cpp:1358: FXSYS_assert(pRect);
meh. Only called in one place with pRect as &local, change to pass by const ref
instead?

[Lei Zhang, 2015-12-15 01:38:33]

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/ttgsubtable.h (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/ttgsubtable.h:280: if (SubTableCount > 0 && SubTable)
{
On 2015/12/14 19:14:07, Tom Sepez wrote:
> nit: could we write 
>    if (SubTable && SubTableCount > 0)
> 
> I don't know why, but it reads much easier to me that way ...
> And looking at this, I think its wrong, since we want to delete[] subtable
even
> if we have a subtablecout of 0?  And then the for loop guards deleting elts in
> the 0 length case ... and ...  so
> 
> if (SubTable) {
>   for (...)
>     delete ...;
>   delete ...;
> }

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:164: ASSERT(this);
On 2015/12/14 19:14:07, Tom Sepez wrote:
> Uh, no.

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:530: ASSERT(pObjs);
On 2015/12/14 19:14:08, Tom Sepez wrote:
> Probably segvs under next line?  Or is it special cased in CPDF_Reference()?

The next line dereferences |pObj| but not |pObjs|.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:537: ASSERT(pObjs);
On 2015/12/14 19:14:07, Tom Sepez wrote:
> and here.

Same as above.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render.cpp:548: if (!pDocCache) {
On 2015/12/14 19:14:08, Tom Sepez wrote:
> nit: use ? oper.

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render.cpp:1358: FXSYS_assert(pRect);
On 2015/12/14 21:10:27, Tom Sepez wrote:
> meh. Only called in one place with pRect as &local, change to pass by const
ref
> instead?

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_ac...
File core/src/fpdfdoc/doc_action.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_ac...
core/src/fpdfdoc/doc_action.cpp:182: if (pJS) {
On 2015/12/14 19:14:08, Tom Sepez wrote:
> nit: use ? oper.

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_ba...
File core/src/fpdfdoc/doc_basic.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_ba...
core/src/fpdfdoc/doc_basic.cpp:392: ASSERT(pObj);
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: will segv at line 399.

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_ba...
core/src/fpdfdoc/doc_basic.cpp:408: ASSERT(m_pObj);
On 2015/12/14 19:14:09, Tom Sepez wrote:
> nitto

Done.

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

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:910: ASSERT(pPage);
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: segv on next line.

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_form.cpp:1130: ASSERT(pFieldDict);
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nitto

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_formfield.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_formfield.cpp:634: if (GetType() == ComboBox) {
On 2015/12/14 19:14:11, Tom Sepez wrote:
> Nit: else if

Only if line 632 doesn't invoke something crazy. I'm going to leave it alone.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_oc...
File core/src/fpdfdoc/doc_ocg.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_oc...
core/src/fpdfdoc/doc_ocg.cpp:27: FXSYS_assert(pDict);
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: segv on next line.

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fpdfdoc/doc_oc...
core/src/fpdfdoc/doc_ocg.cpp:238: FXSYS_assert(pOCMDDict);
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nitto.

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec_jpeg.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpeg.cpp:285: if (icc_buf_ptr) {
On 2015/12/14 19:14:11, Tom Sepez wrote:
> Nit: assign it always, eh?

Not sure what you are suggesting exactly. Remove the if ?

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_buffer.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_buffer.cpp:154: ASSERT(m_pBuffer);
On 2015/12/14 19:14:11, Tom Sepez wrote:
> Can expandbuf still fail?

It can return early, so if the default CFX_BinaryBuf ctor starts with a NULL
|m_pBuffer|, and ExpandBuf() does something stupid...

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_list.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_list.cpp:94: ASSERT(m_pNodeFree);
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: segv two lines below.

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_list.cpp:127: return (FX_POSITION)pNode;
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: as long as you're fixing braces, the if doesn't need them but the for
does
> ... as you've done.

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_maps.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_maps.cpp:42: if ((pAssocRet = m_pHashTable[nBucket]) !=
NULL) {
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: {} in for statement, not if statement.

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_maps.cpp:99: if (pAssoc->key == key) {
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: no {} for if

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_maps.cpp:117: ASSERT(m_pFreeList);
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: seg  at 119.

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_maps.cpp:297: int index, key_len = key.GetLength();
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: one per line.

Done.

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_plex.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_plex.cpp:23: CFX_Plex* pNext = p->pNext;
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: no need for pNext, just re-assign p here?, also why cast to uint8_t?
maybe
> just Plex* old = p; or some such.

Done.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
File fpdfsdk/src/formfiller/FFL_CheckBox.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
fpdfsdk/src/formfiller/FFL_CheckBox.cpp:94: if (CPWL_CheckBox* pWnd =
(CPWL_CheckBox*)GetPDFWindow(pPageView, FALSE)) {
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: maybe assignment outside if() and then
> 
> return pWnd && pWjnd->IsChecked() != m_Widget->IsChecked():

Done.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
File fpdfsdk/src/formfiller/FFL_IFormFiller.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
fpdfsdk/src/formfiller/FFL_IFormFiller.cpp:161: ASSERT(pAnnot);
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: Assert below segvs otherwise.

Done.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
fpdfsdk/src/formfiller/FFL_IFormFiller.cpp:200:
ASSERT(pAnnot->GetPDFAnnot()->GetSubType() == "Widget");
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nitto

Done.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
fpdfsdk/src/formfiller/FFL_IFormFiller.cpp:318:
ASSERT(pAnnot->GetPDFAnnot()->GetSubType() == "Widget");
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nitto, several other places.

Done.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
fpdfsdk/src/formfiller/FFL_IFormFiller.cpp:761: CFFL_PrivateData* pData =
(CFFL_PrivateData*)pPrivateData;
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: segv if !pData in second part of assert.

Done.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
File fpdfsdk/src/formfiller/FFL_TextField.cpp (left):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/formfiller/...
fpdfsdk/src/formfiller/FFL_TextField.cpp:272: ASSERT(m_pApp != NULL);
On 2015/12/14 19:14:11, Tom Sepez wrote:
> Might want to keep the m_pApp assert, we might return without doing anything
> that would otherwise reference it.

Done.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/fsdk_annoth...
File fpdfsdk/src/fsdk_annothandler.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/fsdk_annoth...
fpdfsdk/src/fsdk_annothandler.cpp:63: ASSERT(pAnnot);
On 2015/12/14 19:14:11, Tom Sepez wrote:
> nit: segv on next line.

Done.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/fsdk_basean...
File fpdfsdk/src/fsdk_baseannot.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/fsdk_basean...
fpdfsdk/src/fsdk_baseannot.cpp:543: m_pAnnot->GetRect(rect);
On 2015/12/14 19:14:11, Tom Sepez wrote:
> Oh, someday to assume RVO and to have GetRect return a rect

Acknowledged.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/pdfwindow/P...
File fpdfsdk/src/pdfwindow/PWL_ListBox.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/pdfwindow/P...
fpdfsdk/src/pdfwindow/PWL_ListBox.cpp:77: ASSERT(m_pList);
On 2015/12/14 19:14:12, Tom Sepez wrote:
> Nit: can this fail?

No, removed.

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/pdfwindow/P...
File fpdfsdk/src/pdfwindow/PWL_Note.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/fpdfsdk/src/pdfwindow/P...
fpdfsdk/src/pdfwindow/PWL_Note.cpp:1245: // TODO(thestig): Make this return an
enum.
On 2015/12/14 19:14:12, Tom Sepez wrote:
> no kidding :).

Acknowledged.

[Tom Sepez, 2015-12-15 01:58:11]

lgtm

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec_jpeg.cpp (right):

https://codereview.chromium.org/1512763013/diff/40001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpeg.cpp:285: if (icc_buf_ptr) {
On 2015/12/15 01:38:32, Lei Zhang wrote:
> On 2015/12/14 19:14:11, Tom Sepez wrote:
> > Nit: assign it always, eh?
> 
> Not sure what you are suggesting exactly. Remove the if ?

NM.  I thought they were always passed, but that's not so.

[Lei Zhang, 2015-12-15 02:10:33]

Description was changed from

==========
Get rid of most instance of 'foo != NULL'
==========

to

==========
Get rid of most instance of 'foo != NULL'

R=tsepez@chromium.org

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

[Lei Zhang, 2015-12-15 02:10:35]

Committed patchset #4 (id:80001) manually as
e3c7c2b54348da4a6939f6672f6c6bff126815a7 (presubmit successful).