Return unique_ptrs from CFX_DIBitmap::Clone().

Return unique_ptrs from CFX_DIBitmap::Clone().

Because that's what clone does.  Perform immediate release
in some spots to avoid disrupting too much at once.

Committed: https://pdfium.googlesource.com/pdfium/+/1a1d7648d3e338b756e464cebb2ae1a815359afa

[Tom Sepez, 2016-12-02 18:26:27]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 18:26:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 18:29:04]

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

[commit-bot: I haz the power, 2016-12-02 18:29:05]

Dry run: Try jobs failed on following builders:
  win on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win/builds/3082)
  win_no_v8 on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_no_v8/build...)
  win_xfa_32 on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_xfa_32/buil...)

[Tom Sepez, 2016-12-02 18:32:06]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 18:32:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-12-02 23:12:30]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 23:12:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 23:29:08]

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

[commit-bot: I haz the power, 2016-12-02 23:29:09]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-12-05 19:01:06]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 19:01:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-12-05 19:04:46]

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

[Tom Sepez, 2016-12-05 19:04:47]

NPM, ready for review.

[commit-bot: I haz the power, 2016-12-05 19:06:03]

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

[commit-bot: I haz the power, 2016-12-05 19:06:04]

Dry run: Try jobs failed on following builders:
  win_xfa_clang_32 on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_xfa_clang_3...)

[Tom Sepez, 2016-12-05 19:06:54]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 19:07:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 19:27:55]

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

[commit-bot: I haz the power, 2016-12-05 19:27:56]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-12-05 20:27:06]

Description was changed from

==========
Return unique_ptrs from CFX_DIBitmap::Clone().

Because that's what clone does.  Perform immediate release
in some spots to avoid disrupting too much at once.
==========

to

==========
Return unique_ptrs from CFX_DIBitmap::Clone().

Because that's what clone does.  Perform immediate release
in some spots to avoid disrupting too much at once.
==========

[Tom Sepez, 2016-12-05 20:27:06]

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

[Tom Sepez, 2016-12-05 20:27:15]

Dan, too.

[npm, 2016-12-05 20:37:23]

Great! Just a couple of comments

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_c...
File core/fxge/dib/fx_dib_convert.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_c...
core/fxge/dib/fx_dib_convert.cpp:789: std::unique_ptr<CFX_DIBitmap> pClone(new
CFX_DIBitmap);
nit: MakeUnique

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
File core/fxge/dib/fx_dib_main.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
core/fxge/dib/fx_dib_main.cpp:205: if (m_Pitch < (uint32_t)copy_len) {
Nit: {}

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
core/fxge/dib/fx_dib_main.cpp:840:
static_cast<CFX_DIBitmap*>(const_cast<CFX_DIBSource*>(pSrcBitmap)));
Why 2 casts?

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
core/fxge/dib/fx_dib_main.cpp:856: if (!((1 << (7 - col % 8)) & src_scan[col /
8])) {
Nit: {}

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/win32/fx_win...
File core/fxge/win32/fx_win32_device.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/win32/fx_win...
core/fxge/win32/fx_win32_device.cpp:855: CFX_DIBitmap* pToStrechBitmap =
pBitmap;
I think you can use CFX_MaybeOwned here instead of |del|

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/win32/fx_win...
File core/fxge/win32/fx_win32_gdipext.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/win32/fx_win...
core/fxge/win32/fx_win32_gdipext.cpp:615: if (!pCloned) {
Nit: no {}

https://codereview.chromium.org/2534953004/diff/140001/xfa/fxfa/app/xfa_ffwid...
File xfa/fxfa/app/xfa_ffwidget.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/xfa/fxfa/app/xfa_ffwid...
xfa/fxfa/app/xfa_ffwidget.cpp:816: !(m_pDevice->GetRenderCaps() &
FXRC_ALPHA_IMAGE)) {
Can you do an if for the negation of this and early return?

[dsinclair, 2016-12-05 20:41:27]

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
File core/fxge/dib/fx_dib_main.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
core/fxge/dib/fx_dib_main.cpp:670: const_cast<CFX_DIBSource*>(pSrcBitmap));
Should we remove the const from the signature?

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
core/fxge/dib/fx_dib_main.cpp:689: (FXDIB_Format)((pSrcBitmap->GetFormat() &
0xff00) | 0x20));
static_cast? and below

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/win32/fx_win...
File core/fxge/win32/fx_win32_gdipext.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/win32/fx_win...
core/fxge/win32/fx_win32_gdipext.cpp:561: CFX_DIBitmap* pStretched = nullptr;
unique_ptr?

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/win32/fx_win...
core/fxge/win32/fx_win32_gdipext.cpp:564: _StretchMonoToGray(dest_width,
dest_height, pBitmap, &image_clip);
Drop the _ from the method name

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/win32/fx_win...
core/fxge/win32/fx_win32_gdipext.cpp:614: CFX_DIBitmap* pCloned =
pBitmap->Clone(pSrcRect).release();
unique_ptr?

https://codereview.chromium.org/2534953004/diff/140001/xfa/fxbarcode/BC_TwoDi...
File xfa/fxbarcode/BC_TwoDimWriter.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/xfa/fxbarcode/BC_TwoDi...
xfa/fxbarcode/BC_TwoDimWriter.cpp:88: pOutBitmap = pStretchBitmap.release();
can pOutBitmap be a unique_ptr& instead of a *&?

https://codereview.chromium.org/2534953004/diff/140001/xfa/fxbarcode/oned/BC_...
File xfa/fxbarcode/oned/BC_OneDimWriter.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/xfa/fxbarcode/oned/BC_...
xfa/fxbarcode/oned/BC_OneDimWriter.cpp:349: pOutBitmap =
pStretchBitmap.release();
ditto

[Tom Sepez, 2016-12-05 21:38:14]

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_c...
File core/fxge/dib/fx_dib_convert.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_c...
core/fxge/dib/fx_dib_convert.cpp:789: std::unique_ptr<CFX_DIBitmap> pClone(new
CFX_DIBitmap);
On 2016/12/05 20:37:23, npm wrote:
> nit: MakeUnique

yes!! Thanks!! Done.

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
File core/fxge/dib/fx_dib_main.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
core/fxge/dib/fx_dib_main.cpp:205: if (m_Pitch < (uint32_t)copy_len) {
On 2016/12/05 20:37:23, npm wrote:
> Nit: {}

Done.

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
core/fxge/dib/fx_dib_main.cpp:670: const_cast<CFX_DIBSource*>(pSrcBitmap));
On 2016/12/05 20:41:26, dsinclair wrote:
> Should we remove the const from the signature?

Ok. Not too many ripples ...

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
core/fxge/dib/fx_dib_main.cpp:689: (FXDIB_Format)((pSrcBitmap->GetFormat() &
0xff00) | 0x20));
On 2016/12/05 20:41:26, dsinclair wrote:
> static_cast? and below

Done.

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
core/fxge/dib/fx_dib_main.cpp:840:
static_cast<CFX_DIBitmap*>(const_cast<CFX_DIBSource*>(pSrcBitmap)));
On 2016/12/05 20:37:23, npm wrote:
> Why 2 casts?

Because a const_cast can't change type, and a static_cast can't change
const-ness, and reinterpret_cast is dangerous as replacement.

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/dib/fx_dib_m...
core/fxge/dib/fx_dib_main.cpp:856: if (!((1 << (7 - col % 8)) & src_scan[col /
8])) {
On 2016/12/05 20:37:23, npm wrote:
> Nit: {}

Done.

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/win32/fx_win...
File core/fxge/win32/fx_win32_device.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/win32/fx_win...
core/fxge/win32/fx_win32_device.cpp:855: CFX_DIBitmap* pToStrechBitmap =
pBitmap;
On 2016/12/05 20:37:23, npm wrote:
> I think you can use CFX_MaybeOwned here instead of |del|

Yes I can, but its windows-only code so I left it for the moment.
Please check the new diff carefully.

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/win32/fx_win...
File core/fxge/win32/fx_win32_gdipext.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/core/fxge/win32/fx_win...
core/fxge/win32/fx_win32_gdipext.cpp:564: _StretchMonoToGray(dest_width,
dest_height, pBitmap, &image_clip);
On 2016/12/05 20:41:26, dsinclair wrote:
> Drop the _ from the method name

Done.

https://codereview.chromium.org/2534953004/diff/140001/xfa/fxbarcode/BC_TwoDi...
File xfa/fxbarcode/BC_TwoDimWriter.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/xfa/fxbarcode/BC_TwoDi...
xfa/fxbarcode/BC_TwoDimWriter.cpp:88: pOutBitmap = pStretchBitmap.release();
On 2016/12/05 20:41:26, dsinclair wrote:
> can pOutBitmap be a unique_ptr& instead of a *&?

Yes, but it ripples through a lot of places so I left it for now.

https://codereview.chromium.org/2534953004/diff/140001/xfa/fxbarcode/oned/BC_...
File xfa/fxbarcode/oned/BC_OneDimWriter.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/xfa/fxbarcode/oned/BC_...
xfa/fxbarcode/oned/BC_OneDimWriter.cpp:349: pOutBitmap =
pStretchBitmap.release();
On 2016/12/05 20:41:26, dsinclair wrote:
> ditto
ditto

https://codereview.chromium.org/2534953004/diff/140001/xfa/fxfa/app/xfa_ffwid...
File xfa/fxfa/app/xfa_ffwidget.cpp (right):

https://codereview.chromium.org/2534953004/diff/140001/xfa/fxfa/app/xfa_ffwid...
xfa/fxfa/app/xfa_ffwidget.cpp:816: !(m_pDevice->GetRenderCaps() &
FXRC_ALPHA_IMAGE)) {
On 2016/12/05 20:37:23, npm wrote:
> Can you do an if for the negation of this and early return?

Done.

[Tom Sepez, 2016-12-05 21:38:17]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 21:38:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-12-05 21:40:35]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 21:40:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[npm, 2016-12-05 21:57:05]

lgtm

https://codereview.chromium.org/2534953004/diff/180001/core/fxge/dib/fx_dib_m...
File core/fxge/dib/fx_dib_main.cpp (right):

https://codereview.chromium.org/2534953004/diff/180001/core/fxge/dib/fx_dib_m...
core/fxge/dib/fx_dib_main.cpp:839:
static_cast<CFX_DIBitmap*>(const_cast<CFX_DIBSource*>(pSrcBitmap)));
I think you no longer need const_cast, right?

https://codereview.chromium.org/2534953004/diff/180001/core/fxge/win32/fx_win...
File core/fxge/win32/fx_win32_gdipext.cpp (right):

https://codereview.chromium.org/2534953004/diff/180001/core/fxge/win32/fx_win...
core/fxge/win32/fx_win32_gdipext.cpp:480: std::unique_ptr<CFX_DIBitmap>
pStretched = pdfium::MakeUnique<CFX_DIBitmap>();
Maybe auto pStretched = ...

[commit-bot: I haz the power, 2016-12-05 21:57:30]

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

[commit-bot: I haz the power, 2016-12-05 21:57:31]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-12-05 22:01:25]

https://codereview.chromium.org/2534953004/diff/180001/core/fxge/dib/fx_dib_m...
File core/fxge/dib/fx_dib_main.cpp (right):

https://codereview.chromium.org/2534953004/diff/180001/core/fxge/dib/fx_dib_m...
core/fxge/dib/fx_dib_main.cpp:839:
static_cast<CFX_DIBitmap*>(const_cast<CFX_DIBSource*>(pSrcBitmap)));
On 2016/12/05 21:57:05, npm wrote:
> I think you no longer need const_cast, right?

Done.

https://codereview.chromium.org/2534953004/diff/180001/core/fxge/win32/fx_win...
File core/fxge/win32/fx_win32_gdipext.cpp (right):

https://codereview.chromium.org/2534953004/diff/180001/core/fxge/win32/fx_win...
core/fxge/win32/fx_win32_gdipext.cpp:480: std::unique_ptr<CFX_DIBitmap>
pStretched = pdfium::MakeUnique<CFX_DIBitmap>();
On 2016/12/05 21:57:05, npm wrote:
> Maybe auto pStretched = ...

Agreed. Done.

[dsinclair, 2016-12-06 14:10:23]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-12-06 14:10:24]

lgtm

[dsinclair, 2016-12-06 14:10:24]

The patchset sent to the CQ was uploaded after l-g-t-m from npm@chromium.org
Link to the patchset: https://codereview.chromium.org/2534953004/#ps200001
(title: "nits")

[commit-bot: I haz the power, 2016-12-06 14:10:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 14:29:29]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1481033423501080,
"parent_rev": "7341149c634e0ab9a619898826440f6e952cf0aa", "commit_rev":
"1a1d7648d3e338b756e464cebb2ae1a815359afa"}

[commit-bot: I haz the power, 2016-12-06 14:29:32]

Description was changed from

==========
Return unique_ptrs from CFX_DIBitmap::Clone().

Because that's what clone does.  Perform immediate release
in some spots to avoid disrupting too much at once.
==========

to

==========
Return unique_ptrs from CFX_DIBitmap::Clone().

Because that's what clone does.  Perform immediate release
in some spots to avoid disrupting too much at once.

Committed:
https://pdfium.googlesource.com/pdfium/+/1a1d7648d3e338b756e464cebb2ae1a81535...
==========

[commit-bot: I haz the power, 2016-12-06 14:29:34]

Committed patchset #11 (id:200001) as
https://pdfium.googlesource.com/pdfium/+/1a1d7648d3e338b756e464cebb2ae1a81535...