Do not clone in CPDF_ImageCacheEntry::GetCachedBitmap

Do not clone in CPDF_ImageCacheEntry::GetCachedBitmap

Committed: https://pdfium.googlesource.com/pdfium/+/708ef623c2e989a9ad909bf0c94a7dff89694412

[npm, 2016-11-29 16:35:48]

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

[npm, 2016-11-29 16:36:46]

ptal, similar to
https://codereview.chromium.org/2518063005/

[Tom Sepez, 2016-11-29 17:00:53]

https://codereview.chromium.org/2536903004/diff/1/core/fpdfapi/render/fpdf_re...
File core/fpdfapi/render/fpdf_render_cache.cpp (right):

https://codereview.chromium.org/2536903004/diff/1/core/fpdfapi/render/fpdf_re...
core/fpdfapi/render/fpdf_render_cache.cpp:186: delete m_pCachedBitmap;
Can we make these two unique_ptr's first? 
Similarly, we should make any method called "clone" return an unique_ptr.

Then it's easier to reason about whether we're passing ownership or not.  I
suspect the clone()s may have been used to prevent this from freeing things it
didn't own.

[npm, 2016-11-29 17:05:31]

https://codereview.chromium.org/2536903004/diff/1/core/fpdfapi/render/fpdf_re...
File core/fpdfapi/render/fpdf_render_cache.cpp (right):

https://codereview.chromium.org/2536903004/diff/1/core/fpdfapi/render/fpdf_re...
core/fpdfapi/render/fpdf_render_cache.cpp:186: delete m_pCachedBitmap;
On 2016/11/29 17:00:53, Tom Sepez wrote:
> Can we make these two unique_ptr's first? 
> Similarly, we should make any method called "clone" return an unique_ptr.
> 
> Then it's easier to reason about whether we're passing ownership or not.  I
> suspect the clone()s may have been used to prevent this from freeing things it
> didn't own.

Sure I can do that. But in this particular case, you can see the cloned objects
are deleted right about cloning. So that's not the reason for the cloning.

[npm, 2016-11-29 17:06:27]

https://codereview.chromium.org/2536903004/diff/1/core/fpdfapi/render/fpdf_re...
File core/fpdfapi/render/fpdf_render_cache.cpp (right):

https://codereview.chromium.org/2536903004/diff/1/core/fpdfapi/render/fpdf_re...
core/fpdfapi/render/fpdf_render_cache.cpp:186: delete m_pCachedBitmap;
On 2016/11/29 17:05:31, npm wrote:
> On 2016/11/29 17:00:53, Tom Sepez wrote:
> > Can we make these two unique_ptr's first? 
> > Similarly, we should make any method called "clone" return an unique_ptr.
> > 
> > Then it's easier to reason about whether we're passing ownership or not.  I
> > suspect the clone()s may have been used to prevent this from freeing things
it
> > didn't own.
> 
> Sure I can do that. But in this particular case, you can see the cloned
objects
> are deleted right about cloning. So that's not the reason for the cloning.

s/about/after

[Tom Sepez, 2016-11-29 17:24:53]

On 2016/11/29 17:06:27, npm wrote:
>
https://codereview.chromium.org/2536903004/diff/1/core/fpdfapi/render/fpdf_re...
> File core/fpdfapi/render/fpdf_render_cache.cpp (right):
> 
>
https://codereview.chromium.org/2536903004/diff/1/core/fpdfapi/render/fpdf_re...
> core/fpdfapi/render/fpdf_render_cache.cpp:186: delete m_pCachedBitmap;
> On 2016/11/29 17:05:31, npm wrote:
> > On 2016/11/29 17:00:53, Tom Sepez wrote:
> > > Can we make these two unique_ptr's first? 
> > > Similarly, we should make any method called "clone" return an unique_ptr.
> > > 
> > > Then it's easier to reason about whether we're passing ownership or not. 
I
> > > suspect the clone()s may have been used to prevent this from freeing
things
> it
> > > didn't own.
> > 
> > Sure I can do that. But in this particular case, you can see the cloned
> objects
> > are deleted right about cloning. So that's not the reason for the cloning.
> 
> s/about/after

Ah, lgtm

[npm, 2016-11-29 18:00:54]

The CQ bit was checked by npm@chromium.org

[commit-bot: I haz the power, 2016-11-29 18:00:56]

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-11-29 18:10:32]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1480442454055080, "parent_rev":
"05e01698444726fae302cd335fa4880932d7c543", "commit_rev":
"708ef623c2e989a9ad909bf0c94a7dff89694412"}

[commit-bot: I haz the power, 2016-11-29 18:10:35]

Description was changed from

==========
Do not clone in CPDF_ImageCacheEntry::GetCachedBitmap
==========

to

==========
Do not clone in CPDF_ImageCacheEntry::GetCachedBitmap

Committed:
https://pdfium.googlesource.com/pdfium/+/708ef623c2e989a9ad909bf0c94a7dff8969...
==========

[commit-bot: I haz the power, 2016-11-29 18:10:36]

Committed patchset #1 (id:1) as
https://pdfium.googlesource.com/pdfium/+/708ef623c2e989a9ad909bf0c94a7dff8969...