Some cleanup of fpdf_render_loadimage.cpp

Some cleanup of fpdf_render_loadimage.cpp

- Generalise GetBits8()
- Get rid of C-style casts.
- Make CFX_DIBSource::SetDownSampleSize() non const. It's only overriden
  once and called in one place and it doesn't make sense for it to be const.
- Get rid of a macro
- Make public member vars of CPDF_DIBSource private
- And others...

R=thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/487935f662ba4711caf1c2c06873b676fd3fba3e

[Oliver Chang, 2016-01-29 18:15:40]

ptal

[Lei Zhang, 2016-01-30 00:39:25]

https://codereview.chromium.org/1644403003/diff/20001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1644403003/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:124: CPDF_DIBSource*
pSource = new CPDF_DIBSource;
Do you want to use a unique_ptr here, and release() it to |m_pDIBSource| when
appropriate? Or maybe |m_pDIBSource| can be a unique_ptr as well?

https://codereview.chromium.org/1644403003/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:159: m_pDocument =
nullptr;
Would you mind making this an init list?

https://codereview.chromium.org/1644403003/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1017: R = std::min(R,
max_data);
Did you mean to ClampValue() here?

https://codereview.chromium.org/1644403003/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1097: return nullptr;
Reduce to 1 liner with a ternary operator?

[Oliver Chang, 2016-01-30 01:48:19]

https://codereview.chromium.org/1644403003/diff/20001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1644403003/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:124: CPDF_DIBSource*
pSource = new CPDF_DIBSource;
On 2016/01/30 00:39:25, Lei Zhang wrote:
> Do you want to use a unique_ptr here, and release() it to |m_pDIBSource| when
> appropriate? Or maybe |m_pDIBSource| can be a unique_ptr as well?

It's not actually clear whether if CPDF_Image actually owns m_pDIBSource.. I'll
err on the side of caution and just use a unique_ptr and release.

https://codereview.chromium.org/1644403003/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:159: m_pDocument =
nullptr;
On 2016/01/30 00:39:24, Lei Zhang wrote:
> Would you mind making this an init list?

Done. Also made more member vars private.

https://codereview.chromium.org/1644403003/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1017: R = std::min(R,
max_data);
On 2016/01/30 00:39:25, Lei Zhang wrote:
> Did you mean to ClampValue() here?

Hmm, it didn't make sense here since R/G/B and max_data are unsigned ints.

https://codereview.chromium.org/1644403003/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1097: return nullptr;
On 2016/01/30 00:39:25, Lei Zhang wrote:
> Reduce to 1 liner with a ternary operator?

Done.

[Lei Zhang, 2016-01-30 02:08:33]

lgtm

https://codereview.chromium.org/1644403003/diff/20001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1644403003/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:159: m_pDocument =
nullptr;
On 2016/01/30 01:48:18, Oliver Chang wrote:
> On 2016/01/30 00:39:24, Lei Zhang wrote:
> > Would you mind making this an init list?
> 
> Done. Also made more member vars private.

Thanks!

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

https://codereview.chromium.org/1644403003/diff/40001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:183: m_bpp = 0;
This couldn't be in the initializer list? The |m_pMask| assignment below looks
redundant.

[Oliver Chang, 2016-01-30 02:18:33]

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

https://codereview.chromium.org/1644403003/diff/40001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:183: m_bpp = 0;
On 2016/01/30 02:08:33, Lei Zhang wrote:
> This couldn't be in the initializer list? The |m_pMask| assignment below looks
> redundant.

Oops, got rid of |m_pMask|. 

These are inherited from CFX_DIBSource, so can't be put in the initialiser list.
Taking a look again, it looks like the CFX_DIBSource constructor does  the same
initialisation, so I'm removing them here.

[Oliver Chang, 2016-01-30 02:19:08]

And thanks for the review, as always :)

[Oliver Chang, 2016-01-30 02:24:36]

Description was changed from

==========
Some cleanup of fpdf_render_loadimage.cpp

- Generalise GetBits8()
- Get rid of C-style casts.
- Make CFX_DIBSource::SetDownSampleSize() non const. It's only overriden
  once and called in one place and it doesn't make sense for it to be const.
- Get rid of a macro
- Use unique_ptr, nulltpr
- Whitespace

R=thestig@chromium.org
==========

to

==========
Some cleanup of fpdf_render_loadimage.cpp

- Generalise GetBits8()
- Get rid of C-style casts.
- Make CFX_DIBSource::SetDownSampleSize() non const. It's only overriden
  once and called in one place and it doesn't make sense for it to be const.
- Get rid of a macro
- Make public member vars of CPDF_DIBSource private
- And others...

R=thestig@chromium.org
==========

[Oliver Chang, 2016-01-30 02:25:09]

Description was changed from

==========
Some cleanup of fpdf_render_loadimage.cpp

- Generalise GetBits8()
- Get rid of C-style casts.
- Make CFX_DIBSource::SetDownSampleSize() non const. It's only overriden
  once and called in one place and it doesn't make sense for it to be const.
- Get rid of a macro
- Make public member vars of CPDF_DIBSource private
- And others...

R=thestig@chromium.org
==========

to

==========
Some cleanup of fpdf_render_loadimage.cpp

- Generalise GetBits8()
- Get rid of C-style casts.
- Make CFX_DIBSource::SetDownSampleSize() non const. It's only overriden
  once and called in one place and it doesn't make sense for it to be const.
- Get rid of a macro
- Make public member vars of CPDF_DIBSource private
- And others...

R=thestig@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/487935f662ba4711caf1c2c06873b676fd3f...
==========

[Oliver Chang, 2016-01-30 02:25:10]

Committed patchset #4 (id:60001) manually as
487935f662ba4711caf1c2c06873b676fd3fba3e (presubmit successful).