ShapeDetection: implement barcode detection, blink part

ShapeDetection: implement barcode detection, blink part

This CL introduces the QR/barcode detection API idls
(BarcodeDetector.idl and DetectedBarcode.idl), and
the Blink-side implementation and mojo definition.

FaceDetector.cpp is almost emptied to become a
storefront for ShapeDetector, where most of the code
is moved to and that is used for both Face and Barcode
detection.

LayoutTests and mock mojo implementation are adapted
all over -- in particular, the tests under
LayoutTests/shapedetection are 'parameterised'.

BUG=665150
Committed: https://crrev.com/cd43abe6b873841f33cdbe5cc2789bd1611a3d4c
Cr-Commit-Position: refs/heads/master@{#432429}

[mcasas, 2016-11-14 21:38:55]

Description was changed from

==========
ShapeDetection: QR code detection, blink parts

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

to

==========
ShapeDetection: QR code detection, blink parts

BUG=665150
==========

[mcasas, 2016-11-14 21:39:45]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-11-14 21:42:23]

Description was changed from

==========
ShapeDetection: QR code detection, blink parts

BUG=665150
==========

to

==========
ShapeDetection: QR code detection, blink parts

This CL introduces the QR/barcode detection API idls 
(BarcodeDetector.idl and DetectedBarcode.idl), and
the Blink-side implementation and mojo definition.

The code has huge parallelisms with FaceDetector.cpp,
so the code of this class is factored out into a parent
ShapeDetector class which is used for Face and Barcode
detection.

LayoutTests and mock mojo implementation are adapted 
all over.

BUG=665150
==========

[mcasas, 2016-11-14 21:44:20]

Description was changed from

==========
ShapeDetection: QR code detection, blink parts

This CL introduces the QR/barcode detection API idls 
(BarcodeDetector.idl and DetectedBarcode.idl), and
the Blink-side implementation and mojo definition.

The code has huge parallelisms with FaceDetector.cpp,
so the code of this class is factored out into a parent
ShapeDetector class which is used for Face and Barcode
detection.

LayoutTests and mock mojo implementation are adapted 
all over.

BUG=665150
==========

to

==========
ShapeDetection: QR code detection, blink parts

This CL introduces the QR/barcode detection API idls 
(BarcodeDetector.idl and DetectedBarcode.idl), and
the Blink-side implementation and mojo definition.

FaceDetector.cpp is almost emptied to become a 
storefront for ShapeDetector, where most of the code
is moved to and that is used for both Face and Barcode
detection.

LayoutTests and mock mojo implementation are adapted 
all over.

BUG=665150
==========

[mcasas, 2016-11-14 21:44:37]

Patchset #2 (id:40001) has been deleted

[mcasas, 2016-11-14 21:44:45]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-11-14 21:51:53]

Description was changed from

==========
ShapeDetection: QR code detection, blink parts

This CL introduces the QR/barcode detection API idls 
(BarcodeDetector.idl and DetectedBarcode.idl), and
the Blink-side implementation and mojo definition.

FaceDetector.cpp is almost emptied to become a 
storefront for ShapeDetector, where most of the code
is moved to and that is used for both Face and Barcode
detection.

LayoutTests and mock mojo implementation are adapted 
all over.

BUG=665150
==========

to

==========
ShapeDetection: QR code detection, blink parts

This CL introduces the QR/barcode detection API idls 
(BarcodeDetector.idl and DetectedBarcode.idl), and
the Blink-side implementation and mojo definition.

FaceDetector.cpp is almost emptied to become a 
storefront for ShapeDetector, where most of the code
is moved to and that is used for both Face and Barcode
detection.

LayoutTests and mock mojo implementation are adapted 
all over -- in particular, the tests under 
LayoutTests/shapedetection are 'parameterised'.

BUG=665150
==========

[mcasas, 2016-11-14 21:55:40]

Patchset #1 (id:60001) has been deleted

[mcasas, 2016-11-14 22:40:05]

Patchset #1 (id:80001) has been deleted

[mcasas, 2016-11-14 23:19:49]

Description was changed from

==========
ShapeDetection: QR code detection, blink parts

This CL introduces the QR/barcode detection API idls 
(BarcodeDetector.idl and DetectedBarcode.idl), and
the Blink-side implementation and mojo definition.

FaceDetector.cpp is almost emptied to become a 
storefront for ShapeDetector, where most of the code
is moved to and that is used for both Face and Barcode
detection.

LayoutTests and mock mojo implementation are adapted 
all over -- in particular, the tests under 
LayoutTests/shapedetection are 'parameterised'.

BUG=665150
==========

to

==========
ShapeDetection: implement barcode detection, blink part

This CL introduces the QR/barcode detection API idls 
(BarcodeDetector.idl and DetectedBarcode.idl), and
the Blink-side implementation and mojo definition.

FaceDetector.cpp is almost emptied to become a 
storefront for ShapeDetector, where most of the code
is moved to and that is used for both Face and Barcode
detection.

LayoutTests and mock mojo implementation are adapted 
all over -- in particular, the tests under 
LayoutTests/shapedetection are 'parameterised'.

BUG=665150
==========

[mcasas, 2016-11-15 00:28:09]

Patchset #1 (id:100001) has been deleted

[mcasas, 2016-11-15 01:42:52]

Patchset #1 (id:120001) has been deleted

[mcasas, 2016-11-15 01:46:03]

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

[mcasas, 2016-11-15 01:46:04]

xianglu@ PTAL

tsepez@ additions to mojom file  plz.

[dcheng, 2016-11-15 01:47:37]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-11-15 01:47:38]

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
(right):

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:29:
DetectBarcodes(handle<shared_buffer> frame_data, uint32 width, uint32 height)
I think rsesek@ has mentioned this elsewhere, but let's use an actual type to
represent bitmaps (in this case, SkBitmap from
https://cs.chromium.org/chromium/src/skia/public/interfaces/bitmap.mojom?rcl=...
should work nicely).

[Robert Sesek, 2016-11-15 14:29:16]

rsesek@chromium.org changed reviewers:
+ rsesek@chromium.org

[Robert Sesek, 2016-11-15 14:29:17]

Why can't this detection be done in the renderer process directly?

[mcasas, 2016-11-15 17:38:35]

On 2016/11/15 14:29:17, Robert Sesek wrote:
> Why can't this detection be done in the renderer process directly?

Well, all this code is in the renderer process :)

This refactoring is necessary and in parallel to the
work of xianglu@ in https://crrev.com/2459043002 to
implement the functionality for Mac inside Blink. I'd
love that to work out to avoid having to make a frame 
copy for the mojo messages. 

Android implementation is hanging from the frame 
host and I'm not 100% sure it can be moved to the
renderer process due to access to Context and/or
VideoCaptureDevice.

dcheng@ thanks for the suggestion of using 
mojo::Bitmap, added bug https://crbug.com/665488
for it.

[Tom Sepez, 2016-11-15 18:31:00]

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
(right):

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:19:
string raw_value;
What values can this string take?  Can we use an enum instead?

[mcasas, 2016-11-15 19:05:25]

ptal

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
(right):

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:19:
string raw_value;
On 2016/11/15 18:31:00, Tom Sepez wrote:
> What values can this string take?  Can we use an enum instead?

This is the string encoded in a QR/barcode and
can take many forms, e.g. it can be a phone
number "tel:+1-212-555-1212", or a URL
"MEBKM:TITLE:Google;URL:https://www.google.com;;",
whose semantics are delegated to the WebApp.
Added explanation.

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:29:
DetectBarcodes(handle<shared_buffer> frame_data, uint32 width, uint32 height)
On 2016/11/15 01:47:38, dcheng wrote:
> I think rsesek@ has mentioned this elsewhere, but let's use an actual type to
> represent bitmaps (in this case, SkBitmap from
>
https://cs.chromium.org/chromium/src/skia/public/interfaces/bitmap.mojom?rcl=...
> should work nicely).

Added a TODO+bug to address this suggestion, thanks!

[Robert Sesek, 2016-11-15 19:07:52]

On 2016/11/15 17:38:35, mcasas wrote:
> On 2016/11/15 14:29:17, Robert Sesek wrote:
> > Why can't this detection be done in the renderer process directly?
> 
> Well, all this code is in the renderer process :)

But it's teeing up for an implementation in the browser, right?

> This refactoring is necessary and in parallel to the
> work of xianglu@ in https://crrev.com/2459043002 to
> implement the functionality for Mac inside Blink. I'd
> love that to work out to avoid having to make a frame 
> copy for the mojo messages. 
> 
> Android implementation is hanging from the frame 
> host and I'm not 100% sure it can be moved to the
> renderer process due to access to Context and/or
> VideoCaptureDevice.

If the API can work on the frame data being sent by the renderer, why does the
VideoCaptureDevice come in to play? There is a Context associated with the
renderer process, but it is an isolated_app so it indeed may restrict the
functionality. But I think a definitive answer here is important. 

> dcheng@ thanks for the suggestion of using 
> mojo::Bitmap, added bug https://crbug.com/665488
> for it.

[xianglu, 2016-11-15 19:19:30]

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-empty-input.html
(right):

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-empty-input.html:7:
var makeAsyncTest = function(detectorName) {
Consider changing this name to something more explanatory like: testEmptyInput?
This also applies to other test files.

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/modules_idl_files.gni (right):

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/modules_idl_files.gni:243:
"shapedetection/BarcodeDetector.idl",
nit: This line should go before l.241.

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.h (right):

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.h:23: static
DetectedBarcode* create(String rawValue, DOMRect* boundingBox);
I think variable names are not necessary in .h files.

[mcasas, 2016-11-15 20:22:56]

PTAL



rsesek@ comment:
> If the API can work on the frame data being sent by the renderer, why does the
> VideoCaptureDevice come in to play? 

When the input <video> tag is fed by a webcam,
there are faster ways to detect objects on then,
particularly Faces; currently they'll be under the
umbrella of VideoCaptureCamera{2}.java.

> There is a Context associated with the
> renderer process, but it is an isolated_app so it indeed may restrict the
> functionality. But I think a definitive answer here is important. 

We discussed this offline: there are currently
no .java files under the renderer now (IOW, 
under content//public/android/.../content/renderer),
meaning this is not a common pattern, but it
might not be impossible, we need to find that
out.  Again, this functionality is behind a flag, and 
it won't be shipped before all these questions are 
substantially answered, but I think it's beyond the 
scope of this CL.

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-empty-input.html
(right):

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-empty-input.html:7:
var makeAsyncTest = function(detectorName) {
On 2016/11/15 19:19:29, xianglu wrote:
> Consider changing this name to something more explanatory like:
testEmptyInput?
> This also applies to other test files.

Done renaming after offline discussion.

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/modules_idl_files.gni (right):

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/modules_idl_files.gni:243:
"shapedetection/BarcodeDetector.idl",
On 2016/11/15 19:19:29, xianglu wrote:
> nit: This line should go before l.241.

Done.

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.h (right):

https://codereview.chromium.org/2502723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.h:23: static
DetectedBarcode* create(String rawValue, DOMRect* boundingBox);
On 2016/11/15 19:19:29, xianglu wrote:
> I think variable names are not necessary in .h files.

On Blink style they're not. Removed.

[Tom Sepez, 2016-11-15 22:23:57]

lgtm

[xianglu, 2016-11-15 22:45:10]

lgtm

[mcasas, 2016-11-15 22:53:02]

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

[mcasas, 2016-11-15 22:53:03]

boliu@: please RS
content/public/android/java.../shapedetection/ShapeDetectionImpl.java

(I'll work on moving the Java mojo interface implementation
into renderer after this CL as we talked about offline)

[boliu, 2016-11-15 22:53:47]

java lgtm

[mcasas, 2016-11-15 22:54:01]

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

[mcasas, 2016-11-15 22:54:01]

haraken@ RS plz /PTAL at the WebKit/Source files

[haraken, 2016-11-16 04:15:11]

LGTM

https://codereview.chromium.org/2502723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.cpp
(right):

https://codereview.chromium.org/2502723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.cpp:12: return
new DetectedBarcode("", DOMRect::create(0, 0, 0, 0));

Use String::empty().

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

https://codereview.chromium.org/2502723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:22: return
detectShapes(ShapeDetector::DetectorType::Face, scriptState,

Would you make ScriptState* as the first parameter?

https://codereview.chromium.org/2502723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right):

https://codereview.chromium.org/2502723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:359:
m_serviceRequests.remove(resolver);

Nit: I'd prefer removing this at line 348 (to make onDetectBarcodes re-entrant;
although it shouldn't happen in reality).

[mcasas, 2016-11-16 05:23:10]

https://codereview.chromium.org/2502723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.cpp
(right):

https://codereview.chromium.org/2502723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.cpp:12: return
new DetectedBarcode("", DOMRect::create(0, 0, 0, 0));
On 2016/11/16 04:15:10, haraken wrote:
> 
> Use String::empty().

Done.

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

https://codereview.chromium.org/2502723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:22: return
detectShapes(ShapeDetector::DetectorType::Face, scriptState,
On 2016/11/16 04:15:10, haraken wrote:
> 
> Would you make ScriptState* as the first parameter?

Done.

https://codereview.chromium.org/2502723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right):

https://codereview.chromium.org/2502723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:359:
m_serviceRequests.remove(resolver);
On 2016/11/16 04:15:10, haraken wrote:
> 
> Nit: I'd prefer removing this at line 348 (to make onDetectBarcodes
re-entrant;
> although it shouldn't happen in reality).

Done.

[mcasas, 2016-11-16 05:25:53]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-11-16 05:25:54]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
xianglu@chromium.org, haraken@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2502723002/#ps200001
(title: "haraken@ comments")

[commit-bot: I haz the power, 2016-11-16 05:26:27]

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-11-16 08:33:12]

Description was changed from

==========
ShapeDetection: implement barcode detection, blink part

This CL introduces the QR/barcode detection API idls 
(BarcodeDetector.idl and DetectedBarcode.idl), and
the Blink-side implementation and mojo definition.

FaceDetector.cpp is almost emptied to become a 
storefront for ShapeDetector, where most of the code
is moved to and that is used for both Face and Barcode
detection.

LayoutTests and mock mojo implementation are adapted 
all over -- in particular, the tests under 
LayoutTests/shapedetection are 'parameterised'.

BUG=665150
==========

to

==========
ShapeDetection: implement barcode detection, blink part

This CL introduces the QR/barcode detection API idls 
(BarcodeDetector.idl and DetectedBarcode.idl), and
the Blink-side implementation and mojo definition.

FaceDetector.cpp is almost emptied to become a 
storefront for ShapeDetector, where most of the code
is moved to and that is used for both Face and Barcode
detection.

LayoutTests and mock mojo implementation are adapted 
all over -- in particular, the tests under 
LayoutTests/shapedetection are 'parameterised'.

BUG=665150
==========

[commit-bot: I haz the power, 2016-11-16 08:33:13]

Committed patchset #4 (id:200001)

[commit-bot: I haz the power, 2016-11-16 08:34:59]

Description was changed from

==========
ShapeDetection: implement barcode detection, blink part

This CL introduces the QR/barcode detection API idls 
(BarcodeDetector.idl and DetectedBarcode.idl), and
the Blink-side implementation and mojo definition.

FaceDetector.cpp is almost emptied to become a 
storefront for ShapeDetector, where most of the code
is moved to and that is used for both Face and Barcode
detection.

LayoutTests and mock mojo implementation are adapted 
all over -- in particular, the tests under 
LayoutTests/shapedetection are 'parameterised'.

BUG=665150
==========

to

==========
ShapeDetection: implement barcode detection, blink part

This CL introduces the QR/barcode detection API idls
(BarcodeDetector.idl and DetectedBarcode.idl), and
the Blink-side implementation and mojo definition.

FaceDetector.cpp is almost emptied to become a
storefront for ShapeDetector, where most of the code
is moved to and that is used for both Face and Barcode
detection.

LayoutTests and mock mojo implementation are adapted
all over -- in particular, the tests under
LayoutTests/shapedetection are 'parameterised'.

BUG=665150

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

[commit-bot: I haz the power, 2016-11-16 08:35:00]

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