Fix a bunch of -Wunused-but-set-variable warnings.

Fix a bunch of -Wunused-but-set-variable warnings.

Also fix a few nits and other errors along the way.

R=tsepez@chromium.org

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

[Lei Zhang, 2015-04-17 00:34:11]

https://codereview.chromium.org/1098583002/diff/1/core/src/fpdfapi/fpdf_page/...
File core/src/fpdfapi/fpdf_page/pageint.h (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/pageint.h:428: FX_INT32               
m_nSrcComponents;
This is the only one I can make private, and it made the decl order here match
with the init order in the .cpp file. This quells a compiler warning.

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_font...
File core/src/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_font...
core/src/fxge/ge/fx_ge_fontmap.cpp:1370: if (readCnt != 12) {
Actually checking here to prevent uninit reads.

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_text...
File core/src/fxge/ge/fx_ge_text.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_text...
core/src/fxge/ge/fx_ge_text.cpp:1192: FXFT_Set_MM_Design_Coordinates(m_Face, 2,
coords);
Maybe we should check here?

[Lei Zhang, 2015-04-17 00:34:21]

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

[Lei Zhang, 2015-04-17 00:34:22]

Forgot to add you.

[Tom Sepez, 2015-04-17 15:38:20]

On 2015/04/17 00:34:22, Lei Zhang wrote:
> Forgot to add you.

Also see https://codereview.chromium.org/818363006/, which is unlikely to land. 
In that CL, Bruce and I took a stab at this, but we were concerned that many
cases were actual bugs where the code didn't correspond to the author's intent.

[Tom Sepez, 2015-04-17 16:20:17]

https://codereview.chromium.org/1098583002/diff/1/core/src/fpdfapi/fpdf_edit/...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_image.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fpdfapi/fpdf_edit/...
core/src/fpdfapi/fpdf_edit/fpdf_edit_image.cpp:200: pMaskBitmap =
pBitmap->GetAlphaMask();
// Despite it's name, GetAlphaMask() does the |new CFX_DIBitmap|.
pDeleteMaskBitmap = TRUE;

https://codereview.chromium.org/1098583002/diff/1/core/src/fpdfapi/fpdf_edit/...
core/src/fpdfapi/fpdf_edit/fpdf_edit_image.cpp:205: pMaskBitmap = pMask;
// presumably this is the non-allocating path.

https://codereview.chromium.org/1098583002/diff/1/core/src/fpdfapi/fpdf_edit/...
core/src/fpdfapi/fpdf_edit/fpdf_edit_image.cpp:247: if (pBitmap->HasAlpha()) {
This duplicates the test that lead to the allocation above, and feels fragile. 
I'd do the following:
1. rename bDeleteMask to bDeleteMaskBitmap since there are other mask_* locals
here.
2. Declare and set bDeleteMask to FALSE at line 198 at the same time we declare
pMaskBitmap.
3. set pMaskBitmap to TRUE at line 200
4. test pDeleteMaskBitmap here instead of the hasalpha.

https://codereview.chromium.org/1098583002/diff/1/core/src/fpdftext/fpdf_text...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fpdftext/fpdf_text...
core/src/fpdftext/fpdf_text_int.cpp:2536: return;
nit: pointless return statement.

https://codereview.chromium.org/1098583002/diff/1/core/src/fxcodec/codec/fx_c...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:366: out_space =
cmsGetColorSpace(in_prof);
nit: declaration of out_space can move here from line 356.

https://codereview.chromium.org/1098583002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:785: failed:
using |flag| might have avoided this code duplication. This is screaming for
unique_ptr, which I am going to add one of these days.

I suppose you could:
1. change |failed:| to |done:| for clarity
2. change |flag| to |result| for clarity
3. remove the duplicated code
4. test channel_bufs and ajdust_comps before freeing them.
5. initialize channel_bufs and adjust_comps to NULL up top.
6. conver returns to goto done.

If you wanted to be a crufty C programmer.  Or just leave it.

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/dib/fx_dib_co...
File core/src/fxge/dib/fx_dib_composite.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/dib/fx_dib_co...
core/src/fxge/dib/fx_dib_composite.cpp:4154: bRgb = TRUE;
nit: or maybe just ???
    FX_BOOL bRgb = src_Bpp > 1 && !pSrcBitmap->IsCmykImage();

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_font...
File core/src/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_font...
core/src/fxge/ge/fx_ge_fontmap.cpp:1377: FX_LPBYTE offsets = FX_Alloc(FX_BYTE,
nFaces * 4);
no reason to believe this doesn't overflow.

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_font...
core/src/fxge/ge/fx_ge_fontmap.cpp:1387: for (FX_DWORD i = 0; i < nFaces; i ++)
{
Array bounds violation if overflow.

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_text...
File core/src/fxge/ge/fx_ge_text.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_text...
core/src/fxge/ge/fx_ge_text.cpp:1192: FXFT_Set_MM_Design_Coordinates(m_Face, 2,
coords);
On 2015/04/17 00:34:10, Lei Zhang wrote:
> Maybe we should check here?

Maybe at least:
     (void) FXFT_Set_MM_Design_Coordinates(m_Face, 2, coords);
etc. to indicate deliberately ignoring error for now.

[Lei Zhang, 2015-05-08 05:40:39]

https://codereview.chromium.org/1098583002/diff/1/core/src/fpdfapi/fpdf_edit/...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_image.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fpdfapi/fpdf_edit/...
core/src/fpdfapi/fpdf_edit/fpdf_edit_image.cpp:200: pMaskBitmap =
pBitmap->GetAlphaMask();
On 2015/04/17 16:20:16, Tom Sepez wrote:
> // Despite it's name, GetAlphaMask() does the |new CFX_DIBitmap|.
> pDeleteMaskBitmap = TRUE;

I'm punting on this file as well for now. Will revisit.

https://codereview.chromium.org/1098583002/diff/1/core/src/fpdftext/fpdf_text...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fpdftext/fpdf_text...
core/src/fpdftext/fpdf_text_int.cpp:2536: return;
On 2015/04/17 16:20:16, Tom Sepez wrote:
> nit: pointless return statement.

Done.

https://codereview.chromium.org/1098583002/diff/1/core/src/fxcodec/codec/fx_c...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:366: out_space =
cmsGetColorSpace(in_prof);
On 2015/04/17 16:20:16, Tom Sepez wrote:
> nit: declaration of out_space can move here from line 356.

Done.

https://codereview.chromium.org/1098583002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:785: failed:
On 2015/04/17 16:20:16, Tom Sepez wrote:
> using |flag| might have avoided this code duplication. This is screaming for
> unique_ptr, which I am going to add one of these days.
> 
> I suppose you could:
> 1. change |failed:| to |done:| for clarity
> 2. change |flag| to |result| for clarity
> 3. remove the duplicated code
> 4. test channel_bufs and ajdust_comps before freeing them.
> 5. initialize channel_bufs and adjust_comps to NULL up top.
> 6. conver returns to goto done.
> 
> If you wanted to be a crufty C programmer.  Or just leave it.

Done. I'll work on the beard.

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/dib/fx_dib_co...
File core/src/fxge/dib/fx_dib_composite.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/dib/fx_dib_co...
core/src/fxge/dib/fx_dib_composite.cpp:4154: bRgb = TRUE;
On 2015/04/17 16:20:16, Tom Sepez wrote:
> nit: or maybe just ???
>     FX_BOOL bRgb = src_Bpp > 1 && !pSrcBitmap->IsCmykImage();

Done.

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_font...
File core/src/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_font...
core/src/fxge/ge/fx_ge_fontmap.cpp:1377: FX_LPBYTE offsets = FX_Alloc(FX_BYTE,
nFaces * 4);
On 2015/04/17 16:20:17, Tom Sepez wrote:
> no reason to believe this doesn't overflow.

Done.

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_font...
core/src/fxge/ge/fx_ge_fontmap.cpp:1387: for (FX_DWORD i = 0; i < nFaces; i ++)
{
On 2015/04/17 16:20:16, Tom Sepez wrote:
> Array bounds violation if overflow.

Should be ok with an overflow check, right?

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_text...
File core/src/fxge/ge/fx_ge_text.cpp (right):

https://codereview.chromium.org/1098583002/diff/1/core/src/fxge/ge/fx_ge_text...
core/src/fxge/ge/fx_ge_text.cpp:1192: FXFT_Set_MM_Design_Coordinates(m_Face, 2,
coords);
On 2015/04/17 16:20:17, Tom Sepez wrote:
> On 2015/04/17 00:34:10, Lei Zhang wrote:
> > Maybe we should check here?
> 
> Maybe at least:
>      (void) FXFT_Set_MM_Design_Coordinates(m_Face, 2, coords);
> etc. to indicate deliberately ignoring error for now.

I'll try to fix this separately later.

[Tom Sepez, 2015-05-08 16:03:00]

lgtm

https://codereview.chromium.org/1098583002/diff/20001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/1098583002/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:355: cmsUInt32Number in_type,
out_type;
nit: one per line.

https://codereview.chromium.org/1098583002/diff/20001/core/src/fxge/ge/fx_ge_...
File core/src/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1098583002/diff/20001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_fontmap.cpp:1360: {
nit: too bad this doesn't return errors.

[Lei Zhang, 2015-05-08 18:02:05]

https://codereview.chromium.org/1098583002/diff/20001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/1098583002/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:355: cmsUInt32Number in_type,
out_type;
On 2015/05/08 16:03:00, Tom Sepez wrote:
> nit: one per line.

Done. I took care of the ints below as well.

https://codereview.chromium.org/1098583002/diff/20001/core/src/fxge/ge/fx_ge_...
File core/src/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1098583002/diff/20001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_fontmap.cpp:1360: {
On 2015/05/08 16:03:00, Tom Sepez wrote:
> nit: too bad this doesn't return errors.

I'll look at plumbing the result up later.

[Lei Zhang, 2015-05-08 18:04:04]

Committed patchset #3 (id:40001) manually as
f0a169e6fd5718995fa6ef8749c8d16cdad84985 (presubmit successful).

[Lei Zhang, 2015-05-08 18:15:07]

I made the bots unhappy. Looking...

[Lei Zhang, 2015-05-08 20:30:28]

On 2015/05/08 18:15:07, Lei Zhang wrote:
> I made the bots unhappy. Looking...

FTR, fixed in https://codereview.chromium.org/1135713004/