Shape Detection: Implement FaceDetection on Mac as out-of-process service

Shape Detection: Implement FaceDetection on Mac as out-of-process service

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from the Renderer process to a Utility process, which detects
faces using Mac native CoreImage library and returns detection result.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2528743002
Cr-Commit-Position: refs/heads/master@{#445942}
Committed: https://chromium.googlesource.com/chromium/src/+/6dd8f460d0fe4567a7cb14ee5a3acc183d5325fb

[xianglu, 2016-11-23 19:44:57]

Patchset #1 (id:1) has been deleted

[yzshen1, 2016-11-23 20:14:04]

yzshen@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2016-11-23 20:14:05]

https://codereview.chromium.org/2528743002/diff/20001/services/shape_detectio...
File services/shape_detection/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/2528743002/diff/20001/services/shape_detectio...
services/shape_detection/public/interfaces/BUILD.gn:12: }
You need to add

  public_deps = [
    "//ui/gfx/geometry/mojo",
  ]

For it to compile.

[xianglu, 2016-11-29 16:13:21]

Patchset #2 (id:40001) has been deleted

[xianglu, 2016-11-29 17:28:08]

Patchset #2 (id:60001) has been deleted

[xianglu, 2016-11-29 18:46:43]

Patchset #2 (id:80001) has been deleted

[xianglu, 2016-11-29 20:07:42]

Patchset #2 (id:100001) has been deleted

[xianglu, 2016-11-29 23:04:23]

Patchset #2 (id:120001) has been deleted

[xianglu, 2016-11-29 23:05:29]

Patchset #1 (id:20001) has been deleted

[xianglu, 2016-11-30 01:25:27]

Patchset #1 (id:140001) has been deleted

[xianglu, 2016-11-30 19:09:33]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 19:10:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 19:15:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 19:15:27]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[xianglu, 2016-11-30 19:57:53]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[xianglu, 2016-11-30 19:58:02]

Patchset #2 (id:180001) has been deleted

[xianglu, 2016-11-30 19:58:14]

Patchset #2 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-11-30 19:58:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 20:03:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 20:03:11]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  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_...)

[xianglu, 2016-11-30 20:07:26]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[xianglu, 2016-11-30 20:07:32]

Patchset #3 (id:240001) has been deleted

[xianglu, 2016-11-30 20:07:38]

Patchset #2 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-11-30 20:08:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-11-30 20:08:21]

Description was changed from

==========
ShapeDetection: Move ShapeDetection implementation to isolated mojo service.

BUG=659139
==========

to

==========
ShapeDetection: Move ShapeDetection implementation to isolated mojo service.

BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[commit-bot: I haz the power, 2016-11-30 20:12:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 20:12:43]

Dry run: 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_...)

[xianglu, 2016-11-30 20:17:25]

Description was changed from

==========
ShapeDetection: Move ShapeDetection implementation to isolated mojo service.

BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move ShapeDetection implementation to isolated mojo service.

This CL moves implementation of ShapeDetection under services/shape_detection/ 
(temporally for MACOS only). The mojo service can be easily pluged in different
processes. In this CL the service runs in the browser process, and Blink
requests
through a serviceConnector.

BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-11-30 20:18:34]

Patchset #1 (id:160001) has been deleted

[xianglu, 2016-11-30 20:27:22]

xianglu@chromium.org changed reviewers:
+ rockot@chromium.org

[xianglu, 2016-11-30 20:27:23]

ptal, especially service_manager_context.cc and browser_context.cc

[xianglu, 2016-11-30 20:30:13]

Description was changed from

==========
ShapeDetection: Move ShapeDetection implementation to isolated mojo service.

This CL moves implementation of ShapeDetection under services/shape_detection/ 
(temporally for MACOS only). The mojo service can be easily pluged in different
processes. In this CL the service runs in the browser process, and Blink
requests
through a serviceConnector.

BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and
Blink
requests through a serviceConnector.

BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-11-30 20:31:42]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and
Blink
requests through a serviceConnector.

BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and if
crashes happen, we should consider moving it to a utility process

BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-11-30 20:31:56]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and if
crashes happen, we should consider moving it to a utility process

BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process

BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-11-30 20:32:53]

xianglu@chromium.org changed reviewers:
+ mcasas@chromium.org

[xianglu, 2016-11-30 20:33:52]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process

BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process.

BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-11-30 21:53:47]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 21:54:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-11-30 21:54:21]

Patchset #1 (id:260001) has been deleted

[commit-bot: I haz the power, 2016-11-30 22:21:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 22:21:58]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[xianglu, 2016-11-30 23:21:24]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 23:21:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-11-30 23:30:51]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 23:31:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 23:48:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 23:48:02]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[xianglu, 2016-12-01 00:09:14]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 00:09:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-01 00:10:16]

Patchset #2 (id:220002) has been deleted

[commit-bot: I haz the power, 2016-12-01 00:13:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 00:13:28]

Dry run: Try jobs failed on following builders:
  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_...)

[xianglu, 2016-12-01 00:30:26]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 00:30:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-01 00:31:09]

Patchset #3 (id:330001) has been deleted

[xianglu, 2016-12-01 00:31:37]

Patchset #2 (id:310001) has been deleted

[commit-bot: I haz the power, 2016-12-01 00:34:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 00:34:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xianglu, 2016-12-01 01:37:37]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 01:38:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-01 01:38:07]

Patchset #2 (id:350001) has been deleted

[commit-bot: I haz the power, 2016-12-01 03:32:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 03:32:15]

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, 2016-12-01 16:03:51]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 16:04:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-01 16:04:50]

Patchset #2 (id:370001) has been deleted

[xianglu, 2016-12-01 17:20:35]

Patchset #1 (id:280001) has been deleted

[xianglu, 2016-12-01 17:27:07]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process.

BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process.

The following files are copied from https://codereview.chromium.org/2460723003/
and are already under review:
content/child/blink_platform_impl.h
content/child/blink_platform_impl.cc
third_party/WebKit/Source/platform/ServiceConnector.h
third_party/WebKit/Source/platform/exported/Platform.cpp
third_party/WebKit/public/platform/Platform.h


BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-12-01 17:27:27]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process.

The following files are copied from https://codereview.chromium.org/2460723003/
and are already under review:
content/child/blink_platform_impl.h
content/child/blink_platform_impl.cc
third_party/WebKit/Source/platform/ServiceConnector.h
third_party/WebKit/Source/platform/exported/Platform.cpp
third_party/WebKit/public/platform/Platform.h


BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process.

The following files are copied from https://codereview.chromium.org/2460723003/
and are already under review:
content/child/blink_platform_impl.h
content/child/blink_platform_impl.cc
third_party/WebKit/Source/platform/ServiceConnector.h
third_party/WebKit/Source/platform/exported/Platform.cpp
third_party/WebKit/public/platform/Platform.h


BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-12-01 17:28:03]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process.

The following files are copied from https://codereview.chromium.org/2460723003/
and are already under review:
content/child/blink_platform_impl.h
content/child/blink_platform_impl.cc
third_party/WebKit/Source/platform/ServiceConnector.h
third_party/WebKit/Source/platform/exported/Platform.cpp
third_party/WebKit/public/platform/Platform.h


BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process.

The following files are copied from https://codereview.chromium.org/2460723003/
and are already under review:
-content/child/blink_platform_impl.h
-content/child/blink_platform_impl.cc
-third_party/WebKit/Source/platform/ServiceConnector.h
-third_party/WebKit/Source/platform/exported/Platform.cpp
-third_party/WebKit/public/platform/Platform.h


BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[commit-bot: I haz the power, 2016-12-01 17:41:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 17:41:24]

Dry run: 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...)

[Ken Rockot(use gerrit already), 2016-12-01 17:54:31]

https://codereview.chromium.org/2528743002/diff/390001/content/browser/servic...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2528743002/diff/390001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:268: ServiceInfo
shape_detection_info;
You can remove this registration for now. We aren't quite ready to support
embedded singleton services, so we'll have to accept an instance per profile
here.

https://codereview.chromium.org/2528743002/diff/390001/content/public/app/moj...
File content/public/app/mojo/content_browser_manifest.json (left):

https://codereview.chromium.org/2528743002/diff/390001/content/public/app/moj...
content/public/app/mojo/content_browser_manifest.json:18:
"blink::mojom::FaceDetection",
It looks like there is still renderer code trying to connect to these interfaces
on the RenderFrameImpl's interface provider. At least in tests, there are some
failures related to this.

https://codereview.chromium.org/2528743002/diff/390001/content/public/app/moj...
File content/public/app/mojo/content_browser_manifest.json (right):

https://codereview.chromium.org/2528743002/diff/390001/content/public/app/moj...
content/public/app/mojo/content_browser_manifest.json:58: "shape_detection": [
"face_detection", "barcode_detection" ]
This is unnecessary since the browser itself does not need to connect to
shape_detection.

https://codereview.chromium.org/2528743002/diff/390001/services/shape_detecti...
File services/shape_detection/README.md (right):

https://codereview.chromium.org/2528743002/diff/390001/services/shape_detecti...
services/shape_detection/README.md:1: Shape Detection Service is an isolated
mojo service that exists to facilitate
language nit: "mojo service" is redundant, just "service" please

https://codereview.chromium.org/2528743002/diff/390001/services/shape_detecti...
File services/shape_detection/manifest.json (right):

https://codereview.chromium.org/2528743002/diff/390001/services/shape_detecti...
services/shape_detection/manifest.json:10: "requires": {
nit: not necessary

https://codereview.chromium.org/2528743002/diff/390001/services/shape_detecti...
File services/shape_detection/shape_detection_service.h (right):

https://codereview.chromium.org/2528743002/diff/390001/services/shape_detecti...
services/shape_detection/shape_detection_service.h:19:
std::unique_ptr<service_manager::Service> CreateShapeDetectionService();
nit: Maybe just make it a static Create method on ShapeDetectionService?

[xianglu, 2016-12-01 19:52:38]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 19:53:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 21:22:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 21:22:14]

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_...)

[yzshen1, 2016-12-01 21:28:26]

https://codereview.chromium.org/2528743002/diff/410001/services/shape_detecti...
File services/shape_detection/BUILD.gn (right):

https://codereview.chromium.org/2528743002/diff/410001/services/shape_detecti...
services/shape_detection/BUILD.gn:34:
"//third_party/WebKit/public:mojo_bindings__generator",
Could you please explain why you need to depend on the __generator target
instead of mojo_bindings?

https://codereview.chromium.org/2528743002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right):

https://codereview.chromium.org/2528743002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:53:
ServiceConnector::instance().ConnectToInterface(
Do we always need both m_faceService and m_barcodeService? could they be
initilalized when needed?

https://codereview.chromium.org/2528743002/diff/430001/content/child/blink_pl...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/2528743002/diff/430001/content/child/blink_pl...
content/child/blink_platform_impl.cc:820: BlinkPlatformImpl::serviceConnector()
{
If such changes are from another CL Foo. You could set this CL (let's call it
Bar) to depend on Foo.

I usually do:

git new-branch branch_for_foo
git cl patch CL_number_for_foo
git checkout -b branch_for_bar
git branch -u branch_for_foo
// develop your change on branch_for_bar
git cl upload

[xianglu, 2016-12-02 01:11:42]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 01:12:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-02 01:56:18]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 01:56:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 05:12:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 05:12:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xianglu, 2016-12-02 08:34:44]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 08:35:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-02 08:35:41]

Patchset #5 (id:470001) has been deleted

[commit-bot: I haz the power, 2016-12-02 10:52:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 10:52:37]

Dry run: This issue passed the CQ dry run.

[xianglu, 2016-12-02 16:15:15]

Patchset #4 (id:450001) has been deleted

[xianglu, 2016-12-02 16:23:46]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 16:24:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-02 16:52:42]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 16:53:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-02 17:12:14]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 17:12:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-02 17:13:25]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 17:13:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-02 18:07:14]

Patchset #6 (id:530001) has been deleted

[xianglu, 2016-12-02 18:11:24]

PTAL.

https://codereview.chromium.org/2528743002/diff/390001/content/browser/servic...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2528743002/diff/390001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:268: ServiceInfo
shape_detection_info;
On 2016/12/01 17:54:30, Ken Rockot wrote:
> You can remove this registration for now. We aren't quite ready to support
> embedded singleton services, so we'll have to accept an instance per profile
> here.

Done.

https://codereview.chromium.org/2528743002/diff/390001/content/public/app/moj...
File content/public/app/mojo/content_browser_manifest.json (left):

https://codereview.chromium.org/2528743002/diff/390001/content/public/app/moj...
content/public/app/mojo/content_browser_manifest.json:18:
"blink::mojom::FaceDetection",
On 2016/12/01 17:54:31, Ken Rockot wrote:
> It looks like there is still renderer code trying to connect to these
interfaces
> on the RenderFrameImpl's interface provider. At least in tests, there are some
> failures related to this.

Done.

https://codereview.chromium.org/2528743002/diff/390001/content/public/app/moj...
File content/public/app/mojo/content_browser_manifest.json (right):

https://codereview.chromium.org/2528743002/diff/390001/content/public/app/moj...
content/public/app/mojo/content_browser_manifest.json:58: "shape_detection": [
"face_detection", "barcode_detection" ]
On 2016/12/01 17:54:30, Ken Rockot wrote:
> This is unnecessary since the browser itself does not need to connect to
> shape_detection.

Done.

https://codereview.chromium.org/2528743002/diff/390001/services/shape_detecti...
File services/shape_detection/README.md (right):

https://codereview.chromium.org/2528743002/diff/390001/services/shape_detecti...
services/shape_detection/README.md:1: Shape Detection Service is an isolated
mojo service that exists to facilitate
On 2016/12/01 17:54:31, Ken Rockot wrote:
> language nit: "mojo service" is redundant, just "service" please

Done.

https://codereview.chromium.org/2528743002/diff/390001/services/shape_detecti...
File services/shape_detection/manifest.json (right):

https://codereview.chromium.org/2528743002/diff/390001/services/shape_detecti...
services/shape_detection/manifest.json:10: "requires": {
On 2016/12/01 17:54:31, Ken Rockot wrote:
> nit: not necessary

Done.

https://codereview.chromium.org/2528743002/diff/390001/services/shape_detecti...
File services/shape_detection/shape_detection_service.h (right):

https://codereview.chromium.org/2528743002/diff/390001/services/shape_detecti...
services/shape_detection/shape_detection_service.h:19:
std::unique_ptr<service_manager::Service> CreateShapeDetectionService();
On 2016/12/01 17:54:31, Ken Rockot wrote:
> nit: Maybe just make it a static Create method on ShapeDetectionService?

Done.

https://codereview.chromium.org/2528743002/diff/410001/services/shape_detecti...
File services/shape_detection/BUILD.gn (right):

https://codereview.chromium.org/2528743002/diff/410001/services/shape_detecti...
services/shape_detection/BUILD.gn:34:
"//third_party/WebKit/public:mojo_bindings__generator",
On 2016/12/01 21:28:26, yzshen1 wrote:
> Could you please explain why you need to depend on the __generator target
> instead of mojo_bindings?

The bots used to fail with error like "invalid include .mojom.h". But it seems
fine now. Changed back.

https://codereview.chromium.org/2528743002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right):

https://codereview.chromium.org/2528743002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:53:
ServiceConnector::instance().ConnectToInterface(
On 2016/12/01 21:28:26, yzshen1 wrote:
> Do we always need both m_faceService and m_barcodeService? could they be
> initilalized when needed?

That's a good point. I separated them to two constructors.

https://codereview.chromium.org/2528743002/diff/430001/content/child/blink_pl...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/2528743002/diff/430001/content/child/blink_pl...
content/child/blink_platform_impl.cc:820: BlinkPlatformImpl::serviceConnector()
{
On 2016/12/01 21:28:26, yzshen1 wrote:
> If such changes are from another CL Foo. You could set this CL (let's call it
> Bar) to depend on Foo.
> 
> I usually do:
> 
> git new-branch branch_for_foo
> git cl patch CL_number_for_foo
> git checkout -b branch_for_bar
> git branch -u branch_for_foo
> // develop your change on branch_for_bar
> git cl upload

Thanks for the instruction!

[xianglu, 2016-12-02 18:13:02]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process.

The following files are copied from https://codereview.chromium.org/2460723003/
and are already under review:
-content/child/blink_platform_impl.h
-content/child/blink_platform_impl.cc
-third_party/WebKit/Source/platform/ServiceConnector.h
-third_party/WebKit/Source/platform/exported/Platform.cpp
-third_party/WebKit/public/platform/Platform.h


BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process.

Based on CL: https://codereview.chromium.org/2460723003/


BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-12-02 18:22:44]

Patchset #5 (id:510001) has been deleted

[commit-bot: I haz the power, 2016-12-02 22:30:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 22:30:05]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-12-05 17:41:20]

https://codereview.chromium.org/2528743002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right):

https://codereview.chromium.org/2528743002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:53:
ServiceConnector::instance().ConnectToInterface(
On 2016/12/02 18:11:24, xianglu wrote:
> On 2016/12/01 21:28:26, yzshen1 wrote:
> > Do we always need both m_faceService and m_barcodeService? could they be
> > initilalized when needed?
> 
> That's a good point. I separated them to two constructors.

Depending on which constructor is called, this instance can only serve one type
of detection task (either barcode or face). In that case, it is redundant to
have |detectorType| parameter in the detectShapes() function signature. Right?

It seems to me you could either (1) eliminate |detectorType|, or (2) still use a
single constructor and lazily initialize |m_barcodeService| / |m_barcodeService|
when needed.

https://codereview.chromium.org/2528743002/diff/550001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/2528743002/diff/550001/content/browser/browse...
content/browser/browser_context.cc:472: // Shape Detection Service is in browser
process for now. If we see crashes
 I feel that this comment is not very useful. Do we expect crashes to be
"normal" for this service?

[xianglu, 2016-12-05 18:50:02]

https://codereview.chromium.org/2528743002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right):

https://codereview.chromium.org/2528743002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:53:
ServiceConnector::instance().ConnectToInterface(
On 2016/12/05 17:41:19, yzshen1 wrote:
> On 2016/12/02 18:11:24, xianglu wrote:
> > On 2016/12/01 21:28:26, yzshen1 wrote:
> > > Do we always need both m_faceService and m_barcodeService? could they be
> > > initilalized when needed?
> > 
> > That's a good point. I separated them to two constructors.
> 
> Depending on which constructor is called, this instance can only serve one
type
> of detection task (either barcode or face). In that case, it is redundant to
> have |detectorType| parameter in the detectShapes() function signature. Right?
> 
> It seems to me you could either (1) eliminate |detectorType|, or (2) still use
a
> single constructor and lazily initialize |m_barcodeService| /
|m_barcodeService|
> when needed.

Yes. ShapeDetector only serves one task, and users call either FaceDetector() or
BarcodeDetector(). Removing detectorType would be a good solution. However, it
might involve quite a bit of code refactoring. I think it's better to include
that in a separate CL.

https://codereview.chromium.org/2528743002/diff/550001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/2528743002/diff/550001/content/browser/browse...
content/browser/browser_context.cc:472: // Shape Detection Service is in browser
process for now. If we see crashes
On 2016/12/05 17:41:19, yzshen1 wrote:
>  I feel that this comment is not very useful. Do we expect crashes to be
> "normal" for this service?

There have been different opinions on the process this service should live in.
Shape Detection is moved into //services/ mostly because we can easily plug it
into another process. I put the comment here as a reminder that this feature is
very experimental and we are open to moving the code around if necessary.

[yzshen1, 2016-12-05 19:38:29]

LGTM (please wait for other code reviewers' LGs)

https://codereview.chromium.org/2528743002/diff/550001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/2528743002/diff/550001/content/browser/browse...
content/browser/browser_context.cc:472: // Shape Detection Service is in browser
process for now. If we see crashes
On 2016/12/05 18:50:02, xianglu wrote:
> On 2016/12/05 17:41:19, yzshen1 wrote:
> >  I feel that this comment is not very useful. Do we expect crashes to be
> > "normal" for this service?
> 
> There have been different opinions on the process this service should live in.
> Shape Detection is moved into //services/ mostly because we can easily plug it
> into another process. I put the comment here as a reminder that this feature
is
> very experimental and we are open to moving the code around if necessary.

I mean, usually we don't consider crashes as something tolerable and should fix
them properly. I understand that we have exceptions such as the gpu process. But
I don't know whether this service falls in the same category.

It probably makes sense to add more context information why moving to a separate
process is a reasonable solution for addressing crashes, and maybe a bug link .

[xianglu, 2016-12-05 19:58:47]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 19:59:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-12-05 20:56:52]

https://codereview.chromium.org/2528743002/diff/570001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/2528743002/diff/570001/content/browser/browse...
content/browser/browser_context.cc:473: // Mac API |CIDetector| does
GPU-previliged operations, and we don't have
nit: s/previliged/privileged/

https://codereview.chromium.org/2528743002/diff/570001/content/browser/servic...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2528743002/diff/570001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:17: #include
"base/strings/utf_string_conversions.h"
nit: Please undo changes to this file since you're no longer registering the
service here

https://codereview.chromium.org/2528743002/diff/570001/services/shape_detecti...
File services/shape_detection/BUILD.gn (right):

https://codereview.chromium.org/2528743002/diff/570001/services/shape_detecti...
services/shape_detection/BUILD.gn:22: }
nit: Instead of subtracting face_detection_impl.cc here and always adding it
above, how about only adding it in an else block here?

https://codereview.chromium.org/2528743002/diff/570001/services/shape_detecti...
services/shape_detection/BUILD.gn:31: public_deps = [
nit: //base should be in public_deps too (technically only needed on mac but i
see no point in making a distinction)

https://codereview.chromium.org/2528743002/diff/570001/services/shape_detecti...
File services/shape_detection/shape_detection_service.cc (right):

https://codereview.chromium.org/2528743002/diff/570001/services/shape_detecti...
services/shape_detection/shape_detection_service.cc:32:
registry->AddInterface(base::Bind(&FaceDetectionImpl::Create));
While I would like to avoid every service needing to do this, I don't have a
better solution yet: Can you please copy the pattern applied to the
image_decoder service here
(https://cs.chromium.org/chromium/src/services/image_decoder/image_decoder_ser...),
i.e. add a connection error handler to |registry| which is bound to a
ServiceContextRef? This ensures that the registry connection itself holds a ref
to the service.

https://codereview.chromium.org/2528743002/diff/570001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/DEPS (right):

https://codereview.chromium.org/2528743002/diff/570001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/DEPS:12:
"+services/service_manager/public",
no longer needed

[xianglu, 2016-12-05 21:25:13]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 21:26:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-05 22:13:22]

PTAL.

https://codereview.chromium.org/2528743002/diff/570001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/2528743002/diff/570001/content/browser/browse...
content/browser/browser_context.cc:473: // Mac API |CIDetector| does
GPU-previliged operations, and we don't have
On 2016/12/05 20:56:52, Ken Rockot wrote:
> nit: s/previliged/privileged/

Done.

https://codereview.chromium.org/2528743002/diff/570001/content/browser/servic...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2528743002/diff/570001/content/browser/servic...
content/browser/service_manager/service_manager_context.cc:17: #include
"base/strings/utf_string_conversions.h"
On 2016/12/05 20:56:52, Ken Rockot wrote:
> nit: Please undo changes to this file since you're no longer registering the
> service here

Done.

https://codereview.chromium.org/2528743002/diff/570001/services/shape_detecti...
File services/shape_detection/BUILD.gn (right):

https://codereview.chromium.org/2528743002/diff/570001/services/shape_detecti...
services/shape_detection/BUILD.gn:22: }
On 2016/12/05 20:56:52, Ken Rockot wrote:
> nit: Instead of subtracting face_detection_impl.cc here and always adding it
> above, how about only adding it in an else block here?

Done.

https://codereview.chromium.org/2528743002/diff/570001/services/shape_detecti...
services/shape_detection/BUILD.gn:31: public_deps = [
On 2016/12/05 20:56:52, Ken Rockot wrote:
> nit: //base should be in public_deps too (technically only needed on mac but i
> see no point in making a distinction)

Done.

https://codereview.chromium.org/2528743002/diff/570001/services/shape_detecti...
File services/shape_detection/shape_detection_service.cc (right):

https://codereview.chromium.org/2528743002/diff/570001/services/shape_detecti...
services/shape_detection/shape_detection_service.cc:32:
registry->AddInterface(base::Bind(&FaceDetectionImpl::Create));
On 2016/12/05 20:56:52, Ken Rockot wrote:
> While I would like to avoid every service needing to do this, I don't have a
> better solution yet: Can you please copy the pattern applied to the
> image_decoder service here
>
(https://cs.chromium.org/chromium/src/services/image_decoder/image_decoder_ser...),
> i.e. add a connection error handler to |registry| which is bound to a
> ServiceContextRef? This ensures that the registry connection itself holds a
ref
> to the service.

Done.

https://codereview.chromium.org/2528743002/diff/570001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/DEPS (right):

https://codereview.chromium.org/2528743002/diff/570001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/DEPS:12:
"+services/service_manager/public",
On 2016/12/05 20:56:52, Ken Rockot wrote:
> no longer needed

Done.

[Ken Rockot(use gerrit already), 2016-12-05 22:15:23]

lgtm

https://codereview.chromium.org/2528743002/diff/590001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/BUILD.gn (right):

https://codereview.chromium.org/2528743002/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/BUILD.gn:166:
"//services/service_manager/public/cpp",
Oops, missed this. Please remove this too, not allowed and not needed.

[xianglu, 2016-12-05 22:41:51]

xianglu@chromium.org changed reviewers:
+ avi@chromium.org, haraken@chromium.org, tsepez@chromium.org

[xianglu, 2016-12-05 22:41:52]

Thanks.
tsepez@, PTAL at content/public/app/mojo/*, trivial.
avi@, PTAL at content/browser/* and content/public/*
haraken@, PTAL at third_party/WebKit/*

[mcasas, 2016-12-06 00:04:24]

some small comments

https://codereview.chromium.org/2528743002/diff/590001/services/shape_detecti...
File services/shape_detection/face_detection_impl_mac.mm (right):

https://codereview.chromium.org/2528743002/diff/590001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:15: // kCIFormatRGBA8 is not
exposed to public until Mac 10.11. So we define s
nit: s/s/the/ (at the end of the sentence).

https://codereview.chromium.org/2528743002/diff/590001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:17: #if
!defined(MAC_OS_X_VERSION_10_11)
We should use here

#if MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_11

to include e.g. 10.12 etc.

https://codereview.chromium.org/2528743002/diff/590001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:53:
callback.Run(std::move(faces));
Having to Run the callback with an empty Face array 
is error prone. Instead, and for similar reasons, there's
media::ScopedResultCallback<>:

https://cs.chromium.org/chromium/src/media/capture/video/scoped_result_callba...

that holds a callback and a way of calling it if error:
|on_error_callback|. This |on_error_callback| is also
called  if the scoped callback is destroyed without 
having been Run().

In this case, we can define two static functions:

void RunCallbackWithFaces(const DetectCallback& callback,
                          blink::mojom::FaceDetectionResultPtr faces) {
  callback.Run(std::move(faces));
}

void RunCallbackWithNoFaces(const DetectCallback& callback) {
  callback.Run(blink::mojom::FaceDetectionResult::New());
}

and instead of l.45:

  media::ScopedResultCallback<DetectCallback> scoped_callback(
      base::Bind(&RunCallbackWithFaces, callback),
      base::Bind(&RunCallbackWithNoFaces));

then you can remove the callback.Run(std::move(faces));
and in l.108 simply do
  scoped_callback.Run(std::move(faces));

https://codereview.chromium.org/2528743002/diff/590001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:62:
DCHECK_EQ(MOJO_RESULT_OK, result) << "Failed to unwrap redBufferHandle";
nit: s/redBufferHandle/SharedBuffer/ ?

https://codereview.chromium.org/2528743002/diff/590001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:85:
base::scoped_nsobject<CIImage> ciimage([[CIImage alloc]
s/ciimage/ci_image/

[haraken, 2016-12-06 00:07:44]

WebKit LGTM/ % the change to BUILD.gn needs to be removed.

[commit-bot: I haz the power, 2016-12-06 00:25:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 00:25:27]

Dry run: This issue passed the CQ dry run.

[xianglu, 2016-12-06 02:29:40]

Patchset #8 (id:610001) has been deleted

[Avi (use Gerrit), 2016-12-06 05:44:19]

avi@chromium.org changed reviewers:
+ rsesek@chromium.org

[Avi (use Gerrit), 2016-12-06 05:44:21]

Hmmm...

https://codereview.chromium.org/2528743002/diff/630001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/2528743002/diff/630001/content/browser/browse...
content/browser/browser_context.cc:475: // we see crashes in the future.
I'm not sure that this solves the concerns of running the code in the browser
process. Adding Robert.

https://codereview.chromium.org/2528743002/diff/630001/services/shape_detecti...
File services/shape_detection/face_detection_impl_mac.mm (right):

https://codereview.chromium.org/2528743002/diff/630001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:72: new
base::SharedMemory(memory_handle, true /* read_only */));
Can we do

auto shared_memory = base::MakeUnique<base::SharedMemory>(...)

? That will help us move to c++14's make_unique.

https://codereview.chromium.org/2528743002/diff/630001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:85: // CIImage will return
nil when RGBA8 is not supported in a certain sion.
"sion"? Do you mean version?

Have you actually tested this code on earlier versions of the macOS? Which
versions are actually supported?

https://codereview.chromium.org/2528743002/diff/630001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:104: // We need to adjust
|y| coordinate of bounding box before sending it .
no space before the .

[Tom Sepez, 2016-12-06 17:35:55]

The mojoms/manifests are fine, but we really want to run this from a
partially-sandboxed utility processes.

[xianglu, 2016-12-07 02:30:49]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process.

Based on CL: https://codereview.chromium.org/2460723003/


BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process.

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-12-07 02:32:38]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes happen, we should consider moving it to a utility process.

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-12-07 02:35:16]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.

Note: We haven't seen any crashes on this API, so we believe there is no need to
create a separate process for it. 

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-12-07 02:36:21]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.

Note: We haven't seen any crashes on this API, so we believe there is no need to
create a separate process for it. 

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.

Note: We haven't seen any crashes on this API, so we believe there is no need
to create a separate process for it. Please feel free to comment on the design
doc below.

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-12-07 20:23:08]

Patchset #10 (id:670001) has been deleted

[xianglu, 2016-12-07 21:12:05]

Patchset #9 (id:650001) has been deleted

[xianglu, 2016-12-07 21:14:59]

Patchset #9 (id:690001) has been deleted

[xianglu, 2016-12-07 21:24:43]

Patchset #9 (id:710001) has been deleted

[xianglu, 2016-12-07 22:56:11]

Patchset #9 (id:730001) has been deleted

[xianglu, 2016-12-07 23:30:27]

Patchset #9 (id:750001) has been deleted

[xianglu, 2016-12-08 00:03:39]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 00:04:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-08 00:05:46]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[xianglu, 2016-12-08 00:06:12]

Patchset #9 (id:770001) has been deleted

[xianglu, 2016-12-08 00:06:21]

Patchset #9 (id:790001) has been deleted

[commit-bot: I haz the power, 2016-12-08 00:06:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 00:09:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 00:09:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xianglu, 2016-12-08 00:16:16]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.

Note: We haven't seen any crashes on this API, so we believe there is no need
to create a separate process for it. Please feel free to comment on the design
doc below.

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.

Note: 
Shape Detection Service implementation calls platform specific API. These native
APIs are stable and no crash is seen so far. We believe there is no need yet to
create a separate process for it. Please feel free to comment on the design
doc below.

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-12-08 00:16:42]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.

Note: 
Shape Detection Service implementation calls platform specific API. These native
APIs are stable and no crash is seen so far. We believe there is no need yet to
create a separate process for it. Please feel free to comment on the design
doc below.

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.

Note: 
Shape Detection Service implementation calls platform specific API. These native
APIs are stable and no crash is seen so far. We believe there is no need yet to
create a separate process for it. Please feel free to comment on the design doc
below.

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-12-08 00:17:48]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.

Note: 
Shape Detection Service implementation calls platform specific API. These native
APIs are stable and no crash is seen so far. We believe there is no need yet to
create a separate process for it. Please feel free to comment on the design doc
below.

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.

Note: 
Shape Detection Service implementation calls platform specific API. These
native APIs are stable and no crash is seen so far. We believe that there is no
need yet to create a separate process for it. Please feel free to comment on the
design doc below.

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-12-08 00:26:20]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 00:26:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-08 00:26:54]

Patchset #9 (id:810001) has been deleted

[xianglu, 2016-12-08 00:28:48]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[xianglu, 2016-12-08 00:28:55]

Patchset #9 (id:830001) has been deleted

[commit-bot: I haz the power, 2016-12-08 00:29:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-08 00:32:39]

Patchset #9 (id:850001) has been deleted

[xianglu, 2016-12-08 00:39:41]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.

Note: 
Shape Detection Service implementation calls platform specific API. These
native APIs are stable and no crash is seen so far. We believe that there is no
need yet to create a separate process for it. Please feel free to comment on the
design doc below.

Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.


Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

[xianglu, 2016-12-08 01:09:48]

On 2016/12/06 17:35:55, Tom Sepez wrote:
> The mojoms/manifests are fine, but we really want to run this from a
> partially-sandboxed utility processes.

Sorry about the misleading comment. Actually this API is stable and has been
used for years. We've also tested it  and no crash is seen so far. So we believe
that a separate process just for it is not yet necessary.

Please see the design doc for it: http://tinyurl.com/shape-detection-in-chromium

[xianglu, 2016-12-08 01:20:54]

PTAL.

https://codereview.chromium.org/2528743002/diff/590001/services/shape_detecti...
File services/shape_detection/face_detection_impl_mac.mm (right):

https://codereview.chromium.org/2528743002/diff/590001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:15: // kCIFormatRGBA8 is not
exposed to public until Mac 10.11. So we define s
On 2016/12/06 00:04:24, mcasas wrote:
> nit: s/s/the/ (at the end of the sentence).

Done.

https://codereview.chromium.org/2528743002/diff/590001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:17: #if
!defined(MAC_OS_X_VERSION_10_11)
On 2016/12/06 00:04:24, mcasas wrote:
> We should use here
> 
> #if MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_11
> 
> to include e.g. 10.12 etc.

Done.

https://codereview.chromium.org/2528743002/diff/590001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:53:
callback.Run(std::move(faces));
On 2016/12/06 00:04:23, mcasas wrote:
> Having to Run the callback with an empty Face array 
> is error prone. Instead, and for similar reasons, there's
> media::ScopedResultCallback<>:
> 
>
https://cs.chromium.org/chromium/src/media/capture/video/scoped_result_callba...
> 
> that holds a callback and a way of calling it if error:
> |on_error_callback|. This |on_error_callback| is also
> called  if the scoped callback is destroyed without 
> having been Run().
> 
> In this case, we can define two static functions:
> 
> void RunCallbackWithFaces(const DetectCallback& callback,
>                           blink::mojom::FaceDetectionResultPtr faces) {
>   callback.Run(std::move(faces));
> }
> 
> void RunCallbackWithNoFaces(const DetectCallback& callback) {
>   callback.Run(blink::mojom::FaceDetectionResult::New());
> }
> 
> and instead of l.45:
> 
>   media::ScopedResultCallback<DetectCallback> scoped_callback(
>       base::Bind(&RunCallbackWithFaces, callback),
>       base::Bind(&RunCallbackWithNoFaces));
> 
> then you can remove the callback.Run(std::move(faces));
> and in l.108 simply do
>   scoped_callback.Run(std::move(faces));

Done.

https://codereview.chromium.org/2528743002/diff/590001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:62:
DCHECK_EQ(MOJO_RESULT_OK, result) << "Failed to unwrap redBufferHandle";
On 2016/12/06 00:04:24, mcasas wrote:
> nit: s/redBufferHandle/SharedBuffer/ ?

Done.

https://codereview.chromium.org/2528743002/diff/590001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:85:
base::scoped_nsobject<CIImage> ciimage([[CIImage alloc]
On 2016/12/06 00:04:23, mcasas wrote:
> s/ciimage/ci_image/ 

Done.

https://codereview.chromium.org/2528743002/diff/590001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/BUILD.gn (right):

https://codereview.chromium.org/2528743002/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/BUILD.gn:166:
"//services/service_manager/public/cpp",
On 2016/12/05 22:15:22, Ken Rockot wrote:
> Oops, missed this. Please remove this too, not allowed and not needed.

Done.

https://codereview.chromium.org/2528743002/diff/630001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/2528743002/diff/630001/content/browser/browse...
content/browser/browser_context.cc:475: // we see crashes in the future.
On 2016/12/06 05:44:21, Avi (at offsite M-Th) wrote:
> I'm not sure that this solves the concerns of running the code in the browser
> process. Adding Robert.

My apologies. This comment is too misleading. I meant hypothetical but yet
unseen API crashes. We tested this API on different versions of MacOS and it was
very stable. So we believe there is no need yet to create a separate process for
it.

https://codereview.chromium.org/2528743002/diff/630001/services/shape_detecti...
File services/shape_detection/face_detection_impl_mac.mm (right):

https://codereview.chromium.org/2528743002/diff/630001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:72: new
base::SharedMemory(memory_handle, true /* read_only */));
On 2016/12/06 05:44:21, Avi (at offsite M-Th) wrote:
> Can we do
> 
> auto shared_memory = base::MakeUnique<base::SharedMemory>(...)
> 
> ? That will help us move to c++14's make_unique.

Done.

https://codereview.chromium.org/2528743002/diff/630001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:85: // CIImage will return
nil when RGBA8 is not supported in a certain sion.
On 2016/12/06 05:44:21, Avi (at offsite M-Th) wrote:
> "sion"? Do you mean version?
> 
> Have you actually tested this code on earlier versions of the macOS? Which
> versions are actually supported?

Yes. The format is supported since 10.4.

https://codereview.chromium.org/2528743002/diff/630001/services/shape_detecti...
services/shape_detection/face_detection_impl_mac.mm:104: // We need to adjust
|y| coordinate of bounding box before sending it .
On 2016/12/06 05:44:21, Avi (at offsite M-Th) wrote:
> no space before the .

Done.

[Avi (use Gerrit), 2016-12-08 04:47:10]

On 2016/12/08 01:09:48, xianglu wrote:
> Actually this API is stable and has been
> used for years. We've also tested it  and no crash is seen so far. So we
believe
> that a separate process just for it is not yet necessary.

The issue isn't stability. We're not interested in putting this in a utility
process because it's crashy.

The issue is security. You are taking arbitrary web data and processing it in
the privileged browser process. In addition, you are not even processing it
using your code, but passing it to a system function over whose implementation
you have no control. *That* is the concern. Your statements about the stability
of the API and that you haven't seen it crash yet does not address this issue.

[Ken Rockot(use gerrit already), 2016-12-08 05:06:24]

Alright, if this CL can wait another 2-3 days to land, I should have
singleton service registration working by then and you'll be able to
register for out-of-process execution. Sorry for the trouble!

On Wed, Dec 7, 2016 at 8:47 PM, <avi@chromium.org> wrote:

> On 2016/12/08 01:09:48, xianglu wrote:
> > Actually this API is stable and has been
> > used for years. We've also tested it and no crash is seen so far. So we
> believe
> > that a separate process just for it is not yet necessary.
>
> The issue isn't stability. We're not interested in putting this in a
> utility
> process because it's crashy.
>
> The issue is security. You are taking arbitrary web data and processing it
> in
> the privileged browser process. In addition, you are not even processing it
> using your code, but passing it to a system function over whose
> implementation
> you have no control. *That* is the concern. Your statements about the
> stability
> of the API and that you haven't seen it crash yet does not address this
> issue.
>
> https://codereview.chromium.org/2528743002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xianglu, 2016-12-22 01:39:30]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 01:39:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 01:42:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 01:42:32]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[xianglu, 2016-12-22 22:28:47]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 22:29:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 22:33:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 22:33:45]

Dry run: 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...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_...)

[xianglu, 2016-12-22 22:38:52]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 22:39:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-22 22:39:23]

Patchset #12 (id:930001) has been deleted

[commit-bot: I haz the power, 2016-12-22 22:51:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 22:51:51]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xianglu, 2016-12-22 23:02:39]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[xianglu, 2016-12-22 23:02:52]

Patchset #12 (id:950001) has been deleted

[commit-bot: I haz the power, 2016-12-22 23:03:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-22 23:59:30]

Patchset #11 (id:910001) has been deleted

[commit-bot: I haz the power, 2016-12-23 00:04:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 00:04:43]

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, 2016-12-23 01:04:16]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 01:04:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2016-12-23 01:05:06]

Patchset #11 (id:970001) has been deleted

[commit-bot: I haz the power, 2016-12-23 02:11:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 02:11:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2016-12-27 18:22:03]

https://codereview.chromium.org/2528743002/diff/990001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/2528743002/diff/990001/content/browser/browse...
content/browser/browser_context.cc:474:
connection->AddEmbeddedService(shape_detection::mojom::kServiceName,
You can remove this now.

https://codereview.chromium.org/2528743002/diff/990001/content/utility/utilit...
File content/utility/utility_service_factory.cc (right):

https://codereview.chromium.org/2528743002/diff/990001/content/utility/utilit...
content/utility/utility_service_factory.cc:37: //
services->insert(std::make_pair(shape_detection::mojom::kServiceName,
And you can re-enable this

https://codereview.chromium.org/2528743002/diff/990001/services/shape_detecti...
File services/shape_detection/manifest.json (right):

https://codereview.chromium.org/2528743002/diff/990001/services/shape_detecti...
services/shape_detection/manifest.json:10: }
If you add this block within "service_manager:connector", next to "provides":

  "requires": {
    "service_manager": [ "service_manager:all_users" ]
  }

the service should operate as a singleton shared by all renderers, and your OOP
utility launching should just work.

[Tom Sepez, 2017-01-03 18:53:20]

On 2016/12/08 01:09:48, xianglu wrote:
> On 2016/12/06 17:35:55, Tom Sepez wrote:
> > The mojoms/manifests are fine, but we really want to run this from a
> > partially-sandboxed utility processes.
> 
> Sorry about the misleading comment. Actually this API is stable and has been
> used for years. We've also tested it  and no crash is seen so far. So we
believe
> that a separate process just for it is not yet necessary.
> 
> Please see the design doc for it:
http://tinyurl.com/shape-detection-in-chromium

That hardly seems like a justification for non-sanboxing, e.g. just because it
hasn't crashed doesn't mean that there aren't lurking vulnerabilities in the
underlying code, into which we have no visibility.

[jschuh, 2017-01-03 19:03:00]

On 2017/01/03 18:53:20, Tom Sepez wrote:
> On 2016/12/08 01:09:48, xianglu wrote:
> > On 2016/12/06 17:35:55, Tom Sepez wrote:
> > > The mojoms/manifests are fine, but we really want to run this from a
> > > partially-sandboxed utility processes.
> > 
> > Sorry about the misleading comment. Actually this API is stable and has been
> > used for years. We've also tested it  and no crash is seen so far. So we
> believe
> > that a separate process just for it is not yet necessary.
> > 
> > Please see the design doc for it:
> http://tinyurl.com/shape-detection-in-chromium
> 
> That hardly seems like a justification for non-sanboxing, e.g. just because it
> hasn't crashed doesn't mean that there aren't lurking vulnerabilities in the
> underlying code, into which we have no visibility.

To put it more directly, we have sandboxes precisely because these kinds of
system APIs are historically prone to serious vulnerabilities.

[xianglu, 2017-01-04 00:02:07]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 00:02:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-04 00:10:28]

Patchset #10 (id:890001) has been deleted

[xianglu, 2017-01-04 00:20:53]

Patchset #12 (id:1030001) has been deleted

[xianglu, 2017-01-04 20:38:41]

Patchset #12 (id:1050001) has been deleted

[xianglu, 2017-01-06 01:54:44]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.


Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
==========

to

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.


Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[xianglu, 2017-01-06 01:55:36]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 01:56:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-06 01:56:56]

Patchset #11 (id:1010001) has been deleted

[commit-bot: I haz the power, 2017-01-06 01:59:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 01:59:46]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[xianglu, 2017-01-06 02:32:21]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[xianglu, 2017-01-06 02:32:44]

Patchset #12 (id:1090001) has been deleted

[xianglu, 2017-01-06 02:33:01]

Patchset #11 (id:1070001) has been deleted

[commit-bot: I haz the power, 2017-01-06 02:33:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 02:37:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 02:37:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_...)

[xianglu, 2017-01-06 17:01:54]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[xianglu, 2017-01-06 17:02:30]

Patchset #11 (id:1110001) has been deleted

[commit-bot: I haz the power, 2017-01-06 17:02:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 17:05:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 17:05:39]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[xianglu, 2017-01-06 18:31:58]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 18:33:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-06 18:34:55]

Patchset #11 (id:1130001) has been deleted

[xianglu, 2017-01-06 19:36:23]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 19:36:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 20:35:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 20:35:51]

Dry run: 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_...)

[xianglu, 2017-01-06 20:42:45]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 20:43:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-06 20:44:00]

Patchset #11 (id:1150001) has been deleted

[commit-bot: I haz the power, 2017-01-06 21:11:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 21:11:28]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[xianglu, 2017-01-06 21:35:05]

Patchset #11 (id:1170001) has been deleted

[xianglu, 2017-01-06 21:56:31]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 21:56:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-06 21:59:37]

Patchset #9 (id:870001) has been deleted

[commit-bot: I haz the power, 2017-01-06 22:48:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 22:48:30]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[xianglu, 2017-01-06 23:10:09]

Patchset #10 (id:1190001) has been deleted

[xianglu, 2017-01-06 23:38:42]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 23:39:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 00:24:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-07 00:24:48]

Dry run: 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_...)

[xianglu, 2017-01-07 00:36:29]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 00:36:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-07 00:41:47]

Patchset #11 (id:1230001) has been deleted

[xianglu, 2017-01-07 00:42:03]

Patchset #10 (id:1210001) has been deleted

[commit-bot: I haz the power, 2017-01-07 01:34:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-07 01:34:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xianglu, 2017-01-07 02:30:07]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 02:30:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-07 02:30:24]

The CQ bit was unchecked by xianglu@chromium.org

[xianglu, 2017-01-07 02:37:42]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 02:37:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 02:50:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-07 02:50:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xianglu, 2017-01-09 06:08:23]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 06:08:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 06:10:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 06:10:28]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[xianglu, 2017-01-10 23:19:47]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[xianglu, 2017-01-10 23:20:39]

Description was changed from

==========
ShapeDetection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.


Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Shape Detection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.


Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-01-10 23:20:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-10 23:35:09]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 23:35:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-10 23:39:36]

Patchset #11 (id:1270001) has been deleted

[xianglu, 2017-01-10 23:55:09]

Description was changed from

==========
Shape Detection: Move FaceDetection implementation to isolated mojo service.

This CL moves implementation of FaceDetection under services/shape_detection/ 
(temporally for MACOS only). Shape Detection mojo service can be hosted in
different processes. In this CL the service runs in the browser process, and 
if crashes (yet unseen) happen, we can consider moving it to a utility process.


Based on CL: https://codereview.chromium.org/2460723003/

DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Shape Detection: Implement FaceDetection on MACOS

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from Blink to a Utility process, which uses Mac's native
CoreImage library and returns detected faces.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[xianglu, 2017-01-10 23:56:06]

Description was changed from

==========
Shape Detection: Implement FaceDetection on MACOS

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from Blink to a Utility process, which uses Mac's native
CoreImage library and returns detected faces.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Shape Detection: Implement FaceDetection on Mac

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from Blink to a Utility process, which uses Mac's native
CoreImage library and returns detected faces.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-01-11 00:43:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 00:43:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xianglu, 2017-01-11 18:07:16]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 18:07:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 18:12:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 18:12:56]

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_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xianglu, 2017-01-11 21:04:46]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 21:05:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 22:23:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 22:23:24]

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, 2017-01-11 23:36:18]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 23:37:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-12 00:15:42]

Patchset #12 (id:1310001) has been deleted

[commit-bot: I haz the power, 2017-01-12 00:45:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 00:45:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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, 2017-01-12 01:53:01]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 01:53:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 02:04:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 02:04:09]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[xianglu, 2017-01-12 17:01:54]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 17:02:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-12 17:02:45]

Patchset #12 (id:1330001) has been deleted

[xianglu, 2017-01-12 17:03:02]

Patchset #13 (id:1370001) has been deleted

[xianglu, 2017-01-12 17:04:09]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 17:04:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-12 17:05:01]

Patchset #12 (id:1350001) has been deleted

[commit-bot: I haz the power, 2017-01-12 17:55:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 17:55:59]

Dry run: 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_...)

[xianglu, 2017-01-12 19:46:43]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 19:47:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-12 20:55:00]

Patchset #12 (id:1390001) has been deleted

[commit-bot: I haz the power, 2017-01-12 20:58:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 20:58:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xianglu, 2017-01-13 02:49:07]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 02:49:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 05:40:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 05:40:38]

Dry run: This issue passed the CQ dry run.

[xianglu, 2017-01-13 07:17:39]

Patchset #12 (id:1410001) has been deleted

[xianglu, 2017-01-13 07:18:59]

Description was changed from

==========
Shape Detection: Implement FaceDetection on Mac

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from Blink to a Utility process, which uses Mac's native
CoreImage library and returns detected faces.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Shape Detection: Implement FaceDetection on Mac

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from Blink to a Utility process, which uses Mac's native
CoreImage library to detect faces and returns the result.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[xianglu, 2017-01-13 07:22:21]

Description was changed from

==========
Shape Detection: Implement FaceDetection on Mac

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from Blink to a Utility process, which uses Mac's native
CoreImage library to detect faces and returns the result.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Shape Detection: Implement FaceDetection on Mac as out-of-process service

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from Renderer process to a Utility process, which detect faces
using Mac native CoreImage library and returns the result.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[xianglu, 2017-01-13 07:22:47]

Description was changed from

==========
Shape Detection: Implement FaceDetection on Mac as out-of-process service

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from Renderer process to a Utility process, which detect faces
using Mac native CoreImage library and returns the result.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Shape Detection: Implement FaceDetection on Mac as out-of-process service

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from Renderer process to a Utility process, which detect
faces using Mac native CoreImage library and returns the result.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[xianglu, 2017-01-13 07:23:47]

Description was changed from

==========
Shape Detection: Implement FaceDetection on Mac as out-of-process service

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from Renderer process to a Utility process, which detect
faces using Mac native CoreImage library and returns the result.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Shape Detection: Implement FaceDetection on Mac as out-of-process service

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from the Renderer process to a Utility process, which detects
faces using Mac native CoreImage library and returns detection result.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[xianglu, 2017-01-13 08:10:49]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 08:11:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-13 08:21:13]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 08:21:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-13 08:42:58]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 08:43:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 11:56:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 11:56:42]

Dry run: This issue passed the CQ dry run.

[xianglu, 2017-01-13 14:18:35]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 14:18:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-13 14:20:03]

Patchset #15 (id:1490001) has been deleted

[xianglu, 2017-01-13 14:29:16]

Patchset #13 (id:1450001) has been deleted

[xianglu, 2017-01-13 14:30:34]

Patchset #13 (id:1470001) has been deleted

[xianglu, 2017-01-13 14:32:12]

Patchset #10 (id:1250001) has been deleted

[xianglu, 2017-01-13 14:33:45]

Patchset #11 (id:1430001) has been deleted

[xianglu, 2017-01-13 14:34:52]

Patchset #10 (id:1290001) has been deleted

[xianglu, 2017-01-13 14:39:46]

Thank you all for pointing out the issue. I've moved face detection
implementation to out-of-process service. The browser is going to forward the
request to service manager, which starts the service in a utility process.

rsesek@ could you take a look please? Thanks

[commit-bot: I haz the power, 2017-01-13 16:26:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 16:26:43]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2017-01-13 23:42:12]

Mojom and design LGTM, but get rsesek sign-off on implementation details.

[Robert Sesek, 2017-01-17 20:30:50]

On 2017/01/03 19:03:00, jschuh wrote:
> To put it more directly, we have sandboxes precisely because these kinds of
> system APIs are historically prone to serious vulnerabilities.

I just want to call attention to Justin's point again...

https://codereview.chromium.org/2528743002/diff/1510001/content/browser/servi...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2528743002/diff/1510001/content/browser/servi...
content/browser/service_manager/service_manager_context.cc:316:
unsandboxed_services.insert(
What is the plan for sandboxing this?

https://codereview.chromium.org/2528743002/diff/1510001/content/browser/servi...
content/browser/service_manager/service_manager_context.cc:318:
base::ASCIIToUTF16("Shape Detection Service")));
Can this not use the display name from the mojom too?

[Ken Rockot(use gerrit already), 2017-01-17 20:45:01]

On Tue, Jan 17, 2017 at 12:30 PM, <rsesek@chromium.org> wrote:

> On 2017/01/03 19:03:00, jschuh wrote:
> > To put it more directly, we have sandboxes precisely because these kinds
> of
> > system APIs are historically prone to serious vulnerabilities.
>
> I just want to call attention to Justin's point again...
>
>
> https://codereview.chromium.org/2528743002/diff/1510001/
> content/browser/service_manager/service_manager_context.cc
> File content/browser/service_manager/service_manager_context.cc (right):
>
> https://codereview.chromium.org/2528743002/diff/1510001/
> content/browser/service_manager/service_manager_context.cc#newcode316
> content/browser/service_manager/service_manager_context.cc:316:
> unsandboxed_services.insert(
> What is the plan for sandboxing this?
>
> https://codereview.chromium.org/2528743002/diff/1510001/
> content/browser/service_manager/service_manager_context.cc#newcode318
> content/browser/service_manager/service_manager_context.cc:318:
> base::ASCIIToUTF16("Shape Detection Service")));
> Can this not use the display name from the mojom too?
>

Drive-by: We don't really have a convenient way of exposing that metadata
yet, and we're in the process of changing how these processes are launched
anyway. It would probably not be worth the effort to plumb the data through
to this place at this time.


> https://codereview.chromium.org/2528743002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xianglu, 2017-01-17 22:37:29]

https://codereview.chromium.org/2528743002/diff/1510001/content/browser/servi...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2528743002/diff/1510001/content/browser/servi...
content/browser/service_manager/service_manager_context.cc:316:
unsandboxed_services.insert(
On 2017/01/17 20:30:50, Robert Sesek wrote:
> What is the plan for sandboxing this?

I think we probably don't need to sandbox this for now since browser process is
only used to forward the request to utility process. After that renderer is
connected to service directly. Nothing is sent back to browser process. So even
if utility process is compromised, it won't affect the browser.

[Robert Sesek, 2017-01-18 05:29:21]

https://codereview.chromium.org/2528743002/diff/1510001/content/browser/servi...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2528743002/diff/1510001/content/browser/servi...
content/browser/service_manager/service_manager_context.cc:316:
unsandboxed_services.insert(
On 2017/01/17 22:37:29, xianglu wrote:
> On 2017/01/17 20:30:50, Robert Sesek wrote:
> > What is the plan for sandboxing this?
> 
> I think we probably don't need to sandbox this for now since browser process
is
> only used to forward the request to utility process. After that renderer is
> connected to service directly. Nothing is sent back to browser process. So
even
> if utility process is compromised, it won't affect the browser.

An un-sandboxed utility process has the same capabilities and security principal
as the browser process. Isolating this code in a utility process is a good and
necessary first step, and it increases the robustness in terms of fault (crash)
tolerance. But from the security perspective, there is little difference between
an un-sandboxed utility process and the browser process.

[Robert Sesek, 2017-01-23 19:52:16]

Met with Xianglu, Ken, and Miguel offline. We discussed that this must be
sandboxed before it can be exposed to the web without a flag, but in the
interest of letting this CL land, LGTM.

[xianglu, 2017-01-23 20:41:23]

The CQ bit was checked by xianglu@chromium.org

[xianglu, 2017-01-23 20:41:24]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
rockot@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2528743002/#ps1510001
(title: "Launch shape detection service as a utility process")

[xianglu, 2017-01-23 20:41:41]

The CQ bit was unchecked by xianglu@chromium.org

[xianglu, 2017-01-23 21:35:39]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 21:36:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-24 00:21:39]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 00:22:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-24 00:51:29]

Patchset #11 (id:1530001) has been deleted

[xianglu, 2017-01-24 00:59:08]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 00:59:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-24 00:59:55]

Patchset #11 (id:1550001) has been deleted

[commit-bot: I haz the power, 2017-01-24 02:33:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 02:33:33]

Dry run: This issue passed the CQ dry run.

[xianglu, 2017-01-24 02:38:54]

The CQ bit was checked by xianglu@chromium.org

[xianglu, 2017-01-24 02:38:55]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
haraken@chromium.org, rsesek@chromium.org, yzshen@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2528743002/#ps1570001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-24 02:39:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 02:50:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 02:50:28]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mcasas, 2017-01-24 03:43:21]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2017-01-24 03:43:23]

lgtm

[commit-bot: I haz the power, 2017-01-24 03:43:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 03:52:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 03:52:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xianglu, 2017-01-24 05:38:37]

xianglu@chromium.org changed reviewers:
+ nick@chromium.org

[xianglu, 2017-01-24 07:02:17]

xianglu@chromium.org changed reviewers:
+ jochen@chromium.org
- nick@chromium.org

[xianglu, 2017-01-24 07:05:44]

jochen@, could you take a look at //content please? Thanks.

[jochen (gone - plz use gerrit), 2017-01-24 12:31:59]

lgtm with nit

https://codereview.chromium.org/2528743002/diff/1570001/content/browser/shape...
File content/browser/shapedetection/face_detection_service_dispatcher.h (right):

https://codereview.chromium.org/2528743002/diff/1570001/content/browser/shape...
content/browser/shapedetection/face_detection_service_dispatcher.h:16: class
CONTENT_EXPORT FaceDetectionServiceDispatcher {
nit. use namespace FaceDetectionServiceDispatcher {} instead of a class

[xianglu, 2017-01-24 18:33:34]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 18:34:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 18:50:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 18:50:39]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[xianglu, 2017-01-24 18:56:20]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[xianglu, 2017-01-24 18:57:20]

Patchset #12 (id:1590001) has been deleted

[commit-bot: I haz the power, 2017-01-24 18:57:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 21:33:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 21:33:34]

Dry run: This issue passed the CQ dry run.

[xianglu, 2017-01-24 21:58:46]

The CQ bit was checked by xianglu@chromium.org

[xianglu, 2017-01-24 21:58:47]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jochen@chromium.org, haraken@chromium.org, mcasas@chromium.org,
yzshen@chromium.org, rsesek@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2528743002/#ps1610001
(title: "jochen@: namespace FaceDetectionServiceDispatcher")

[commit-bot: I haz the power, 2017-01-24 21:59:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 22:08:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 22:08:59]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xianglu, 2017-01-24 22:21:52]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[xianglu, 2017-01-24 22:22:24]

The CQ bit was checked by xianglu@chromium.org

[xianglu, 2017-01-24 22:22:25]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jochen@chromium.org, haraken@chromium.org, mcasas@chromium.org,
yzshen@chromium.org, rsesek@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2528743002/#ps1630001
(title: "jochen@: namespace FaceDetectionServiceDispatcher")

[commit-bot: I haz the power, 2017-01-24 22:22:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-25 01:20:04]

The CQ bit was checked by xianglu@chromium.org

[xianglu, 2017-01-25 01:20:04]

The CQ bit was unchecked by xianglu@chromium.org

[commit-bot: I haz the power, 2017-01-25 01:20:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 01:20:14]

Failed to apply patch for content/public/app/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: content/public/app/BUILD.gn:181
  error: content/public/app/BUILD.gn: patch does not apply

Patch:       content/public/app/BUILD.gn
Index: content/public/app/BUILD.gn
diff --git a/content/public/app/BUILD.gn b/content/public/app/BUILD.gn
index
09adc3781eac0c9d0ce5ab3b8987c663697ee543..451ae9dc572b07173fd2308c25bb129fb269cf90
100644
--- a/content/public/app/BUILD.gn
+++ b/content/public/app/BUILD.gn
@@ -181,11 +181,13 @@ service_manifest("browser_manifest") {
     "device",
     "file",
     "media",
+    "shape_detection",
   ]
   deps = [
     "//media/mojo/services:media_manifest",
     "//services/device:manifest",
     "//services/file:manifest",
+    "//services/shape_detection:manifest",
   ]
 }

[xianglu, 2017-01-25 01:39:26]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 01:40:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 01:43:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 01:43:51]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xianglu, 2017-01-25 01:49:52]

The CQ bit was checked by xianglu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 01:50:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xianglu, 2017-01-25 01:51:13]

Patchset #14 (id:1650001) has been deleted

[commit-bot: I haz the power, 2017-01-25 03:28:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 03:28:06]

Dry run: This issue passed the CQ dry run.

[xianglu, 2017-01-25 04:24:11]

The CQ bit was checked by xianglu@chromium.org

[xianglu, 2017-01-25 04:24:12]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jochen@chromium.org, haraken@chromium.org, mcasas@chromium.org,
yzshen@chromium.org, rsesek@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2528743002/#ps1670001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-25 04:24:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 04:30:46]

CQ is committing da patch.

Bot data: {"patchset_id": 1670001, "attempt_start_ts": 1485318251635340,
"parent_rev": "12040fdc5ae7503f5802dabb558851ba7c569ad3", "commit_rev":
"6dd8f460d0fe4567a7cb14ee5a3acc183d5325fb"}

[commit-bot: I haz the power, 2017-01-25 04:31:47]

Description was changed from

==========
Shape Detection: Implement FaceDetection on Mac as out-of-process service

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from the Renderer process to a Utility process, which detects
faces using Mac native CoreImage library and returns detection result.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Shape Detection: Implement FaceDetection on Mac as out-of-process service

This CL implements FaceDetection in //services/shape_detection. The Browser
forwards requests from the Renderer process to a Utility process, which detects
faces using Mac native CoreImage library and returns detection result.



DESIGN_DOC=http://tinyurl.com/shape-detection-in-chromium
BUG=659139
TEST=content/browser/shapedetection/shapedetection_browsertest.cc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2528743002
Cr-Commit-Position: refs/heads/master@{#445942}
Committed:
https://chromium.googlesource.com/chromium/src/+/6dd8f460d0fe4567a7cb14ee5a3a...
==========

[mastiz, 2017-01-25 15:09:32]

A revert of this CL (patchset #14 id:1670001) has been created in
https://codereview.chromium.org/2653223003/ by mastiz@chromium.org.

The reason for reverting is: Culprit suspect for blink_heap_unittests failing on
chromium.webkit/WebKit Android (Nexus4), e.g.
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Andr...
.