Clean up fx_codec_tiff.cpp.

Clean up fx_codec_tiff.cpp.

Fix regressions from commit 4997b22.

BUG=618164
Committed: https://pdfium.googlesource.com/pdfium/+/fcf61b39ee597c73e80ba789833fb7fe49878422

[Lei Zhang, 2016-06-09 07:49:09]

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

[Tom Sepez, 2016-06-09 17:53:17]

https://codereview.chromium.org/2053573003/diff/1/core/fxcodec/codec/fx_codec...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2053573003/diff/1/core/fxcodec/codec/fx_codec...
core/fxcodec/codec/fx_codec_tiff.cpp:52: FX_BOOL isDecoder;
Agree this is always true and can be removed.

https://codereview.chromium.org/2053573003/diff/1/core/fxcodec/codec/fx_codec...
core/fxcodec/codec/fx_codec_tiff.cpp:74: void* _TIFFmalloc(tmsize_t size) {
nit: _CAPS is reserved for the pre-processor.  Fix later.

https://codereview.chromium.org/2053573003/diff/60001/core/fxcodec/codec/fx_c...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2053573003/diff/60001/core/fxcodec/codec/fx_c...
core/fxcodec/codec/fx_codec_tiff.cpp:194: void
CCodec_TiffContext::GetFrames(int32_t& frames) {
nit: pity this doesn't just return the value.

https://codereview.chromium.org/2053573003/diff/80001/core/fxcodec/codec/fx_c...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2053573003/diff/80001/core/fxcodec/codec/fx_c...
core/fxcodec/codec/fx_codec_tiff.cpp:192: m_tif_ctx = tiff_open(this, "r");
Ack.

https://codereview.chromium.org/2053573003/diff/100001/core/fxcodec/codec/fx_...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2053573003/diff/100001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_tiff.cpp:196: int32_t* width,
Can these instead be unsigned throughout?  Worry about negative widths below
slipping through some bounds check.

https://codereview.chromium.org/2053573003/diff/120001/core/fxcodec/codec/fx_...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2053573003/diff/120001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_tiff.cpp:34: FX_BOOL IsSupport(CFX_DIBitmap*
pDIBitmap);
nit: can this be a const method?  A const argument?

https://codereview.chromium.org/2053573003/diff/180001/core/fxcodec/codec/fx_...
File core/fxcodec/codec/fx_codec_progress.cpp (right):

https://codereview.chromium.org/2053573003/diff/180001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_progress.cpp:16: #undef FXCODEC_PNG_GAMMA
nit: can we do this with a #else instead of a def/undef?

https://codereview.chromium.org/2053573003/diff/180001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_progress.cpp:1196: }
still need a break here, the return above is conditional, or a return true or
something?

https://codereview.chromium.org/2053573003/diff/230001/core/fxcodec/codec/fx_...
File core/fxcodec/codec/fx_codec_progress.cpp (right):

https://codereview.chromium.org/2053573003/diff/230001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_progress.cpp:791: int des_row_1 = des_row -
static_cast<int>(2 * scale_y);
This overflows/underflows. Can we figure out a way to clip this before
truncating to int?

https://codereview.chromium.org/2053573003/diff/230001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_progress.cpp:948: int des_row_1 = des_row +
static_cast<int>(scale_y);
same here.

https://codereview.chromium.org/2053573003/diff/230001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_progress.cpp:1672: int des_row_1 = des_row -
static_cast<int>(scale_y);
and here.

https://codereview.chromium.org/2053573003/diff/250001/core/fxcodec/codec/fx_...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2053573003/diff/250001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_tiff.cpp:242: *height =
pdfium::base::checked_cast<int32_t>(tif_height);
Ah, ok, see comment on PS#1, I think this is ok now.

[Nathan Parker, 2016-06-09 18:54:40]

Description was changed from

==========
Clean up fx_codec_tiff.cpp.

Fix regressions from commit 4997b22.
==========

to

==========
Clean up fx_codec_tiff.cpp.

Fix regressions from commit 4997b22.

BUG=618164
==========

[Lei Zhang, 2016-06-09 22:00:40]

https://codereview.chromium.org/2053573003/diff/1/core/fxcodec/codec/fx_codec...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2053573003/diff/1/core/fxcodec/codec/fx_codec...
core/fxcodec/codec/fx_codec_tiff.cpp:52: FX_BOOL isDecoder;
On 2016/06/09 17:53:16, Tom Sepez wrote:
> Agree this is always true and can be removed.

Acknowledged.

https://codereview.chromium.org/2053573003/diff/1/core/fxcodec/codec/fx_codec...
core/fxcodec/codec/fx_codec_tiff.cpp:74: void* _TIFFmalloc(tmsize_t size) {
On 2016/06/09 17:53:16, Tom Sepez wrote:
> nit: _CAPS is reserved for the pre-processor.  Fix later.

We'll have to argue with libtiff on these. See externs in
third_party/libtiff/tiffio.h.

https://codereview.chromium.org/2053573003/diff/60001/core/fxcodec/codec/fx_c...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2053573003/diff/60001/core/fxcodec/codec/fx_c...
core/fxcodec/codec/fx_codec_tiff.cpp:194: void
CCodec_TiffContext::GetFrames(int32_t& frames) {
On 2016/06/09 17:53:16, Tom Sepez wrote:
> nit: pity this doesn't just return the value.

This method got removed in a later patch set.

https://codereview.chromium.org/2053573003/diff/100001/core/fxcodec/codec/fx_...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2053573003/diff/100001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_tiff.cpp:196: int32_t* width,
On 2016/06/09 17:53:16, Tom Sepez wrote:
> Can these instead be unsigned throughout?  Worry about negative widths below
> slipping through some bounds check.

Fixed later.

https://codereview.chromium.org/2053573003/diff/120001/core/fxcodec/codec/fx_...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2053573003/diff/120001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_tiff.cpp:34: FX_BOOL IsSupport(CFX_DIBitmap*
pDIBitmap);
On 2016/06/09 17:53:16, Tom Sepez wrote:
> nit: can this be a const method?  A const argument?

Done.

https://codereview.chromium.org/2053573003/diff/180001/core/fxcodec/codec/fx_...
File core/fxcodec/codec/fx_codec_progress.cpp (right):

https://codereview.chromium.org/2053573003/diff/180001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_progress.cpp:16: #undef FXCODEC_PNG_GAMMA
On 2016/06/09 17:53:16, Tom Sepez wrote:
> nit: can we do this with a #else instead of a def/undef?

Done.

https://codereview.chromium.org/2053573003/diff/180001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_progress.cpp:1196: }
On 2016/06/09 17:53:16, Tom Sepez wrote:
> still need a break here, the return above is conditional, or a return true or
> something?

Whoops.

https://codereview.chromium.org/2053573003/diff/230001/core/fxcodec/codec/fx_...
File core/fxcodec/codec/fx_codec_progress.cpp (right):

https://codereview.chromium.org/2053573003/diff/230001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_progress.cpp:791: int des_row_1 = des_row -
static_cast<int>(2 * scale_y);
On 2016/06/09 17:53:16, Tom Sepez wrote:
> This overflows/underflows. Can we figure out a way to clip this before
> truncating to int?

I put in some liberal uses of pdfium::base::CheckedNumeric. It's a start.

https://codereview.chromium.org/2053573003/diff/250001/core/fxcodec/codec/fx_...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2053573003/diff/250001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_tiff.cpp:242: *height =
pdfium::base::checked_cast<int32_t>(tif_height);
On 2016/06/09 17:53:17, Tom Sepez wrote:
> Ah, ok, see comment on PS#1, I think this is ok now.

Ignoring the "Can these instead be unsigned throughout?" comment.

[Tom Sepez, 2016-06-09 22:11:22]

lgtm

[Lei Zhang, 2016-06-09 22:20:03]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-06-09 22:20:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2053573003/270001

[commit-bot: I haz the power, 2016-06-10 00:01:03]

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

[commit-bot: I haz the power, 2016-06-10 00:01:04]

Try jobs failed on following builders:
  mac_no_v8 on master.tryserver.client.pdfium (JOB_TIMED_OUT, no build URL)
  mac_xfa on master.tryserver.client.pdfium (JOB_TIMED_OUT, no build URL)

[Lei Zhang, 2016-06-10 00:45:07]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-06-10 00:45:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2053573003/270001

[commit-bot: I haz the power, 2016-06-10 01:29:38]

Description was changed from

==========
Clean up fx_codec_tiff.cpp.

Fix regressions from commit 4997b22.

BUG=618164
==========

to

==========
Clean up fx_codec_tiff.cpp.

Fix regressions from commit 4997b22.

BUG=618164

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

[commit-bot: I haz the power, 2016-06-10 01:29:39]

Committed patchset #15 (id:270001) as
https://pdfium.googlesource.com/pdfium/+/fcf61b39ee597c73e80ba789833fb7fe4987...