Migrate LorgnetteManagerClient into DBusMethod.

BUG=739622
TEST=Ran trybots. Ran documentscan_AppTestWithFakeLorgnette on DUT.

Change-Id: I53358981be8b33e934ea5846391cad5fbc2af62b
Reviewed-on: https://chromium-review.googlesource.com/796491Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520856}

chromeos/dbus/fake_lorgnette_manager_client.cc

@@ -21,33 +21,35 @@ FakeLorgnetteManagerClient::~FakeLorgnetteManagerClient() = default;
 void FakeLorgnetteManagerClient::Init(dbus::Bus* bus) {}
 
 void FakeLorgnetteManagerClient::ListScanners(
-    const ListScannersCallback& callback) {
+    DBusMethodCallback<ScannerTable> callback) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(callback, !scanner_table_.empty(), scanner_table_));
+      FROM_HERE, base::BindOnce(std::move(callback),
+                                scanner_table_.empty()
+                                    ? base::nullopt
+                                    : base::make_optional(scanner_table_)));
 }
 
 void FakeLorgnetteManagerClient::ScanImageToString(
     std::string device_name,
     const ScanProperties& properties,
-    const ScanImageToStringCallback& callback) {
+    DBusMethodCallback<std::string> callback) {
   auto it = scan_data_.find(
       std::make_tuple(device_name, properties.mode, properties.resolution_dpi));
-  auto task = it != scan_data_.end()
-                  ? base::BindOnce(callback, true, it->second)
-                  : base::BindOnce(callback, false, std::string());
-  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, std::move(task));
+  auto data =
+      it == scan_data_.end() ? base::nullopt : base::make_optional(it->second);
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback), std::move(data)));
 }
 
 void FakeLorgnetteManagerClient::AddScannerTableEntry(
-    const std::string device_name,
+    const std::string& device_name,
     const ScannerTableEntry& entry) {
   scanner_table_[device_name] = entry;
 }
 
 void FakeLorgnetteManagerClient::AddScanData(const std::string& device_name,
                                              const ScanProperties& properties,
-                                             const std::string data) {
+                                             const std::string& data) {
   scan_data_[std::make_tuple(device_name, properties.mode,
                              properties.resolution_dpi)] = data;
 }

chromeos/dbus/fake_lorgnette_manager_client.h

@@ -24,21 +24,20 @@ class CHROMEOS_EXPORT FakeLorgnetteManagerClient
 
   void Init(dbus::Bus* bus) override;
 
-  void ListScanners(const ListScannersCallback& callback) override;
-  void ScanImageToString(
-      std::string device_name,
-      const ScanProperties& properties,
-      const ScanImageToStringCallback& callback) override;
+  void ListScanners(DBusMethodCallback<ScannerTable> callback) override;
+  void ScanImageToString(std::string device_name,
+                         const ScanProperties& properties,
+                         DBusMethodCallback<std::string> callback) override;
 
   // Adds a fake scanner table entry, which will be returned by ListScanners().
-  void AddScannerTableEntry(const std::string device_name,
+  void AddScannerTableEntry(const std::string& device_name,
                             const ScannerTableEntry& entry);
 
   // Adds a fake scan data, which will be returned by ScanImageToString(),
   // if |device_name| and |properties| are matched.
   void AddScanData(const std::string& device_name,
                    const ScanProperties& properties,
-                   const std::string data);
+                   const std::string& data);
 
  private:
   ScannerTable scanner_table_;

chromeos/dbus/lorgnette_manager_client.cc

@@ -28,25 +28,22 @@ namespace chromeos {
 // The LorgnetteManagerClient implementation used in production.
 class LorgnetteManagerClientImpl : public LorgnetteManagerClient {
  public:
-  LorgnetteManagerClientImpl() :
-      lorgnette_daemon_proxy_(NULL), weak_ptr_factory_(this) {}
-
+  LorgnetteManagerClientImpl() = default;
   ~LorgnetteManagerClientImpl() override = default;
 
-  void ListScanners(const ListScannersCallback& callback) override {
+  void ListScanners(DBusMethodCallback<ScannerTable> callback) override {
     dbus::MethodCall method_call(lorgnette::kManagerServiceInterface,
                                  lorgnette::kListScannersMethod);
     lorgnette_daemon_proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::BindOnce(&LorgnetteManagerClientImpl::OnListScanners,
-                       weak_ptr_factory_.GetWeakPtr(), callback));
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
   // LorgnetteManagerClient override.
-  void ScanImageToString(
-      std::string device_name,
-      const ScanProperties& properties,
-      const ScanImageToStringCallback& callback) override {
+  void ScanImageToString(std::string device_name,
+                         const ScanProperties& properties,
+                         DBusMethodCallback<std::string> callback) override {
     auto scan_data_reader = std::make_unique<ScanDataReader>();
     base::ScopedFD fd = scan_data_reader->Start();
 
@@ -57,8 +54,8 @@ class LorgnetteManagerClientImpl : public LorgnetteManagerClient {
     writer.AppendString(device_name);
     writer.AppendFileDescriptor(fd.get());
 
-    dbus::MessageWriter option_writer(NULL);
-    dbus::MessageWriter element_writer(NULL);
+    dbus::MessageWriter option_writer(nullptr);
+    dbus::MessageWriter element_writer(nullptr);
     writer.OpenArray("{sv}", &option_writer);
     if (!properties.mode.empty()) {
       option_writer.OpenDictEntry(&element_writer);
@@ -77,7 +74,7 @@ class LorgnetteManagerClientImpl : public LorgnetteManagerClient {
     lorgnette_daemon_proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::BindOnce(&LorgnetteManagerClientImpl::OnScanImageComplete,
-                       weak_ptr_factory_.GetWeakPtr(), callback,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback),
                        std::move(scan_data_reader)));
   }
 
@@ -97,7 +94,7 @@ class LorgnetteManagerClientImpl : public LorgnetteManagerClient {
     using CompletionCallback =
         base::OnceCallback<void(base::Optional<std::string> data)>;
 
-    ScanDataReader() : weak_ptr_factory_(this) {}
+    ScanDataReader() = default;
 
     // Creates a pipe to read the scan data from the D-Bus service.
     // Returns a write-side FD.
@@ -150,86 +147,81 @@ class LorgnetteManagerClientImpl : public LorgnetteManagerClient {
 
     CompletionCallback callback_;
 
-    base::WeakPtrFactory<ScanDataReader> weak_ptr_factory_;
+    base::WeakPtrFactory<ScanDataReader> weak_ptr_factory_{this};
     DISALLOW_COPY_AND_ASSIGN(ScanDataReader);
   };
 
   // Called when ListScanners completes.
-  void OnListScanners(const ListScannersCallback& callback,
+  void OnListScanners(DBusMethodCallback<ScannerTable> callback,
                       dbus::Response* response) {
-    ScannerTable scanners;
-    dbus::MessageReader table_reader(NULL);
+    dbus::MessageReader table_reader(nullptr);
     if (!response || !dbus::MessageReader(response).PopArray(&table_reader)) {
-      callback.Run(false, scanners);
+      std::move(callback).Run(base::nullopt);
       return;
     }
 
-    bool decode_failure = false;
+    ScannerTable scanners;
     while (table_reader.HasMoreData()) {
       std::string device_name;
-      dbus::MessageReader device_entry_reader(NULL);
-      dbus::MessageReader device_element_reader(NULL);
+      dbus::MessageReader device_entry_reader(nullptr);
+      dbus::MessageReader device_element_reader(nullptr);
       if (!table_reader.PopDictEntry(&device_entry_reader) ||
           !device_entry_reader.PopString(&device_name) ||
           !device_entry_reader.PopArray(&device_element_reader)) {
-        decode_failure = true;
-        break;
+        LOG(ERROR) << "Failed to decode response from ListScanners";
+        std::move(callback).Run(base::nullopt);
+        return;
       }
 
       ScannerTableEntry scanner_entry;
       while (device_element_reader.HasMoreData()) {
-        dbus::MessageReader device_attribute_reader(NULL);
         std::string attribute;
         std::string value;
+        dbus::MessageReader device_attribute_reader(nullptr);
         if (!device_element_reader.PopDictEntry(&device_attribute_reader) ||
             !device_attribute_reader.PopString(&attribute) ||
             !device_attribute_reader.PopString(&value)) {
-          decode_failure = true;
-          break;
+          LOG(ERROR) << "Failed to decode response from ListScanners";
+          std::move(callback).Run(base::nullopt);
+          return;
         }
-        scanner_entry[attribute] = value;
+        scanner_entry.emplace(std::move(attribute), std::move(value));
       }
 
-      if (decode_failure)
-          break;
-
-      scanners[device_name] = scanner_entry;
+      scanners.emplace(std::move(device_name), std::move(scanner_entry));
     }
 
-    if (decode_failure) {
-      LOG(ERROR) << "Failed to decode response from ListScanners";
-      callback.Run(false, scanners);
-    } else {
-      callback.Run(true, scanners);
-    }
+    std::move(callback).Run(std::move(scanners));
   }
 
   // Called when a response for ScanImage() is received.
-  void OnScanImageComplete(const ScanImageToStringCallback& callback,
+  void OnScanImageComplete(DBusMethodCallback<std::string> callback,
                            std::unique_ptr<ScanDataReader> scan_data_reader,
                            dbus::Response* response) {
     if (!response) {
       LOG(ERROR) << "Failed to scan image";
       // Do not touch |scan_data_reader|, so that RAII deletes it and
       // cancels the inflight operation.
-      callback.Run(false, std::string());
+      std::move(callback).Run(base::nullopt);
       return;
     }
     auto* reader = scan_data_reader.get();
-    reader->Wait(base::BindOnce(
-        &LorgnetteManagerClientImpl::OnScanDataCompleted,
-        weak_ptr_factory_.GetWeakPtr(), callback, std::move(scan_data_reader)));
+    reader->Wait(
+        base::BindOnce(&LorgnetteManagerClientImpl::OnScanDataCompleted,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback),
+                       std::move(scan_data_reader)));
   }
 
   // Called when scan data read is completed.
-  void OnScanDataCompleted(const ScanImageToStringCallback& callback,
+  // This is to maintain the lifetime of ScanDataReader instance.
+  void OnScanDataCompleted(DBusMethodCallback<std::string> callback,
                            std::unique_ptr<ScanDataReader> scan_data_reader,
                            base::Optional<std::string> data) {
-    callback.Run(data.has_value(), data.value_or(std::string()));
+    std::move(callback).Run(std::move(data));
   }
 
-  dbus::ObjectProxy* lorgnette_daemon_proxy_;
-  base::WeakPtrFactory<LorgnetteManagerClientImpl> weak_ptr_factory_;
+  dbus::ObjectProxy* lorgnette_daemon_proxy_ = nullptr;
+  base::WeakPtrFactory<LorgnetteManagerClientImpl> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(LorgnetteManagerClientImpl);
 };

chromeos/dbus/lorgnette_manager_client.h

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/dbus_client.h"
+#include "chromeos/dbus/dbus_method_call_status.h"
 
 namespace chromeos {
 
@@ -20,20 +21,8 @@ namespace chromeos {
 class CHROMEOS_EXPORT LorgnetteManagerClient : public DBusClient {
  public:
   // The property information for each scanner retured by ListScanners.
-  typedef std::map<std::string, std::string> ScannerTableEntry;
-  typedef std::map<std::string, ScannerTableEntry> ScannerTable;
-
-  // Callback type for ListScanners().  Returns a map which contains
-  // a ScannerTableEntry for each available scanner.
-  typedef base::Callback<void(
-      bool succeeded, const ScannerTable&)> ListScannersCallback;
-
-  // Called once ScanImageToString() is complete. Takes two parameters:
-  // - succeeded: was the scan completed successfully.
-  // - image_data: the contents of the image.
-  typedef base::Callback<void(
-      bool succeeded,
-      const std::string& image_data)> ScanImageToStringCallback;
+  using ScannerTableEntry = std::map<std::string, std::string>;
+  using ScannerTable = std::map<std::string, ScannerTableEntry>;
 
   // Attributes provided to a scan request.
   struct ScanProperties {
@@ -44,14 +33,14 @@ class CHROMEOS_EXPORT LorgnetteManagerClient : public DBusClient {
   ~LorgnetteManagerClient() override;
 
   // Gets a list of scanners from the lorgnette manager.
-  virtual void ListScanners(const ListScannersCallback& callback) = 0;
+  virtual void ListScanners(DBusMethodCallback<ScannerTable> callback) = 0;
 
   // Request a scanned image and calls |callback| when completed with a string
   // pointing at the scanned image data.  Image data will be stored in the .png
   // format.
   virtual void ScanImageToString(std::string device_name,
                                  const ScanProperties& properties,
-                                 const ScanImageToStringCallback& callback) = 0;
+                                 DBusMethodCallback<std::string> callback) = 0;
 
   // Factory function, creates a new instance and returns ownership.
   // For normal usage, access the singleton via DBusThreadManager::Get().

extensions/browser/api/document_scan/document_scan_interface_chromeos.cc

@@ -14,9 +14,9 @@
 
 namespace {
 
-const char kImageScanFailedError[] = "Image scan failed";
-const char kScannerImageMimeTypePng[] = "image/png";
-const char kPngImageDataUrlPrefix[] = "data:image/png;base64,";
+constexpr char kImageScanFailedError[] = "Image scan failed";
+constexpr char kScannerImageMimeTypePng[] = "image/png";
+constexpr char kPngImageDataUrlPrefix[] = "data:image/png;base64,";
 
 chromeos::LorgnetteManagerClient* GetLorgnetteManagerClient() {
   DCHECK(chromeos::DBusThreadManager::IsInitialized());
@@ -36,33 +36,34 @@ DocumentScanInterfaceChromeos::~DocumentScanInterfaceChromeos() = default;
 void DocumentScanInterfaceChromeos::ListScanners(
     const ListScannersResultsCallback& callback) {
   GetLorgnetteManagerClient()->ListScanners(
-      base::Bind(&DocumentScanInterfaceChromeos::OnScannerListReceived,
-                 base::Unretained(this), callback));
+      base::BindOnce(&DocumentScanInterfaceChromeos::OnScannerListReceived,
+                     base::Unretained(this), callback));
 }
 
 void DocumentScanInterfaceChromeos::OnScannerListReceived(
     const ListScannersResultsCallback& callback,
-    bool succeeded,
-    const chromeos::LorgnetteManagerClient::ScannerTable& scanners) {
+    base::Optional<chromeos::LorgnetteManagerClient::ScannerTable> scanners) {
   std::vector<ScannerDescription> scanner_descriptions;
-  for (const auto& scanner : scanners) {
-    ScannerDescription description;
-    description.name = scanner.first;
-    const auto& entry = scanner.second;
-    auto info_it = entry.find(lorgnette::kScannerPropertyManufacturer);
-    if (info_it != entry.end()) {
-      description.manufacturer = info_it->second;
+  if (scanners.has_value()) {
+    for (const auto& scanner : scanners.value()) {
+      ScannerDescription description;
+      description.name = scanner.first;
+      const auto& entry = scanner.second;
+      auto info_it = entry.find(lorgnette::kScannerPropertyManufacturer);
+      if (info_it != entry.end()) {
+        description.manufacturer = info_it->second;
+      }
+      info_it = entry.find(lorgnette::kScannerPropertyModel);
+      if (info_it != entry.end()) {
+        description.model = info_it->second;
+      }
+      info_it = entry.find(lorgnette::kScannerPropertyType);
+      if (info_it != entry.end()) {
+        description.scanner_type = info_it->second;
+      }
+      description.image_mime_type = kScannerImageMimeTypePng;
+      scanner_descriptions.push_back(description);
     }
-    info_it = entry.find(lorgnette::kScannerPropertyModel);
-    if (info_it != entry.end()) {
-      description.model = info_it->second;
-    }
-    info_it = entry.find(lorgnette::kScannerPropertyType);
-    if (info_it != entry.end()) {
-      description.scanner_type = info_it->second;
-    }
-    description.image_mime_type = kScannerImageMimeTypePng;
-    scanner_descriptions.push_back(description);
   }
   const std::string kNoError;
   callback.Run(scanner_descriptions, kNoError);
@@ -94,25 +95,22 @@ void DocumentScanInterfaceChromeos::Scan(const std::string& scanner_name,
 
   GetLorgnetteManagerClient()->ScanImageToString(
       scanner_name, properties,
-      base::Bind(&DocumentScanInterfaceChromeos::OnScanCompleted,
-                 base::Unretained(this), callback));
+      base::BindOnce(&DocumentScanInterfaceChromeos::OnScanCompleted,
+                     base::Unretained(this), callback));
 }
 
 void DocumentScanInterfaceChromeos::OnScanCompleted(
     const ScanResultsCallback& callback,
-    bool succeeded,
-    const std::string& image_data) {
-  VLOG(1) << "ScanImage returns " << succeeded;
-  std::string error_string;
-  if (!succeeded) {
-    error_string = kImageScanFailedError;
+    base::Optional<std::string> image_data) {
+  VLOG(1) << "ScanImage returns " << image_data.has_value();
+  if (!image_data.has_value()) {
+    callback.Run(std::string(), std::string(), kImageScanFailedError);
+    return;
   }
-
   std::string image_base64;
-  base::Base64Encode(image_data, &image_base64);
-  std::string image_url(std::string(kPngImageDataUrlPrefix) + image_base64);
-
-  callback.Run(image_url, kScannerImageMimeTypePng, error_string);
+  base::Base64Encode(image_data.value(), &image_base64);
+  callback.Run(kPngImageDataUrlPrefix + image_base64, kScannerImageMimeTypePng,
+               std::string() /* error */);
 }
 
 // static

extensions/browser/api/document_scan/document_scan_interface_chromeos.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/macros.h"
+#include "base/optional.h"
 #include "chromeos/dbus/lorgnette_manager_client.h"
 #include "extensions/browser/api/document_scan/document_scan_interface.h"
 
@@ -29,11 +30,9 @@ class DocumentScanInterfaceChromeos : public DocumentScanInterface {
  private:
   void OnScannerListReceived(
       const ListScannersResultsCallback& callback,
-      bool succeeded,
-      const chromeos::LorgnetteManagerClient::ScannerTable& scanners);
+      base::Optional<chromeos::LorgnetteManagerClient::ScannerTable> scanners);
   void OnScanCompleted(const ScanResultsCallback& callback,
-                       bool succeeded,
-                       const std::string& image_data);
+                       base::Optional<std::string> image_data);
 
   DISALLOW_COPY_AND_ASSIGN(DocumentScanInterfaceChromeos);
 };

extensions/browser/api/document_scan/document_scan_interface_chromeos_unittest.cc

@@ -83,8 +83,8 @@ TEST_F(DocumentScanInterfaceChromeosTest, ScanFailure) {
           [](base::RunLoop* run_loop, const std::string& scanned_image,
              const std::string& mime_type, const std::string& error) {
             run_loop->Quit();
-            EXPECT_EQ("data:image/png;base64,", scanned_image);
-            EXPECT_EQ("image/png", mime_type);
+            EXPECT_EQ("", scanned_image);
+            EXPECT_EQ("", mime_type);
             EXPECT_EQ("Image scan failed", error);
           },
           &run_loop));