ShapeDetection: support CanvasImageSource as detect() source

ShapeDetection: support CanvasImageSource as detect() source

This CL changes the input parameter to FaceDetector.detect()
to be a CanvasImageSource, refactors the current
code to handle HTMLImageElement inputs, and adds support
for ImageBitmap.  So now we'll have a single detect() method
doing the common stuff and delegating the specifics to e.g.
detectFacesOnImageElement(), detectFacesOnImageBitmap()
etc.

It also beefs up the corresponding LayoutTests,
renaming some files/functions where appropriate

BUG=659138
TEST=layout tests (fast, non fast, http).

Committed: https://crrev.com/a4f0895546f2d20906d5b276a9aafae9af50dd30
Cr-Commit-Position: refs/heads/master@{#428094}

[mcasas, 2016-10-25 17:46:24]

Description was changed from

==========
ShapeDetection: support ImageBitmap as detect() source

BUG=659138
==========

to

==========
ShapeDetection: support ImageBitmap as detect() source

This CL adds support for FaceDetector.detect() ImageBitmap
sources. It also beefs up the corresponding LayoutTests, 
renaming some files/functions where appropriate

BUG=659138
TEST=layout tests (fast, non fast, http).
==========

[mcasas, 2016-10-25 17:48:36]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-10-25 17:48:45]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-10-25 19:16:10]

mcasas@chromium.org changed reviewers:
+ xianglu@chromium.org

[mcasas, 2016-10-25 19:16:11]

xianglu@ PTAL

[xianglu, 2016-10-25 21:07:46]

Just a few comments.

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/shapedetection/shapedetection-cross-origin.html
(right):

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/shapedetection/shapedetection-cross-origin.html:7:
const imageUrl = "http://localhost:8080/security/resources/abe.png";
IMAGE_URL? Not sure if this is the right style guide though.
https://google.github.io/styleguide/javascriptguide.xml#Constants

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/shapedetection/shapedetection-cross-origin.html:23:
image.onerror = () => {};  // Explicitly ignore expected error events.
It is possible that other security checks might fall into onerror() event( ex.
empty image). I would consider taking care of this event as well if this
function is ever going to be reused.

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/shapedetection/detectface-ImageBitmap.html
(right):

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/detectface-ImageBitmap.html:37:
assert_equals(imageReceivedByMock.byteLength, 2500,
"imageReceivedByMock.byteLength");
nit: format here

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/detectface-ImageBitmap.html:48: },
'exercises the ShapeDetection API detect()');
nit: 'exercises the ShapeDetection API detect(ImageBitmap).'

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right):

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:155:
RefPtr<Uint8Array> pixelData;
Consider moving this line above l.165.

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.h (right):

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.h:30:
ScriptPromise detect(ScriptState*, ImageBitmap*);
const?

[mcasas, 2016-10-25 21:47:38]

PTAL

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/shapedetection/shapedetection-cross-origin.html
(right):

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/shapedetection/shapedetection-cross-origin.html:7:
const imageUrl = "http://localhost:8080/security/resources/abe.png";
On 2016/10/25 21:07:45, xianglu wrote:
> IMAGE_URL? Not sure if this is the right style guide though.
> https://google.github.io/styleguide/javascriptguide.xml#Constants

Done.

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/shapedetection/shapedetection-cross-origin.html:23:
image.onerror = () => {};  // Explicitly ignore expected error events.
On 2016/10/25 21:07:45, xianglu wrote:
> It is possible that other security checks might fall into onerror() event( ex.
> empty image). I would consider taking care of this event as well if this
> function is ever going to be reused.

Done.

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/shapedetection/detectface-ImageBitmap.html
(right):

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/detectface-ImageBitmap.html:37:
assert_equals(imageReceivedByMock.byteLength, 2500,
"imageReceivedByMock.byteLength");
On 2016/10/25 21:07:45, xianglu wrote:
> nit: format here

Done.

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/detectface-ImageBitmap.html:48: },
'exercises the ShapeDetection API detect()');
On 2016/10/25 21:07:45, xianglu wrote:
> nit: 'exercises the ShapeDetection API detect(ImageBitmap).'

Done.

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right):

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:155:
RefPtr<Uint8Array> pixelData;
On 2016/10/25 21:07:45, xianglu wrote:
> Consider moving this line above l.165.

Can't move it in there, or it will fall out of scope
and invalidate the pixelData->data() referenced
in l.169.

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.h (right):

https://codereview.chromium.org/2448193002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.h:30:
ScriptPromise detect(ScriptState*, ImageBitmap*);
On 2016/10/25 21:07:45, xianglu wrote:
> const?

I tried that but apparently V8 binding generates this
function call with a non-const *; also, we use 
copyBitmapData() inside the implementation, which 
is non-const and the compiler complains.

[xianglu, 2016-10-25 22:06:46]

lgtm.

[mcasas, 2016-10-25 22:21:31]

mcasas@chromium.org changed reviewers:
+ haraken@chromium.org

[mcasas, 2016-10-25 22:21:31]

haraken@ please RS modules/shapedetection

[mcasas, 2016-10-25 22:24:05]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 22:24:37]

Dry run: 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-10-25 23:29:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 23:29:54]

Dry run: This issue passed the CQ dry run.

[mcasas, 2016-10-26 15:46:13]

mcasas@chromium.org changed reviewers:
+ esprehn@chromium.org

[mcasas, 2016-10-26 15:46:14]

esprehn@ RS plz changes in modules/shapedetection

[haraken, 2016-10-26 15:50:59]

Sorry, I forgot to publish the comment. LGTM.

https://codereview.chromium.org/2448193002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right):

https://codereview.chromium.org/2448193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:170:
allocationSize = imageBitmap->size().area() * 4;  // 4 bpp.

Do you want to check overflow?

You can use CheckedNumeric in wtf/.

[esprehn, 2016-10-26 16:03:50]

I don't think we should land this, the api should just take a CanvasImageSource
and accept the same things as drawImage. There's nothing special about this api,
it should take the same inputs. That also makes you support all of the various
image inputs from day one.

[mcasas, 2016-10-26 17:44:37]

On 2016/10/26 16:03:50, esprehn wrote:
> I don't think we should land this, the api should just take a
CanvasImageSource
> and accept the same things as drawImage. There's nothing special about this
api,
> it should take the same inputs. That also makes you support all of the various
> image inputs from day one.

You're right, I took a look at CanvasImageSource and is
indeed very well suited to our purpose. I will rewrite this
CL and upload a new patch with this comment taken. 
Note that the LayoutTests would not change.

[mcasas, 2016-10-26 17:58:31]

Description was changed from

==========
ShapeDetection: support ImageBitmap as detect() source

This CL adds support for FaceDetector.detect() ImageBitmap
sources. It also beefs up the corresponding LayoutTests, 
renaming some files/functions where appropriate

BUG=659138
TEST=layout tests (fast, non fast, http).
==========

to

==========
ShapeDetection: support ImageBitmap as detect() source

This CL changes the input parameter to FaceDetector.detect() 
to be a CanvasImageSource, refactors the current 
code to handle HTMLImageElement inputs, and adds support
for ImageBitmap.  So now we'll have a single detect() method
doing the common stuff and delegating the specifics to e.g.
detectFacesOnImageElement(), detectFacesOnImageBitmap()
etc.

It also beefs up the corresponding LayoutTests, 
renaming some files/functions where appropriate

BUG=659138
TEST=layout tests (fast, non fast, http).
==========

[mcasas, 2016-10-26 17:59:00]

Description was changed from

==========
ShapeDetection: support ImageBitmap as detect() source

This CL changes the input parameter to FaceDetector.detect() 
to be a CanvasImageSource, refactors the current 
code to handle HTMLImageElement inputs, and adds support
for ImageBitmap.  So now we'll have a single detect() method
doing the common stuff and delegating the specifics to e.g.
detectFacesOnImageElement(), detectFacesOnImageBitmap()
etc.

It also beefs up the corresponding LayoutTests, 
renaming some files/functions where appropriate

BUG=659138
TEST=layout tests (fast, non fast, http).
==========

to

==========
ShapeDetection: support CanvasImageSource as detect() source

This CL changes the input parameter to FaceDetector.detect() 
to be a CanvasImageSource, refactors the current 
code to handle HTMLImageElement inputs, and adds support
for ImageBitmap.  So now we'll have a single detect() method
doing the common stuff and delegating the specifics to e.g.
detectFacesOnImageElement(), detectFacesOnImageBitmap()
etc.

It also beefs up the corresponding LayoutTests, 
renaming some files/functions where appropriate

BUG=659138
TEST=layout tests (fast, non fast, http).
==========

[mcasas, 2016-10-26 18:06:50]

Patchset #3 (id:80001) has been deleted

[mcasas, 2016-10-26 18:08:26]

PTAL

https://codereview.chromium.org/2448193002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right):

https://codereview.chromium.org/2448193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:170:
allocationSize = imageBitmap->size().area() * 4;  // 4 bpp.
On 2016/10/26 15:50:58, haraken wrote:
> 
> Do you want to check overflow?
> 
> You can use CheckedNumeric in wtf/.

Done.

[xianglu, 2016-10-27 02:55:20]

still lgtm.

[mcasas, 2016-10-27 03:45:39]

On 2016/10/27 02:55:20, xianglu wrote:
> still lgtm.

haraken@, esprehn@ PTAL

[haraken, 2016-10-27 06:53:10]

On 2016/10/27 03:45:39, mcasas wrote:
> On 2016/10/27 02:55:20, xianglu wrote:
> > still lgtm.
> 
> haraken@, esprehn@ PTAL

(Elliott should be a good reviewer for shape detection APIs.)

[esprehn, 2016-10-27 15:46:47]

Lgtm, Im not sure why the spec wants to throw for zero sized images though. Can
you drawImage() a zero sized image? We should match whatever behavior it has.

https://codereview.chromium.org/2448193002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right):

https://codereview.chromium.org/2448193002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:90: if
(img->bitmapSourceSize().isZero()) {
This seems like a strange restriction, an empty image should just contain no
faces, I don't see any reason why it should throw an exception.

https://codereview.chromium.org/2448193002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:175: if
(imageBitmap->size().area() == 0) {
Ditto

[mcasas, 2016-10-27 17:08:11]

Patchset #4 (id:120001) has been deleted

[mcasas, 2016-10-27 17:09:12]

https://codereview.chromium.org/2448193002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right):

https://codereview.chromium.org/2448193002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:90: if
(img->bitmapSourceSize().isZero()) {
On 2016/10/27 15:46:47, esprehn wrote:
> This seems like a strange restriction, an empty image should just contain no
> faces, I don't see any reason why it should throw an exception.

Canvas drawImage() does not like empty images (or
too large ones) but doesn't raise and exception [1],
so I guess here we should resolve() with an empty
vector. Also modified the LayoutTest to check for
this ISO rejection. 


[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/canvas...

https://codereview.chromium.org/2448193002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:175: if
(imageBitmap->size().area() == 0) {
On 2016/10/27 15:46:47, esprehn wrote:
> Ditto
Done.

[mcasas, 2016-10-27 17:54:03]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-10-27 17:54:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, esprehn@chromium.org, xianglu@chromium.org
Link to the patchset: https://codereview.chromium.org/2448193002/#ps140001
(title: "esprehn@ comments and specific LayoutTest for empty HTMLImageElement")

[commit-bot: I haz the power, 2016-10-27 17:54:54]

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-10-27 18:47:09]

Description was changed from

==========
ShapeDetection: support CanvasImageSource as detect() source

This CL changes the input parameter to FaceDetector.detect() 
to be a CanvasImageSource, refactors the current 
code to handle HTMLImageElement inputs, and adds support
for ImageBitmap.  So now we'll have a single detect() method
doing the common stuff and delegating the specifics to e.g.
detectFacesOnImageElement(), detectFacesOnImageBitmap()
etc.

It also beefs up the corresponding LayoutTests, 
renaming some files/functions where appropriate

BUG=659138
TEST=layout tests (fast, non fast, http).
==========

to

==========
ShapeDetection: support CanvasImageSource as detect() source

This CL changes the input parameter to FaceDetector.detect() 
to be a CanvasImageSource, refactors the current 
code to handle HTMLImageElement inputs, and adds support
for ImageBitmap.  So now we'll have a single detect() method
doing the common stuff and delegating the specifics to e.g.
detectFacesOnImageElement(), detectFacesOnImageBitmap()
etc.

It also beefs up the corresponding LayoutTests, 
renaming some files/functions where appropriate

BUG=659138
TEST=layout tests (fast, non fast, http).
==========

[commit-bot: I haz the power, 2016-10-27 18:47:10]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-10-27 19:01:50]

Description was changed from

==========
ShapeDetection: support CanvasImageSource as detect() source

This CL changes the input parameter to FaceDetector.detect() 
to be a CanvasImageSource, refactors the current 
code to handle HTMLImageElement inputs, and adds support
for ImageBitmap.  So now we'll have a single detect() method
doing the common stuff and delegating the specifics to e.g.
detectFacesOnImageElement(), detectFacesOnImageBitmap()
etc.

It also beefs up the corresponding LayoutTests, 
renaming some files/functions where appropriate

BUG=659138
TEST=layout tests (fast, non fast, http).
==========

to

==========
ShapeDetection: support CanvasImageSource as detect() source

This CL changes the input parameter to FaceDetector.detect()
to be a CanvasImageSource, refactors the current
code to handle HTMLImageElement inputs, and adds support
for ImageBitmap.  So now we'll have a single detect() method
doing the common stuff and delegating the specifics to e.g.
detectFacesOnImageElement(), detectFacesOnImageBitmap()
etc.

It also beefs up the corresponding LayoutTests,
renaming some files/functions where appropriate

BUG=659138
TEST=layout tests (fast, non fast, http).

Committed: https://crrev.com/a4f0895546f2d20906d5b276a9aafae9af50dd30
Cr-Commit-Position: refs/heads/master@{#428094}
==========

[commit-bot: I haz the power, 2016-10-27 19:01:52]

Patchset 4 (id:??) landed as
https://crrev.com/a4f0895546f2d20906d5b276a9aafae9af50dd30
Cr-Commit-Position: refs/heads/master@{#428094}