Remove "this==NULL" and adjust corresponding callers

Remove "this==NULL" and adjust corresponding callers

BUG=
R=thakis@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/287e11a

[Bo Xu, 2014-06-27 22:57:44]

Hi Nico, code change is done. Please review.

[Nico, 2014-06-27 23:06:22]

Thanks!

It looks like this adds tab characters in the new lines while the old files
mostly contain spaces as far as I can tell – can you make sure to not add tabs
to files that currently use spaces?

[Bo Xu, 2014-06-28 00:40:25]

On 2014/06/27 23:06:22, Nico (away) wrote:
> Thanks!
> 
> It looks like this adds tab characters in the new lines while the old files
> mostly contain spaces as far as I can tell – can you make sure to not add
tabs
> to files that currently use spaces?

Changes made. Change back to spaces for those files using spaces before.

[Nico, 2014-06-28 01:01:08]

Thanks! This looks good, some comments below.

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:1463: num = pObj ?
pObj->GetNumber() : 0;
nit: You could say "FX_FLOAT num = ..." here, then you don't need the
declaration in line 1452

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp (right):

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:595: while (pDict) {
CPDF_Dictionary::Create() probably never will return 0, so this shouldn't change
anything (?)

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_fdf.cpp (right):

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_fdf.cpp:114: CPDF_Dictionary* pDict =
m_pRootDict->GetDict(FX_BSTRC("FDF"));
is m_pRootDict guaranteed to be not NULL here?

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:958: return pOther ? FALSE
: TRUE;
You want pOther->m_pDict here, not pOther I think.

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:100: if (trailer =
m_Trailers.GetAt(i))
Nit: If you want, you can write this as

  if (CPDF_Dictionary* trailer = m_Trailers.GetAt(i))
    trailer->Release();

then you don't need the declaration on line 98.

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_re...
File core/src/fpdfapi/fpdf_render/fpdf_render_image.cpp (right):

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_re...
core/src/fpdfapi/fpdf_render/fpdf_render_image.cpp:1085: CPDF_Dictionary* pDict
= pGroup->GetDict();
It looks like this file shouldn't have tabs either

https://codereview.chromium.org/361553002/diff/20001/fpdfsdk/src/formfiller/F...
File fpdfsdk/src/formfiller/FFL_CBA_Fontmap.cpp (right):

https://codereview.chromium.org/361553002/diff/20001/fpdfsdk/src/formfiller/F...
fpdfsdk/src/formfiller/FFL_CBA_Fontmap.cpp:246: if (bWidget && sDA.IsEmpty() &&
(pObj = FPDF_GetFieldAttr(pAcroFormDict, "DA")))
I don't know if it matters, but this changes behavior: Previoulsy, if bWidget &&
sDA.IsEmpty(), then if !FPDF_GetFieldAttr(pAcroFormDict, "DA") would get
overwritten with an empty byte string. Now it retains the value it had before.

[Bo Xu, 2014-06-28 01:26:27]

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/361553002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:100: if (trailer =
m_Trailers.GetAt(i))
I was wondering if this will initialize |trailer| each time in the for loop. But
guess the compiler will optimize?

On 2014/06/28 01:01:07, Nico (away) wrote:
> Nit: If you want, you can write this as
> 
>   if (CPDF_Dictionary* trailer = m_Trailers.GetAt(i))
>     trailer->Release();
> 
> then you don't need the declaration on line 98.

[Bo Xu, 2014-06-28 01:52:23]

Attach a new patch. Please review.

[Bo Xu, 2014-06-28 01:52:31]

https://codereview.chromium.org/361553002/diff/20001/fpdfsdk/src/formfiller/F...
File fpdfsdk/src/formfiller/FFL_CBA_Fontmap.cpp (right):

https://codereview.chromium.org/361553002/diff/20001/fpdfsdk/src/formfiller/F...
fpdfsdk/src/formfiller/FFL_CBA_Fontmap.cpp:246: if (bWidget && sDA.IsEmpty() &&
(pObj = FPDF_GetFieldAttr(pAcroFormDict, "DA")))
You are right, this should be adjusted.

On 2014/06/28 01:01:07, Nico (away) wrote:
> I don't know if it matters, but this changes behavior: Previoulsy, if bWidget
&&
> sDA.IsEmpty(), then if !FPDF_GetFieldAttr(pAcroFormDict, "DA") would get
> overwritten with an empty byte string. Now it retains the value it had before.

[Nico, 2014-06-28 02:01:53]

lgtm, thanks.

jam, please merge if this lgty

[Bo Xu, 2014-06-28 02:48:47]

On 2014/06/28 02:01:53, Nico (away) wrote:
> lgtm, thanks.
> 
> jam, please merge if this lgty

Thanks Nico. jam, if this lgty, I can land it.

[Bo Xu, 2014-06-29 00:45:41]

@Jam, do you think this can be landed?

[Nico, 2014-06-29 20:07:37]

https://codereview.chromium.org/361553002/diff/40001/fpdfsdk/src/formfiller/F...
File fpdfsdk/src/formfiller/FFL_CBA_Fontmap.cpp (right):

https://codereview.chromium.org/361553002/diff/40001/fpdfsdk/src/formfiller/F...
fpdfsdk/src/formfiller/FFL_CBA_Fontmap.cpp:243: if ( pObj =
FPDF_GetFieldAttr(m_pAnnotDict, "DA"))
I patched in your change to check that it fixes all this/NULL-related warnings.
It does, but this line adds a new warning:

../../third_party/pdfium/fpdfsdk/src/formfiller/FFL_CBA_Fontmap.cpp:243:12:
warning: using the result of an assignment as a condition without parentheses
[-Wparentheses]
        if ( pObj = FPDF_GetFieldAttr(m_pAnnotDict, "DA"))
             ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/pdfium/fpdfsdk/src/formfiller/FFL_CBA_Fontmap.cpp:243:12:
note: place parentheses around the assignment to silence this warning
        if ( pObj = FPDF_GetFieldAttr(m_pAnnotDict, "DA"))
                  ^
             (                                           )
../../third_party/pdfium/fpdfsdk/src/formfiller/FFL_CBA_Fontmap.cpp:243:12:
note: use '==' to turn this assignment into an equality comparison
        if ( pObj = FPDF_GetFieldAttr(m_pAnnotDict, "DA"))
                  ^
                  ==
1 warning generated.

Please write this as `if ((pObj = FPDF_GetFieldAttr(m_pAnnotDict, "DA")))`.

Thanks!

[Bo Xu, 2014-06-30 00:31:59]

https://codereview.chromium.org/361553002/diff/40001/fpdfsdk/src/formfiller/F...
File fpdfsdk/src/formfiller/FFL_CBA_Fontmap.cpp (right):

https://codereview.chromium.org/361553002/diff/40001/fpdfsdk/src/formfiller/F...
fpdfsdk/src/formfiller/FFL_CBA_Fontmap.cpp:243: if ( pObj =
FPDF_GetFieldAttr(m_pAnnotDict, "DA"))
Done

[jam, 2014-06-30 16:14:32]

On 2014/06/29 00:45:41, bo_xu wrote:
> @Jam, do you think this can be landed?

since it's reviewed by Nico, yes please land, thanks.

[Bo Xu, 2014-06-30 16:49:35]

Committed patchset #4 manually as r287e11a (presubmit successful).