Modernize MTPDeviceDelegateImplLinux code.

- Change CachedPathToId() to return base::Optional.
- Use more C++11 features.
- Use more early returns and mark things const when it makes sense.

Change-Id: I8f8cc6d5aac232d526b18abf6eea347a3161ee5d
Reviewed-on: https://chromium-review.googlesource.com/1193930Reviewed-by: Anand Mistry <amistry@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587357}

chrome/browser/media_galleries/chromeos/mtp_device_delegate_impl_chromeos.cc

@@ -37,6 +37,13 @@ namespace {
 // File path separator constant.
 const char kRootPath[] = "/";
 
+// Helper function to create |MTPDeviceDelegateImplLinux::storage_name_|.
+std::string CreateStorageName(const std::string& device_location) {
+  std::string storage_name;
+  base::RemoveChars(device_location, kRootPath, &storage_name);
+  return storage_name;
+}
+
 // Returns the device relative file path given |file_path|.
 // E.g.: If the |file_path| is "/usb:2,2:12345/DCIM" and |registered_dev_path|
 // is "/usb:2,2:12345", this function returns the device relative path which is
@@ -504,24 +511,22 @@ bool MTPDeviceDelegateImplLinux::MTPFileNode::DeleteChild(uint32_t file_id) {
 
 bool MTPDeviceDelegateImplLinux::MTPFileNode::HasChildren() const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-  return children_.size() > 0;
+  return !children_.empty();
 }
 
 MTPDeviceDelegateImplLinux::MTPDeviceDelegateImplLinux(
     const std::string& device_location,
     const bool read_only)
-    : init_state_(UNINITIALIZED),
-      task_in_progress_(false),
-      device_path_(device_location),
+    : device_path_(device_location),
+      storage_name_(CreateStorageName(device_location)),
       read_only_(read_only),
-      root_node_(new MTPFileNode(mtpd::kRootFileId,
+      root_node_(std::make_unique<MTPFileNode>(mtpd::kRootFileId,
                                                "",  // Root node has no name.
-                                 NULL,  // And no parent node.
+                                               nullptr,  // And no parent node.
                                                &file_id_to_node_map_)),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   DCHECK(!device_path_.empty());
-  base::RemoveChars(device_location, kRootPath, &storage_name_);
   DCHECK(!storage_name_.empty());
 }
 
@@ -857,25 +862,19 @@ void MTPDeviceDelegateImplLinux::GetFileInfoInternal(
     const ErrorCallback& error_callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  uint32_t file_id;
-  if (CachedPathToId(file_path, &file_id)) {
+  base::Optional<uint32_t> file_id = CachedPathToId(file_path);
+  if (file_id) {
     GetFileInfoSuccessCallback success_callback_wrapper =
         base::Bind(&MTPDeviceDelegateImplLinux::OnDidGetFileInfo,
                    weak_ptr_factory_.GetWeakPtr(),
                    success_callback);
     ErrorCallback error_callback_wrapper =
         base::Bind(&MTPDeviceDelegateImplLinux::HandleDeviceFileError,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   error_callback,
-                   file_id);
-
+                   weak_ptr_factory_.GetWeakPtr(), error_callback, *file_id);
 
-    base::Closure closure = base::Bind(&GetFileInfoOnUIThread,
-                                       storage_name_,
-                                       read_only_,
-                                       file_id,
-                                       success_callback_wrapper,
-                                       error_callback_wrapper);
+    base::Closure closure =
+        base::Bind(&GetFileInfoOnUIThread, storage_name_, read_only_, *file_id,
+                   success_callback_wrapper, error_callback_wrapper);
     EnsureInitAndRunTask(PendingTaskInfo(base::FilePath(),
                                          content::BrowserThread::UI,
                                          FROM_HERE,
@@ -900,8 +899,9 @@ void MTPDeviceDelegateImplLinux::CreateDirectoryInternal(
   if (other_components.empty()) {
     // Either we reached the last component in the recursive case, or this is
     // the non-recursive case.
-    uint32_t parent_id;
-    if (CachedPathToId(current_component.DirName(), &parent_id)) {
+    base::Optional<uint32_t> parent_id =
+        CachedPathToId(current_component.DirName());
+    if (parent_id) {
       const base::Closure closure =
           base::Bind(&MTPDeviceDelegateImplLinux::CreateSingleDirectory,
                      weak_ptr_factory_.GetWeakPtr(), current_component,
@@ -913,8 +913,8 @@ void MTPDeviceDelegateImplLinux::CreateDirectoryInternal(
     }
   } else {
     // Ensures that parent directories are created for recursive case.
-    uint32_t directory_id;
-    if (CachedPathToId(current_component, &directory_id)) {
+    base::Optional<uint32_t> directory_id = CachedPathToId(current_component);
+    if (directory_id) {
       // Parent directory |current_component| already exists, continue creating
       // directories.
       const base::Closure closure =
@@ -956,25 +956,18 @@ void MTPDeviceDelegateImplLinux::ReadDirectoryInternal(
     const ErrorCallback& error_callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  uint32_t dir_id;
-  if (CachedPathToId(root, &dir_id)) {
+  base::Optional<uint32_t> dir_id = CachedPathToId(root);
+  if (dir_id) {
     GetFileInfoSuccessCallback success_callback_wrapper =
         base::Bind(&MTPDeviceDelegateImplLinux::OnDidGetFileInfoToReadDirectory,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   dir_id,
-                   success_callback,
+                   weak_ptr_factory_.GetWeakPtr(), *dir_id, success_callback,
                    error_callback);
     ErrorCallback error_callback_wrapper =
         base::Bind(&MTPDeviceDelegateImplLinux::HandleDeviceFileError,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   error_callback,
-                   dir_id);
-    base::Closure closure = base::Bind(&GetFileInfoOnUIThread,
-                                       storage_name_,
-                                       read_only_,
-                                       dir_id,
-                                       success_callback_wrapper,
-                                       error_callback_wrapper);
+                   weak_ptr_factory_.GetWeakPtr(), error_callback, *dir_id);
+    base::Closure closure =
+        base::Bind(&GetFileInfoOnUIThread, storage_name_, read_only_, *dir_id,
+                   success_callback_wrapper, error_callback_wrapper);
     EnsureInitAndRunTask(PendingTaskInfo(base::FilePath(),
                                          content::BrowserThread::UI,
                                          FROM_HERE,
@@ -992,10 +985,10 @@ void MTPDeviceDelegateImplLinux::CreateSnapshotFileInternal(
     const ErrorCallback& error_callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  uint32_t file_id;
-  if (CachedPathToId(device_file_path, &file_id)) {
-    std::unique_ptr<SnapshotRequestInfo> request_info(new SnapshotRequestInfo(
-        file_id, local_path, success_callback, error_callback));
+  base::Optional<uint32_t> file_id = CachedPathToId(device_file_path);
+  if (file_id) {
+    auto request_info = std::make_unique<SnapshotRequestInfo>(
+        *file_id, local_path, success_callback, error_callback);
     GetFileInfoSuccessCallback success_callback_wrapper =
         base::Bind(
             &MTPDeviceDelegateImplLinux::OnDidGetFileInfoToCreateSnapshotFile,
@@ -1003,15 +996,10 @@ void MTPDeviceDelegateImplLinux::CreateSnapshotFileInternal(
             base::Passed(&request_info));
     ErrorCallback error_callback_wrapper =
         base::Bind(&MTPDeviceDelegateImplLinux::HandleDeviceFileError,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   error_callback,
-                   file_id);
-    base::Closure closure = base::Bind(&GetFileInfoOnUIThread,
-                                       storage_name_,
-                                       read_only_,
-                                       file_id,
-                                       success_callback_wrapper,
-                                       error_callback_wrapper);
+                   weak_ptr_factory_.GetWeakPtr(), error_callback, *file_id);
+    base::Closure closure =
+        base::Bind(&GetFileInfoOnUIThread, storage_name_, read_only_, *file_id,
+                   success_callback_wrapper, error_callback_wrapper);
     EnsureInitAndRunTask(PendingTaskInfo(base::FilePath(),
                                          content::BrowserThread::UI,
                                          FROM_HERE,
@@ -1031,17 +1019,14 @@ void MTPDeviceDelegateImplLinux::ReadBytesInternal(
     const ErrorCallback& error_callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  uint32_t file_id;
-  if (CachedPathToId(device_file_path, &file_id)) {
+  base::Optional<uint32_t> file_id = CachedPathToId(device_file_path);
+  if (file_id) {
     ReadBytesRequest request(
-        file_id, buf, offset, buf_len,
+        *file_id, buf, offset, buf_len,
         base::Bind(&MTPDeviceDelegateImplLinux::OnDidReadBytes,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   success_callback),
+                   weak_ptr_factory_.GetWeakPtr(), success_callback),
         base::Bind(&MTPDeviceDelegateImplLinux::HandleDeviceFileError,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   error_callback,
-                   file_id));
+                   weak_ptr_factory_.GetWeakPtr(), error_callback, *file_id));
 
     base::Closure closure =
         base::Bind(&ReadBytesOnUIThread, storage_name_, read_only_, request);
@@ -1071,26 +1056,28 @@ void MTPDeviceDelegateImplLinux::MoveFileLocalInternal(
 
   if (source_file_path.DirName() == device_file_path.DirName()) {
     // If a file is moved in a same directory, rename the file.
-    uint32_t file_id;
-    if (CachedPathToId(source_file_path, &file_id)) {
+    base::Optional<uint32_t> file_id = CachedPathToId(source_file_path);
+    if (file_id) {
       const MTPDeviceTaskHelper::RenameObjectSuccessCallback
           success_callback_wrapper = base::Bind(
               &MTPDeviceDelegateImplLinux::OnDidMoveFileLocalWithRename,
               weak_ptr_factory_.GetWeakPtr(), success_callback,
-              source_file_path, file_id);
+              source_file_path, *file_id);
       const MTPDeviceTaskHelper::ErrorCallback error_callback_wrapper =
           base::Bind(&MTPDeviceDelegateImplLinux::HandleDeviceFileError,
-                     weak_ptr_factory_.GetWeakPtr(), error_callback, file_id);
+                     weak_ptr_factory_.GetWeakPtr(), error_callback, *file_id);
       const base::Closure closure =
           base::Bind(&RenameObjectOnUIThread, storage_name_, read_only_,
-                     file_id, device_file_path.BaseName().value(),
+                     *file_id, device_file_path.BaseName().value(),
                      success_callback_wrapper, error_callback_wrapper);
       EnsureInitAndRunTask(PendingTaskInfo(
           base::FilePath(), content::BrowserThread::UI, FROM_HERE, closure));
     } else {
       error_callback.Run(base::File::FILE_ERROR_NOT_FOUND);
     }
-  } else {
+    return;
+  }
+
   // If a file is moved to a different directory, create a copy to the
   // destination path, and remove source file.
   const CopyFileLocalSuccessCallback& success_callback_wrapper =
@@ -1102,7 +1089,6 @@ void MTPDeviceDelegateImplLinux::MoveFileLocalInternal(
                 create_temporary_file_callback,
                 base::Bind(&FakeCopyFileProgressCallback),
                 success_callback_wrapper, error_callback);
-  }
 }
 
 void MTPDeviceDelegateImplLinux::OnDidOpenFDToCopyFileFromLocal(
@@ -1118,8 +1104,14 @@ void MTPDeviceDelegateImplLinux::OnDidOpenFDToCopyFileFromLocal(
   }
 
   const int source_file_descriptor = open_fd_result.first;
-  uint32_t parent_id;
-  if (CachedPathToId(device_file_path.DirName(), &parent_id)) {
+  base::Optional<uint32_t> parent_id =
+      CachedPathToId(device_file_path.DirName());
+  if (!parent_id) {
+    HandleCopyFileFromLocalError(error_callback, source_file_descriptor,
+                                 base::File::FILE_ERROR_NOT_FOUND);
+    return;
+  }
+
   CopyFileFromLocalSuccessCallback success_callback_wrapper =
       base::Bind(&MTPDeviceDelegateImplLinux::OnDidCopyFileFromLocal,
                  weak_ptr_factory_.GetWeakPtr(), success_callback,
@@ -1129,21 +1121,13 @@ void MTPDeviceDelegateImplLinux::OnDidOpenFDToCopyFileFromLocal(
       &MTPDeviceDelegateImplLinux::HandleCopyFileFromLocalError,
       weak_ptr_factory_.GetWeakPtr(), error_callback, source_file_descriptor);
 
-    base::Closure closure = base::Bind(&CopyFileFromLocalOnUIThread,
-                                       storage_name_,
-                                       read_only_,
-                                       source_file_descriptor,
-                                       parent_id,
-                                       device_file_path.BaseName().value(),
-                                       success_callback_wrapper,
-                                       error_callback_wrapper);
+  base::Closure closure = base::Bind(
+      &CopyFileFromLocalOnUIThread, storage_name_, read_only_,
+      source_file_descriptor, *parent_id, device_file_path.BaseName().value(),
+      success_callback_wrapper, error_callback_wrapper);
 
   EnsureInitAndRunTask(PendingTaskInfo(
       base::FilePath(), content::BrowserThread::UI, FROM_HERE, closure));
-  } else {
-    HandleCopyFileFromLocalError(error_callback, source_file_descriptor,
-                                 base::File::FILE_ERROR_NOT_FOUND);
-  }
 }
 
 void MTPDeviceDelegateImplLinux::DeleteFileInternal(
@@ -1155,14 +1139,17 @@ void MTPDeviceDelegateImplLinux::DeleteFileInternal(
 
   if (file_info.is_directory) {
     error_callback.Run(base::File::FILE_ERROR_NOT_A_FILE);
-  } else {
-    uint32_t file_id;
-    if (CachedPathToId(file_path, &file_id))
-      RunDeleteObjectOnUIThread(file_path, file_id, success_callback,
-                                error_callback);
-    else
+    return;
+  }
+
+  base::Optional<uint32_t> file_id = CachedPathToId(file_path);
+  if (!file_id) {
     error_callback.Run(base::File::FILE_ERROR_NOT_FOUND);
+    return;
   }
+
+  RunDeleteObjectOnUIThread(file_path, *file_id, success_callback,
+                            error_callback);
 }
 
 void MTPDeviceDelegateImplLinux::DeleteDirectoryInternal(
@@ -1177,8 +1164,8 @@ void MTPDeviceDelegateImplLinux::DeleteDirectoryInternal(
     return;
   }
 
-  uint32_t directory_id;
-  if (!CachedPathToId(file_path, &directory_id)) {
+  base::Optional<uint32_t> directory_id = CachedPathToId(file_path);
+  if (!directory_id) {
     error_callback.Run(base::File::FILE_ERROR_NOT_FOUND);
     return;
   }
@@ -1186,7 +1173,7 @@ void MTPDeviceDelegateImplLinux::DeleteDirectoryInternal(
   // Checks the cache first. If it has children in cache, the directory cannot
   // be empty.
   FileIdToMTPFileNodeMap::const_iterator it =
-      file_id_to_node_map_.find(directory_id);
+      file_id_to_node_map_.find(*directory_id);
   if (it != file_id_to_node_map_.end() && it->second->HasChildren()) {
     error_callback.Run(base::File::FILE_ERROR_NOT_EMPTY);
     return;
@@ -1199,12 +1186,12 @@ void MTPDeviceDelegateImplLinux::DeleteDirectoryInternal(
           base::BindOnce(&MTPDeviceDelegateImplLinux::
                              OnDidCheckDirectoryEmptyToDeleteDirectory,
                          weak_ptr_factory_.GetWeakPtr(), file_path,
-                         directory_id, success_callback, error_callback);
+                         *directory_id, success_callback, error_callback);
   const MTPDeviceTaskHelper::ErrorCallback error_callback_wrapper =
       base::Bind(&MTPDeviceDelegateImplLinux::HandleDeviceFileError,
-                 weak_ptr_factory_.GetWeakPtr(), error_callback, directory_id);
+                 weak_ptr_factory_.GetWeakPtr(), error_callback, *directory_id);
   const base::Closure closure = base::Bind(
-      &CheckDirectoryEmptyOnUIThread, storage_name_, read_only_, directory_id,
+      &CheckDirectoryEmptyOnUIThread, storage_name_, read_only_, *directory_id,
       base::Passed(&success_callback_wrapper), error_callback_wrapper);
   EnsureInitAndRunTask(PendingTaskInfo(
       base::FilePath(), content::BrowserThread::UI, FROM_HERE, closure));
@@ -1426,8 +1413,8 @@ void MTPDeviceDelegateImplLinux::OnPathDoesNotExistForCreateSingleDirectory(
     return;
   }
 
-  uint32_t parent_id;
-  if (!CachedPathToId(directory_path.DirName(), &parent_id)) {
+  base::Optional<uint32_t> parent_id = CachedPathToId(directory_path.DirName());
+  if (!parent_id) {
     error_callback.Run(base::File::FILE_ERROR_NOT_FOUND);
     return;
   }
@@ -1438,10 +1425,10 @@ void MTPDeviceDelegateImplLinux::OnPathDoesNotExistForCreateSingleDirectory(
           weak_ptr_factory_.GetWeakPtr(), directory_path, success_callback);
   const MTPDeviceTaskHelper::ErrorCallback error_callback_wrapper =
       base::Bind(&MTPDeviceDelegateImplLinux::HandleDeviceFileError,
-                 weak_ptr_factory_.GetWeakPtr(), error_callback, parent_id);
+                 weak_ptr_factory_.GetWeakPtr(), error_callback, *parent_id);
   const base::Closure closure =
       base::Bind(&CreateDirectoryOnUIThread, storage_name_, read_only_,
-                 parent_id, directory_path.BaseName().value(),
+                 *parent_id, directory_path.BaseName().value(),
                  success_callback_wrapper, error_callback_wrapper);
   EnsureInitAndRunTask(PendingTaskInfo(
       base::FilePath(), content::BrowserThread::UI, FROM_HERE, closure));
@@ -1843,8 +1830,8 @@ base::FilePath MTPDeviceDelegateImplLinux::NextUncachedPathComponent(
     DCHECK(!device_relpath_components.empty());
     bool all_components_cached = true;
     const MTPFileNode* current_node = root_node_.get();
-    for (size_t i = 0; i < device_relpath_components.size(); ++i) {
-      current_node = current_node->GetChild(device_relpath_components[i]);
+    for (const std::string& component : device_relpath_components) {
+      current_node = current_node->GetChild(component);
       if (!current_node) {
         // With a cache miss, check if it is a genuine failure. If so, pretend
         // the entire |path| is cached, so there is no further attempt to do
@@ -1853,7 +1840,7 @@ base::FilePath MTPDeviceDelegateImplLinux::NextUncachedPathComponent(
             !cached_path.empty() && (uncached_path == cached_path);
         break;
       }
-      uncached_path = uncached_path.Append(device_relpath_components[i]);
+      uncached_path = uncached_path.Append(component);
     }
     if (all_components_cached)
       uncached_path.clear();
@@ -1876,38 +1863,36 @@ void MTPDeviceDelegateImplLinux::FillFileCache(
   ReadDirectoryInternal(uncached_path, success_callback, error_callback);
 }
 
-bool MTPDeviceDelegateImplLinux::CachedPathToId(const base::FilePath& path,
-                                                uint32_t* id) const {
-  DCHECK(id);
-
+base::Optional<uint32_t> MTPDeviceDelegateImplLinux::CachedPathToId(
+    const base::FilePath& path) const {
   std::string device_relpath = GetDeviceRelativePath(device_path_, path);
   if (device_relpath.empty())
-    return false;
+    return {};
   std::vector<std::string> device_relpath_components;
   if (device_relpath != kRootPath) {
     device_relpath_components = base::SplitString(
         device_relpath, "/", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
   }
   const MTPFileNode* current_node = root_node_.get();
-  for (size_t i = 0; i < device_relpath_components.size(); ++i) {
-    current_node = current_node->GetChild(device_relpath_components[i]);
+  for (const std::string& component : device_relpath_components) {
+    current_node = current_node->GetChild(component);
     if (!current_node)
-      return false;
+      return {};
   }
-  *id = current_node->file_id();
-  return true;
+  return current_node->file_id();
 }
 
-void MTPDeviceDelegateImplLinux::EvictCachedPathToId(const uint32_t id) {
+void MTPDeviceDelegateImplLinux::EvictCachedPathToId(uint32_t id) {
   FileIdToMTPFileNodeMap::iterator it = file_id_to_node_map_.find(id);
-  if (it != file_id_to_node_map_.end()) {
+  if (it == file_id_to_node_map_.end())
+    return;
+
   DCHECK(!it->second->HasChildren());
   MTPFileNode* parent = it->second->parent();
   if (parent) {
-      const bool ret = parent->DeleteChild(id);
+    bool ret = parent->DeleteChild(id);
     DCHECK(ret);
   }
-  }
 }
 
 void CreateMTPDeviceAsyncDelegate(

chrome/browser/media_galleries/chromeos/mtp_device_delegate_impl_chromeos.h

@@ -11,6 +11,8 @@
 #include <memory>
 #include <set>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/containers/circular_deque.h"
@@ -18,6 +20,7 @@
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "chrome/browser/media_galleries/chromeos/mtp_device_task_helper.h"
 #include "chrome/browser/media_galleries/fileapi/mtp_device_async_delegate.h"
 #include "content/public/browser/browser_thread.h"
@@ -456,27 +459,26 @@ class MTPDeviceDelegateImplLinux : public MTPDeviceAsyncDelegate {
   // Fills the file cache using the results from NextUncachedPathComponent().
   void FillFileCache(const base::FilePath& uncached_path);
 
-  // Given a full path, if it exists in the cache, writes the file's id to |id|
-  // and return true.
-  bool CachedPathToId(const base::FilePath& path, uint32_t* id) const;
+  // Given a full path, if it exists in the cache, return the id.
+  base::Optional<uint32_t> CachedPathToId(const base::FilePath& path) const;
 
   // Evict the cache of |id|.
-  void EvictCachedPathToId(const uint32_t id);
+  void EvictCachedPathToId(uint32_t id);
 
   // MTP device initialization state.
-  InitializationState init_state_;
+  InitializationState init_state_ = UNINITIALIZED;
 
   // Used to make sure only one task is in progress at any time.
   // Otherwise the browser will try to send too many requests at once and
   // overload the device.
-  bool task_in_progress_;
+  bool task_in_progress_ = false;
 
   // Registered file system device path. This path does not
   // correspond to a real device path (e.g. "/usb:2,2:81282").
   const base::FilePath device_path_;
 
   // MTP device storage name (e.g. "usb:2,2:81282").
-  std::string storage_name_;
+  const std::string storage_name_;
 
   // Mode for opening storage.
   const bool read_only_;
@@ -505,7 +507,7 @@ class MTPDeviceDelegateImplLinux : public MTPDeviceAsyncDelegate {
 
   // The root node of a tree-structure that caches the directory structure of
   // the MTP device.
-  std::unique_ptr<MTPFileNode> root_node_;
+  const std::unique_ptr<MTPFileNode> root_node_;
 
   // A list of child nodes encountered while a ReadDirectory operation, which
   // can return results over multiple callbacks, is in progress.