Break dependency of platform/graphics on html/ImageData.h

Break dependency of platform/graphics on html/ImageData.h

BUG=297477
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161360

[rwlbuis, 2013-11-01 22:00:04]

I am not completely happy with this code yet. I am not 100% happy with the new
class name and there is some code duplication.
Anyway I thought I'd put it up for comments. Maybe there is another, cleaner way
for this that I haven't seen...

[Stephen Chennney, 2013-11-01 23:26:33]

On 2013/11/01 22:00:04, rwlbuis wrote:
> I am not completely happy with this code yet. I am not 100% happy with the new
> class name and there is some code duplication.
> Anyway I thought I'd put it up for comments. Maybe there is another, cleaner
way
> for this that I haven't seen...

I would prefer that everything currently in ImageData move to
platform/ImageDataBuffer (assuming we don't already have such a class) and we
just leave ImageData as a minimal wrapper for the width/height JS bindings.

[rwlbuis, 2013-11-04 16:42:52]

On 2013/11/01 23:26:33, Stephen Chenney wrote:
> On 2013/11/01 22:00:04, rwlbuis wrote:
> > I am not completely happy with this code yet. I am not 100% happy with the
new
> > class name and there is some code duplication.
> > Anyway I thought I'd put it up for comments. Maybe there is another, cleaner
> way
> > for this that I haven't seen...
> 
> I would prefer that everything currently in ImageData move to
> platform/ImageDataBuffer (assuming we don't already have such a class) and we
> just leave ImageData as a minimal wrapper for the width/height JS bindings.

I tried that but it seems hard to do. For one ImageData will need to be ref
counted for the bindings to work right now.

My new patch basically seperates ImageData and its use in core/platform. It
keeps a convenience struct because the template in encodeImage assumes it will
be passed one class/struct.
Things that may need clearing up:

- did I get you right that you prefer ImageDataBuffer? I don't actually like
ImageDataHolder.
- the struct could be moved to ImageBuffer.h since it is only used for encoding.
- I don't like that the integer overflow checking is distributed, should we put
it in a central place?

[Stephen Chennney, 2013-11-04 16:59:16]

On 2013/11/04 16:42:52, rwlbuis wrote:
> On 2013/11/01 23:26:33, Stephen Chenney wrote:
> > On 2013/11/01 22:00:04, rwlbuis wrote:
> > > I am not completely happy with this code yet. I am not 100% happy with the
> new
> > > class name and there is some code duplication.
> > > Anyway I thought I'd put it up for comments. Maybe there is another,
cleaner
> > way
> > > for this that I haven't seen...
> > 
> > I would prefer that everything currently in ImageData move to
> > platform/ImageDataBuffer (assuming we don't already have such a class) and
we
> > just leave ImageData as a minimal wrapper for the width/height JS bindings.
> 
> I tried that but it seems hard to do. For one ImageData will need to be ref
> counted for the bindings to work right now.

I can see that, and I think Patch 2 is the right way to go.
 
> My new patch basically seperates ImageData and its use in core/platform. It
> keeps a convenience struct because the template in encodeImage assumes it will
> be passed one class/struct.
> Things that may need clearing up:
> 
> - did I get you right that you prefer ImageDataBuffer? I don't actually like
> ImageDataHolder.
> - the struct could be moved to ImageBuffer.h since it is only used for
encoding.

I like the second option, just moving the struct into ImageBuffer.

> - I don't like that the integer overflow checking is distributed, should we
put
> it in a central place?

I'm missing this. What code are you refering to?

[rwlbuis, 2013-11-04 17:04:18]

On 2013/11/04 16:59:16, Stephen Chenney wrote:
> > My new patch basically seperates ImageData and its use in core/platform. It
> > keeps a convenience struct because the template in encodeImage assumes it
will
> > be passed one class/struct.
> > Things that may need clearing up:
> > 
> > - did I get you right that you prefer ImageDataBuffer? I don't actually like
> > ImageDataHolder.
> > - the struct could be moved to ImageBuffer.h since it is only used for
> encoding.
> 
> I like the second option, just moving the struct into ImageBuffer.

Sounds good, will make a follow up patch soon.

> > - I don't like that the integer overflow checking is distributed, should we
> put
> > it in a central place?
> 
> I'm missing this. What code are you refering to?

You can see it in the GraphicsContext3d.cpp chunk, look for Checked<int,
RecordOverflow>. This is repeated in both ImageData::create methods.

[Stephen Chennney, 2013-11-04 17:40:02]

On 2013/11/04 17:04:18, rwlbuis wrote:
> On 2013/11/04 16:59:16, Stephen Chenney wrote:
> > > My new patch basically seperates ImageData and its use in core/platform.
It
> > > keeps a convenience struct because the template in encodeImage assumes it
> will
> > > be passed one class/struct.
> > > Things that may need clearing up:
> > > 
> > > - did I get you right that you prefer ImageDataBuffer? I don't actually
like
> > > ImageDataHolder.
> > > - the struct could be moved to ImageBuffer.h since it is only used for
> > encoding.
> > 
> > I like the second option, just moving the struct into ImageBuffer.
> 
> Sounds good, will make a follow up patch soon.
> 
> > > - I don't like that the integer overflow checking is distributed, should
we
> > put
> > > it in a central place?
> > 
> > I'm missing this. What code are you refering to?
> 
> You can see it in the GraphicsContext3d.cpp chunk, look for Checked<int,
> RecordOverflow>. This is repeated in both ImageData::create methods.

I see. Looking at it, it seems you could move the checks to a static method on
UInt8ClampedArray and pass around a null array if the checks fail. I would
support a follow on patch that does that.

[rwlbuis, 2013-11-04 18:51:21]

On 2013/11/04 17:40:02, Stephen Chenney wrote:
> On 2013/11/04 17:04:18, rwlbuis wrote:
> > On 2013/11/04 16:59:16, Stephen Chenney wrote:
> > > > My new patch basically seperates ImageData and its use in core/platform.
> It
> > > > keeps a convenience struct because the template in encodeImage assumes
it
> > will
> > > > be passed one class/struct.
> > > > Things that may need clearing up:
> > > > 
> > > > - did I get you right that you prefer ImageDataBuffer? I don't actually
> like
> > > > ImageDataHolder.
> > > > - the struct could be moved to ImageBuffer.h since it is only used for
> > > encoding.
> > > 
> > > I like the second option, just moving the struct into ImageBuffer.
> > 
> > Sounds good, will make a follow up patch soon.
> > 
> > > > - I don't like that the integer overflow checking is distributed, should
> we
> > > put
> > > > it in a central place?
> > > 
> > > I'm missing this. What code are you refering to?
> > 
> > You can see it in the GraphicsContext3d.cpp chunk, look for Checked<int,
> > RecordOverflow>. This is repeated in both ImageData::create methods.
> 
> I see. Looking at it, it seems you could move the checks to a static method on
> UInt8ClampedArray and pass around a null array if the checks fail. I would
> support a follow on patch that does that.

That sounds good. I now have a new changeset up which renames the struct and
moves it into ImageBuffer.h as discussed.

[Stephen Chennney, 2013-11-05 16:30:20]

lgtm

[commit-bot: I haz the power, 2013-11-05 16:35:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/54653004/120001

[commit-bot: I haz the power, 2013-11-05 17:31:03]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-11-05 18:05:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/54653004/120001

[commit-bot: I haz the power, 2013-11-05 20:28:57]

Change committed as 161360