Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655303005:
Shape detection service: Add QR detection in Mac (Closed)
DescriptionShape detection service: Add QR detection in Mac
This CL lands support for QR/barcode detection in Mac by
adding the correspondent files in services/shape_detection.
Most of the work is reshuffling code that was specific to
Face detection, to suit both Face and Barcode detection:
- Removed shape_detection_service_dispatcher.h and merged
its two methods as statics in render_process_host_impl.cc.
- Moved common service/ code to detection_utils_mac.h/mm
(1 helper function)
** Important note re. testing **
Chromium Mac bots seem to have troubles running CoreImage
methods, probably because they are virtual machines (no GPU).
This prevents testing the Barcode detection API properly, and also
renders void the current Face detection test. So, the plan is:
a) Remove the current Mac-Face detection test, because is not
testing anything, really (there was a bug in the error-return path
that is cleaned up here).
b) land (imminently) new unit tests for face and barcode, running
in Mac Gpu bots.
This CL does a); b) will be done in a subsequent CL.
BUG=665150
TEST=
./out/gn/Chromium.app/Contents/MacOS/Chromium --enable-blink-features=ShapeDetection,GeometryInterfaces https://codepen.io/miguelao/full/wgrYjZ
NOTE: unittests for {face/barcode}_detection_impl_mac.mm coming
imminently after this).
Review-Url: https://codereview.chromium.org/2655303005
Cr-Commit-Position: refs/heads/master@{#447900}
Committed: https://chromium.googlesource.com/chromium/src/+/90149a4889e454cdca844de420d9deac61b99db6
Patch Set 1 : #
Total comments: 12
Patch Set 2 : rsesek@ comments #
Total comments: 6
Patch Set 3 : rsesek@ and rockot@s comments #Patch Set 4 : Don't init explicitly a vector of vectors #Messages
Total messages: 66 (48 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
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 #1 (id:120001) has been deleted
Description was changed from
==========
[wip]QR detection in Mac service
Moved face_detection_service_dispatcher.h to
shape_detection_service_dispatcher.h
Moved a new service function CreateCIImageFromSharedMemory() to a
_utils_mac.h/mm file
Moved face_detection_service_dispatcher.h to
shape_detection_service_dispatcher.h
BUG=665150
==========
to
==========
Shape detection service: Add QR detection in Mac
This CL lands support for QR/barcode detection in Mac by
adding the correspondent files in services/shape_detection.
Most of the work is reshuffling code that was specific to
Face detection, to suit both Face and Barcode detection:
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
- Moved common code to detection_utils_mac.h/mm (1 service function)
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
content_browsertests add a DetectBarcodesOnBlankImage and
a DetectBarcodesOn{BlankImage, ImageWithOneBarcode}, the
latter operating on a new qrcode.png example (465Bytes).
Made face_detection_test.html to a more general
shape_detection_test.html
BUG=665150
TEST=
./out/gn/content_browsertests --gtest_filter=ShapeDetectionBrowserTest.*
./out/gn/Chromium.app/Contents/MacOS/Chromium
--enable-blink-features=ShapeDetection,GeometryInterfaces
https://codepen.io/miguelao/full/wgrYjZ
NOTE: unittests for barcode_detection_impl_mac.mm coming
imminently after this).
==========
Patchset #1 (id:140001) has been deleted
Description was changed from
==========
Shape detection service: Add QR detection in Mac
This CL lands support for QR/barcode detection in Mac by
adding the correspondent files in services/shape_detection.
Most of the work is reshuffling code that was specific to
Face detection, to suit both Face and Barcode detection:
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
- Moved common code to detection_utils_mac.h/mm (1 service function)
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
content_browsertests add a DetectBarcodesOnBlankImage and
a DetectBarcodesOn{BlankImage, ImageWithOneBarcode}, the
latter operating on a new qrcode.png example (465Bytes).
Made face_detection_test.html to a more general
shape_detection_test.html
BUG=665150
TEST=
./out/gn/content_browsertests --gtest_filter=ShapeDetectionBrowserTest.*
./out/gn/Chromium.app/Contents/MacOS/Chromium
--enable-blink-features=ShapeDetection,GeometryInterfaces
https://codepen.io/miguelao/full/wgrYjZ
NOTE: unittests for barcode_detection_impl_mac.mm coming
imminently after this).
==========
to
==========
Shape detection service: Add QR detection in Mac
This CL lands support for QR/barcode detection in Mac by
adding the correspondent files in services/shape_detection.
Most of the work is reshuffling code that was specific to
Face detection, to suit both Face and Barcode detection:
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
- Moved common code to detection_utils_mac.h/mm (1 service function)
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
content_browsertests add a DetectBarcodesOnBlankImage and
a DetectBarcodesOn{BlankImage, ImageWithOneBarcode}, the
latter operating on a new qrcode.png example (465Bytes).
Made face_detection_test.html to a more general
shape_detection_test.html, and corrected a bug in which the
face was never detected but was mistakenly done so due to the
error info containing a callstack and confusing the parser. A
new (6167B image) is included.
BUG=665150
TEST=
./out/gn/content_browsertests --gtest_filter=ShapeDetectionBrowserTest.*
./out/gn/Chromium.app/Contents/MacOS/Chromium
--enable-blink-features=ShapeDetection,GeometryInterfaces
https://codepen.io/miguelao/full/wgrYjZ
NOTE: unittests for barcode_detection_impl_mac.mm coming
imminently after this).
==========
Description was changed from
==========
Shape detection service: Add QR detection in Mac
This CL lands support for QR/barcode detection in Mac by
adding the correspondent files in services/shape_detection.
Most of the work is reshuffling code that was specific to
Face detection, to suit both Face and Barcode detection:
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
- Moved common code to detection_utils_mac.h/mm (1 service function)
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
content_browsertests add a DetectBarcodesOnBlankImage and
a DetectBarcodesOn{BlankImage, ImageWithOneBarcode}, the
latter operating on a new qrcode.png example (465Bytes).
Made face_detection_test.html to a more general
shape_detection_test.html, and corrected a bug in which the
face was never detected but was mistakenly done so due to the
error info containing a callstack and confusing the parser. A
new (6167B image) is included.
BUG=665150
TEST=
./out/gn/content_browsertests --gtest_filter=ShapeDetectionBrowserTest.*
./out/gn/Chromium.app/Contents/MacOS/Chromium
--enable-blink-features=ShapeDetection,GeometryInterfaces
https://codepen.io/miguelao/full/wgrYjZ
NOTE: unittests for barcode_detection_impl_mac.mm coming
imminently after this).
==========
to
==========
Shape detection service: Add QR detection in Mac
This CL lands support for QR/barcode detection in Mac by
adding the correspondent files in services/shape_detection.
Most of the work is reshuffling code that was specific to
Face detection, to suit both Face and Barcode detection:
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
- Moved common code to detection_utils_mac.h/mm (1 service function)
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
content_browsertests add a DetectBarcodesOnBlankImage and
a DetectBarcodesOn{BlankImage, ImageWithOneBarcode}, the
latter operating on a new qrcode.png example (465Bytes).
Made face_detection_test.html to a more general
shape_detection_test.html, and corrected a bug in which the
face was never detected but was mistakenly done so due to the
error info containing a callstack and confusing the parser. A
new (6167B image, ~1KB less) is included.
BUG=665150
TEST=
./out/gn/content_browsertests --gtest_filter=ShapeDetectionBrowserTest.*
./out/gn/Chromium.app/Contents/MacOS/Chromium
--enable-blink-features=ShapeDetection,GeometryInterfaces
https://codepen.io/miguelao/full/wgrYjZ
NOTE: unittests for barcode_detection_impl_mac.mm coming
imminently after this).
==========
Patchset #1 (id:160001) has been deleted
Patchset #2 (id:190001) has been deleted
Patchset #2 (id:210001) has been deleted
Patchset #2 (id:130019) has been deleted
Patchset #2 (id:230020) has been deleted
Patchset #2 (id:250001) has been deleted
Patchset #2 (id:270001) has been deleted
Patchset #2 (id:290001) has been deleted
Patchset #2 (id:310001) has been deleted
Description was changed from
==========
Shape detection service: Add QR detection in Mac
This CL lands support for QR/barcode detection in Mac by
adding the correspondent files in services/shape_detection.
Most of the work is reshuffling code that was specific to
Face detection, to suit both Face and Barcode detection:
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
- Moved common code to detection_utils_mac.h/mm (1 service function)
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
content_browsertests add a DetectBarcodesOnBlankImage and
a DetectBarcodesOn{BlankImage, ImageWithOneBarcode}, the
latter operating on a new qrcode.png example (465Bytes).
Made face_detection_test.html to a more general
shape_detection_test.html, and corrected a bug in which the
face was never detected but was mistakenly done so due to the
error info containing a callstack and confusing the parser. A
new (6167B image, ~1KB less) is included.
BUG=665150
TEST=
./out/gn/content_browsertests --gtest_filter=ShapeDetectionBrowserTest.*
./out/gn/Chromium.app/Contents/MacOS/Chromium
--enable-blink-features=ShapeDetection,GeometryInterfaces
https://codepen.io/miguelao/full/wgrYjZ
NOTE: unittests for barcode_detection_impl_mac.mm coming
imminently after this).
==========
to
==========
Shape detection service: Add QR detection in Mac
This CL lands support for QR/barcode detection in Mac by
adding the correspondent files in services/shape_detection.
Most of the work is reshuffling code that was specific to
Face detection, to suit both Face and Barcode detection:
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
- Moved face_detection_service_dispatcher.h -->
shape_detection_service_dispatcher.h
- Moved common code to detection_utils_mac.h/mm (1 service function)
** Important note re. testing **
chromium Mac bots seem to have troubles running CoreImage
methods, probably because they are virtual machines. This
prevents testing the Barcode detection API properly, and also
renders void the current Face detection test. So, the plan is:
a) remove the current Face detection test, because is not testing
anything, really
b) land (imminently) new unittests for face and barcode, running
in Mac Gpu bots.
This CL does a); b) will be done in an imminent subsequent CL.
Nonetheless a new CC (6167B image, ~1KB less) is included.
BUG=665150
TEST=
./out/gn/Chromium.app/Contents/MacOS/Chromium
--enable-blink-features=ShapeDetection,GeometryInterfaces
https://codepen.io/miguelao/full/wgrYjZ
NOTE: unittests for barcode_detection_impl_mac.mm coming
imminently after this).
==========
Patchset #1 (id:160002) has been deleted
Patchset #1 (id:330001) has been deleted
Patchset #1 (id:350001) has been deleted
Patchset #1 (id:370001) has been deleted
Patchset #1 (id:390001) has been deleted
Description was changed from ========== Shape detection service: Add QR detection in Mac This CL lands support for QR/barcode detection in Mac by adding the correspondent files in services/shape_detection. Most of the work is reshuffling code that was specific to Face detection, to suit both Face and Barcode detection: - Moved face_detection_service_dispatcher.h --> shape_detection_service_dispatcher.h - Moved face_detection_service_dispatcher.h --> shape_detection_service_dispatcher.h - Moved common code to detection_utils_mac.h/mm (1 service function) ** Important note re. testing ** chromium Mac bots seem to have troubles running CoreImage methods, probably because they are virtual machines. This prevents testing the Barcode detection API properly, and also renders void the current Face detection test. So, the plan is: a) remove the current Face detection test, because is not testing anything, really b) land (imminently) new unittests for face and barcode, running in Mac Gpu bots. This CL does a); b) will be done in an imminent subsequent CL. Nonetheless a new CC (6167B image, ~1KB less) is included. BUG=665150 TEST= ./out/gn/Chromium.app/Contents/MacOS/Chromium --enable-blink-features=ShapeDetection,GeometryInterfaces https://codepen.io/miguelao/full/wgrYjZ NOTE: unittests for barcode_detection_impl_mac.mm coming imminently after this). ========== to ========== Shape detection service: Add QR detection in Mac This CL lands support for QR/barcode detection in Mac by adding the correspondent files in services/shape_detection. Most of the work is reshuffling code that was specific to Face detection, to suit both Face and Barcode detection: - Moved face_detection_service_dispatcher.h --> shape_detection_service_dispatcher.h - Moved face_detection_service_dispatcher.h --> shape_detection_service_dispatcher.h - Moved common code to detection_utils_mac.h/mm (1 service function) ** Important note re. testing ** chromium Mac bots seem to have troubles running CoreImage methods, probably because they are virtual machines. This prevents testing the Barcode detection API properly, and also renders void the current Face detection test. So, the plan is: a) remove the current Face detection test, because is not testing anything, really b) land (imminently) new unittests for face and barcode, running in Mac Gpu bots. This CL does a); b) will be done in an imminent subsequent CL. Nonetheless a new CC (6167B image, ~1KB less) is included. BUG=665150 TEST= ./out/gn/Chromium.app/Contents/MacOS/Chromium --enable-blink-features=ShapeDetection,GeometryInterfaces https://codepen.io/miguelao/full/wgrYjZ NOTE: unittests for {face/barcode}_detection_impl_mac.mm coming imminently after this). ==========
Patchset #1 (id:410001) has been deleted
Patchset #2 (id:450001) has been deleted
Description was changed from ========== Shape detection service: Add QR detection in Mac This CL lands support for QR/barcode detection in Mac by adding the correspondent files in services/shape_detection. Most of the work is reshuffling code that was specific to Face detection, to suit both Face and Barcode detection: - Moved face_detection_service_dispatcher.h --> shape_detection_service_dispatcher.h - Moved face_detection_service_dispatcher.h --> shape_detection_service_dispatcher.h - Moved common code to detection_utils_mac.h/mm (1 service function) ** Important note re. testing ** chromium Mac bots seem to have troubles running CoreImage methods, probably because they are virtual machines. This prevents testing the Barcode detection API properly, and also renders void the current Face detection test. So, the plan is: a) remove the current Face detection test, because is not testing anything, really b) land (imminently) new unittests for face and barcode, running in Mac Gpu bots. This CL does a); b) will be done in an imminent subsequent CL. Nonetheless a new CC (6167B image, ~1KB less) is included. BUG=665150 TEST= ./out/gn/Chromium.app/Contents/MacOS/Chromium --enable-blink-features=ShapeDetection,GeometryInterfaces https://codepen.io/miguelao/full/wgrYjZ NOTE: unittests for {face/barcode}_detection_impl_mac.mm coming imminently after this). ========== to ========== Shape detection service: Add QR detection in Mac This CL lands support for QR/barcode detection in Mac by adding the correspondent files in services/shape_detection. Most of the work is reshuffling code that was specific to Face detection, to suit both Face and Barcode detection: - Moved face_detection_service_dispatcher.h --> shape_detection_service_dispatcher.h - Moved common code to detection_utils_mac.h/mm (1 service function) ** Important note re. testing ** chromium Mac bots seem to have troubles running CoreImage methods, probably because they are virtual machines. This prevents testing the Barcode detection API properly, and also renders void the current Face detection test. So, the plan is: a) remove the current Face detection test, because is not testing anything, really b) land (imminently) new unittests for face and barcode, running in Mac Gpu bots. This CL does a); b) will be done in an imminent subsequent CL. Nonetheless a new CC (6167B image, ~1KB less) is included. BUG=665150 TEST= ./out/gn/Chromium.app/Contents/MacOS/Chromium --enable-blink-features=ShapeDetection,GeometryInterfaces https://codepen.io/miguelao/full/wgrYjZ NOTE: unittests for {face/barcode}_detection_impl_mac.mm coming imminently after this). ==========
Patchset #1 (id:430001) has been deleted
Description was changed from ========== Shape detection service: Add QR detection in Mac This CL lands support for QR/barcode detection in Mac by adding the correspondent files in services/shape_detection. Most of the work is reshuffling code that was specific to Face detection, to suit both Face and Barcode detection: - Moved face_detection_service_dispatcher.h --> shape_detection_service_dispatcher.h - Moved common code to detection_utils_mac.h/mm (1 service function) ** Important note re. testing ** chromium Mac bots seem to have troubles running CoreImage methods, probably because they are virtual machines. This prevents testing the Barcode detection API properly, and also renders void the current Face detection test. So, the plan is: a) remove the current Face detection test, because is not testing anything, really b) land (imminently) new unittests for face and barcode, running in Mac Gpu bots. This CL does a); b) will be done in an imminent subsequent CL. Nonetheless a new CC (6167B image, ~1KB less) is included. BUG=665150 TEST= ./out/gn/Chromium.app/Contents/MacOS/Chromium --enable-blink-features=ShapeDetection,GeometryInterfaces https://codepen.io/miguelao/full/wgrYjZ NOTE: unittests for {face/barcode}_detection_impl_mac.mm coming imminently after this). ========== to ========== Shape detection service: Add QR detection in Mac This CL lands support for QR/barcode detection in Mac by adding the correspondent files in services/shape_detection. Most of the work is reshuffling code that was specific to Face detection, to suit both Face and Barcode detection: - Moved face_detection_service_dispatcher.h --> shape_detection_service_dispatcher.h - Moved common code to detection_utils_mac.h/mm (1 service function) ** Important note re. testing ** chromium Mac bots seem to have troubles running CoreImage methods, probably because they are virtual machines. This prevents testing the Barcode detection API properly, and also renders void the current Face detection test. So, the plan is: a) Remove the current Mac-Face detection test, because is not testing anything, really (there was a bug in the error-return path that is cleaned up here). b) land (imminently) new unittests for face and barcode, running in Mac Gpu bots. This CL does a); b) will be done in a subsequent CL. BUG=665150 TEST= ./out/gn/Chromium.app/Contents/MacOS/Chromium --enable-blink-features=ShapeDetection,GeometryInterfaces https://codepen.io/miguelao/full/wgrYjZ NOTE: unittests for {face/barcode}_detection_impl_mac.mm coming imminently after this). ==========
mcasas@chromium.org changed reviewers: + rockot@chromium.org, rsesek@chromium.org
rockot@: PTAL at the content/ request forwarding rsesek@ PTAL at the Mac parts.
https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... File services/shape_detection/barcode_detection_impl.cc (right): https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... services/shape_detection/barcode_detection_impl.cc:9: //static nit: space after / https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... File services/shape_detection/barcode_detection_impl_mac.mm (right): https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... services/shape_detection/barcode_detection_impl_mac.mm:33: //static nit: space after / https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... services/shape_detection/barcode_detection_impl_mac.mm:38: return; Does this not need to error out? https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... services/shape_detection/barcode_detection_impl_mac.mm:44: context_.reset([[CIContext alloc] init]); Is it worth finding a way to share the CIContext between the barcode and QR detectors? https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... services/shape_detection/barcode_detection_impl_mac.mm:47: #if MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_10 The general approach to doing this is to a) do it in sdk_forward_declarations.h, and b) name it the same as the constant that isn't defined: https://cs.chromium.org/chromium/src/base/mac/sdk_forward_declarations.h?type... https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... File services/shape_detection/shape_detection_service.cc (right): https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... services/shape_detection/shape_detection_service.cc:40: DVLOG(1) << __func__; Remove?
Patchset #2 (id:490001) has been deleted
ptal https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... File services/shape_detection/barcode_detection_impl.cc (right): https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... services/shape_detection/barcode_detection_impl.cc:9: //static On 2017/02/01 19:34:11, Robert Sesek wrote: > nit: space after / Done. https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... File services/shape_detection/barcode_detection_impl_mac.mm (right): https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... services/shape_detection/barcode_detection_impl_mac.mm:33: //static On 2017/02/01 19:34:11, Robert Sesek wrote: > nit: space after / Done. https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... services/shape_detection/barcode_detection_impl_mac.mm:38: return; On 2017/02/01 19:34:11, Robert Sesek wrote: > Does this not need to error out? No; mojo requests should be dropped so that the client's side on_error_connection_handler() is called. https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... services/shape_detection/barcode_detection_impl_mac.mm:44: context_.reset([[CIContext alloc] init]); On 2017/02/01 19:34:11, Robert Sesek wrote: > Is it worth finding a way to share the CIContext between the barcode and QR > detectors? Not really, I think the idea was to have a CIContext initialized in a background thread or pool and reuse it for the CIDetector, but since we are now inside a background process it's not even worth keeping, so I removed it from both detectors. https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... services/shape_detection/barcode_detection_impl_mac.mm:47: #if MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_10 On 2017/02/01 19:34:11, Robert Sesek wrote: > The general approach to doing this is to a) do it in sdk_forward_declarations.h, > and b) name it the same as the constant that isn't defined: > > https://cs.chromium.org/chromium/src/base/mac/sdk_forward_declarations.h?type... Cool. Done. https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... File services/shape_detection/shape_detection_service.cc (right): https://codereview.chromium.org/2655303005/diff/470001/services/shape_detecti... services/shape_detection/shape_detection_service.cc:40: DVLOG(1) << __func__; On 2017/02/01 19:34:11, Robert Sesek wrote: > Remove? Oops, done.
https://codereview.chromium.org/2655303005/diff/510001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2655303005/diff/510001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1204: AddUIThreadInterface( Note that in both of these cases, trying to use the API on non-Android non-OS X will (intentionally) crash the renderer because there won't be a registered request handler for these interfaces. Consider registering no-op handlers for other platforms. https://codereview.chromium.org/2655303005/diff/510001/content/browser/shaped... File content/browser/shapedetection/shape_detection_service_dispatcher.h (right): https://codereview.chromium.org/2655303005/diff/510001/content/browser/shaped... content/browser/shapedetection/shape_detection_service_dispatcher.h:20: namespace ShapeDetectionServiceDispatcher { Maybe I missed this in a previous CL, but this is not correct style for namespace names. But more importantly, there's no need for this to be in a separate namespace or even in its own header. This function definition can be entirely internal to render_process_host_impl.cc in an anonymous namespace.
Mac bits lgtm https://codereview.chromium.org/2655303005/diff/510001/services/shape_detecti... File services/shape_detection/barcode_detection_impl.h (right): https://codereview.chromium.org/2655303005/diff/510001/services/shape_detecti... services/shape_detection/barcode_detection_impl.h:18: private: Make the constructor private too?
Description was changed from ========== Shape detection service: Add QR detection in Mac This CL lands support for QR/barcode detection in Mac by adding the correspondent files in services/shape_detection. Most of the work is reshuffling code that was specific to Face detection, to suit both Face and Barcode detection: - Moved face_detection_service_dispatcher.h --> shape_detection_service_dispatcher.h - Moved common code to detection_utils_mac.h/mm (1 service function) ** Important note re. testing ** chromium Mac bots seem to have troubles running CoreImage methods, probably because they are virtual machines. This prevents testing the Barcode detection API properly, and also renders void the current Face detection test. So, the plan is: a) Remove the current Mac-Face detection test, because is not testing anything, really (there was a bug in the error-return path that is cleaned up here). b) land (imminently) new unittests for face and barcode, running in Mac Gpu bots. This CL does a); b) will be done in a subsequent CL. BUG=665150 TEST= ./out/gn/Chromium.app/Contents/MacOS/Chromium --enable-blink-features=ShapeDetection,GeometryInterfaces https://codepen.io/miguelao/full/wgrYjZ NOTE: unittests for {face/barcode}_detection_impl_mac.mm coming imminently after this). ========== to ========== Shape detection service: Add QR detection in Mac This CL lands support for QR/barcode detection in Mac by adding the correspondent files in services/shape_detection. Most of the work is reshuffling code that was specific to Face detection, to suit both Face and Barcode detection: - Removed shape_detection_service_dispatcher.h and merged its two methods as statics in render_process_host_impl.cc. - Moved common service/ code to detection_utils_mac.h/mm (1 helper function) ** Important note re. testing ** Chromium Mac bots seem to have troubles running CoreImage methods, probably because they are virtual machines (no GPU). This prevents testing the Barcode detection API properly, and also renders void the current Face detection test. So, the plan is: a) Remove the current Mac-Face detection test, because is not testing anything, really (there was a bug in the error-return path that is cleaned up here). b) land (imminently) new unit tests for face and barcode, running in Mac Gpu bots. This CL does a); b) will be done in a subsequent CL. BUG=665150 TEST= ./out/gn/Chromium.app/Contents/MacOS/Chromium --enable-blink-features=ShapeDetection,GeometryInterfaces https://codepen.io/miguelao/full/wgrYjZ NOTE: unittests for {face/barcode}_detection_impl_mac.mm coming imminently after this). ==========
Patchset #3 (id:520001) has been deleted
mcasas@chromium.org changed reviewers: + avi@chromium.org
rockot@ PTAL avi@ RS content/ changes plz https://codereview.chromium.org/2655303005/diff/510001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2655303005/diff/510001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:1204: AddUIThreadInterface( On 2017/02/01 21:01:07, Ken Rockot wrote: > Note that in both of these cases, trying to use the API on non-Android non-OS X > will (intentionally) crash the renderer because there won't be a registered > request handler for these interfaces. Consider registering no-op handlers for > other platforms. True. What we want is, in non-Android, to route the request to the service, which will drop it in non-MacOsX platforms, so that the client will be notified via its connection's |error_handler|. Modified accordingly. https://codereview.chromium.org/2655303005/diff/510001/content/browser/shaped... File content/browser/shapedetection/shape_detection_service_dispatcher.h (right): https://codereview.chromium.org/2655303005/diff/510001/content/browser/shaped... content/browser/shapedetection/shape_detection_service_dispatcher.h:20: namespace ShapeDetectionServiceDispatcher { On 2017/02/01 21:01:07, Ken Rockot wrote: > Maybe I missed this in a previous CL, but this is not correct style for > namespace names. But more importantly, there's no need for this to be in a > separate namespace or even in its own header. This function definition can be > entirely internal to render_process_host_impl.cc in an anonymous namespace. That's very correct. Moved to render_process_host_impl.cc and nuked this file. https://codereview.chromium.org/2655303005/diff/510001/services/shape_detecti... File services/shape_detection/barcode_detection_impl.h (right): https://codereview.chromium.org/2655303005/diff/510001/services/shape_detecti... services/shape_detection/barcode_detection_impl.h:18: private: On 2017/02/02 12:53:45, Robert Sesek wrote: > Make the constructor private too? Done.
lgtm
mcasas@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@ RS plz the additions to base/mac/sdk_forward_declaration
On 2017/02/02 20:48:49, mcasas wrote: > dcheng@ RS plz the additions to base/mac/sdk_forward_declaration That shouldn't be necessary https://cs.chromium.org/chromium/src/base/mac/OWNERS?type=cs&q=base/mac/OWNER...
On 2017/02/02 22:18:40, Robert Sesek wrote: > On 2017/02/02 20:48:49, mcasas wrote: > > dcheng@ RS plz the additions to base/mac/sdk_forward_declaration > > That shouldn't be necessary > https://cs.chromium.org/chromium/src/base/mac/OWNERS?type=cs&q=base/mac/OWNER... Oops - you're right, I rely too much on Chromite butler and it doesn't follow the file:// inclusions in OWNERS.
mcasas@chromium.org changed reviewers: + nick@chromium.org - avi@chromium.org, dcheng@chromium.org
nick@ RS plz the small things in content/browser/* (ISO avi@ ooo)
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 rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2655303005/#ps540001 (title: "rsesek@ and rockot@s comments")
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_compile_dbg_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
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, rockot@chromium.org, nick@chromium.org Link to the patchset: https://codereview.chromium.org/2655303005/#ps560001 (title: "Don't init explicitly a vector of vectors")
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: 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 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": 560001, "attempt_start_ts": 1486086019782400,
"parent_rev": "75dc75e5f12aa17db7d46d999d3029e2c9658a84", "commit_rev":
"90149a4889e454cdca844de420d9deac61b99db6"}
Message was sent while issue was closed.
Description was changed from ========== Shape detection service: Add QR detection in Mac This CL lands support for QR/barcode detection in Mac by adding the correspondent files in services/shape_detection. Most of the work is reshuffling code that was specific to Face detection, to suit both Face and Barcode detection: - Removed shape_detection_service_dispatcher.h and merged its two methods as statics in render_process_host_impl.cc. - Moved common service/ code to detection_utils_mac.h/mm (1 helper function) ** Important note re. testing ** Chromium Mac bots seem to have troubles running CoreImage methods, probably because they are virtual machines (no GPU). This prevents testing the Barcode detection API properly, and also renders void the current Face detection test. So, the plan is: a) Remove the current Mac-Face detection test, because is not testing anything, really (there was a bug in the error-return path that is cleaned up here). b) land (imminently) new unit tests for face and barcode, running in Mac Gpu bots. This CL does a); b) will be done in a subsequent CL. BUG=665150 TEST= ./out/gn/Chromium.app/Contents/MacOS/Chromium --enable-blink-features=ShapeDetection,GeometryInterfaces https://codepen.io/miguelao/full/wgrYjZ NOTE: unittests for {face/barcode}_detection_impl_mac.mm coming imminently after this). ========== to ========== Shape detection service: Add QR detection in Mac This CL lands support for QR/barcode detection in Mac by adding the correspondent files in services/shape_detection. Most of the work is reshuffling code that was specific to Face detection, to suit both Face and Barcode detection: - Removed shape_detection_service_dispatcher.h and merged its two methods as statics in render_process_host_impl.cc. - Moved common service/ code to detection_utils_mac.h/mm (1 helper function) ** Important note re. testing ** Chromium Mac bots seem to have troubles running CoreImage methods, probably because they are virtual machines (no GPU). This prevents testing the Barcode detection API properly, and also renders void the current Face detection test. So, the plan is: a) Remove the current Mac-Face detection test, because is not testing anything, really (there was a bug in the error-return path that is cleaned up here). b) land (imminently) new unit tests for face and barcode, running in Mac Gpu bots. This CL does a); b) will be done in a subsequent CL. BUG=665150 TEST= ./out/gn/Chromium.app/Contents/MacOS/Chromium --enable-blink-features=ShapeDetection,GeometryInterfaces https://codepen.io/miguelao/full/wgrYjZ NOTE: unittests for {face/barcode}_detection_impl_mac.mm coming imminently after this). Review-Url: https://codereview.chromium.org/2655303005 Cr-Commit-Position: refs/heads/master@{#447900} Committed: https://chromium.googlesource.com/chromium/src/+/90149a4889e454cdca844de420d9... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:560001) as https://chromium.googlesource.com/chromium/src/+/90149a4889e454cdca844de420d9... |
