Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2369693002:
Shapedetection module: Blink side implementation (Closed)
|
Created:
4 years, 2 months ago by xianglu Modified:
4 years, 2 months ago CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), dglazkov+blink, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShapedetection 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
Committed: https://crrev.com/ad9ded8c22140567a8bb457f1e64904da7ab4a19
Cr-Commit-Position: refs/heads/master@{#422729}
Patch Set 1 : Refactor code into getSharedBufferHandle() && Update *-expected.txt #
Total comments: 26
Patch Set 2 : Naming and const correctness suggestions by mcasas@ #
Total comments: 10
Patch Set 3 : rockot@ comments #
Total comments: 14
Patch Set 4 : Code reformatting #
Total comments: 12
Patch Set 5 : mcasas@ comments #
Total comments: 5
Patch Set 6 : FaceDetector: remove normalization && use ScriptState in constructor #
Total comments: 1
Patch Set 7 : tests outside the [0, 1] range #Messages
Total messages: 95 (72 generated)
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== mock-shapedetection.js experimental! Added mojom service BUG= ========== to ========== Shapedetection module: Blink side implementation This CL implements mojom client on blink side. FaceDetector::detect() sends a request to the browser; FaceDetector::onDetectFace() receives a list of DOMRects as the result from Chrome. BUG=646083 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Shapedetection module: Blink side implementation This CL implements mojom client on blink side. FaceDetector::detect() sends a request to the browser; FaceDetector::onDetectFace() receives a list of DOMRects as the result from Chrome. BUG=646083 ========== to ========== Shapedetection module: Blink side implementation This CL implements mojom client on blink side. - FaceDetector::detect() sends a request to the browser; - FaceDetector::onDetectFace() receives a list of DOMRects from Chrome. A mock mojom server is added to test the connection. BUG=646083 TEST=Pass LayoutTest detectface.html ==========
Patchset #5 (id:80001) has been deleted
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Shapedetection module: Blink side implementation This CL implements mojom client on blink side. - FaceDetector::detect() sends a request to the browser; - FaceDetector::onDetectFace() receives a list of DOMRects from Chrome. A mock mojom server is added to test the connection. BUG=646083 TEST=Pass LayoutTest detectface.html ========== to ========== Shapedetection module: Blink side implementation This CL implements mojom client on blink side. - FaceDetector::detect() sends an image to the browser through mojo shared_buffer; - FaceDetector::onDetectFace() receives a list of boundingBoxes from Chrome. A mock mojom server is added to test the connection. BUG=646083 TEST=Pass LayoutTest detectface.html ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Shapedetection module: Blink side implementation This CL implements mojom client on blink side. - FaceDetector::detect() sends an image to the browser through mojo shared_buffer; - FaceDetector::onDetectFace() receives a list of boundingBoxes from Chrome. A mock mojom server is added to test the connection. BUG=646083 TEST=Pass LayoutTest detectface.html ========== to ========== Shapedetection module: Blink side implementation This CL implements mojom client on blink side. - FaceDetector::detect() sends an image to the browser through mojo shared_buffer; - FaceDetector::onDetectFace() receives a list of boundingBoxes from Chrome. A mock mojom server is added to test the connection. This CL also reduced the API surface by removing Detector and DetectedObject interface. BUG=646083 TEST=Pass LayoutTest detectface.html ==========
Description was changed from ========== Shapedetection module: Blink side implementation This CL implements mojom client on blink side. - FaceDetector::detect() sends an image to the browser through mojo shared_buffer; - FaceDetector::onDetectFace() receives a list of boundingBoxes from Chrome. A mock mojom server is added to test the connection. This CL also reduced the API surface by removing Detector and DetectedObject interface. BUG=646083 TEST=Pass LayoutTest detectface.html ========== to ========== 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 ==========
Patchset #7 (id:160001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Patchset #1 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
xianglu@chromium.org changed reviewers: + mcasas@chromium.org
ptal.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:220001) has been deleted
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A few minor comments. https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/detectface.html (right): https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Lay... 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/Lay... 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/Lay... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js (right): https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Lay... 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/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right): https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/pub... File third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom (right): https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/pub... 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.
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal. https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/detectface.html (right): https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Lay... 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/Lay... 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/Lay... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js (right): https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Lay... 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/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right): https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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/Sou... 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. Done. https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:120: } 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/pub... File third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom (right): https://codereview.chromium.org/2369693002/diff/260001/third_party/WebKit/pub... 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. Done.
xianglu@chromium.org changed reviewers: + esprehn@chromium.org
ptal.
xianglu@chromium.org changed reviewers: + rockot@chromium.org,
Hi Ken, please take a look at SharedBuffer in FaceDetector.cpp, shapedetection.mojom, mock-shapedetection.js and detectface.html. Thanks!
rockot@chromium.org changed reviewers: + rockot@chromium.org - rockot@chromium.org,
Generally looks good, a few comments https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js (right): https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/Lay... 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/Lay... 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/pub... File third_party/WebKit/public/BUILD.gn (right): https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/pub... 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/pub... File third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom (right): https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/pub... 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/pub... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Patchset #3 (id:300001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal. https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js (right): https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/Lay... 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/Lay... 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/pub... File third_party/WebKit/public/BUILD.gn (right): https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/pub... 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/pub... File third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom (right): https://codereview.chromium.org/2369693002/diff/280001/third_party/WebKit/pub... 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/pub... 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. Done.
lgtm
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:320001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xianglu@chromium.org changed reviewers: + tsepez@chromium.org
tsepez@, PTAL at public/platform/modules/shapedetection/shapedetection.mojom
looking good, a few comments. https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js (right): https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/Lay... 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/Lay... 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/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right): https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/Sou... 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/Sou... 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/pub... File third_party/WebKit/public/platform/modules/shapedetection/OWNERS (right): https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/shapedetection/OWNERS:2: xianglu@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/pub... File third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom (right): https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/pub... 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/pub... 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/.
mojom LGTM after reformatting to 80 cols.
mcasas@ ptal. https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js (right): https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/Lay... 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/Lay... 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/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right): https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/Sou... 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/Sou... 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. https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/shapedetection/OWNERS (right): https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/shapedetection/OWNERS:2: xianglu@chromium.org 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/pub... File third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom (right): https://codereview.chromium.org/2369693002/diff/340001/third_party/WebKit/pub... 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/pub... 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/. Done.
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with some comments/nits https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/DetectedFace.idl (right): https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/modules/shapedetection/DetectedObject.h (right): https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right): https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/pub... File third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom (right): https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:19: // frame_data contains tightly packed image pixels in ARGB32 format, Indent by two spaces.
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:400001) has been deleted
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/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right): https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Sou... 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/Sou... third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:123: DCHECK_LE(boundingBox->y + boundingBox->height, 1.0); ditto
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl (right): https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl:10: ConstructorCallWith=Document, Use ConstructorCallWith=ScriptState. We're unifying all information about script execution to ScriptState.
Patchset #6 (id:420001) has been deleted
esprehn@, haraken@, ptal. https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/DetectedFace.idl (right): https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/modules/shapedetection/DetectedObject.h (right): https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right): https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/pub... File third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom (right): https://codereview.chromium.org/2369693002/diff/360001/third_party/WebKit/pub... 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/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right): https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.idl (right): https://codereview.chromium.org/2369693002/diff/380001/third_party/WebKit/Sou... 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. Done.
lgtm https://codereview.chromium.org/2369693002/diff/440001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-shapedetection.js (right): https://codereview.chromium.org/2369693002/diff/440001/third_party/WebKit/Lay... 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.
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xianglu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, tsepez@chromium.org, mcasas@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2369693002/#ps460001 (title: "tests outside the [0, 1] range")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/ad9ded8c22140567a8bb457f1e64904da7ab4a19 Cr-Commit-Position: refs/heads/master@{#422729} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ad9ded8c22140567a8bb457f1e64904da7ab4a19 Cr-Commit-Position: refs/heads/master@{#422729}
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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. Update: This is http://crbug.com/652761, should be fixed by http://crrev.com/2389093003. |
