Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2575943002:
Barcode Detection: Add |cornerPoints| to DetectedBarcode.idl (Closed)
|
Created:
4 years ago by xianglu Modified:
4 years ago Reviewers:
David Trainor- moved to gerrit, Reilly Grant (use Gerrit), Maria, haraken, mcasas, Tom Sepez CC:
chromium-reviews, qsr+mojo_chromium.org, Aaron Boodman, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, dglazkov+blink, blink-reviews, darin (slow to review), blink-reviews-api_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBarcode Detection: Add |cornerPoints| to DetectedBarcode.idl
This CL implements and wires |cornerPoints| field in DetectedBarcode.idl.
BUG=673052
Committed: https://crrev.com/e222cf23689ba10d8c7213591ff4bd6d6ab3c4bc
Cr-Commit-Position: refs/heads/master@{#439249}
Patch Set 1 #
Total comments: 10
Patch Set 2 : mcasas@ comments #
Total comments: 4
Patch Set 3 : reillyg@ comments #
Total comments: 4
Patch Set 4 : Correct coordinates (in clockwise direction starting with top-left) #
Total comments: 1
Messages
Total messages: 75 (60 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Description was changed from ========== Barcode Detection: Add |cornerPoints| field BUG=673052 ========== to ========== Barcode Detection: Add |cornerPoints| field to DetectedBarcode.idl BUG=673052 ==========
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
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:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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 #2 (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...
Patchset #1 (id:60001) 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:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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:120001) 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: + mcasas@chromium.org
PTAL.
Description was changed from ========== Barcode Detection: Add |cornerPoints| field to DetectedBarcode.idl BUG=673052 ========== to ========== This CL adds |cornerPoints| field to DetectedBarcode.idl and the corresponding implementation in Blink and Android. BUG=673052 ==========
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...
mcasas@chromium.org changed reviewers: + reillyg@chromium.org
looking good, a few comments. reillyg@ could you also take a look plz? https://codereview.chromium.org/2575943002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2575943002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:109: // Point[] from Android starts from top-left corner. No need to say "from Android" or, in any case, you should be more specific (like, from Barcode). Also: ".. in clockwise orientation" (orientation is important) https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/paint/invalidation/filter-repaint-accelerated-on-accelerated-filter-expected.txt (left): https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/filter-repaint-accelerated-on-accelerated-filter-expected.txt:36: } I'm not sure we want this file to be touched :-) probably a wrong rebase. https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-barcodedetection.js (right): https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/shapedetection/resources/mock-barcodedetection.js:37: { x : 1.0, y: 1.0} These corner points would not align with the previous |bounding_box|, which would not be a big issue, but would not make a box either, which would be wrong. Please write something more meaningful. https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/BarcodeDetector.cpp (right): https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/BarcodeDetector.cpp:63: } for (const auto& point: barcode->corner_points) { ... } https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.idl (right): https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.idl:13: // |x|, |y| refer to the coordinates of the bottom-left corner. Who are |x| and |y| inside |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...
Patchset #3 (id:180001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 #4 (id:220001) has been deleted
The CQ bit was unchecked by xianglu@chromium.org
Patchset #2 (id:160001) has been deleted
lgtm with a nit Please update the change description so that it contains the single-line subject as its first line. https://codereview.chromium.org/2575943002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-barcodedetection.js (right): https://codereview.chromium.org/2575943002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/shapedetection/resources/mock-barcodedetection.js:32: bounding_box: { x : 1.0, y: 1.0, width: 100.0, height: 100.0 }, s/x :/x:/g https://codereview.chromium.org/2575943002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/shapedetection/resources/mock-barcodedetection.js:36: { x : 101.0, y: 1.0}, { x: 101.0, y: 101.0 } ?
Description was changed from ========== This CL adds |cornerPoints| field to DetectedBarcode.idl and the corresponding implementation in Blink and Android. BUG=673052 ========== to ========== Barcode Detection: Add |cornerPoints| to DetectedBarcode.idl This CL implements and wires |cornerPoints| field in DetectedBarcode.idl. BUG=673052 ==========
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...
xianglu@chromium.org changed reviewers: + dtrainor@chromium.org, haraken@chromium.org, tsepez@chromium.org
dtrainor@: Please review changes in one java file: BarcodeDetectionImpl.java. haraken@: Please review changes in WebKit. tsepez@ mojom PTAL. https://codereview.chromium.org/2575943002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2575943002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:109: // Point[] from Android starts from top-left corner. On 2016/12/15 19:01:58, mcasas wrote: > No need to say "from Android" or, in any case, you should > be more specific (like, from Barcode). > Also: ".. in clockwise orientation" (orientation is important) Done. https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/paint/invalidation/filter-repaint-accelerated-on-accelerated-filter-expected.txt (left): https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/filter-repaint-accelerated-on-accelerated-filter-expected.txt:36: } On 2016/12/15 19:01:58, mcasas wrote: > I'm not sure we want this file to be touched :-) > probably a wrong rebase. Removed. https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-barcodedetection.js (right): https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/shapedetection/resources/mock-barcodedetection.js:37: { x : 1.0, y: 1.0} On 2016/12/15 19:01:58, mcasas wrote: > These corner points would not align with the > previous |bounding_box|, which would not be > a big issue, but would not make a box either, > which would be wrong. Please write something > more meaningful. Done. https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/BarcodeDetector.cpp (right): https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/BarcodeDetector.cpp:63: } On 2016/12/15 19:01:58, mcasas wrote: > for (const auto& point: barcode->corner_points) { > ... > } Done. https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.idl (right): https://codereview.chromium.org/2575943002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.idl:13: // |x|, |y| refer to the coordinates of the bottom-left corner. On 2016/12/15 19:01:58, mcasas wrote: > Who are |x| and |y| inside |boundingBox| ? I meant to say that boundingBox and cornerPoints shared the same setup. But it is actually DOMRect's feature anyway. Removed. https://codereview.chromium.org/2575943002/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-barcodedetection.js (right): https://codereview.chromium.org/2575943002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/shapedetection/resources/mock-barcodedetection.js:32: bounding_box: { x : 1.0, y: 1.0, width: 100.0, height: 100.0 }, On 2016/12/15 23:44:30, Reilly Grant wrote: > s/x :/x:/g Done. https://codereview.chromium.org/2575943002/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/shapedetection/resources/mock-barcodedetection.js:36: { x : 101.0, y: 1.0}, On 2016/12/15 23:44:30, Reilly Grant wrote: > { x: 101.0, y: 101.0 } ? Done.
lgtm with my comments addressed https://codereview.chromium.org/2575943002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2575943002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:107: for (int j = 0; j < 4; j++) { Either make a constant for these two 4s, or use here barcodeArray[i].cornerPoints.size(). https://codereview.chromium.org/2575943002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:111: barcodeArray[i].cornerPoints[j].y = corners[(j + 3) % 4].y; Barcode.cornerPoints [1] says: "4 corner points in clockwise direction starting with top-left." and our WICG spec defines those [2] " in clockwise direction and starting with top-left." They match each other, there's no need to rearrange the points, right? [1] https://developers.google.com/android/reference/com/google/android/gms/vision... [2] https://wicg.github.io/shape-detection-api/#barcode-detection-api https://codereview.chromium.org/2575943002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-barcodedetection.js (right): https://codereview.chromium.org/2575943002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/shapedetection/resources/mock-barcodedetection.js:37: { x: 101.0, y: 1.0} I just notice that it's not 100% clear in the definition of Point2D, but the origin of coordinates is the upper left corner, with x being the horizontal axis and y the vertical. With that, these points are ordered counterclockwise, so please order them like this: { x: 1.0, y: 1.0}, { x: 101.0, y: 1.0} { x: 101.0, y: 101.0}, { x: 1.0, y: 101.0}, https://codereview.chromium.org/2575943002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.idl (right): https://codereview.chromium.org/2575943002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/DetectedBarcode.idl:14: // 4 corner points in clockwise direction starting with bottom-left. Due to s/bottom-left/top-left/ [1] [1] https://wicg.github.io/shape-detection-api/#barcode-detection-api
WebKit 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
xianglu@chromium.org changed reviewers: + mariakhomenko@chromium.org
mariakhomenko@, java RS PTAL.
lgtm
lgtm % nit https://codereview.chromium.org/2575943002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2575943002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:109: barcodeArray[i].cornerPoints[j].x = corners[j].x; barcodeArray[i].cornerPoints[j].set(corners[j])?
The CQ bit was checked by xianglu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org, haraken@chromium.org, mcasas@chromium.org, dtrainor@chromium.org, mariakhomenko@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2575943002/#ps250010 (title: "dtrainor@ comment")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #5 (id:250010) has been deleted
The CQ bit was checked by xianglu@chromium.org
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": 260001, "attempt_start_ts": 1481933567784180,
"parent_rev": "4dd7f8b73501ad3d17d5ac95ee3de93c50587f9b", "commit_rev":
"653efd5638381b9f9aec033ec66fcc5e4be86d14"}
Message was sent while issue was closed.
Description was changed from ========== Barcode Detection: Add |cornerPoints| to DetectedBarcode.idl This CL implements and wires |cornerPoints| field in DetectedBarcode.idl. BUG=673052 ========== to ========== Barcode Detection: Add |cornerPoints| to DetectedBarcode.idl This CL implements and wires |cornerPoints| field in DetectedBarcode.idl. BUG=673052 Review-Url: https://codereview.chromium.org/2575943002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Barcode Detection: Add |cornerPoints| to DetectedBarcode.idl This CL implements and wires |cornerPoints| field in DetectedBarcode.idl. BUG=673052 Review-Url: https://codereview.chromium.org/2575943002 ========== to ========== Barcode Detection: Add |cornerPoints| to DetectedBarcode.idl This CL implements and wires |cornerPoints| field in DetectedBarcode.idl. BUG=673052 Committed: https://crrev.com/e222cf23689ba10d8c7213591ff4bd6d6ab3c4bc Cr-Commit-Position: refs/heads/master@{#439249} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e222cf23689ba10d8c7213591ff4bd6d6ab3c4bc Cr-Commit-Position: refs/heads/master@{#439249} |
