ShapeDetection: Simplify the usage of AsyncOperation on windows 10

Since this operation is not cancellable, it's better to guarantee that
|AsyncCallbackInternal| will be called instead of canceling, but |callback_|
still need to take weak reference count so that it will not be run if its object
has been already destroyed. This would simplify callers since they would not need
to own the result returned by Create.

BUG=790843

Cq-Include-Trybots: master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Change-Id: I640cbe769c999ef20d6b9310038023c4fd3d7714
Reviewed-on: https://chromium-review.googlesource.com/968121Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Junwei Fu <junwei.fu@intel.com>
Cr-Commit-Position: refs/heads/master@{#545688}

services/shape_detection/detection_utils_win.h

@@ -9,17 +9,12 @@
 #include <wrl/client.h>
 #include <wrl/event.h>
 #include <wrl/implements.h>
-#include <memory>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
-#include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
-#include "base/memory/ptr_util.h"
-#include "base/memory/ref_counted.h"
-#include "base/memory/weak_ptr.h"
 #include "base/sequenced_task_runner.h"
 
 class SkBitmap;
@@ -35,10 +30,8 @@ using ABI::Windows::Graphics::Imaging::ISoftwareBitmap;
 using ABI::Windows::Graphics::Imaging::BitmapPixelFormat;
 
 // This template represents an asynchronous operation which returns a result
-// upon completion, internal async callback will be not called if the instance
-// is deleted. RuntimeType is Windows Runtime APIs that has a result.
-// TODO(junwei.fu): https://crbug.com/791371 consider moving the implementation
-// of AsyncOperation to .cc file.
+// upon completion, |callback_| will not be run if its object has been already
+// destroyed.
 template <typename RuntimeType>
 class AsyncOperation {
  public:
@@ -52,14 +45,11 @@ class AsyncOperation {
 
   // Creates an AsyncOperation instance which sets |callback| to be called when
   // the asynchronous action completes.
-  static std::unique_ptr<AsyncOperation<RuntimeType>> Create(
-      Callback callback,
-      IAsyncOperationPtr async_op_ptr) {
-    auto instance = base::WrapUnique(
-        new AsyncOperation<RuntimeType>(std::move(callback), async_op_ptr));
-
-    base::WeakPtr<AsyncOperation> weak_ptr =
-        instance->weak_factory_.GetWeakPtr();
+  static HRESULT BeginAsyncOperation(Callback callback,
+                                     IAsyncOperationPtr async_op_ptr) {
+    auto instance =
+        new AsyncOperation<RuntimeType>(std::move(callback), async_op_ptr);
+
     scoped_refptr<base::SequencedTaskRunner> task_runner =
         base::SequencedTaskRunnerHandle::Get();
 
@@ -68,33 +58,25 @@ class AsyncOperation {
                             WRL::FtmBase>
         AsyncCallback;
     auto async_callback = WRL::Callback<AsyncCallback>(
-        [weak_ptr, task_runner](IAsyncOperation<RuntimeType*>* async_op,
+        [instance, task_runner](IAsyncOperation<RuntimeType*>* async_op,
                                 AsyncStatus status) {
           // A reference to |async_op| is kept in |async_op_ptr_|, safe to pass
           // outside.  This is happening on an OS thread.
           task_runner->PostTask(
               FROM_HERE, base::BindOnce(&AsyncOperation::AsyncCallbackInternal,
-                                        std::move(weak_ptr),
+                                        base::Owned(instance),
                                         base::Unretained(async_op), status));
 
           return S_OK;
         });
 
-    const HRESULT hr = async_op_ptr->put_Completed(async_callback.Get());
-    if (FAILED(hr)) {
-      DLOG(ERROR) << "Async put completed failed: "
-                  << logging::SystemErrorCodeToString(hr);
-      return nullptr;
-    }
-
-    return instance;
+    return async_op_ptr->put_Completed(async_callback.Get());
   }
 
  private:
   AsyncOperation(Callback callback, IAsyncOperationPtr async_op_ptr)
       : async_op_ptr_(std::move(async_op_ptr)),
-        callback_(std::move(callback)),
-        weak_factory_(this) {}
+        callback_(std::move(callback)) {}
 
   void AsyncCallbackInternal(IAsyncOperation<RuntimeType*>* async_op,
                              AsyncStatus status) {
@@ -107,9 +89,6 @@ class AsyncOperation {
 
   IAsyncOperationPtr async_op_ptr_;
   Callback callback_;
-  // TODO(junwei.fu): https://crbug.com/790843 guarantee |callback_| will be
-  // called instead of canceling the callback if this object is freed.
-  base::WeakPtrFactory<AsyncOperation<RuntimeType>> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(AsyncOperation);
 };

services/shape_detection/face_detection_impl_win.cc

@@ -23,7 +23,8 @@ FaceDetectionImplWin::FaceDetectionImplWin(
     BitmapPixelFormat pixel_format)
     : face_detector_(std::move(face_detector)),
       bitmap_factory_(std::move(bitmap_factory)),
-      pixel_format_(pixel_format) {
+      pixel_format_(pixel_format),
+      weak_factory_(this) {
   DCHECK(face_detector_);
   DCHECK(bitmap_factory_);
 }
@@ -31,25 +32,24 @@ FaceDetectionImplWin::~FaceDetectionImplWin() = default;
 
 void FaceDetectionImplWin::Detect(const SkBitmap& bitmap,
                                   DetectCallback callback) {
-  if ((async_detect_face_ops_ = BeginDetect(bitmap))) {
-    // Hold on the callback until AsyncOperation completes.
-    detected_face_callback_ = std::move(callback);
-    // This prevents the Detect function from being called before the
-    // AsyncOperation completes.
-    binding_->PauseIncomingMethodCallProcessing();
-  } else {
+  if (FAILED(BeginDetect(bitmap))) {
     // No detection taking place; run |callback| with an empty array of results.
     std::move(callback).Run(std::vector<mojom::FaceDetectionResultPtr>());
+    return;
   }
+  // Hold on the callback until AsyncOperation completes.
+  detected_face_callback_ = std::move(callback);
+  // This prevents the Detect function from being called before the
+  // AsyncOperation completes.
+  binding_->PauseIncomingMethodCallProcessing();
 }
 
-std::unique_ptr<AsyncOperation<IVector<DetectedFace*>>>
-FaceDetectionImplWin::BeginDetect(const SkBitmap& bitmap) {
+HRESULT FaceDetectionImplWin::BeginDetect(const SkBitmap& bitmap) {
   Microsoft::WRL::ComPtr<ISoftwareBitmap> win_bitmap =
       CreateWinBitmapWithPixelFormat(bitmap, bitmap_factory_.Get(),
                                      pixel_format_);
   if (!win_bitmap)
-    return nullptr;
+    return E_FAIL;
 
   // Detect faces asynchronously.
   AsyncOperation<IVector<DetectedFace*>>::IAsyncOperationPtr async_op;
@@ -58,15 +58,15 @@ FaceDetectionImplWin::BeginDetect(const SkBitmap& bitmap) {
   if (FAILED(hr)) {
     DLOG(ERROR) << "Detect faces asynchronously failed: "
                 << logging::SystemErrorCodeToString(hr);
-    return nullptr;
+    return hr;
   }
 
-  // The once callback will not be called if this object is deleted, so it's
-  // fine to use Unretained to bind the callback. |win_bitmap| needs to be kept
+  // Use WeakPtr to bind the callback so that the once callback will not be run
+  // if this object has been already destroyed. |win_bitmap| needs to be kept
   // alive until OnFaceDetected().
-  return AsyncOperation<IVector<DetectedFace*>>::Create(
+  return AsyncOperation<IVector<DetectedFace*>>::BeginAsyncOperation(
       base::BindOnce(&FaceDetectionImplWin::OnFaceDetected,
-                     base::Unretained(this), std::move(win_bitmap)),
+                     weak_factory_.GetWeakPtr(), std::move(win_bitmap)),
       std::move(async_op));
 }
 
@@ -117,7 +117,6 @@ void FaceDetectionImplWin::OnFaceDetected(
     AsyncOperation<IVector<DetectedFace*>>::IAsyncOperationPtr async_op) {
   std::move(detected_face_callback_)
       .Run(BuildFaceDetectionResult(std::move(async_op)));
-  async_detect_face_ops_.reset();
   binding_->ResumeIncomingMethodCallProcessing();
 }
 

services/shape_detection/face_detection_impl_win.h

@@ -14,6 +14,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "services/shape_detection/detection_utils_win.h"
 #include "services/shape_detection/public/mojom/facedetection.mojom.h"
@@ -49,8 +50,7 @@ class FaceDetectionImplWin : public mojom::FaceDetection {
               mojom::FaceDetection::DetectCallback callback) override;
 
  private:
-  std::unique_ptr<AsyncOperation<IVector<DetectedFace*>>> BeginDetect(
-      const SkBitmap& bitmap);
+  HRESULT BeginDetect(const SkBitmap& bitmap);
   std::vector<mojom::FaceDetectionResultPtr> BuildFaceDetectionResult(
       AsyncOperation<IVector<DetectedFace*>>::IAsyncOperationPtr async_op);
   void OnFaceDetected(
@@ -62,11 +62,11 @@ class FaceDetectionImplWin : public mojom::FaceDetection {
   Microsoft::WRL::ComPtr<ISoftwareBitmapStatics> bitmap_factory_;
   BitmapPixelFormat pixel_format_;
 
-  std::unique_ptr<AsyncOperation<IVector<DetectedFace*>>>
-      async_detect_face_ops_;
   DetectCallback detected_face_callback_;
   mojo::StrongBindingPtr<mojom::FaceDetection> binding_;
 
+  base::WeakPtrFactory<FaceDetectionImplWin> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(FaceDetectionImplWin);
 };
 

services/shape_detection/face_detection_provider_win.cc

@@ -41,13 +41,6 @@ BitmapPixelFormat GetPreferredPixelFormat(IFaceDetectorStatics* factory) {
 void FaceDetectionProviderWin::CreateFaceDetection(
     shape_detection::mojom::FaceDetectionRequest request,
     shape_detection::mojom::FaceDetectorOptionsPtr options) {
-  if (async_create_detector_ops_) {
-    mojo::ReportBadMessage(
-        "FaceDetectionProvider client may only create one FaceDetection at a "
-        "time.");
-    return;
-  }
-
   // FaceDetector class is only available in Win 10 onwards (v10.0.10240.0).
   if (base::win::GetVersion() < base::win::VERSION_WIN10) {
     DVLOG(1) << "FaceDetector not supported before Windows 10";
@@ -93,24 +86,28 @@ void FaceDetectionProviderWin::CreateFaceDetection(
     return;
   }
 
-  // The once callback will not be called if this object is deleted, so it's
-  // fine to use Unretained to bind the callback.
-  auto async_operation = AsyncOperation<FaceDetector>::Create(
+  // Use WeakPtr to bind the callback so that the once callback will not be run
+  // if this object has been already destroyed.
+  hr = AsyncOperation<FaceDetector>::BeginAsyncOperation(
       base::BindOnce(&FaceDetectionProviderWin::OnFaceDetectorCreated,
-                     base::Unretained(this), std::move(request), pixel_format),
+                     weak_factory_.GetWeakPtr(), std::move(request),
+                     pixel_format),
       std::move(async_op));
-  if (!async_operation)
+  if (FAILED(hr)) {
+    DLOG(ERROR) << "Begin async operation failed: "
+                << logging::SystemErrorCodeToString(hr);
     return;
+  }
 
-  async_create_detector_ops_ = std::move(async_operation);
-  // When |provider| goes out of scope it will immediately closes its end of
-  // the message pipe, then |async_create_detector_ops_| will be deleted and the
-  // callback OnFaceDetectorCreated will be not called. This prevents this
-  // object from being destroyed before the AsyncOperation completes.
+  // When |provider| goes out of scope it will immediately close its end of
+  // the message pipe, then the callback OnFaceDetectorCreated will be not
+  // called. This prevents this object from being destroyed before the
+  // AsyncOperation completes.
   binding_->PauseIncomingMethodCallProcessing();
 }
 
-FaceDetectionProviderWin::FaceDetectionProviderWin() = default;
+FaceDetectionProviderWin::FaceDetectionProviderWin() : weak_factory_(this) {}
+
 FaceDetectionProviderWin::~FaceDetectionProviderWin() = default;
 
 void FaceDetectionProviderWin::OnFaceDetectorCreated(
@@ -118,7 +115,6 @@ void FaceDetectionProviderWin::OnFaceDetectorCreated(
     BitmapPixelFormat pixel_format,
     AsyncOperation<FaceDetector>::IAsyncOperationPtr async_op) {
   binding_->ResumeIncomingMethodCallProcessing();
-  async_create_detector_ops_.reset();
 
   Microsoft::WRL::ComPtr<IFaceDetector> face_detector;
   HRESULT hr =

services/shape_detection/face_detection_provider_win.h

@@ -11,6 +11,7 @@
 #include <utility>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "services/shape_detection/detection_utils_win.h"
 #include "services/shape_detection/face_detection_impl_win.h"
@@ -48,7 +49,7 @@ class FaceDetectionProviderWin
 
   FRIEND_TEST_ALL_PREFIXES(FaceDetectionImplWinTest, ScanOneFace);
   mojo::StrongBindingPtr<mojom::FaceDetectionProvider> binding_;
-  std::unique_ptr<AsyncOperation<FaceDetector>> async_create_detector_ops_;
+  base::WeakPtrFactory<FaceDetectionProviderWin> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FaceDetectionProviderWin);
 };

services/shape_detection/text_detection_impl_win.cc

@@ -115,7 +115,8 @@ TextDetectionImplWin::TextDetectionImplWin(
     Microsoft::WRL::ComPtr<IOcrEngine> ocr_engine,
     Microsoft::WRL::ComPtr<ISoftwareBitmapStatics> bitmap_factory)
     : ocr_engine_(std::move(ocr_engine)),
-      bitmap_factory_(std::move(bitmap_factory)) {
+      bitmap_factory_(std::move(bitmap_factory)),
+      weak_factory_(this) {
   DCHECK(ocr_engine_);
   DCHECK(bitmap_factory_);
 }
@@ -124,24 +125,23 @@ TextDetectionImplWin::~TextDetectionImplWin() = default;
 
 void TextDetectionImplWin::Detect(const SkBitmap& bitmap,
                                   DetectCallback callback) {
-  if ((async_recognize_ops_ = BeginDetect(bitmap))) {
-    // Hold on the callback until AsyncOperation completes.
-    recognize_text_callback_ = std::move(callback);
-    // This prevents the Detect function from being called before the
-    // AsyncOperation completes.
-    binding_->PauseIncomingMethodCallProcessing();
-  } else {
+  if (FAILED(BeginDetect(bitmap))) {
     // No detection taking place; run |callback| with an empty array of results.
     std::move(callback).Run(std::vector<mojom::TextDetectionResultPtr>());
+    return;
   }
+  // Hold on the callback until AsyncOperation completes.
+  recognize_text_callback_ = std::move(callback);
+  // This prevents the Detect function from being called before the
+  // AsyncOperation completes.
+  binding_->PauseIncomingMethodCallProcessing();
 }
 
-std::unique_ptr<AsyncOperation<OcrResult>> TextDetectionImplWin::BeginDetect(
-    const SkBitmap& bitmap) {
+HRESULT TextDetectionImplWin::BeginDetect(const SkBitmap& bitmap) {
   Microsoft::WRL::ComPtr<ISoftwareBitmap> win_bitmap =
       CreateWinBitmapFromSkBitmap(bitmap, bitmap_factory_.Get());
   if (!win_bitmap)
-    return nullptr;
+    return E_FAIL;
 
   // Recognize text asynchronously.
   AsyncOperation<OcrResult>::IAsyncOperationPtr async_op;
@@ -149,15 +149,15 @@ std::unique_ptr<AsyncOperation<OcrResult>> TextDetectionImplWin::BeginDetect(
   if (FAILED(hr)) {
     DLOG(ERROR) << "Recognize text asynchronously failed: "
                 << logging::SystemErrorCodeToString(hr);
-    return nullptr;
+    return hr;
   }
 
-  // The once callback will not be called if this object is deleted, so it's
-  // fine to use Unretained to bind the callback.
-  // |win_bitmap| needs to be kept alive until OnTextDetected().
-  return AsyncOperation<OcrResult>::Create(
+  // Use WeakPtr to bind the callback so that the once callback will not be run
+  // if this object has been already destroyed. |win_bitmap| needs to be kept
+  // alive until OnTextDetected().
+  return AsyncOperation<OcrResult>::BeginAsyncOperation(
       base::BindOnce(&TextDetectionImplWin::OnTextDetected,
-                     base::Unretained(this), std::move(win_bitmap)),
+                     weak_factory_.GetWeakPtr(), std::move(win_bitmap)),
       std::move(async_op));
 }
 
@@ -214,7 +214,6 @@ void TextDetectionImplWin::OnTextDetected(
     AsyncOperation<OcrResult>::IAsyncOperationPtr async_op) {
   std::move(recognize_text_callback_)
       .Run(BuildTextDetectionResult(std::move(async_op)));
-  async_recognize_ops_.reset();
   binding_->ResumeIncomingMethodCallProcessing();
 }
 

services/shape_detection/text_detection_impl_win.h

@@ -13,6 +13,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "services/shape_detection/detection_utils_win.h"
 #include "services/shape_detection/public/mojom/textdetection.mojom.h"
@@ -44,17 +45,17 @@ class TextDetectionImplWin : public mojom::TextDetection {
  private:
   Microsoft::WRL::ComPtr<IOcrEngine> ocr_engine_;
   Microsoft::WRL::ComPtr<ISoftwareBitmapStatics> bitmap_factory_;
-  std::unique_ptr<AsyncOperation<OcrResult>> async_recognize_ops_;
   DetectCallback recognize_text_callback_;
   mojo::StrongBindingPtr<mojom::TextDetection> binding_;
 
-  std::unique_ptr<AsyncOperation<OcrResult>> BeginDetect(
-      const SkBitmap& bitmap);
+  HRESULT BeginDetect(const SkBitmap& bitmap);
   std::vector<mojom::TextDetectionResultPtr> BuildTextDetectionResult(
       AsyncOperation<OcrResult>::IAsyncOperationPtr async_op);
   void OnTextDetected(Microsoft::WRL::ComPtr<ISoftwareBitmap> win_bitmap,
                       AsyncOperation<OcrResult>::IAsyncOperationPtr async_op);
 
+  base::WeakPtrFactory<TextDetectionImplWin> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(TextDetectionImplWin);
 };