Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2502763005:
ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces (Closed)
|
Created:
4 years, 1 month ago by xianglu Modified:
4 years, 1 month ago CC:
Reilly Grant (use Gerrit), Aaron Boodman, abarth-chromium, agrieve+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces
This CL adds FaceDetectorOptions to FaceDetector constructor, and changed
mojo interface accordingly. It also adds two Layoutests to test correct
propagation of FaceDetectorOptions.
Naming style is fixed in shapedetection.mojom.
BUG=659139, 646083
TEST=
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html
third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html
Committed: https://crrev.com/ad251159eb951b5433a8dca6c1c4d2cce407b091
Cr-Commit-Position: refs/heads/master@{#433940}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Change naming in mojom #
Total comments: 1
Patch Set 3 : 80 cols #
Total comments: 3
Messages
Total messages: 55 (43 generated)
Description was changed from ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces Add FaceDetectorOptions to constructor. This CL also adds Layoutests to test correct construction of FaceDetector. BUG=659139, 646083 ========== to ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces Add FaceDetectorOptions to constructor. This CL also adds Layoutests to test correct construction of FaceDetector. BUG=659139, 646083 TEST+third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ==========
Description was changed from ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces Add FaceDetectorOptions to constructor. This CL also adds Layoutests to test correct construction of FaceDetector. BUG=659139, 646083 TEST+third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ========== to ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces Add FaceDetectorOptions to constructor. This CL also adds Layoutests to test correct construction of FaceDetector. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ==========
Description was changed from ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces Add FaceDetectorOptions to constructor. This CL also adds Layoutests to test correct construction of FaceDetector. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ========== to ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces Add FaceDetectorOptions to constructor. This CL also adds Layoutests to test correct construction of FaceDetector. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ==========
Description was changed from ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces Add FaceDetectorOptions to constructor. This CL also adds Layoutests to test correct construction of FaceDetector. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ========== to ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces This CL adds FaceDetectorOptions to FaceDetector constructor, and changed mojo interface accordingly. It also adds some Layoutests to test correct propagation of FaceDetectorOptions. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ==========
xianglu@chromium.org changed reviewers: + mcasas@chromium.org
Patchset #1 (id:1) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #2 (id:40001) 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:20001) has been deleted
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
Description was changed from ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces This CL adds FaceDetectorOptions to FaceDetector constructor, and changed mojo interface accordingly. It also adds some Layoutests to test correct propagation of FaceDetectorOptions. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ========== to ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces This CL adds FaceDetectorOptions to FaceDetector constructor, and changed mojo interface accordingly. It also adds two Layoutests to test correct propagation of FaceDetectorOptions. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal.
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_...)
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Patchset #1 (id:80001) 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...
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % comments https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html (right): https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html:18: // This test verifies that FaceDetector can be created with maxDetectedFaces nit: |maxDetectedFaces| and |fastMode| in l.25. https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html (right): https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html:30: }); nit: collapse l.27-30 ? https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right): https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:22: m_options = mojom::blink::FaceDetectorOptions::New(); Can you add this to the initialization list? I.e. FaceDetector(...) : m_options(mojom::blink::FaceDetectorOptions::New()) { ...} https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom (right): https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:27: bool fastMode; s/maxDetectedFaces/max_detected_faces/ s/fastMode/fast_mode/ This also applies to l.15, which came up in another review as having the incorrect case; you can use this to change that or a subsequent one, but don't forget it :)
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...
Implementation-wise modules/ LGTM (although Elliott might have thoughts on the API)
xianglu@chromium.org changed reviewers: + haraken@chromium.org, nick@chromium.org, tsepez@chromium.org
nick@ RS ptal on java file. tsepez@ RS ptal on mojom file. https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html (right): https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html:18: // This test verifies that FaceDetector can be created with maxDetectedFaces On 2016/11/21 23:19:26, mcasas wrote: > nit: |maxDetectedFaces| and |fastMode| in l.25. Done. https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html (right): https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html:30: }); On 2016/11/21 23:19:26, mcasas wrote: > nit: collapse l.27-30 ? Done. https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right): https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:22: m_options = mojom::blink::FaceDetectorOptions::New(); On 2016/11/21 23:19:26, mcasas wrote: > Can you add this to the initialization list? > I.e. > > FaceDetector(...) : m_options(mojom::blink::FaceDetectorOptions::New()) { ...} In fact |m_options| is a protected variable of base class. I can create a constructor in ShapeDetector in order to put it in the initialization list. But I think that would be a little complicated and less intuitive. https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom (right): https://codereview.chromium.org/2502763005/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:27: bool fastMode; On 2016/11/21 23:19:26, mcasas wrote: > s/maxDetectedFaces/max_detected_faces/ > s/fastMode/fast_mode/ > > This also applies to l.15, which came up in another > review as having the incorrect case; you can use > this to change that or a subsequent one, but don't > forget it :) Done.
RS LGTM https://codereview.chromium.org/2502763005/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom (right): https://codereview.chromium.org/2502763005/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:34: DetectFaces(handle<shared_buffer> frame_data, uint32 width, uint32 height, FaceDetectorOptions options) nit: 80 cols.
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
xianglu@chromium.org changed reviewers: + avi@chromium.org
avi@ RS ptal at one java file.
lgtm java stamp
Description was changed from ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces This CL adds FaceDetectorOptions to FaceDetector constructor, and changed mojo interface accordingly. It also adds two Layoutests to test correct propagation of FaceDetectorOptions. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ========== to ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces This CL adds FaceDetectorOptions to FaceDetector constructor, and changed mojo interface accordingly. It also adds two Layoutests to test correct propagation of FaceDetectorOptions. Naming style is fixed in shapedetection.mojom. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ==========
The CQ bit was checked by xianglu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org, tsepez@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2502763005/#ps160001 (title: "80 cols")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1479842009869520,
"parent_rev": "0aac4f8fbeafc90c8bc328d20f42664b0d3f75fe", "commit_rev":
"5e78cb8c9b31df10f1f17c3f8071bd9e2c97c524"}
Message was sent while issue was closed.
Description was changed from ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces This CL adds FaceDetectorOptions to FaceDetector constructor, and changed mojo interface accordingly. It also adds two Layoutests to test correct propagation of FaceDetectorOptions. Naming style is fixed in shapedetection.mojom. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ========== to ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces This CL adds FaceDetectorOptions to FaceDetector constructor, and changed mojo interface accordingly. It also adds two Layoutests to test correct propagation of FaceDetectorOptions. Naming style is fixed in shapedetection.mojom. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ==========
Message was sent while issue was closed.
Committed patchset #3 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces This CL adds FaceDetectorOptions to FaceDetector constructor, and changed mojo interface accordingly. It also adds two Layoutests to test correct propagation of FaceDetectorOptions. Naming style is fixed in shapedetection.mojom. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html ========== to ========== ShapeDetection: Add FaceDetectorOptions for fastMode and maxDetectedFaces This CL adds FaceDetectorOptions to FaceDetector constructor, and changed mojo interface accordingly. It also adds two Layoutests to test correct propagation of FaceDetectorOptions. Naming style is fixed in shapedetection.mojom. BUG=659139, 646083 TEST= third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html Committed: https://crrev.com/ad251159eb951b5433a8dca6c1c4d2cce407b091 Cr-Commit-Position: refs/heads/master@{#433940} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ad251159eb951b5433a8dca6c1c4d2cce407b091 Cr-Commit-Position: refs/heads/master@{#433940}
Message was sent while issue was closed.
reillyg@chromium.org changed reviewers: + reillyg@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2502763005/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html (right): https://codereview.chromium.org/2502763005/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/shapedetection/shapedetection-options.html:23: }) nit: the return is unnecessary, .then(detectorWithDefault => detectorWithDefault.detect(img)) https://codereview.chromium.org/2502763005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right): https://codereview.chromium.org/2502763005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:24: m_options->fast_mode = options.fastMode(); Pass options to the ShapeDetector constructor and unpack them into the Mojo struct there. https://codereview.chromium.org/2502763005/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom (right): https://codereview.chromium.org/2502763005/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom:35: FaceDetectorOptions options) nit: indentation
Message was sent while issue was closed.
lgtm |
