Fix for bug 6100:...

Fix for bug 6100:
- Introduce a CanvasPixelArray to serve as the holder for the ImageData
  pixel array of a canvas element.

This is the third_party/WebKit change.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=8326

[darin (slow to review), 2009-01-16 22:33:39]

you should create a ScriptPixelBuffer.{h,cpp} (or ScriptByteArray.{h,cpp}) in
WebCore/bindings/js to preserve the JSC bindings.

http://codereview.chromium.org/18178/diff/1/3
File third_party/WebKit/WebCore/html/ImageData.h (right):

http://codereview.chromium.org/18178/diff/1/3#newcode32
Line 32: #include <ScriptPixelBuffer.h>
for includes of files within WebCore, the convention is to use "" instead of <>.

i recommend preflighting the ByteArray -> PixelBuffer name change.  they may
push back since technically their object is an array of bytes as the name says. 
maybe they intend to use in contexts other than for holding pixels.  the path of
least resistance is to call this ScriptByteArray.

http://codereview.chromium.org/18178/diff/1/4
File third_party/WebKit/WebCore/html/ImageData.idl (right):

http://codereview.chromium.org/18178/diff/1/4#newcode37
Line 37: readonly attribute ScriptPixelBuffer data;
is this IDL change really intended?  i'm not sure how it would work with the JSC
bindings.

[Feng Qian, 2009-01-16 22:36:35]

http://codereview.chromium.org/18178/diff/1/3
File third_party/WebKit/WebCore/html/ImageData.h (right):

http://codereview.chromium.org/18178/diff/1/3#newcode32
Line 32: #include <ScriptPixelBuffer.h>
I feel the same way as Darin for naming, ScriptByteArray seems better.

http://codereview.chromium.org/18178/diff/1/4
File third_party/WebKit/WebCore/html/ImageData.idl (right):

http://codereview.chromium.org/18178/diff/1/4#newcode37
Line 37: readonly attribute ScriptPixelBuffer data;
Why not just call it as ByteArray? Scritp<> I think is only used in
implementation abstraction.

[iposva, 2009-01-16 22:55:05]

http://codereview.chromium.org/18178/diff/1/3
File third_party/WebKit/WebCore/html/ImageData.h (right):

http://codereview.chromium.org/18178/diff/1/3#newcode32
Line 32: #include <ScriptPixelBuffer.h>
Here is a question for both of you: What do you expect to happen if you write an
int with a value of 257 into an array of bytes?
Everybody I have polled so far expected the value to be 1 since byte arrays do
modulo 256 math. Where as pixel buffers do clamping math, where values larger
than 255 are clamped to 255 and value less than 0 (and NaN) are clamped to 0.
Their use of the term byte array is very misleading.

I used the term PixelBuffer because that is the term used when doing graphics
programming. Alternatively I could use CanvasPixelArray, the term used in the
canvas spec.

On 2009/01/16 22:33:39, darin wrote:
> for includes of files within WebCore, the convention is to use "" instead of
<>.
> 
> i recommend preflighting the ByteArray -> PixelBuffer name change.  they may
> push back since technically their object is an array of bytes as the name
says. 
> maybe they intend to use in contexts other than for holding pixels.  the path
of
> least resistance is to call this ScriptByteArray.

http://codereview.chromium.org/18178/diff/1/4
File third_party/WebKit/WebCore/html/ImageData.idl (right):

http://codereview.chromium.org/18178/diff/1/4#newcode37
Line 37: readonly attribute ScriptPixelBuffer data;
On 2009/01/16 22:33:39, darin wrote:
> is this IDL change really intended?  i'm not sure how it would work with the
JSC
> bindings.

The data field which is defined in the canvas spec was recently removed from the
IDL file and the field is manually added to the ImageData JavaScript object when
it is constructed by using JSC APIs.

Their approach has two problems the IDL file does not reflect the spec and I am
sure that it must break their other language bindings.

http://codereview.chromium.org/18178/diff/1/4#newcode37
Line 37: readonly attribute ScriptPixelBuffer data;
On 2009/01/16 22:36:35, Feng Qian wrote:
> Why not just call it as ByteArray? Scritp<> I think is only used in
> implementation abstraction.

Please see my comment about using byte array, which is very misleading about the
behaviour of the data object. Whether or not to use Script here is unclear to
me, I will have to check other areas that have this set of V8/JSC differences.

[darin (slow to review), 2009-01-16 23:04:06]

So, at the end of the day, you don't have to convince me or feng.  You have to
convince a WebKit reviewer ;-)

I agree that their change would seem to screw over the other language bindings. 
This seems like a great topic to bring up on webkit-dev or on #webkit.

[iposva, 2009-01-17 00:49:12]

Renamed ScriptPixelBuffer -> CanvasPixelArray.

[darin (slow to review), 2009-01-20 18:26:20]

LGTM

http://codereview.chromium.org/18178/diff/17/19
File third_party/WebKit/WebCore/html/ImageData.h (right):

http://codereview.chromium.org/18178/diff/17/19#newcode32
Line 32: #include "CanvasPixelArray.h"
i guess when we upstream this change, we'll need to create this file in
WebCore/bindings/js to contain a typedef ByteArray CanvasPixelArray

http://codereview.chromium.org/18178/diff/17/20
File third_party/WebKit/WebCore/html/ImageData.idl (right):

http://codereview.chromium.org/18178/diff/17/20#newcode37
Line 37: #if defined(LANGUAGE_JAVASCRIPT) && defined(V8_BINDING)
I believe olliej's suggestion was to make this !JSC instead, presumably to cover
all other bindings.  However, I don't think we have a way to express that in IDL
:(

http://codereview.chromium.org/18178/diff/17/20#newcode39
Line 39: #endif // defined(LANGUAGE_JAVASCRIPT) && defined(V8_BINDING)
nit: it doesn't appear to be webkit style to comment the #endif like this

[iposva, 2009-01-20 19:49:51]

http://codereview.chromium.org/18178/diff/17/19
File third_party/WebKit/WebCore/html/ImageData.h (right):

http://codereview.chromium.org/18178/diff/17/19#newcode32
Line 32: #include "CanvasPixelArray.h"
On 2009/01/20 18:26:21, darin wrote:
> i guess when we upstream this change, we'll need to create this file in
> WebCore/bindings/js to contain a typedef ByteArray CanvasPixelArray

Correct, but this will be part of the upstream change and then will come down
with the next merge.

http://codereview.chromium.org/18178/diff/17/20
File third_party/WebKit/WebCore/html/ImageData.idl (right):

http://codereview.chromium.org/18178/diff/17/20#newcode37
Line 37: #if defined(LANGUAGE_JAVASCRIPT) && defined(V8_BINDING)
On 2009/01/20 18:26:21, darin wrote:
> I believe olliej's suggestion was to make this !JSC instead, presumably to
cover
> all other bindings.  However, I don't think we have a way to express that in
IDL
> :(
Exactly, have not found a way to express it either.

http://codereview.chromium.org/18178/diff/17/20#newcode39
Line 39: #endif // defined(LANGUAGE_JAVASCRIPT) && defined(V8_BINDING)
On 2009/01/20 18:26:21, darin wrote:
> nit: it doesn't appear to be webkit style to comment the #endif like this

Done.