Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2711893003:
Shape detection: moar content_browsertests (face, qr/barcode) (Closed)
DescriptionShape detection: moar content_browsertests (face, qr/barcode)
In the spirit of "if it can be tested, it ought to be tested", this
CL cleans up and expands the a few tests related to Shape Detection.
- the current content_browsertest, which on ToT is extended to
work on Mac and cleaned up (generalized and parameterized, variable
names clarified).
- the test face is changed to a smaller, public domain one of size
6182B (~1.4K smaller).
- Also corrected names: s/shapedetection/shape_detection/
- QR/barcode and text detection in Android is based on coreGMS,
so need to be tested under chrome/. A new ShapeDetectionTest.java
is added for that. To use coreGMS, we have to guarantee that
StrictMode.allowThreadDiskWrites().
BUG=665150, 676124
Review-Url: https://codereview.chromium.org/2711893003
Cr-Commit-Position: refs/heads/master@{#453818}
Committed: https://chromium.googlesource.com/chromium/src/+/04e8ca70316308763a6f51d1ee11960819887f7e
Patch Set 1 #
Total comments: 10
Patch Set 2 : reillyg@s comments #
Total comments: 11
Patch Set 3 : mariakhomenko@ comments and follow-up reillyg@s comment #Patch Set 4 : Make initialization of std::vector<std::vector<float>> explicit to avoid 'error: chosen constructor… #Messages
Total messages: 46 (29 generated)
Description was changed from ========== Shape detection: moar content_browsertests (face, qr/barcode) wip BUG=665150, 676124 ========== to ========== Shape detection: moar content_browsertests (face, qr/barcode) In the spirit of "if it can be tested, it ought to be tested", this CL cleans up and expands the current Face detection content_browsertest to test for QR codes as well. For that, the test is generalized and parameterized. A new QR code image is added (self generated, says "https://chromium.org"), 412 bytes, and the face is changed to a smaller, public domain one of size 6182B (~1K smaller). BUG=665150, 676124 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Shape detection: moar content_browsertests (face, qr/barcode) In the spirit of "if it can be tested, it ought to be tested", this CL cleans up and expands the current Face detection content_browsertest to test for QR codes as well. For that, the test is generalized and parameterized. A new QR code image is added (self generated, says "https://chromium.org"), 412 bytes, and the face is changed to a smaller, public domain one of size 6182B (~1K smaller). BUG=665150, 676124 ========== to ========== Shape detection: moar content_browsertests (face, qr/barcode) In the spirit of "if it can be tested, it ought to be tested", this CL cleans up and expands the a few tests related to Shape Detection. - the current content_browsertest, which on ToT is extended to work on Mac and cleaned up (generalized and parameterized, variable names clarified). - the test face is changed to a smaller, public domain one of size 6182B (~1.5K smaller). - QR/barcode and text detection in Android is based on coreGMS, so need to be tested under chrome/. A new ShapeDetectionTest.java is added for that. To use coreGMS, we have to guarantee that StrictMode.allowThreadDiskWrites(). BUG=665150, 676124 ==========
Description was changed from ========== Shape detection: moar content_browsertests (face, qr/barcode) In the spirit of "if it can be tested, it ought to be tested", this CL cleans up and expands the a few tests related to Shape Detection. - the current content_browsertest, which on ToT is extended to work on Mac and cleaned up (generalized and parameterized, variable names clarified). - the test face is changed to a smaller, public domain one of size 6182B (~1.5K smaller). - QR/barcode and text detection in Android is based on coreGMS, so need to be tested under chrome/. A new ShapeDetectionTest.java is added for that. To use coreGMS, we have to guarantee that StrictMode.allowThreadDiskWrites(). BUG=665150, 676124 ========== to ========== Shape detection: moar content_browsertests (face, qr/barcode) In the spirit of "if it can be tested, it ought to be tested", this CL cleans up and expands the a few tests related to Shape Detection. - the current content_browsertest, which on ToT is extended to work on Mac and cleaned up (generalized and parameterized, variable names clarified). - the test face is changed to a smaller, public domain one of size 6182B (~1.4K smaller). - QR/barcode and text detection in Android is based on coreGMS, so need to be tested under chrome/. A new ShapeDetectionTest.java is added for that. To use coreGMS, we have to guarantee that StrictMode.allowThreadDiskWrites(). BUG=665150, 676124 ==========
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #1 (id:120001) has been deleted
Description was changed from ========== Shape detection: moar content_browsertests (face, qr/barcode) In the spirit of "if it can be tested, it ought to be tested", this CL cleans up and expands the a few tests related to Shape Detection. - the current content_browsertest, which on ToT is extended to work on Mac and cleaned up (generalized and parameterized, variable names clarified). - the test face is changed to a smaller, public domain one of size 6182B (~1.4K smaller). - QR/barcode and text detection in Android is based on coreGMS, so need to be tested under chrome/. A new ShapeDetectionTest.java is added for that. To use coreGMS, we have to guarantee that StrictMode.allowThreadDiskWrites(). BUG=665150, 676124 ========== to ========== Shape detection: moar content_browsertests (face, qr/barcode) In the spirit of "if it can be tested, it ought to be tested", this CL cleans up and expands the a few tests related to Shape Detection. - the current content_browsertest, which on ToT is extended to work on Mac and cleaned up (generalized and parameterized, variable names clarified). - the test face is changed to a smaller, public domain one of size 6182B (~1.4K smaller). - Also corrected names: s/shapedetection/shape_detection/ - QR/barcode and text detection in Android is based on coreGMS, so need to be tested under chrome/. A new ShapeDetectionTest.java is added for that. To use coreGMS, we have to guarantee that StrictMode.allowThreadDiskWrites(). BUG=665150, 676124 ==========
mcasas@chromium.org changed reviewers: + mariakhomenko@chromium.org, reillyg@chromium.org
mariakhomenko@ PTAL at the chrome/ android tests reillyg@ PTAL at content/ test changes
lgtm with nits https://codereview.chromium.org/2711893003/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java (right): https://codereview.chromium.org/2711893003/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java:25: * Testing of the Shape Detection API. This API has three parts: QR/Barcodes, nit: extra indentation here https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... File content/browser/shape_detection/shape_detection_browsertest.cc (right): https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... content/browser/shape_detection/shape_detection_browsertest.cc:75: std::vector<std::vector<float>> detected_bounding_boxes = {}; Why initialize this with {}? https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... content/browser/shape_detection/shape_detection_browsertest.cc:98: // TODO(xianglu): Enable the test on other platforms. https://crbug.com/659138 nit: TODO(https://crbug.com/659138) https://codereview.chromium.org/2711893003/diff/160001/content/test/data/medi... File content/test/data/media/shape_detection_test.html (right): https://codereview.chromium.org/2711893003/diff/160001/content/test/data/medi... content/test/data/media/shape_detection_test.html:31: console.log('something failed'); These logs seem like leftovers from development. That said, logging |error.message| might be useful.
mcasas@chromium.org changed reviewers: + avi@chromium.org
mcasas@chromium.org changed reviewers: + avi@chromium.org
https://codereview.chromium.org/2711893003/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java (right): https://codereview.chromium.org/2711893003/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java:25: * Testing of the Shape Detection API. This API has three parts: QR/Barcodes, On 2017/02/28 00:35:11, Reilly Grant wrote: > nit: extra indentation here It's supposed to be like that when using JavaDocs [1] see e.g. [2] [1] https://source.android.com/source/code-style.html#use-javadoc-standard-comments [2] https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... File content/browser/shape_detection/shape_detection_browsertest.cc (right): https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... content/browser/shape_detection/shape_detection_browsertest.cc:75: std::vector<std::vector<float>> detected_bounding_boxes = {}; On 2017/02/28 00:35:11, Reilly Grant wrote: > Why initialize this with {}? Because if the loop in l.76 is not executed at all (empty |response_string|) then the comparison of l.85 would fail against the expectation in l.26. https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... content/browser/shape_detection/shape_detection_browsertest.cc:98: // TODO(xianglu): Enable the test on other platforms. https://crbug.com/659138 On 2017/02/28 00:35:11, Reilly Grant wrote: > nit: TODO(https://crbug.com/659138) Done. https://codereview.chromium.org/2711893003/diff/160001/content/test/data/medi... File content/test/data/media/shape_detection_test.html (right): https://codereview.chromium.org/2711893003/diff/160001/content/test/data/medi... content/test/data/media/shape_detection_test.html:31: console.log('something failed'); On 2017/02/28 00:35:11, Reilly Grant wrote: > These logs seem like leftovers from development. That said, logging > |error.message| might be useful. Done.
https://codereview.chromium.org/2711893003/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java (right): https://codereview.chromium.org/2711893003/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java:25: * Testing of the Shape Detection API. This API has three parts: QR/Barcodes, On 2017/02/28 00:35:11, Reilly Grant wrote: > nit: extra indentation here It's supposed to be like that when using JavaDocs [1] see e.g. [2] [1] https://source.android.com/source/code-style.html#use-javadoc-standard-comments [2] https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... File content/browser/shape_detection/shape_detection_browsertest.cc (right): https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... content/browser/shape_detection/shape_detection_browsertest.cc:75: std::vector<std::vector<float>> detected_bounding_boxes = {}; On 2017/02/28 00:35:11, Reilly Grant wrote: > Why initialize this with {}? Because if the loop in l.76 is not executed at all (empty |response_string|) then the comparison of l.85 would fail against the expectation in l.26. https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... content/browser/shape_detection/shape_detection_browsertest.cc:98: // TODO(xianglu): Enable the test on other platforms. https://crbug.com/659138 On 2017/02/28 00:35:11, Reilly Grant wrote: > nit: TODO(https://crbug.com/659138) Done. https://codereview.chromium.org/2711893003/diff/160001/content/test/data/medi... File content/test/data/media/shape_detection_test.html (right): https://codereview.chromium.org/2711893003/diff/160001/content/test/data/medi... content/test/data/media/shape_detection_test.html:31: console.log('something failed'); On 2017/02/28 00:35:11, Reilly Grant wrote: > These logs seem like leftovers from development. That said, logging > |error.message| might be useful. Done.
Too fast clicking! :-) avi@ please RS cnotent/ changes mariakhomenko@ ping PTAL
avayvod@chromium.org changed reviewers: + avayvod@chromium.org
javatests lgtm
https://codereview.chromium.org/2711893003/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java (right): https://codereview.chromium.org/2711893003/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java:42: @MediumTest I think this is actually a large test because it starts the test server https://codereview.chromium.org/2711893003/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java:44: @RetryOnFailure Why are you adding @RetryOnFailure here? This is an annotation for flaky tests only -- please remove and fix sources of flakiness if needed. https://codereview.chromium.org/2711893003/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java:54: assertEquals("found 1 shapes", tab.getTitle()); extract title into a const? https://codereview.chromium.org/2711893003/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java:67: @RetryOnFailure same comments on annotation as above https://codereview.chromium.org/2711893003/diff/180001/chrome/test/data/andro... File chrome/test/data/android/barcode_detection.html (right): https://codereview.chromium.org/2711893003/diff/180001/chrome/test/data/andro... chrome/test/data/android/barcode_detection.html:13: document.title = 'found ' + detectedObjects.length + ' shapes'; wouldn't you want to test that detected object has some property like the URL here rather than just the number of objects? https://codereview.chromium.org/2711893003/diff/180001/chrome/test/data/andro... File chrome/test/data/android/text_detection.html (right): https://codereview.chromium.org/2711893003/diff/180001/chrome/test/data/andro... chrome/test/data/android/text_detection.html:13: document.title = 'found ' + detectedObjects.length + ' shapes'; same comment, check that the right text is detected?
https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... File content/browser/shape_detection/shape_detection_browsertest.cc (right): https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... content/browser/shape_detection/shape_detection_browsertest.cc:75: std::vector<std::vector<float>> detected_bounding_boxes = {}; On 2017/02/28 18:10:18, mcasas wrote: > On 2017/02/28 00:35:11, Reilly Grant wrote: > > Why initialize this with {}? > > Because if the loop in l.76 is not executed > at all (empty |response_string|) then the > comparison of l.85 would fail against the > expectation in l.26. As discussed offline this doesn't make sense to me.
PTAL folks https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... File content/browser/shape_detection/shape_detection_browsertest.cc (right): https://codereview.chromium.org/2711893003/diff/160001/content/browser/shape_... content/browser/shape_detection/shape_detection_browsertest.cc:75: std::vector<std::vector<float>> detected_bounding_boxes = {}; On 2017/02/28 18:49:13, Reilly Grant wrote: > On 2017/02/28 18:10:18, mcasas wrote: > > On 2017/02/28 00:35:11, Reilly Grant wrote: > > > Why initialize this with {}? > > > > Because if the loop in l.76 is not executed > > at all (empty |response_string|) then the > > comparison of l.85 would fail against the > > expectation in l.26. > > As discussed offline this doesn't make sense to me. Removed. https://codereview.chromium.org/2711893003/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java (right): https://codereview.chromium.org/2711893003/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java:42: @MediumTest On 2017/02/28 18:40:56, Maria wrote: > I think this is actually a large test because it starts the test server Done. https://codereview.chromium.org/2711893003/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java:44: @RetryOnFailure On 2017/02/28 18:40:56, Maria wrote: > Why are you adding @RetryOnFailure here? This is an annotation for flaky tests > only -- please remove and fix sources of flakiness if needed. Removed. It was copy-pasted, but should never be flaky. https://codereview.chromium.org/2711893003/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/shape_detection/ShapeDetectionTest.java:54: assertEquals("found 1 shapes", tab.getTitle()); On 2017/02/28 18:40:56, Maria wrote: > extract title into a const? Done. https://codereview.chromium.org/2711893003/diff/180001/chrome/test/data/andro... File chrome/test/data/android/barcode_detection.html (right): https://codereview.chromium.org/2711893003/diff/180001/chrome/test/data/andro... chrome/test/data/android/barcode_detection.html:13: document.title = 'found ' + detectedObjects.length + ' shapes'; On 2017/02/28 18:40:56, Maria wrote: > wouldn't you want to test that detected object has some property like the URL > here rather than just the number of objects? Done. https://codereview.chromium.org/2711893003/diff/180001/chrome/test/data/andro... File chrome/test/data/android/text_detection.html (right): https://codereview.chromium.org/2711893003/diff/180001/chrome/test/data/andro... chrome/test/data/android/text_detection.html:13: document.title = 'found ' + detectedObjects.length + ' shapes'; On 2017/02/28 18:40:56, Maria wrote: > same comment, check that the right text is detected? Done.
The CQ bit was checked by mcasas@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: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm for java
nick@chromium.org changed reviewers: + nick@chromium.org
rs lgtm
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org, avayvod@chromium.org, mariakhomenko@chromium.org, nick@chromium.org Link to the patchset: https://codereview.chromium.org/2711893003/#ps220001 (title: "Make initialization of std::vector<std::vector<float>> explicit to avoid 'error: chosen constructor…")
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_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 mcasas@chromium.org
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_chromium_chromeos_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 mcasas@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": 220001, "attempt_start_ts": 1488333510530220,
"parent_rev": "bd3f676ad85ff5e61204c2e8c41d3328fe2ae182", "commit_rev":
"04e8ca70316308763a6f51d1ee11960819887f7e"}
Message was sent while issue was closed.
Description was changed from ========== Shape detection: moar content_browsertests (face, qr/barcode) In the spirit of "if it can be tested, it ought to be tested", this CL cleans up and expands the a few tests related to Shape Detection. - the current content_browsertest, which on ToT is extended to work on Mac and cleaned up (generalized and parameterized, variable names clarified). - the test face is changed to a smaller, public domain one of size 6182B (~1.4K smaller). - Also corrected names: s/shapedetection/shape_detection/ - QR/barcode and text detection in Android is based on coreGMS, so need to be tested under chrome/. A new ShapeDetectionTest.java is added for that. To use coreGMS, we have to guarantee that StrictMode.allowThreadDiskWrites(). BUG=665150, 676124 ========== to ========== Shape detection: moar content_browsertests (face, qr/barcode) In the spirit of "if it can be tested, it ought to be tested", this CL cleans up and expands the a few tests related to Shape Detection. - the current content_browsertest, which on ToT is extended to work on Mac and cleaned up (generalized and parameterized, variable names clarified). - the test face is changed to a smaller, public domain one of size 6182B (~1.4K smaller). - Also corrected names: s/shapedetection/shape_detection/ - QR/barcode and text detection in Android is based on coreGMS, so need to be tested under chrome/. A new ShapeDetectionTest.java is added for that. To use coreGMS, we have to guarantee that StrictMode.allowThreadDiskWrites(). BUG=665150, 676124 Review-Url: https://codereview.chromium.org/2711893003 Cr-Commit-Position: refs/heads/master@{#453818} Committed: https://chromium.googlesource.com/chromium/src/+/04e8ca70316308763a6f51d1ee11... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:220001) as https://chromium.googlesource.com/chromium/src/+/04e8ca70316308763a6f51d1ee11... |
