Modernize device/media_transfer_protocol code.

Change-Id: Ie19283843b32a6efac47da3345f6a56c440aa600
Reviewed-on: https://chromium-review.googlesource.com/759237
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515834}

components/storage_monitor/storage_monitor_chromeos.cc

@@ -4,6 +4,8 @@
 
 #include "components/storage_monitor/storage_monitor_chromeos.h"
 
+#include <utility>
+
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
@@ -94,14 +96,14 @@ void StorageMonitorCros::Init() {
   DiskMountManager::GetInstance()->AddObserver(this);
   CheckExistingMountPoints();
 
+  // Tests may have already set a MTP manager.
   if (!media_transfer_protocol_manager_) {
-    media_transfer_protocol_manager_.reset(
-        device::MediaTransferProtocolManager::Initialize());
+    media_transfer_protocol_manager_ =
+        device::MediaTransferProtocolManager::Initialize();
   }
-
-  media_transfer_protocol_device_observer_.reset(
-      new MediaTransferProtocolDeviceObserverChromeOS(
-          receiver(), media_transfer_protocol_manager_.get()));
+  media_transfer_protocol_device_observer_ =
+      std::make_unique<MediaTransferProtocolDeviceObserverChromeOS>(
+          receiver(), media_transfer_protocol_manager_.get());
 }
 
 void StorageMonitorCros::CheckExistingMountPoints() {
@@ -111,16 +113,13 @@ void StorageMonitorCros::CheckExistingMountPoints() {
       base::CreateSequencedTaskRunnerWithTraits(
           {base::MayBlock(), base::TaskPriority::BACKGROUND});
 
-  const DiskMountManager::MountPointMap& mount_point_map =
-      DiskMountManager::GetInstance()->mount_points();
-  for (DiskMountManager::MountPointMap::const_iterator it =
-      mount_point_map.begin(); it != mount_point_map.end(); ++it) {
+  for (const auto& it : DiskMountManager::GetInstance()->mount_points()) {
     base::PostTaskAndReplyWithResult(
         blocking_task_runner.get(), FROM_HERE,
         base::Bind(&MediaStorageUtil::HasDcim,
-                   base::FilePath(it->second.mount_path)),
+                   base::FilePath(it.second.mount_path)),
         base::Bind(&StorageMonitorCros::AddMountedPath,
-                   weak_ptr_factory_.GetWeakPtr(), it->second));
+                   weak_ptr_factory_.GetWeakPtr(), it.second));
   }
 
   // Note: Relies on scheduled tasks on the |blocking_task_runner| being

device/media_transfer_protocol/media_transfer_protocol_daemon_client.cc

@@ -161,7 +161,7 @@ class MediaTransferProtocolDaemonClientImpl
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(handle);
     {
-      dbus::MessageWriter array_writer(NULL);
+      dbus::MessageWriter array_writer(nullptr);
       writer.OpenArray("u", &array_writer);
 
       size_t end_offset = file_ids.size();
@@ -260,16 +260,11 @@ class MediaTransferProtocolDaemonClientImpl
       { mtpd::kMTPStorageAttached, true },
       { mtpd::kMTPStorageDetached, false },
     };
-    const size_t kNumSignalEventTuples = arraysize(kSignalEventTuples);
-
-    for (size_t i = 0; i < kNumSignalEventTuples; ++i) {
+    for (const auto& event : kSignalEventTuples) {
       proxy_->ConnectToSignal(
-          mtpd::kMtpdInterface,
-          kSignalEventTuples[i].signal_name,
+          mtpd::kMtpdInterface, event.signal_name,
           base::Bind(&MediaTransferProtocolDaemonClientImpl::OnMTPStorageSignal,
-                     weak_ptr_factory_.GetWeakPtr(),
-                     handler,
-                     kSignalEventTuples[i].is_attach),
+                     weak_ptr_factory_.GetWeakPtr(), handler, event.is_attach),
           base::Bind(&MediaTransferProtocolDaemonClientImpl::OnSignalConnected,
                      weak_ptr_factory_.GetWeakPtr()));
     }
@@ -373,24 +368,23 @@ class MediaTransferProtocolDaemonClientImpl
       return;
     }
 
-    std::vector<uint32_t> file_ids;
     dbus::MessageReader reader(response);
-    dbus::MessageReader array_reader(NULL);
+    dbus::MessageReader array_reader(nullptr);
     if (!reader.PopArray(&array_reader) || reader.HasMoreData()) {
       LOG(ERROR) << kInvalidResponseMsg << response->ToString();
       error_callback.Run();
       return;
     }
 
+    std::vector<uint32_t> file_ids;
     while (array_reader.HasMoreData()) {
       uint32_t file_id;
-      if (array_reader.PopUint32(&file_id)) {
-        file_ids.push_back(file_id);
-      } else {
+      if (!array_reader.PopUint32(&file_id)) {
         LOG(ERROR) << kInvalidResponseMsg << response->ToString();
         error_callback.Run();
         return;
       }
+      file_ids.push_back(file_id);
     }
     callback.Run(file_ids);
   }
@@ -404,7 +398,6 @@ class MediaTransferProtocolDaemonClientImpl
       return;
     }
 
-    std::vector<MtpFileEntry> file_entries;
     dbus::MessageReader reader(response);
     MtpFileEntries entries_protobuf;
     if (!reader.PopArrayOfBytesAsProto(&entries_protobuf)) {
@@ -413,6 +406,8 @@ class MediaTransferProtocolDaemonClientImpl
       return;
     }
 
+    std::vector<MtpFileEntry> file_entries;
+    file_entries.reserve(entries_protobuf.file_entries_size());
     for (int i = 0; i < entries_protobuf.file_entries_size(); ++i)
       file_entries.push_back(entries_protobuf.file_entries(i));
     callback.Run(file_entries);
@@ -428,7 +423,7 @@ class MediaTransferProtocolDaemonClientImpl
       return;
     }
 
-    const uint8_t* data_bytes = NULL;
+    const uint8_t* data_bytes = nullptr;
     size_t data_length = 0;
     dbus::MessageReader reader(response);
     if (!reader.PopArrayOfBytes(&data_bytes, &data_length)) {
@@ -495,7 +490,7 @@ class MediaTransferProtocolDaemonClientImpl
                               << signal << " failed.";
   }
 
-  dbus::ObjectProxy* proxy_;
+  dbus::ObjectProxy* const proxy_;
 
   bool listen_for_changes_called_;
 
@@ -516,9 +511,9 @@ MediaTransferProtocolDaemonClient::MediaTransferProtocolDaemonClient() {}
 MediaTransferProtocolDaemonClient::~MediaTransferProtocolDaemonClient() {}
 
 // static
-MediaTransferProtocolDaemonClient* MediaTransferProtocolDaemonClient::Create(
-    dbus::Bus* bus) {
-  return new MediaTransferProtocolDaemonClientImpl(bus);
+std::unique_ptr<MediaTransferProtocolDaemonClient>
+MediaTransferProtocolDaemonClient::Create(dbus::Bus* bus) {
+  return std::make_unique<MediaTransferProtocolDaemonClientImpl>(bus);
 }
 
 }  // namespace device

device/media_transfer_protocol/media_transfer_protocol_daemon_client.h

@@ -12,6 +12,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -38,57 +39,56 @@ namespace device {
 class MediaTransferProtocolDaemonClient {
  public:
   // A callback to be called when DBus method call fails.
-  typedef base::Closure ErrorCallback;
+  using ErrorCallback = base::Closure;
 
   // A callback to handle the result of EnumerateAutoMountableDevices.
   // The argument is the enumerated storage names.
-  typedef base::Callback<void(const std::vector<std::string>& storage_names)
-                         > EnumerateStoragesCallback;
+  using EnumerateStoragesCallback =
+      base::Callback<void(const std::vector<std::string>& storage_names)>;
 
   // A callback to handle the result of GetStorageInfo.
   // The argument is the information about the specified storage.
-  typedef base::Callback<void(const MtpStorageInfo& storage_info)
-                         > GetStorageInfoCallback;
+  using GetStorageInfoCallback =
+      base::Callback<void(const MtpStorageInfo& storage_info)>;
 
   // A callback to handle the result of OpenStorage.
   // The argument is the returned handle.
-  typedef base::Callback<void(const std::string& handle)> OpenStorageCallback;
+  using OpenStorageCallback = base::Callback<void(const std::string& handle)>;
 
   // A callback to handle the result of CloseStorage.
-  typedef base::Closure CloseStorageCallback;
+  using CloseStorageCallback = base::Closure;
 
   // A callback to handle the result of CreateDirectory.
-  typedef base::Closure CreateDirectoryCallback;
+  using CreateDirectoryCallback = base::Closure;
 
   // A callback to handle the result of ReadDirectoryEntryIds.
   // The argument is a vector of file ids.
-  typedef base::Callback<void(const std::vector<uint32_t>& file_ids)>
-      ReadDirectoryEntryIdsCallback;
+  using ReadDirectoryEntryIdsCallback =
+      base::Callback<void(const std::vector<uint32_t>& file_ids)>;
 
   // A callback to handle the result of GetFileInfo.
   // The argument is a vector of file entries.
-  typedef base::Callback<void(const std::vector<MtpFileEntry>& file_entries)
-                         > GetFileInfoCallback;
+  using GetFileInfoCallback =
+      base::Callback<void(const std::vector<MtpFileEntry>& file_entries)>;
 
   // A callback to handle the result of ReadFileChunkById.
   // The argument is a string containing the file data.
-  typedef base::Callback<void(const std::string& data)> ReadFileCallback;
+  using ReadFileCallback = base::Callback<void(const std::string& data)>;
 
   // A callback to handle the result of RenameObject.
-  typedef base::Closure RenameObjectCallback;
+  using RenameObjectCallback = base::Closure;
 
   // A callback to handle the result of CopyFileFromLocal.
-  typedef base::Closure CopyFileFromLocalCallback;
+  using CopyFileFromLocalCallback = base::Closure;
 
   // A callback to handle the result of DeleteObject.
-  typedef base::Closure DeleteObjectCallback;
+  using DeleteObjectCallback = base::Closure;
 
   // A callback to handle storage attach/detach events.
   // The first argument is true for attach, false for detach.
   // The second argument is the storage name.
-  typedef base::Callback<void(bool is_attach,
-                              const std::string& storage_name)
-                         > MTPStorageEventHandler;
+  using MTPStorageEventHandler =
+      base::Callback<void(bool is_attach, const std::string& storage_name)>;
 
   virtual ~MediaTransferProtocolDaemonClient();
 
@@ -204,8 +204,9 @@ class MediaTransferProtocolDaemonClient {
   // signal is received.
   virtual void ListenForChanges(const MTPStorageEventHandler& handler) = 0;
 
-  // Factory function, creates a new instance and returns ownership.
-  static MediaTransferProtocolDaemonClient* Create(dbus::Bus* bus);
+  // Factory function, creates a new instance.
+  static std::unique_ptr<MediaTransferProtocolDaemonClient> Create(
+      dbus::Bus* bus);
 
  protected:
   // Create() should be used instead.

device/media_transfer_protocol/media_transfer_protocol_manager.cc

@@ -14,6 +14,7 @@
 #include "base/command_line.h"
 #include "base/containers/queue.h"
 #include "base/location.h"
+#include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
@@ -31,7 +32,9 @@ namespace device {
 
 namespace {
 
-MediaTransferProtocolManager* g_media_transfer_protocol_manager = NULL;
+#if DCHECK_IS_ON()
+MediaTransferProtocolManager* g_media_transfer_protocol_manager = nullptr;
+#endif
 
 // When reading directory entries, this is the number of entries for
 // GetFileInfo() to read in one operation. If set too low, efficiency goes down
@@ -63,8 +66,10 @@ class MediaTransferProtocolManagerImpl : public MediaTransferProtocolManager {
   }
 
   ~MediaTransferProtocolManagerImpl() override {
+#if DCHECK_IS_ON()
     DCHECK(g_media_transfer_protocol_manager);
-    g_media_transfer_protocol_manager = NULL;
+    g_media_transfer_protocol_manager = nullptr;
+#endif
     if (bus_) {
       bus_->UnlistenForServiceOwnerChange(mtpd::kMtpdServiceName,
                                           mtpd_owner_changed_callback_);
@@ -89,11 +94,9 @@ class MediaTransferProtocolManagerImpl : public MediaTransferProtocolManager {
   const std::vector<std::string> GetStorages() const override {
     DCHECK(thread_checker_.CalledOnValidThread());
     std::vector<std::string> storages;
-    for (StorageInfoMap::const_iterator it = storage_info_map_.begin();
-         it != storage_info_map_.end();
-         ++it) {
-      storages.push_back(it->first);
-    }
+    storages.reserve(storage_info_map_.size());
+    for (const auto& info : storage_info_map_)
+      storages.push_back(info.first);
     return storages;
   }
 
@@ -102,7 +105,7 @@ class MediaTransferProtocolManagerImpl : public MediaTransferProtocolManager {
       const std::string& storage_name) const override {
     DCHECK(thread_checker_.CalledOnValidThread());
     StorageInfoMap::const_iterator it = storage_info_map_.find(storage_name);
-    return it != storage_info_map_.end() ? &it->second : NULL;
+    return it != storage_info_map_.end() ? &it->second : nullptr;
   }
 
   // MediaTransferProtocolManager override.
@@ -350,12 +353,12 @@ class MediaTransferProtocolManagerImpl : public MediaTransferProtocolManager {
   void OnEnumerateStorages(const std::vector<std::string>& storage_names) {
     DCHECK(thread_checker_.CalledOnValidThread());
     DCHECK(mtp_client_);
-    for (size_t i = 0; i < storage_names.size(); ++i) {
-      if (base::ContainsKey(storage_info_map_, storage_names[i])) {
+    for (const auto& name : storage_names) {
+      if (base::ContainsKey(storage_info_map_, name)) {
         // OnStorageChanged() might have gotten called first.
         continue;
       }
-      OnStorageAttached(storage_names[i]);
+      OnStorageAttached(name);
     }
   }
 
@@ -479,10 +482,9 @@ class MediaTransferProtocolManagerImpl : public MediaTransferProtocolManager {
 
     // Use |sorted_file_ids| to sanity check and make sure the results are a
     // subset of the requested file ids.
-    for (size_t i = 0; i < file_entries.size(); ++i) {
-      std::vector<uint32_t>::const_iterator it =
-          std::lower_bound(sorted_file_ids.begin(), sorted_file_ids.end(),
-                           file_entries[i].item_id());
+    for (const auto& entry : file_entries) {
+      std::vector<uint32_t>::const_iterator it = std::lower_bound(
+          sorted_file_ids.begin(), sorted_file_ids.end(), entry.item_id());
       if (it == sorted_file_ids.end()) {
         OnReadDirectoryError();
         return;
@@ -603,13 +605,12 @@ class MediaTransferProtocolManagerImpl : public MediaTransferProtocolManager {
     // Save a copy of |storage_info_map_| keys as |storage_info_map_| can
     // change in OnStorageDetached().
     std::vector<std::string> storage_names;
-    for (StorageInfoMap::const_iterator it = storage_info_map_.begin();
-         it != storage_info_map_.end();
-         ++it) {
-      storage_names.push_back(it->first);
-    }
-    for (size_t i = 0; i != storage_names.size(); ++i)
-      OnStorageDetached(storage_names[i]);
+    storage_names.reserve(storage_info_map_.size());
+    for (const auto& info : storage_info_map_)
+      storage_names.push_back(info.first);
+
+    for (const auto& name : storage_names)
+      OnStorageDetached(name);
 
     if (mtpd_service_owner.empty()) {
       current_mtpd_owner_.clear();
@@ -622,7 +623,7 @@ class MediaTransferProtocolManagerImpl : public MediaTransferProtocolManager {
     // |bus_| must be valid here. Otherwise, how did this method get called as a
     // callback in the first place?
     DCHECK(bus_);
-    mtp_client_.reset(MediaTransferProtocolDaemonClient::Create(bus_.get()));
+    mtp_client_ = MediaTransferProtocolDaemonClient::Create(bus_.get());
 
     // Set up signals and start initializing |storage_info_map_|.
     mtp_client_->ListenForChanges(
@@ -677,13 +678,18 @@ class MediaTransferProtocolManagerImpl : public MediaTransferProtocolManager {
 }  // namespace
 
 // static
-MediaTransferProtocolManager* MediaTransferProtocolManager::Initialize() {
-  DCHECK(!g_media_transfer_protocol_manager);
+std::unique_ptr<MediaTransferProtocolManager>
+MediaTransferProtocolManager::Initialize() {
+  auto manager = std::make_unique<MediaTransferProtocolManagerImpl>();
 
-  g_media_transfer_protocol_manager = new MediaTransferProtocolManagerImpl();
   VLOG(1) << "MediaTransferProtocolManager initialized";
 
-  return g_media_transfer_protocol_manager;
+#if DCHECK_IS_ON()
+  DCHECK(!g_media_transfer_protocol_manager);
+  g_media_transfer_protocol_manager = manager.get();
+#endif
+
+  return manager;
 }
 
 }  // namespace device

device/media_transfer_protocol/media_transfer_protocol_manager.h

@@ -8,6 +8,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -31,55 +32,56 @@ class MediaTransferProtocolManager {
   // A callback to handle the result of GetStorageInfoFromDevice.
   // The first argument is the returned storage info.
   // The second argument is true if there was an error.
-  typedef base::Callback<void(const MtpStorageInfo& storage_info,
-                              const bool error)>
-      GetStorageInfoFromDeviceCallback;
+  using GetStorageInfoFromDeviceCallback =
+      base::Callback<void(const MtpStorageInfo& storage_info,
+                          const bool error)>;
 
   // A callback to handle the result of OpenStorage.
   // The first argument is the returned handle.
   // The second argument is true if there was an error.
-  typedef base::Callback<void(const std::string& handle,
-                              bool error)> OpenStorageCallback;
+  using OpenStorageCallback =
+      base::Callback<void(const std::string& handle, bool error)>;
 
   // A callback to handle the result of CloseStorage.
   // The argument is true if there was an error.
-  typedef base::Callback<void(bool error)> CloseStorageCallback;
+  using CloseStorageCallback = base::Callback<void(bool error)>;
 
   // A callback to handle the result of CreateDirectory.
   // The first argument is true if there was an error.
-  typedef base::Callback<void(bool error)> CreateDirectoryCallback;
+  using CreateDirectoryCallback = base::Callback<void(bool error)>;
 
   // A callback to handle the result of ReadDirectory.
   // The first argument is a vector of file entries.
   // The second argument is true if there are more file entries.
   // The third argument is true if there was an error.
-  typedef base::Callback<void(const std::vector<MtpFileEntry>& file_entries,
-                              bool has_more,
-                              bool error)> ReadDirectoryCallback;
+  using ReadDirectoryCallback =
+      base::Callback<void(const std::vector<MtpFileEntry>& file_entries,
+                          bool has_more,
+                          bool error)>;
 
   // A callback to handle the result of ReadFileChunk.
   // The first argument is a string containing the file data.
   // The second argument is true if there was an error.
-  typedef base::Callback<void(const std::string& data,
-                              bool error)> ReadFileCallback;
+  using ReadFileCallback =
+      base::Callback<void(const std::string& data, bool error)>;
 
   // A callback to handle the result of GetFileInfo.
   // The first argument is a file entry.
   // The second argument is true if there was an error.
-  typedef base::Callback<void(const MtpFileEntry& file_entry,
-                              bool error)> GetFileInfoCallback;
+  using GetFileInfoCallback =
+      base::Callback<void(const MtpFileEntry& file_entry, bool error)>;
 
   // A callback to handle the result of RenameObject.
   // The first argument is true if there was an error.
-  typedef base::Callback<void(bool error)> RenameObjectCallback;
+  using RenameObjectCallback = base::Callback<void(bool error)>;
 
   // A callback to handle the result of CopyFileFromLocal.
   // The first argument is true if there was an error.
-  typedef base::Callback<void(bool error)> CopyFileFromLocalCallback;
+  using CopyFileFromLocalCallback = base::Callback<void(bool error)>;
 
   // A callback to handle the result of DeleteObject.
   // The first argument is true if there was an error.
-  typedef base::Callback<void(bool error)> DeleteObjectCallback;
+  using DeleteObjectCallback = base::Callback<void(bool error)>;
 
   // Implement this interface to be notified about MTP storage
   // attachment / detachment events.
@@ -169,7 +171,7 @@ class MediaTransferProtocolManager {
                             const DeleteObjectCallback& callback) = 0;
 
   // Creates and returns the global MediaTransferProtocolManager instance.
-  static MediaTransferProtocolManager* Initialize();
+  static std::unique_ptr<MediaTransferProtocolManager> Initialize();
 };
 
 }  // namespace device