Shapedetection module: Blink side implementation (issue 2369693002 by xianglu@chromium.org)

0 views
Skip to first unread message

xia...@chromium.org

unread,
Sep 29, 2016, 5:51:37 PM9/29/16
to mca...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, a...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, aba...@chromium.org, har...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, da...@chromium.org, blink-re...@chromium.org
Reviewers: mcasas
CL: https://codereview.chromium.org/2369693002/

Message:
ptal.

Description:
Shapedetection module: Blink side implementation

This CL implements mojom client on blink side.
- FaceDetector::detect() sends an image to the browser through shared_buffer;
- FaceDetector::onDetectFace() receives a list of boundingBoxes from Chrome.
A mock mojom server is added to test the connection.

This CL also reduces the API surface by removing Detector and DetectedObject
interface, so that the hierarchy is simpler and cleaner.

BUG=646083
TEST=Pass LayoutTest detectface.html

Affected files (+279, -131 lines):
M content/public/app/mojo/content_browser_manifest.json
A third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation-expected.txt
M third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
A third_party/WebKit/LayoutTests/shapedetection/detectface.html
A third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-dedicated-worker-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-shared-worker-expected.txt
M third_party/WebKit/Source/modules/modules_idl_files.gni
M third_party/WebKit/Source/modules/shapedetection/BUILD.gn
M third_party/WebKit/Source/modules/shapedetection/DEPS
M third_party/WebKit/Source/modules/shapedetection/DetectedFace.h
M third_party/WebKit/Source/modules/shapedetection/DetectedFace.cpp
M third_party/WebKit/Source/modules/shapedetection/DetectedFace.idl
D third_party/WebKit/Source/modules/shapedetection/DetectedObject.h
D third_party/WebKit/Source/modules/shapedetection/DetectedObject.idl
M third_party/WebKit/Source/modules/shapedetection/Detector.h
D third_party/WebKit/Source/modules/shapedetection/Detector.idl
M third_party/WebKit/Source/modules/shapedetection/FaceDetector.h
M third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
M third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl
M third_party/WebKit/public/BUILD.gn
A third_party/WebKit/public/platform/modules/shapedetection/OWNERS
A third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom


mca...@chromium.org

unread,
Sep 29, 2016, 10:14:59 PM9/29/16
to xia...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, a...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, aba...@chromium.org, har...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, da...@chromium.org, blink-re...@chromium.org
A few minor comments.


https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/LayoutTests/shapedetection/detectface.html
File third_party/WebKit/LayoutTests/shapedetection/detectface.html
(right):

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/LayoutTests/shapedetection/detectface.html#newcode30
third_party/WebKit/LayoutTests/shapedetection/detectface.html:30:
uint32ArrayReceivedByMock = theMock.getFrameData();
s/uint32ArrayReceivedByMock/const receivedImage/ ?

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/LayoutTests/shapedetection/detectface.html#newcode32
third_party/WebKit/LayoutTests/shapedetection/detectface.html:32:
assert_equals(uint32ArrayReceivedByMock[0], 4278255360, "First pixel
received by mock should be green.");
const GREEN_PIXEL = 0xFFFF0000;
assert_equals(uint32ArrayReceivedByMock[0], GREEN_PIXEL, "pixel must be
green");

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
File
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
(right):

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js#newcode25
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js:25:
let mojoResult = mojo.mapBuffer(frame_data, 0 /*offest*/,
width*height*4/*size*/, 0/*flag*/);
s/offest/offset/

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
(right):

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode24
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:24:
ImageResource* imgResource = img->cachedImage();
ImageResource* const ?

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode30
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:30:
Image* blinkImage = imgResource->getImage();
Image* const ?

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode36
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:36:
sk_sp<SkImage> skImg = blinkImage->imageForCurrentFrame();
const sk_sp<SkImage> skiaImage ?

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode42
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:42:
SkImageInfo dstInfo = SkImageInfo::MakeN32(skImg->width(),
skImg->height(), skImg->alphaType());
const SkImageInfo skiaInfo ?

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode44
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:44:
uint32_t allocationSize = dstInfo.getSafeSize(dstInfo.minRowBytes());
const

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode49
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:49:
SkPixmap pixmap(dstInfo, mappingPtr.get(), dstInfo.minRowBytes());
const

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode56
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:56:
DCHECK_EQ(img->naturalHeight(), skImg->height());
Move these two contracts/preconditions to l.37,
i.e. as close as possible to |skImage| declaration
as possible.

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode100
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:100:
img->naturalWidth(), img->naturalHeight(),
Strange indenting: either all parameters are in the
same line, or these two should go on different lines.
Should be easier to solve after Friday when Blink
style converges to Cr style though.

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode120
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:120: }
Consider for-range loop and const auto&:

HeapVector<Member<DOMRect>> detectedFaces;
for (const auto& boundingBox : boundingBoxes) {
detectedFaces.append(DOMRect::create(boundingBox.x,
boundingBox.y, boundingBox.width, boundingBox.height));
}

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
File
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
(right):

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom#newcode15
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:15:
interface ShapeDetection {
Add a comment with the link to the Spec URL.
Same with BoundingBox.

https://codereview.chromium.org/2369693002/

xia...@chromium.org

unread,
Sep 30, 2016, 12:37:41 PM9/30/16
to mca...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, a...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, aba...@chromium.org, har...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, da...@chromium.org, blink-re...@chromium.org
ptal.



https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/LayoutTests/shapedetection/detectface.html
File third_party/WebKit/LayoutTests/shapedetection/detectface.html
(right):

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/LayoutTests/shapedetection/detectface.html#newcode30
third_party/WebKit/LayoutTests/shapedetection/detectface.html:30:
uint32ArrayReceivedByMock = theMock.getFrameData();
On 2016/09/30 02:14:59, mcasas wrote:
> s/uint32ArrayReceivedByMock/const receivedImage/ ?

Done.


https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/LayoutTests/shapedetection/detectface.html#newcode32
third_party/WebKit/LayoutTests/shapedetection/detectface.html:32:
assert_equals(uint32ArrayReceivedByMock[0], 4278255360, "First pixel
received by mock should be green.");
On 2016/09/30 02:14:59, mcasas wrote:
> const GREEN_PIXEL = 0xFFFF0000;
> assert_equals(uint32ArrayReceivedByMock[0], GREEN_PIXEL, "pixel must
be green");

Done.


https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
File
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
(right):

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js#newcode25
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js:25:
let mojoResult = mojo.mapBuffer(frame_data, 0 /*offest*/,
width*height*4/*size*/, 0/*flag*/);
On 2016/09/30 02:14:59, mcasas wrote:
> s/offest/offset/

Done.


https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
(right):

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode24
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:24:
ImageResource* imgResource = img->cachedImage();
On 2016/09/30 02:14:59, mcasas wrote:
> ImageResource* const ?

Done.


https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode30
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:30:
Image* blinkImage = imgResource->getImage();
On 2016/09/30 02:14:59, mcasas wrote:
> Image* const ?

Done.


https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode36
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:36:
sk_sp<SkImage> skImg = blinkImage->imageForCurrentFrame();
On 2016/09/30 02:14:59, mcasas wrote:
> const sk_sp<SkImage> skiaImage ?

Done.


https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode42
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:42:
SkImageInfo dstInfo = SkImageInfo::MakeN32(skImg->width(),
skImg->height(), skImg->alphaType());
On 2016/09/30 02:14:59, mcasas wrote:
> const SkImageInfo skiaInfo ?

Done.


https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode44
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:44:
uint32_t allocationSize = dstInfo.getSafeSize(dstInfo.minRowBytes());
On 2016/09/30 02:14:59, mcasas wrote:
> const

Done.


https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode49
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:49:
SkPixmap pixmap(dstInfo, mappingPtr.get(), dstInfo.minRowBytes());
On 2016/09/30 02:14:59, mcasas wrote:
> const

Done.


https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode56
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:56:
DCHECK_EQ(img->naturalHeight(), skImg->height());
On 2016/09/30 02:14:59, mcasas wrote:
> Move these two contracts/preconditions to l.37,
> i.e. as close as possible to |skImage| declaration
> as possible.

Done.


https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode100
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:100:
img->naturalWidth(), img->naturalHeight(),
On 2016/09/30 02:14:59, mcasas wrote:
> Strange indenting: either all parameters are in the
> same line, or these two should go on different lines.
> Should be easier to solve after Friday when Blink
> style converges to Cr style though.


On 2016/09/30 02:14:59, mcasas wrote:
> Consider for-range loop and const auto&:
>
> HeapVector<Member<DOMRect>> detectedFaces;
> for (const auto& boundingBox : boundingBoxes) {
> detectedFaces.append(DOMRect::create(boundingBox.x,
boundingBox.y,
> boundingBox.width, boundingBox.height));
> }

There is no begin() function in mojo::WTFArray.


https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
File
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
(right):

https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom#newcode15
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:15:
interface ShapeDetection {
On 2016/09/30 02:14:59, mcasas wrote:
> Add a comment with the link to the Spec URL.
> Same with BoundingBox.

xia...@chromium.org

unread,
Sep 30, 2016, 12:53:11 PM9/30/16
to mca...@chromium.org, esp...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, a...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, aba...@chromium.org, har...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, da...@chromium.org, blink-re...@chromium.org

xia...@chromium.org

unread,
Sep 30, 2016, 1:01:51 PM9/30/16
to mca...@chromium.org, esp...@chromium.org, roc...@chromium.org, chromium...@chromium.org, qsr+...@chromium.org, a...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, aba...@chromium.org, har...@chromium.org, dglazko...@chromium.org, blink-...@chromium.org, da...@chromium.org, blink-re...@chromium.org
Hi Ken, please take a look at SharedBuffer in FaceDetector.cpp,
shapedetection.mojom, mock-shapedetection.js and detectface.html. Thanks!

https://codereview.chromium.org/2369693002/

roc...@chromium.org

unread,
Sep 30, 2016, 2:05:45 PM9/30/16
to xia...@chromium.org, esp...@chromium.org, mca...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, har...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Generally looks good, a few comments


https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
File
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
(right):

https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js#newcode25
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js:25:
let mojoResult = mojo.mapBuffer(frame_data, 0 /*offset*/,
width*height*4/*size*/, 0/*flag*/);
nit: the inline comments are probably unnecessary

also nit: just result instead of mojoResult?

https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js#newcode28
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js:28:
{ boundingBoxes :
style nit: I don't think this is correct indenting, though I might be
wrong? I'd expect:

return Promise.resolve({
boundingBoxes: [
{ ... },
{ ... },
]
});

https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/public/BUILD.gn
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/public/BUILD.gn#newcode642
third_party/WebKit/public/BUILD.gn:642:
"platform/modules/shapedetection/shapedetection.mojom",
Please add this to new_wrapper_types_mojo_bindings instead.

https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
File
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
(right):

https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom#newcode9
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:9:
struct BoundingBox {
Note that ui/gfx/geometry/mojo/geometry.mojom already defines some
standard rectangle types (Rect for ints and RectF for floats.) I think
we should avoid introducing more in blink mojom.

You can

import "ui/gfx/geomoetry/mojo/geometry.mojom";

in this file and your message can return an array<gfx.mojom.RectF>
instead.

The bindings target will then also need a public_deps on
//ui/gfx/geometry/mojo.

https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom#newcode18
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:18:
DetectFace(handle<shared_buffer> frame_data, uint32 width, uint32
height) => (array<BoundingBox> boundingBoxes);
nit: Please document the format of frame_data. Specifically it's
important to convey the pixel format, the row/column ordering, and that
the data is tightly packed, since all of these things frequently vary in
image buffer formats.

https://codereview.chromium.org/2369693002/

xia...@chromium.org

unread,
Oct 3, 2016, 2:12:08 PM10/3/16
to roc...@chromium.org, esp...@chromium.org, mca...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, har...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
ptal.



https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
File
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
(right):

https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js#newcode25
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js:25:
let mojoResult = mojo.mapBuffer(frame_data, 0 /*offset*/,
width*height*4/*size*/, 0/*flag*/);
On 2016/09/30 18:05:23, Ken Rockot wrote:
> nit: the inline comments are probably unnecessary
>
> also nit: just result instead of mojoResult?

Done.


https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js#newcode28
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js:28:
{ boundingBoxes :
On 2016/09/30 18:05:23, Ken Rockot wrote:
> style nit: I don't think this is correct indenting, though I might be
wrong? I'd
> expect:
>
> return Promise.resolve({
> boundingBoxes: [
> { ... },
> { ... },
> ]
> });

Done.


https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/public/BUILD.gn
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/public/BUILD.gn#newcode642
third_party/WebKit/public/BUILD.gn:642:
"platform/modules/shapedetection/shapedetection.mojom",
On 2016/09/30 18:05:23, Ken Rockot wrote:
> Please add this to new_wrapper_types_mojo_bindings instead.

Done.


https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
File
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
(right):

https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom#newcode9
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:9:
struct BoundingBox {
On 2016/09/30 18:05:24, Ken Rockot wrote:
> Note that ui/gfx/geometry/mojo/geometry.mojom already defines some
standard
> rectangle types (Rect for ints and RectF for floats.) I think we
should avoid
> introducing more in blink mojom.
>
> You can
>
> import "ui/gfx/geomoetry/mojo/geometry.mojom";
>
> in this file and your message can return an array<gfx.mojom.RectF>
instead.
>
> The bindings target will then also need a public_deps on
//ui/gfx/geometry/mojo.

Done.


https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom#newcode18
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:18:
DetectFace(handle<shared_buffer> frame_data, uint32 width, uint32
height) => (array<BoundingBox> boundingBoxes);
On 2016/09/30 18:05:24, Ken Rockot wrote:
> nit: Please document the format of frame_data. Specifically it's
important to
> convey the pixel format, the row/column ordering, and that the data is
tightly
> packed, since all of these things frequently vary in image buffer
formats.

roc...@chromium.org

unread,
Oct 3, 2016, 2:20:39 PM10/3/16
to xia...@chromium.org, esp...@chromium.org, mca...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, har...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

xia...@chromium.org

unread,
Oct 3, 2016, 3:59:30 PM10/3/16
to roc...@chromium.org, esp...@chromium.org, mca...@chromium.org, tse...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, har...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
tsepez@,
PTAL at public/platform/modules/shapedetection/shapedetection.mojom

https://codereview.chromium.org/2369693002/

mca...@chromium.org

unread,
Oct 3, 2016, 5:02:47 PM10/3/16
to xia...@chromium.org, roc...@chromium.org, esp...@chromium.org, tse...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, har...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
looking good, a few comments.


https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
File
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
(right):

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js#newcode20
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js:20:
this.stub_ = connection.bindHandleToStub(pipe,
shapeDetection.ShapeDetection);
Try to stick to 80 characters despite not being
compulsory in JS files. For instance here, line
up |pipe| and |shapeDetection.ShapeDetection|.

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js#newcode32
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js:32:
{ x : 3.0, y: 3.0, width: 300.0, height: 300.0 },
A bounding box should be a normalized coordinate to
simplify its manipulation, IOW, each field should be
in [0.0 ,1.0]. This is in the process of being included
in the Spec.

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
(right):

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode49
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:49:
const mojo::ScopedSharedBufferMapping mappingPtr =
sharedBufferHandle->Map(allocationSize);
Naming: don't use abbreviations like Img for
img, and don't add suffixes to define a
variable data type:

s/imgResource/imageResource/
s/skiaImg/image/
s/mappingPtr/mappedBuffer/

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode118
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:118:
boundingBox->width, boundingBox->height));
These two lines need clang-formatting.

Also, after the big reformatting of last Fri,
we should be using 2 space indenting, so
please rebase your patch against master
to update the indenting.

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/public/platform/modules/shapedetection/OWNERS
File third_party/WebKit/public/platform/modules/shapedetection/OWNERS
(right):

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/public/platform/modules/shapedetection/OWNERS#newcode2
third_party/WebKit/public/platform/modules/shapedetection/OWNERS:2:
xia...@chromium.org
Since this folder has no other files than the
.mojom file, we don't need to be in the OWNERS
and, since public/platform/OWNERS specifies:

per-file *.mojom=file://ipc/SECURITY_OWNERS

I think we can remove this file altogether
(sorry I asked you to add it ;P )

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
File
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
(right):

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom#newcode1
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
s/2015/2016/

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom#newcode12
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:12:
// a wrapper struct, so that gfx.mojom.RectF will not be directly
referenced // inside blink, and browser can still use mojo geometries.
Reflow these comments (this paragraph and l. 17) to 80
characters.

Also s/use mojo geometries/use gfx types/.

https://codereview.chromium.org/2369693002/

tse...@chromium.org

unread,
Oct 3, 2016, 5:54:32 PM10/3/16
to xia...@chromium.org, roc...@chromium.org, esp...@chromium.org, mca...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, har...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
mojom LGTM after reformatting to 80 cols.

https://codereview.chromium.org/2369693002/

xia...@chromium.org

unread,
Oct 3, 2016, 6:00:06 PM10/3/16
to roc...@chromium.org, esp...@chromium.org, mca...@chromium.org, tse...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, har...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
mcasas@ ptal.



https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
File
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
(right):

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js#newcode20
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js:20:
this.stub_ = connection.bindHandleToStub(pipe,
shapeDetection.ShapeDetection);
On 2016/10/03 21:02:46, mcasas wrote:
> Try to stick to 80 characters despite not being
> compulsory in JS files. For instance here, line
> up |pipe| and |shapeDetection.ShapeDetection|.

Done.


https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js#newcode32
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js:32:
{ x : 3.0, y: 3.0, width: 300.0, height: 300.0 },
On 2016/10/03 21:02:46, mcasas wrote:
> A bounding box should be a normalized coordinate to
> simplify its manipulation, IOW, each field should be
> in [0.0 ,1.0]. This is in the process of being included
> in the Spec.

Done.


https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
(right):

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode49
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:49:
const mojo::ScopedSharedBufferMapping mappingPtr =
sharedBufferHandle->Map(allocationSize);
On 2016/10/03 21:02:46, mcasas wrote:
> Naming: don't use abbreviations like Img for
> img, and don't add suffixes to define a
> variable data type:
>
> s/imgResource/imageResource/
> s/skiaImg/image/
> s/mappingPtr/mappedBuffer/

Done.


https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode118
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:118:
boundingBox->width, boundingBox->height));
On 2016/10/03 21:02:46, mcasas wrote:
> These two lines need clang-formatting.
>
> Also, after the big reformatting of last Fri,
> we should be using 2 space indenting, so
> please rebase your patch against master
> to update the indenting.

Done.
On 2016/10/03 21:02:46, mcasas wrote:
> Since this folder has no other files than the
> .mojom file, we don't need to be in the OWNERS
> and, since public/platform/OWNERS specifies:
>
> per-file *.mojom=file://ipc/SECURITY_OWNERS
>
> I think we can remove this file altogether
> (sorry I asked you to add it ;P )

Done.


https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
File
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
(right):

https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom#newcode1
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/10/03 21:02:46, mcasas wrote:
> s/2015/2016/

Done.


https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom#newcode12
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:12:
// a wrapper struct, so that gfx.mojom.RectF will not be directly
referenced // inside blink, and browser can still use mojo geometries.
On 2016/10/03 21:02:47, mcasas wrote:
> Reflow these comments (this paragraph and l. 17) to 80
> characters.
>
> Also s/use mojo geometries/use gfx types/.

mca...@chromium.org

unread,
Oct 3, 2016, 6:18:09 PM10/3/16
to xia...@chromium.org, roc...@chromium.org, esp...@chromium.org, tse...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, har...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
lgtm with some comments/nits


https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/DetectedFace.idl
File third_party/WebKit/Source/modules/shapedetection/DetectedFace.idl
(right):

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/DetectedFace.idl#newcode11
third_party/WebKit/Source/modules/shapedetection/DetectedFace.idl:11: //
TODO(xianglu): Implement missing fields. https://crbug.com/646083
Indent two spaces right.

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/DetectedObject.h
File third_party/WebKit/Source/modules/shapedetection/DetectedObject.h
(right):

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/DetectedObject.h#newcode3
third_party/WebKit/Source/modules/shapedetection/DetectedObject.h:3: //
found in the LICENSE file.
This file should be deleted, right?

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
(right):

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode32
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:32:
DLOG(ERROR) << "Failed to convert ImageSource to blink::Image.";
nit: just for cases like this, we can also consider:

DLOG_IF(ERROR, !blinkImage) << "Failed...";
if (!blinkImage)
return mojo::ScopedSharedBufferHandle();

which allows us to spare a line of text ! :)

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode93
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:93: //
TODO(xianglu): Add security check when the spec is ready.
https://crbug.com/646083
nit: reflow this comment line to 80 chars wide plz.

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode121
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:121:
detectedFaces.append(DOMRect::create(boundingBox->x, boundingBox->y,
nit: consider adding here a contract on the bounding box:
DCHECK_LE(boundingBox->x + boundingBox->width, 1.0);
DCHECK_LE(boundingBox->y + boundingBox->height, 1.0);

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
File
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
(right):

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom#newcode19
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:19:
// frame_data contains tightly packed image pixels in ARGB32 format,
Indent by two spaces.

https://codereview.chromium.org/2369693002/

esp...@chromium.org

unread,
Oct 3, 2016, 7:59:38 PM10/3/16
to xia...@chromium.org, mca...@chromium.org, roc...@chromium.org, tse...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, har...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Hmm, I'm not sure I understand those asserts. You assert that x + width <= 1 and
y + height <= 1, but DOMRects are coordinates in pixels, so this means your
results are always in the top 1x1 block. What are you trying to assert here?


https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
(right):

https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode122
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:122:

DCHECK_LE(boundingBox->x + boundingBox->width, 1.0);
what's the reason for asserting that the x + width is less than or equal
to 1? Doesn't that mean the detected faces must be at 1x1 ? I'm not sure
what this assert is doing.

https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode123
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:123:

DCHECK_LE(boundingBox->y + boundingBox->height, 1.0);

har...@chromium.org

unread,
Oct 3, 2016, 8:03:55 PM10/3/16
to xia...@chromium.org, esp...@chromium.org, mca...@chromium.org, roc...@chromium.org, tse...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl
(right):

https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl#newcode10
third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl:10:
ConstructorCallWith=Document,

Use ConstructorCallWith=ScriptState. We're unifying all information
about script execution to ScriptState.

https://codereview.chromium.org/2369693002/

xia...@chromium.org

unread,
Oct 3, 2016, 8:58:26 PM10/3/16
to esp...@chromium.org, mca...@chromium.org, roc...@chromium.org, tse...@chromium.org, har...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, har...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
esprehn@, haraken@,
ptal.



https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/DetectedFace.idl
File third_party/WebKit/Source/modules/shapedetection/DetectedFace.idl
(right):

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/DetectedFace.idl#newcode11
third_party/WebKit/Source/modules/shapedetection/DetectedFace.idl:11: //
TODO(xianglu): Implement missing fields. https://crbug.com/646083
On 2016/10/03 22:18:09, mcasas wrote:
> Indent two spaces right.

Done.


https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/DetectedObject.h
File third_party/WebKit/Source/modules/shapedetection/DetectedObject.h
(right):

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/DetectedObject.h#newcode3
third_party/WebKit/Source/modules/shapedetection/DetectedObject.h:3: //
found in the LICENSE file.
On 2016/10/03 22:18:09, mcasas wrote:
> This file should be deleted, right?

Done.


https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
(right):

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode32
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:32:
DLOG(ERROR) << "Failed to convert ImageSource to blink::Image.";
On 2016/10/03 22:18:09, mcasas wrote:
> nit: just for cases like this, we can also consider:
>
> DLOG_IF(ERROR, !blinkImage) << "Failed...";
> if (!blinkImage)
> return mojo::ScopedSharedBufferHandle();
>
> which allows us to spare a line of text ! :)

Yeah. But in that way the message will be too long to fit in the same
line.


https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode93
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:93: //
TODO(xianglu): Add security check when the spec is ready.
https://crbug.com/646083
On 2016/10/03 22:18:09, mcasas wrote:
> nit: reflow this comment line to 80 chars wide plz.

Done.


https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode121
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:121:
detectedFaces.append(DOMRect::create(boundingBox->x, boundingBox->y,
On 2016/10/03 22:18:09, mcasas wrote:
> nit: consider adding here a contract on the bounding box:
> DCHECK_LE(boundingBox->x + boundingBox->width, 1.0);
> DCHECK_LE(boundingBox->y + boundingBox->height, 1.0);

Done.


https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
File
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
(right):

https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom#newcode19
third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:19:
// frame_data contains tightly packed image pixels in ARGB32 format,
On 2016/10/03 22:18:09, mcasas wrote:
> Indent by two spaces.

Done.


https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp
(right):

https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp#newcode122
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:122:
DCHECK_LE(boundingBox->x + boundingBox->width, 1.0);
On 2016/10/03 23:59:37, esprehn wrote:
> what's the reason for asserting that the x + width is less than or
equal to 1?
> Doesn't that mean the detected faces must be at 1x1 ? I'm not sure
what this
> assert is doing.

Removed.


https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl
(right):

https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl#newcode10
third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl:10:
ConstructorCallWith=Document,
On 2016/10/04 00:03:54, haraken wrote:
>
> Use ConstructorCallWith=ScriptState. We're unifying all information
about script
> execution to ScriptState.

esp...@chromium.org

unread,
Oct 3, 2016, 9:01:44 PM10/3/16
to xia...@chromium.org, har...@chromium.org, mca...@chromium.org, roc...@chromium.org, tse...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, har...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
lgtm




https://codereview.chromium.org/2369693002/diff/440001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
File
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js
(right):

https://codereview.chromium.org/2369693002/diff/440001/third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js#newcode33
third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js:33:
{ x : 0.0, y: 0.0, width: 0.8, height: 0.5 },
can we get some tests outside the [0, 1] range? You can do it in a
follow up CL though.

https://codereview.chromium.org/2369693002/

har...@chromium.org

unread,
Oct 3, 2016, 10:15:33 PM10/3/16
to xia...@chromium.org, esp...@chromium.org, mca...@chromium.org, roc...@chromium.org, tse...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 4, 2016, 3:18:44 AM10/4/16
to xia...@chromium.org, esp...@chromium.org, har...@chromium.org, mca...@chromium.org, roc...@chromium.org, tse...@chromium.org, commi...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 4, 2016, 3:24:36 AM10/4/16
to xia...@chromium.org, esp...@chromium.org, har...@chromium.org, mca...@chromium.org, roc...@chromium.org, tse...@chromium.org, commi...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Committed patchset #7 (id:460001)

https://codereview.chromium.org/2369693002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 4, 2016, 3:26:23 AM10/4/16
to xia...@chromium.org, esp...@chromium.org, har...@chromium.org, mca...@chromium.org, roc...@chromium.org, tse...@chromium.org, commi...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Patchset 7 (id:??) landed as
https://crrev.com/ad9ded8c22140567a8bb457f1e64904da7ab4a19
Cr-Commit-Position: refs/heads/master@{#422729}

https://codereview.chromium.org/2369693002/

qyea...@chromium.org

unread,
Oct 4, 2016, 12:27:42 PM10/4/16
to xia...@chromium.org, esp...@chromium.org, har...@chromium.org, mca...@chromium.org, roc...@chromium.org, tse...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
On 2016/10/04 at 07:26:23, commit-bot wrote:
> Patchset 7 (id:??) landed as
https://crrev.com/ad9ded8c22140567a8bb457f1e64904da7ab4a19
> Cr-Commit-Position: refs/heads/master@{#422729}

Note, it looks like some builders on chromium.webkit may be failing
shapedetection/detectface.html after this CL. But not all; notably, linux is
passing.

https://codereview.chromium.org/2369693002/

qyea...@chromium.org

unread,
Oct 4, 2016, 5:51:32 PM10/4/16
to xia...@chromium.org, esp...@chromium.org, har...@chromium.org, mca...@chromium.org, roc...@chromium.org, tse...@chromium.org, a...@chromium.org, aba...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, chromium...@chromium.org, da...@chromium.org, dglazko...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
On 2016/10/04 at 16:27:42, qyearsley wrote:
> On 2016/10/04 at 07:26:23, commit-bot wrote:
> > Patchset 7 (id:??) landed as
https://crrev.com/ad9ded8c22140567a8bb457f1e64904da7ab4a19
> > Cr-Commit-Position: refs/heads/master@{#422729}
>
> Note, it looks like some builders on chromium.webkit may be failing
shapedetection/detectface.html after this CL. But not all; notably, linux is
passing.

Reply all
Reply to author
Forward
0 new messages