Implement ImageData constructors.

Implement ImageData constructors.

The spec now defines a pair of ImageData constructors,

  http://www.whatwg.org/specs/web-apps/current-work/#imagedata

Provide an implementation of both here.

R=
BUG=338804
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170325

[sof, 2014-03-19 17:18:05]

Please take a look.

I'm making a pair of assumptions here regarding the treatment of ImageData.data:

 - The older optimization to put the "data" property on the ImageData wrapper is
still valid/worthy for v8.
 - There is no way to express via IDL the insertion of custom initialization
code of the wrapper that a constructor builds and adds to the DOMDataStore.
Hence, a custom constructor is needed here to accomplish just that.

I believe both of these hold true, but am not certain.

[Ken Russell (switch to Gerrit), 2014-03-19 17:33:57]

+bajones

[Inactive, 2014-03-19 18:59:48]

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
File Source/core/html/ImageData.idl (right):

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
Source/core/html/ImageData.idl:30: CustomConstructor,
Even if the constructor is custom, you're expected to provide the prototype of
the constructor (e.g. CustomConstructor(float x, float y)). The bindings
generator relies on that to compute the correct value for
window.ImageData.length property.

Also, are you sure we need to have custom code if we put the constructor in this
specific order in the IDL?
[
    Constructor(Uint8ClampedArray data, unsigned long sw, optional unsigned long
sh),
    Constructor(unsigned long sw, unsigned long sh),
]

iirc, if we put the most specific arg type first, our bindings generator does
the right thing(tm). I may be wrong but it is worth trying I think.

[Inactive, 2014-03-19 19:07:14]

https://codereview.chromium.org/196343032/diff/20001/LayoutTests/canvas/phili...
File LayoutTests/canvas/philip/tests/2d.imageData.object.ctor.html (right):

https://codereview.chromium.org/196343032/diff/20001/LayoutTests/canvas/phili...
LayoutTests/canvas/philip/tests/2d.imageData.object.ctor.html:16: try { var
_thrown = false; new window.ImageData(1,1); } catch (e) { _thrown = true; }
finally { _assert(!_thrown, "should not throw exception: new
window.ImageData(1,1)"); }
This is an imported test suite, I don't know if we can update the layout test
unless the original test suite is also updated. We may need to rebaseline the
test for now (someone should confirm)

https://codereview.chromium.org/196343032/diff/20001/LayoutTests/fast/canvas/...
File LayoutTests/fast/canvas/canvas-ImageData-constructor.html (right):

https://codereview.chromium.org/196343032/diff/20001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-ImageData-constructor.html:3: <script
src="script-tests/canvas-ImageData.js"></script>
Could we inline the script in this file to avoid having an extra js file? I
think we try to avoid this nowadays.

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
File Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
Source/core/html/ImageData.cpp:80: return imageData;
nit: .release()

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
Source/core/html/ImageData.cpp:94: Uint8ClampedArray* clampedData =
static_cast<Uint8ClampedArray*>(data);
Why is this cast needed?

I actually don't understand by we need this local variable at all. We should be
able to use "data" directly unless I am missing something.

[Justin Novosad, 2014-03-19 19:09:21]

I did not see an "Intent to implement" thread for this on blink-dev.

[sof, 2014-03-19 19:20:34]

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
File Source/core/html/ImageData.idl (right):

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
Source/core/html/ImageData.idl:30: CustomConstructor,
On 2014/03/19 18:59:48, Chris Dumez wrote:
...
> 
> Also, are you sure we need to have custom code if we put the constructor in
this
> specific order in the IDL?
> [
>     Constructor(Uint8ClampedArray data, unsigned long sw, optional unsigned
long
> sh),
>     Constructor(unsigned long sw, unsigned long sh),
> ]
> 
> iirc, if we put the most specific arg type first, our bindings generator does
> the right thing(tm). I may be wrong but it is worth trying I think.

That's not the reason behind the custom constructor, but the provision of "data"
on the wrapper to optimize its access. 

(Yes, what you suggest gives a reasonable overloading -- see patchset #1.)

[Justin Novosad, 2014-03-19 19:35:54]

https://codereview.chromium.org/196343032/diff/20001/LayoutTests/canvas/phili...
File LayoutTests/canvas/philip/tests/2d.imageData.object.ctor.html (right):

https://codereview.chromium.org/196343032/diff/20001/LayoutTests/canvas/phili...
LayoutTests/canvas/philip/tests/2d.imageData.object.ctor.html:16: try { var
_thrown = false; new window.ImageData(1,1); } catch (e) { _thrown = true; }
finally { _assert(!_thrown, "should not throw exception: new
window.ImageData(1,1)"); }
On 2014/03/19 19:07:14, Chris Dumez wrote:
> This is an imported test suite, I don't know if we can update the layout test
> unless the original test suite is also updated. We may need to rebaseline the
> test for now (someone should confirm)

Correct.  We only touch the baselines for these tests in the blink repo. If you
need a modified version of this test, you should make a copy of it (probably
under fast/canvas).  Or just write a new test, as you have already done.

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
File Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
Source/core/html/ImageData.cpp:66:
exceptionState.throwDOMException(IndexSizeError, String::format("The source %s
is zero or non-finite.", width ? "height" : "width"));
how can an unsigned be non-finite?

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
Source/core/html/ImageData.cpp:95: unsigned clampedLength =
clampedData->length() / 4;
According to the spec you need to throw an InvalidStateError if
clampedData->length() is not a multiple of 4. You should perform that check
before this division.

[sof, 2014-03-19 19:41:09]

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
File Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
Source/core/html/ImageData.cpp:66:
exceptionState.throwDOMException(IndexSizeError, String::format("The source %s
is zero or non-finite.", width ? "height" : "width"));
On 2014/03/19 19:35:54, junov wrote:
> how can an unsigned be non-finite?

ToUInt32() maps Inf and NaNs (including non-numbers mapped to such by
ToNumber()) to 0. "not a number" is a better term to use here.

[sof, 2014-03-19 22:19:16]

https://codereview.chromium.org/196343032/diff/20001/LayoutTests/canvas/phili...
File LayoutTests/canvas/philip/tests/2d.imageData.object.ctor.html (right):

https://codereview.chromium.org/196343032/diff/20001/LayoutTests/canvas/phili...
LayoutTests/canvas/philip/tests/2d.imageData.object.ctor.html:16: try { var
_thrown = false; new window.ImageData(1,1); } catch (e) { _thrown = true; }
finally { _assert(!_thrown, "should not throw exception: new
window.ImageData(1,1)"); }
On 2014/03/19 19:35:54, junov wrote:
> On 2014/03/19 19:07:14, Chris Dumez wrote:
> > This is an imported test suite, I don't know if we can update the layout
test
> > unless the original test suite is also updated. We may need to rebaseline
the
> > test for now (someone should confirm)
> 
> Correct.  We only touch the baselines for these tests in the blink repo. If
you
> need a modified version of this test, you should make a copy of it (probably
> under fast/canvas).  Or just write a new test, as you have already done.

Thanks, I reverted the pass condition inversion & added a [WontFix] entry to
StaleTestExpectations instead.

As the test file only contains that one test, no loss in test coverage by doing
so.

https://codereview.chromium.org/196343032/diff/20001/LayoutTests/fast/canvas/...
File LayoutTests/fast/canvas/canvas-ImageData-constructor.html (right):

https://codereview.chromium.org/196343032/diff/20001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-ImageData-constructor.html:3: <script
src="script-tests/canvas-ImageData.js"></script>
On 2014/03/19 19:07:14, Chris Dumez wrote:
> Could we inline the script in this file to avoid having an extra js file? I
> think we try to avoid this nowadays.

All fast/canvas tests do this, so I don't think we should create some other
precedent here.

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
File Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
Source/core/html/ImageData.cpp:94: Uint8ClampedArray* clampedData =
static_cast<Uint8ClampedArray*>(data);
On 2014/03/19 19:07:14, Chris Dumez wrote:
> Why is this cast needed?
> 
> I actually don't understand by we need this local variable at all. We should
be
> able to use "data" directly unless I am missing something.

Yes, doesn't make much sense; gone. [I belatedly discovered that the IDL
compiler supported Uint8ClampedArray instance checking, and not just
ArrayBufferViews; this is a remnant.]

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
Source/core/html/ImageData.cpp:95: unsigned clampedLength =
clampedData->length() / 4;
On 2014/03/19 19:35:54, junov wrote:
> According to the spec you need to throw an InvalidStateError if
> clampedData->length() is not a multiple of 4. You should perform that check
> before this division.

Thanks, quite right. Addressed + expanded the testing in this area.

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
File Source/core/html/ImageData.idl (right):

https://codereview.chromium.org/196343032/diff/20001/Source/core/html/ImageDa...
Source/core/html/ImageData.idl:30: CustomConstructor,
On 2014/03/19 18:59:48, Chris Dumez wrote:
> Even if the constructor is custom, you're expected to provide the prototype of
> the constructor (e.g. CustomConstructor(float x, float y)). The bindings
> generator relies on that to compute the correct value for
> window.ImageData.length property.
> 

Added along with a test for expected constructor "length".

[sof, 2014-03-20 22:39:23]

ping?

[sof, 2014-03-21 07:43:56]

On 2014/03/19 17:18:05, sof wrote:
> Please take a look.
> 
> I'm making a pair of assumptions here regarding the treatment of
ImageData.data:
> 
>  - The older optimization to put the "data" property on the ImageData wrapper
is
> still valid/worthy for v8.

Using the snippet in https://bugs.webkit.org/show_bug.cgi?id=49999 as a
testcase,
there are orders of magnitude difference between putting "data" on the wrapper
vs not. Hence doing away with this optimization is not an option, I'd argue.

>  - There is no way to express via IDL the insertion of custom initialization
> code of the wrapper that a constructor builds and adds to the DOMDataStore.
> Hence, a custom constructor is needed here to accomplish just that.
> 

haraken: if there isn't a way to inject a wrap()-like initialization of the
wrapper that a constructor creates, is having a custom constructor acceptable?
Having code generator support for something as specialized as this perhaps
needs another couple of uses before merited?

[sof, 2014-03-21 16:37:52]

On 2014/03/21 07:43:56, sof wrote:
...
> 
> haraken: if there isn't a way to inject a wrap()-like initialization of the
> wrapper that a constructor creates, is having a custom constructor acceptable?
> Having code generator support for something as specialized as this perhaps
> needs another couple of uses before merited?

I think I worked out how to change code generation to emit code that custom
wrap()s constructor results also.

Thus avoiding a custom constructor. bindings/ people, please take a look.

[Justin Novosad, 2014-03-21 18:59:02]

On 2014/03/21 16:37:52, sof wrote:
> On 2014/03/21 07:43:56, sof wrote:
> ...
> > 
> > haraken: if there isn't a way to inject a wrap()-like initialization of the
> > wrapper that a constructor creates, is having a custom constructor
acceptable?
> > Having code generator support for something as specialized as this perhaps
> > needs another couple of uses before merited?
> 
> I think I worked out how to change code generation to emit code that custom
> wrap()s constructor results also.
> 
> Thus avoiding a custom constructor. bindings/ people, please take a look.

I don't know much about bindings, but the changes to Source/core look fine to
me.
FYI you can't ship a new API without proper approval
Please follow these steps:
http://www.chromium.org/blink#launch-process

[sof, 2014-03-21 19:33:18]

On 2014/03/21 18:59:02, junov wrote:
> On 2014/03/21 16:37:52, sof wrote:
> > On 2014/03/21 07:43:56, sof wrote:
> > ...
> > > 
> > > haraken: if there isn't a way to inject a wrap()-like initialization of
the
> > > wrapper that a constructor creates, is having a custom constructor
> acceptable?
> > > Having code generator support for something as specialized as this perhaps
> > > needs another couple of uses before merited?
> > 
> > I think I worked out how to change code generation to emit code that custom
> > wrap()s constructor results also.
> > 
> > Thus avoiding a custom constructor. bindings/ people, please take a look.
> 
> I don't know much about bindings, but the changes to Source/core look fine to
> me.
> FYI you can't ship a new API without proper approval
> Please follow these steps:
> http://www.chromium.org/blink#launch-process

Thank you. If that's the consensus opinion here, I can definitely do that as a
next step. I considered a PSA adequate for these convenience constructors,
bringing the API implementation in-line with the updated ImageData interface,
but (arguably) not adding new functionality.

[Justin Novosad, 2014-03-21 19:36:43]

On 2014/03/21 19:33:18, sof wrote:
> On 2014/03/21 18:59:02, junov wrote:
> > On 2014/03/21 16:37:52, sof wrote:
> > > On 2014/03/21 07:43:56, sof wrote:
> > > ...
> > > > 
> > > > haraken: if there isn't a way to inject a wrap()-like initialization of
> the
> > > > wrapper that a constructor creates, is having a custom constructor
> > acceptable?
> > > > Having code generator support for something as specialized as this
perhaps
> > > > needs another couple of uses before merited?
> > > 
> > > I think I worked out how to change code generation to emit code that
custom
> > > wrap()s constructor results also.
> > > 
> > > Thus avoiding a custom constructor. bindings/ people, please take a look.
> > 
> > I don't know much about bindings, but the changes to Source/core look fine
to
> > me.
> > FYI you can't ship a new API without proper approval
> > Please follow these steps:
> > http://www.chromium.org/blink#launch-process
> 
> Thank you. If that's the consensus opinion here, I can definitely do that as a
> next step. I considered a PSA adequate for these convenience constructors,
> bringing the API implementation in-line with the updated ImageData interface,
> but (arguably) not adding new functionality.

You are adding an API entry point.  Once you do that there is no going back. 
Also, we track these launches on the feature dashboard so that we can, among
other things, let devs know that the new API is available once we ship it.

[sof, 2014-03-21 20:50:09]

On 2014/03/21 19:36:43, junov wrote:
> On 2014/03/21 19:33:18, sof wrote:
> > On 2014/03/21 18:59:02, junov wrote:
> > > On 2014/03/21 16:37:52, sof wrote:
> > > > On 2014/03/21 07:43:56, sof wrote:
> > > > ...
> > > > > 
> > > > > haraken: if there isn't a way to inject a wrap()-like initialization
of
> > the
> > > > > wrapper that a constructor creates, is having a custom constructor
> > > acceptable?
> > > > > Having code generator support for something as specialized as this
> perhaps
> > > > > needs another couple of uses before merited?
> > > > 
> > > > I think I worked out how to change code generation to emit code that
> custom
> > > > wrap()s constructor results also.
> > > > 
> > > > Thus avoiding a custom constructor. bindings/ people, please take a
look.
> > > 
> > > I don't know much about bindings, but the changes to Source/core look fine
> to
> > > me.
> > > FYI you can't ship a new API without proper approval
> > > Please follow these steps:
> > > http://www.chromium.org/blink#launch-process
> > 
> > Thank you. If that's the consensus opinion here, I can definitely do that as
a
> > next step. I considered a PSA adequate for these convenience constructors,
> > bringing the API implementation in-line with the updated ImageData
interface,
> > but (arguably) not adding new functionality.
> 
> You are adding an API entry point.  Once you do that there is no going back. 
> Also, we track these launches on the feature dashboard so that we can, among
> other things, let devs know that the new API is available once we ship it.

thanks, posted
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/FvHQUEXCgEo

(bundled it with also exposing ImageData to workers; I'll handle that separately
from this CL.)

[Nils Barth (inactive), 2014-03-24 02:14:20]

+haraken: is the bindings change ok?

The bindings changes look ok at a glance, and thanks
for avoiding custom bindings!

There are a few other cleanups that seem possible,
since you're fixing {{wrapper_configuration}};
could you take a quick look at these?

Thanks!

https://codereview.chromium.org/196343032/diff/100001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/196343032/diff/100001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:398: {% set wrapper_configuration =
'WrapperConfiguration::Dependent'
Since we're using {{wrapper_configuration}} two places,
could you factor it out into the interface context display
in v8_interface.py?

Also, can you check the 2 other uses of
WrapperConfiguration::Dependent
...and see if we can replace these with {{wrapper_configuration}}?

Thanks!

[haraken, 2014-03-24 02:18:16]

The binding change looks OK but I want to make the change in a separate CL. It
can change timing when active DOM objects are collected, so we want to keep the
change separate from the ImageData change.

[Nils Barth (inactive), 2014-03-24 03:14:37]

On 2014/03/24 02:18:16, haraken wrote:
> The binding change looks OK but I want to make the change in a separate CL. It
> can change timing when active DOM objects are collected, so we want to keep
the
> change separate from the ImageData change.

Agreed; simple changes but bloat the CL otherwise.

I've actually split it off into 2 CLs:
Use wrapper_configuration consistently in bindings
https://codereview.chromium.org/199633014/
...which changes the timing of GC

...and:
Add support for [Constructor, Custom=Wrap]
https://codereview.chromium.org/206793006/
...which is separate but independent so easier to review separately.

[sof, 2014-03-24 06:11:10]

Awesome :) I'll wait for both to land and rebase.

[sof, 2014-03-24 11:20:17]

On 2014/03/24 06:11:10, sof wrote:
> Awesome :) I'll wait for both to land and rebase.

All landed now; rebased.

[Justin Novosad, 2014-03-24 14:00:11]

At this point we can either:
a) wait for three API owner approvals on the intent to ship;
b) land the implementation without shipping, which means hiding the new API
behind an experimental feature flag.

https://codereview.chromium.org/196343032/diff/120001/LayoutTests/fast/canvas...
File LayoutTests/fast/canvas/script-tests/canvas-ImageData.js (right):

https://codereview.chromium.org/196343032/diff/120001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/script-tests/canvas-ImageData.js:35: imageDataFromData =
new ImageData(imageData.data, 100);
Would be good to exercise a successful call to this constructor with the
optional height argument.

[Justin Novosad, 2014-03-24 14:21:08]

The layout test needs to verify that the version of the constructor that takes a
Uint8ClampedArray is not making a copy.  So you need to check that changing the
source data after construction results in an identical change to the ImageData,
and vice versa.

https://codereview.chromium.org/196343032/diff/120001/LayoutTests/fast/canvas...
File LayoutTests/fast/canvas/script-tests/canvas-ImageData.js (right):

https://codereview.chromium.org/196343032/diff/120001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/script-tests/canvas-ImageData.js:10:
shouldBe("imageData.data[11]", "0");
Why 11? Should check at least one of each RGBA component.

https://codereview.chromium.org/196343032/diff/120001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/script-tests/canvas-ImageData.js:12: imageData.data[i] =
231;
Why set everything to 231? You only check element 32 below.

[sof, 2014-03-24 14:57:21]

Thanks, hopefully addressed.

I'll await owner feedback from the 'intent' until Wednesday or so before
deciding.

https://codereview.chromium.org/196343032/diff/120001/LayoutTests/fast/canvas...
File LayoutTests/fast/canvas/script-tests/canvas-ImageData.js (right):

https://codereview.chromium.org/196343032/diff/120001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/script-tests/canvas-ImageData.js:10:
shouldBe("imageData.data[11]", "0");
On 2014/03/24 14:21:09, junov wrote:
> Why 11? Should check at least one of each RGBA component.

Good idea; switched to peeking&poking over RGBA values.

https://codereview.chromium.org/196343032/diff/120001/LayoutTests/fast/canvas...
LayoutTests/fast/canvas/script-tests/canvas-ImageData.js:35: imageDataFromData =
new ImageData(imageData.data, 100);
On 2014/03/24 14:00:11, junov wrote:
> Would be good to exercise a successful call to this constructor with the
> optional height argument.

Done.

[Justin Novosad, 2014-03-24 15:03:55]

On 2014/03/24 14:57:21, sof wrote:

lgtm

API owner lgtm still required for landing as is (i.e. to ship).

[sof, 2014-03-26 18:45:49]

Thanks to owners on this CL (reviewers and Cc:ers) for supporting the "intent".
But it looks as if not enough owners will pass a verdict one way or another.

[Justin Novosad, 2014-03-26 20:06:12]

On 2014/03/26 18:45:49, sof wrote:
> Thanks to owners on this CL (reviewers and Cc:ers) for supporting the
"intent".
> But it looks as if not enough owners will pass a verdict one way or another.

You can always ping the intent to ship thread.

[sof, 2014-03-28 10:31:45]

Constructor overloads are now behind an experimental flag, please take a look.

[Justin Novosad, 2014-03-28 13:03:52]

On 2014/03/28 10:31:45, sof wrote:
> Constructor overloads are now behind an experimental flag, please take a look.

lgtm

[sof, 2014-03-28 13:07:08]

On 2014/03/28 13:03:52, junov wrote:
> On 2014/03/28 10:31:45, sof wrote:
> > Constructor overloads are now behind an experimental flag, please take a
look.
> 
> lgtm

thanks much; all ready now, so will push it along.

[sof, 2014-03-28 13:26:36]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-28 13:26:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/196343032/190001

[commit-bot: I haz the power, 2014-03-28 13:34:56]

Change committed as 170325