Cleanup dead code in CPDF_DIBSource::LoadJpxBitmap() and friends.

Cleanup dead code in CPDF_DIBSource::LoadJpxBitmap() and friends.

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/097297325e0d3d00556d67885547750a2e9d4b32

[Lei Zhang, 2015-09-01 21:31:49]

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

[Lei Zhang, 2015-09-01 21:31:50]

Please look at patch set 1, and then the diff between patch sets to see how I
arrived at patch set 3. I hope you can look at the |m_pColorSpace| usage and
arrive at the same conclusion as me for patch set 2 -> 3.

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

https://codereview.chromium.org/1310603006/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:826:
CCodec_JpxModule::~CCodec_JpxModule() {
git cl format has changed and is now being inconsistent with itself. Sigh.

[Tom Sepez, 2015-09-01 21:38:48]

PS1 LG with nits. Stay tuned.

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

https://codereview.chromium.org/1310603006/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:733: *output_nComps =
*codestream_nComps = (FX_DWORD)image->numcomps;
nit: one per line.

https://codereview.chromium.org/1310603006/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:821: void
initialize_transition_table();
nit: for another day, which header do these belong in?

https://codereview.chromium.org/1310603006/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:826:
CCodec_JpxModule::~CCodec_JpxModule() {
On 2015/09/01 21:31:50, Lei Zhang wrote:
> git cl format has changed and is now being inconsistent with itself. Sigh.
Sigh.

https://codereview.chromium.org/1310603006/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:833: decoder->m_useColorSpace =
useColorSpace;
nit: shoule useColorSpace be an argument to the CTOR?

[Tom Sepez, 2015-09-01 21:43:37]

PS2 LG with nits minus one nit.

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

https://codereview.chromium.org/1310603006/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:729: *components =
(FX_DWORD)image->numcomps;
nevermind.

https://codereview.chromium.org/1310603006/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:837: CJPX_Decoder* pDecoder =
(CJPX_Decoder*)ctx;
nit: can we use an CJPX_Decoder as an incomplete type in the API in place of the
void* casting?

[Lei Zhang, 2015-09-01 22:06:27]

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

https://codereview.chromium.org/1310603006/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:821: void
initialize_transition_table();
On 2015/09/01 21:38:48, Tom Sepez wrote:
> nit: for another day, which header do these belong in?

It's dead code. Will remove in patch set 4.

https://codereview.chromium.org/1310603006/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:833: decoder->m_useColorSpace =
useColorSpace;
On 2015/09/01 21:38:48, Tom Sepez wrote:
> nit: shoule useColorSpace be an argument to the CTOR?

It's going away in patch set 3.

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

https://codereview.chromium.org/1310603006/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:837: CJPX_Decoder* pDecoder =
(CJPX_Decoder*)ctx;
On 2015/09/01 21:43:37, Tom Sepez wrote:
> nit: can we use an CJPX_Decoder as an incomplete type in the API in place of
the
> void* casting?

Probably. I'll wait for your comment and do this in patch set 4.

[Lei Zhang, 2015-09-01 22:16:13]

uploaded patch set 4 & 5.

[Tom Sepez, 2015-09-01 22:17:45]

Not following the logic in PS3:

fpdf_render_cache.cpp:253 CPDF_DIBSource* pSrc = new CPDF_DIBSource;
  then immediately call	CPDF_DIBSource::Load().
     then call CPDF_DIBSource::LoadColorInfo() before any use of
m_pColorSpace,	return on error.
       loadcolorinfo may return	TRUE at	eg. 509	without	setting	m_pColorSpace.
     Load may return TRUE without setting m_pColorSpace.
  Caller may return without setting this.

[Lei Zhang, 2015-09-01 22:23:44]

On 2015/09/01 22:17:45, Tom Sepez wrote:
> Not following the logic in PS3:
> 
> fpdf_render_cache.cpp:253 CPDF_DIBSource* pSrc = new CPDF_DIBSource;
>   then immediately call	CPDF_DIBSource::Load().
>      then call CPDF_DIBSource::LoadColorInfo() before any use of
> m_pColorSpace,	return on error.
>        loadcolorinfo may return	TRUE at	eg. 509	without	setting	m_pColorSpace.
>      Load may return TRUE without setting m_pColorSpace.
>   Caller may return without setting this.

Doh. I missed the earlier returns. Will revert then.

[Tom Sepez, 2015-09-01 22:25:12]

On 2015/09/01 22:23:44, Lei Zhang wrote:
> On 2015/09/01 22:17:45, Tom Sepez wrote:
> > Not following the logic in PS3:
> > 
> > fpdf_render_cache.cpp:253 CPDF_DIBSource* pSrc = new CPDF_DIBSource;
> >   then immediately call	CPDF_DIBSource::Load().
> >      then call CPDF_DIBSource::LoadColorInfo() before any use of
> > m_pColorSpace,	return on error.
> >        loadcolorinfo may return	TRUE at	eg.
509	without	setting	m_pColorSpace.
> >      Load may return TRUE without setting m_pColorSpace.
> >   Caller may return without setting this.
> 
> Doh. I missed the earlier returns. Will revert then.
Ok, but I like your earlier PS.  PS2 LGTM with nits then.

[Lei Zhang, 2015-09-01 22:32:07]

PTAL at the patchset 2 -> 6 diff. That one makes the most sense.

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

https://codereview.chromium.org/1310603006/diff/100001/core/src/fxcodec/codec...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:630: bool m_UseColorSpace;
This generates an unused var warning, but I'll follow up with
https://codereview.chromium.org/1310603006 soon to get rid of that.

[Tom Sepez, 2015-09-01 22:38:28]

lgtm++

[Lei Zhang, 2015-09-01 22:42:04]

Committed patchset #6 (id:100001) manually as
097297325e0d3d00556d67885547750a2e9d4b32 (presubmit successful).

[Tom Sepez, 2015-09-01 23:21:47]

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

https://codereview.chromium.org/1310603006/diff/100001/core/src/fxcodec/codec...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:627: opj_image_t* image;
A truly OCD individual would call these m_pImage, m_pCodec, m_pStream and
m_bUseColorSpace to match the naming conventions in play here.

[Lei Zhang, 2015-09-01 23:23:23]

On 2015/09/01 23:21:47, Tom Sepez wrote:
>
https://codereview.chromium.org/1310603006/diff/100001/core/src/fxcodec/codec...
> File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):
> 
>
https://codereview.chromium.org/1310603006/diff/100001/core/src/fxcodec/codec...
> core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:627: opj_image_t* image;
> A truly OCD individual would call these m_pImage, m_pCodec, m_pStream and
> m_bUseColorSpace to match the naming conventions in play here.

Oh, I intend to.