Simplify SafeMediaMetadataParser

Now that it uses a Mojo service, SafeMediaMetadataParser can be
simplified as it does not require thread hops. Which also means it does
not need to be ref counted anymore.
Also changed Bind calls to BindOnce where appropriate.

Bug: 822922
Change-Id: Id684d9e7bb52f11f19d992f0d5dcb80c8d7e4770
Reviewed-on: https://chromium-review.googlesource.com/967364
Commit-Queue: Jay Civelli <jcivelli@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543948}

chrome/browser/extensions/api/media_galleries/media_galleries_api.cc

@@ -674,17 +674,19 @@ void MediaGalleriesGetMetadataFunction::GetMetadata(
       metadata_type == MediaGalleries::GET_METADATA_TYPE_ALL ||
       metadata_type == MediaGalleries::GET_METADATA_TYPE_NONE;
 
-  auto parser = base::MakeRefCounted<SafeMediaMetadataParser>(
+  auto parser = std::make_unique<SafeMediaMetadataParser>(
       GetProfile(), blob_uuid, total_blob_length, mime_type,
       get_attached_images);
-  parser->Start(
+  SafeMediaMetadataParser* parser_ptr = parser.get();
+  parser_ptr->Start(
       content::ServiceManagerConnection::GetForProcess()->GetConnector(),
-      base::Bind(
+      base::BindOnce(
           &MediaGalleriesGetMetadataFunction::OnSafeMediaMetadataParserDone,
-          this));
+          this, std::move(parser)));
 }
 
 void MediaGalleriesGetMetadataFunction::OnSafeMediaMetadataParserDone(
+    std::unique_ptr<SafeMediaMetadataParser> parser_keep_alive,
     bool parse_success,
     std::unique_ptr<base::DictionaryValue> metadata_dictionary,
     std::unique_ptr<std::vector<metadata::AttachedImage>> attached_images) {

chrome/browser/extensions/api/media_galleries/media_galleries_api.h

@@ -33,6 +33,8 @@ namespace content {
 class BlobHandle;
 }
 
+class SafeMediaMetadataParser;
+
 namespace extensions {
 
 // The profile-keyed service that manages the media galleries extension API.
@@ -177,6 +179,7 @@ class MediaGalleriesGetMetadataFunction : public ChromeAsyncExtensionFunction {
                    int64_t total_blob_length);
 
   void OnSafeMediaMetadataParserDone(
+      std::unique_ptr<SafeMediaMetadataParser> parser_keep_alive,
       bool parse_success,
       std::unique_ptr<base::DictionaryValue> result_dictionary,
       std::unique_ptr<std::vector<metadata::AttachedImage>> attached_images);

chrome/services/media_gallery_util/public/cpp/safe_media_metadata_parser.cc

@@ -6,10 +6,10 @@
 
 #include <utility>
 
+#include "base/callback.h"
 #include "base/memory/ptr_util.h"
 #include "chrome/services/media_gallery_util/public/mojom/constants.mojom.h"
 #include "content/public/browser/browser_context.h"
-#include "content/public/browser/browser_thread.h"
 #include "extensions/browser/blob_reader.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -20,9 +20,7 @@ class SafeMediaMetadataParser::MediaDataSourceImpl
   MediaDataSourceImpl(SafeMediaMetadataParser* owner,
                       chrome::mojom::MediaDataSourcePtr* interface)
       : binding_(this, mojo::MakeRequest(interface)),
-        safe_media_metadata_parser_(owner) {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-  }
+        safe_media_metadata_parser_(owner) {}
 
   ~MediaDataSourceImpl() override = default;
 
@@ -51,125 +49,76 @@ SafeMediaMetadataParser::SafeMediaMetadataParser(
       blob_uuid_(blob_uuid),
       blob_size_(blob_size),
       mime_type_(mime_type),
-      get_attached_images_(get_attached_images) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-}
-
-void SafeMediaMetadataParser::Start(service_manager::Connector* connector,
-                                    const DoneCallback& callback) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  std::unique_ptr<service_manager::Connector> connector_ptr =
-      connector->Clone();
-  content::BrowserThread::PostTask(
-      content::BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&SafeMediaMetadataParser::StartOnIOThread, this,
-                     std::move(connector_ptr), callback));
-}
+      get_attached_images_(get_attached_images),
+      weak_factory_(this) {}
 
 SafeMediaMetadataParser::~SafeMediaMetadataParser() = default;
 
-void SafeMediaMetadataParser::StartOnIOThread(
-    std::unique_ptr<service_manager::Connector> connector,
-    const DoneCallback& callback) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+void SafeMediaMetadataParser::Start(service_manager::Connector* connector,
+                                    DoneCallback callback) {
   DCHECK(!media_parser_ptr_);
   DCHECK(callback);
 
-  callback_ = callback;
+  callback_ = std::move(callback);
 
   connector->BindInterface(chrome::mojom::kMediaGalleryUtilServiceName,
                            mojo::MakeRequest(&media_parser_ptr_));
+  // It's safe to use Unretained below as |this| owns |media_parser_ptr_|.
   media_parser_ptr_.set_connection_error_handler(
-      base::Bind(&SafeMediaMetadataParser::ParseMediaMetadataFailed, this));
+      base::BindOnce(&SafeMediaMetadataParser::ParseMediaMetadataFailed,
+                     base::Unretained(this)));
 
   chrome::mojom::MediaDataSourcePtr source;
   media_data_source_ = std::make_unique<MediaDataSourceImpl>(this, &source);
   media_parser_ptr_->ParseMediaMetadata(
       mime_type_, blob_size_, get_attached_images_, std::move(source),
-      base::Bind(&SafeMediaMetadataParser::ParseMediaMetadataDone, this));
+      base::BindOnce(&SafeMediaMetadataParser::ParseMediaMetadataDone,
+                     base::Unretained(this)));
 }
 
 void SafeMediaMetadataParser::ParseMediaMetadataFailed() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-
   media_parser_ptr_.reset();  // Terminate the utility process.
   media_data_source_.reset();
 
-  std::unique_ptr<base::DictionaryValue> metadata_dictionary =
-      std::make_unique<base::DictionaryValue>();
-  std::unique_ptr<std::vector<metadata::AttachedImage>> attached_images =
+  auto metadata_dictionary = std::make_unique<base::DictionaryValue>();
+  auto attached_images =
       std::make_unique<std::vector<metadata::AttachedImage>>();
 
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::BindOnce(callback_, false, std::move(metadata_dictionary),
-                     std::move(attached_images)));
+  std::move(callback_).Run(/*parse_success=*/false,
+                           std::move(metadata_dictionary),
+                           std::move(attached_images));
 }
 
 void SafeMediaMetadataParser::ParseMediaMetadataDone(
     bool parse_success,
     std::unique_ptr<base::DictionaryValue> metadata_dictionary,
     const std::vector<metadata::AttachedImage>& attached_images) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-
   media_parser_ptr_.reset();  // Terminate the utility process.
   media_data_source_.reset();
 
-  // We need to make a scoped copy of this vector since it will be destroyed
-  // at the end of the handler.
-  std::unique_ptr<std::vector<metadata::AttachedImage>> attached_images_copy =
+  auto attached_images_copy =
       std::make_unique<std::vector<metadata::AttachedImage>>(attached_images);
 
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::BindOnce(callback_, parse_success, std::move(metadata_dictionary),
-                     std::move(attached_images_copy)));
+  std::move(callback_).Run(parse_success, std::move(metadata_dictionary),
+                           std::move(attached_images_copy));
 }
 
 void SafeMediaMetadataParser::StartBlobRequest(
     chrome::mojom::MediaDataSource::ReadBlobCallback callback,
     int64_t position,
     int64_t length) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::BindOnce(&SafeMediaMetadataParser::StartBlobReaderOnUIThread, this,
-                     std::move(callback), position, length));
-}
-
-void SafeMediaMetadataParser::StartBlobReaderOnUIThread(
-    chrome::mojom::MediaDataSource::ReadBlobCallback callback,
-    int64_t position,
-    int64_t length) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
   BlobReader* reader = new BlobReader(  // BlobReader is self-deleting.
       browser_context_, blob_uuid_,
-      base::Bind(&SafeMediaMetadataParser::BlobReaderDoneOnUIThread, this,
-                 base::Passed(&callback)));
+      base::Bind(&SafeMediaMetadataParser::BlobReaderDone,
+                 weak_factory_.GetWeakPtr(), base::Passed(&callback)));
   reader->SetByteRange(position, length);
   reader->Start();
 }
 
-void SafeMediaMetadataParser::BlobReaderDoneOnUIThread(
+void SafeMediaMetadataParser::BlobReaderDone(
     chrome::mojom::MediaDataSource::ReadBlobCallback callback,
     std::unique_ptr<std::string> data,
     int64_t /* blob_total_size */) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  content::BrowserThread::PostTask(
-      content::BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&SafeMediaMetadataParser::FinishBlobRequest, this,
-                     std::move(callback), std::move(data)));
-}
-
-void SafeMediaMetadataParser::FinishBlobRequest(
-    chrome::mojom::MediaDataSource::ReadBlobCallback callback,
-    std::unique_ptr<std::string> data) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-
   if (media_parser_ptr_)
     std::move(callback).Run(std::vector<uint8_t>(data->begin(), data->end()));
 }

chrome/services/media_gallery_util/public/cpp/safe_media_metadata_parser.h

@@ -11,10 +11,9 @@
 #include <string>
 #include <vector>
 
-#include "base/callback.h"
-#include "base/compiler_specific.h"
+#include "base/callback_forward.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/common/media_galleries/metadata_types.h"
 #include "chrome/services/media_gallery_util/public/mojom/media_parser.mojom.h"
 
@@ -29,11 +28,10 @@ class Connector;
 // Parses the media metadata of a Blob safely in a utility process. This class
 // expects the MIME type of the Blob to be already known. It creates a utility
 // process to do further MIME-type-specific metadata extraction from the Blob
-// data. All public methods and callbacks of this class run on the UI thread.
-class SafeMediaMetadataParser
-    : public base::RefCountedThreadSafe<SafeMediaMetadataParser> {
+// data.
+class SafeMediaMetadataParser {
  public:
-  typedef base::Callback<void(
+  typedef base::OnceCallback<void(
       bool parse_success,
       std::unique_ptr<base::DictionaryValue> metadata_dictionary,
       std::unique_ptr<std::vector<metadata::AttachedImage>> attached_images)>
@@ -44,53 +42,36 @@ class SafeMediaMetadataParser
                           int64_t blob_size,
                           const std::string& mime_type,
                           bool get_attached_images);
+  ~SafeMediaMetadataParser();
 
-  // Should be called on the UI thread. |callback| also runs on the UI thread.
-  void Start(service_manager::Connector* connector,
-             const DoneCallback& callback);
+  // Should be called on the thread |connector| is associated with. |callback|
+  // is invoked on that same thread.
+  void Start(service_manager::Connector* connector, DoneCallback callback);
 
  private:
-  friend class base::RefCountedThreadSafe<SafeMediaMetadataParser>;
-
   class MediaDataSourceImpl;
 
-  ~SafeMediaMetadataParser();
-
-  // Starts the utility process and sends it a metadata parse request.
-  // Runs on the IO thread.
-  void StartOnIOThread(std::unique_ptr<service_manager::Connector> connector,
-                       const DoneCallback& callback);
-
   // Callback if the utility process or metadata parse request fails.
-  // Runs on the IO thread.
   void ParseMediaMetadataFailed();
 
   // Callback from utility process when it finishes parsing metadata.
-  // Runs on the IO thread.
   void ParseMediaMetadataDone(
       bool parse_success,
       std::unique_ptr<base::DictionaryValue> metadata_dictionary,
       const std::vector<metadata::AttachedImage>& attached_images);
 
-  // Sequence of functions that bounces from the IO thread to the UI thread to
-  // read the blob data, then sends the data back to the utility process.
+  // Starts to read the blob data and sends the data back to the utility
+  // process.
   void StartBlobRequest(
       chrome::mojom::MediaDataSource::ReadBlobCallback callback,
       int64_t position,
       int64_t length);
-  void StartBlobReaderOnUIThread(
-      chrome::mojom::MediaDataSource::ReadBlobCallback callback,
-      int64_t position,
-      int64_t length);
-  void BlobReaderDoneOnUIThread(
-      chrome::mojom::MediaDataSource::ReadBlobCallback callback,
-      std::unique_ptr<std::string> data,
-      int64_t /* blob_total_size */);
-  void FinishBlobRequest(
-      chrome::mojom::MediaDataSource::ReadBlobCallback callback,
-      std::unique_ptr<std::string> data);
 
-  // All member variables are only accessed on the IO thread.
+  // Invoked when the full blob content has been read.
+  void BlobReaderDone(chrome::mojom::MediaDataSource::ReadBlobCallback callback,
+                      std::unique_ptr<std::string> data,
+                      int64_t /* blob_total_size */);
+
   content::BrowserContext* const browser_context_;
   const std::string blob_uuid_;
   const int64_t blob_size_;
@@ -102,6 +83,8 @@ class SafeMediaMetadataParser
 
   std::unique_ptr<MediaDataSourceImpl> media_data_source_;
 
+  base::WeakPtrFactory<SafeMediaMetadataParser> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(SafeMediaMetadataParser);
 };