Oilpan: keep ImageData on the heap by default.

Oilpan: keep ImageData on the heap by default.

R=haraken, junov
BUG=479467
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195152

[sof, 2015-04-27 07:20:57]

sigbjornf@opera.com changed reviewers:
+ junov@chromium.org, oilpan-reviews@chromium.org

[sof, 2015-04-27 07:20:57]

please take a look.

junov@: acceptable to switch ImageData over?

[haraken, 2015-04-27 08:30:30]

LGTM

[sof, 2015-05-06 12:14:29]

junov@: ping wrt Oilpan by default.

[Justin Novosad, 2015-05-06 20:14:04]

Thanks for the ping.

Just one small concern (see below). If you think it is not an issue, then lgtm.

https://codereview.chromium.org/1084313004/diff/1/Source/core/html/HTMLCanvas...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1084313004/diff/1/Source/core/html/HTMLCanvas...
Source/core/html/HTMLCanvasElement.cpp:487: ImageData* imageData =
ImageData::create(m_size);
In this case the image data is just a temporary local, and it can be wrapping a
pretty large buffer. Would it be worth forcibly destroying it instead of just
letting it go out of scope?

https://codereview.chromium.org/1084313004/diff/1/Source/core/html/HTMLCanvas...
Source/core/html/HTMLCanvasElement.cpp:493: ImageData* imageData =
toWebGLRenderingContextBase(m_context.get())->paintRenderingResultsToImageData(sourceBuffer);
same here

[sof, 2015-05-06 20:30:16]

On 2015/05/06 20:14:04, Justin Novosad wrote:
> Thanks for the ping.
> 
> Just one small concern (see below). If you think it is not an issue, then
lgtm.
> 

Good point about prolonged lifetime in those two cases; will try to address
tomorrow.

[sof, 2015-05-07 08:45:38]

On 2015/05/06 20:30:16, sof wrote:
> On 2015/05/06 20:14:04, Justin Novosad wrote:
> > Thanks for the ping.
> > 
> > Just one small concern (see below). If you think it is not an issue, then
> lgtm.
> > 
> 
> Good point about prolonged lifetime in those two cases; will try to address
> tomorrow.

Addressed by introducing a simple RAII ScopedDisposal<T> abstraction (good names
are hard to find..) to Oilpan & using it here.

[sof, 2015-05-08 07:18:08]

On 2015/05/07 08:45:38, sof wrote:
> On 2015/05/06 20:30:16, sof wrote:
> > On 2015/05/06 20:14:04, Justin Novosad wrote:
> > > Thanks for the ping.
> > > 
> > > Just one small concern (see below). If you think it is not an issue, then
> > lgtm.
> > > 
> > 
> > Good point about prolonged lifetime in those two cases; will try to address
> > tomorrow.
> 
> Addressed by introducing a simple RAII ScopedDisposal<T> abstraction (good
names
> are hard to find..) to Oilpan & using it here.

ps#2 changes are for oilpan-reviews reviewing, primarily. (aka. gentle ping. :)
)

[haraken, 2015-05-08 23:28:01]

LGTM

[sof, 2015-05-09 12:44:08]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-05-09 12:44:09]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1084313004/#ps20001
(title: "Ensure timely disposal of temporary ImageData buffers")

[commit-bot: I haz the power, 2015-05-09 12:44:16]

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

[commit-bot: I haz the power, 2015-05-09 19:13:04]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195152