installer and some misc files: Change ScopedTempDir::path() to GetPath()

chrome/installer/util/copy_tree_work_item.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/installer/util/copy_tree_work_item.h"
 
 #include <shlwapi.h>
 
 #include "base/files/file_util.h"
 #include "base/logging.h"
@@ skipping 53 matching lines @@
       return true;
     }
   } else if ((dest_exist) &&
              (overwrite_option_ == WorkItem::IF_NOT_PRESENT)) {
     // handle overwrite_option_ = IF_NOT_PRESENT case.
     return true;
   }
 
   // In all cases that reach here, move dest to a backup path.
   if (dest_exist) {
-    if (!backup_path_.CreateUniqueTempDirUnderPath(temp_dir_)) {
+    if (!backup_dir_.CreateUniqueTempDirUnderPath(temp_dir_)) {
       PLOG(ERROR) << "Failed to get backup path in folder "
                   << temp_dir_.value();
       return false;
     }
+    backup_path_ = backup_dir_.GetPath();

[gab, 2016/09/28 16:31:59]

This results in an extra copy though. I did read your comment in the header but
I don't understand why GetPath() doesn't work below, feels like both use cases
would be happy with a const FilePath& to Append() to?

[vabr (Chromium), 2016/09/28 20:06:36]

Here yes, but one of the unit tests [1] uses backup_path_ to verify that the
backup directory does not, in fact, exist. If copying the path is a concern, I
can replace the FilePath with just a bool, which would indicate whether the
backup directory has been created or not.

[1]
https://cs.chromium.org/chromium/src/chrome/installer/util/copy_tree_work_ite...

[gab, 2016/09/28 20:25:25]

So a bool would do? I also haven't looked at the tests in detail but testing for
empty() on a private variable feels like it's reaching into the guts more than a
test should (i.e. it's abusing being a friend).

[vabr (Chromium), 2016/09/28 20:33:10]

Happy to switch to a bool.

 
-    base::FilePath backup = backup_path_.path().Append(dest_path_.BaseName());
+    base::FilePath backup = backup_path_.Append(dest_path_.BaseName());
     if (base::Move(dest_path_, backup)) {
       moved_to_backup_ = true;
       VLOG(1) << "Moved destination " << dest_path_.value() <<
                  " to backup path " << backup.value();
     } else {
       PLOG(ERROR) << "failed moving " << dest_path_.value()
                   << " to " << backup.value();
       return false;
     }
   }
@@ skipping 15 matching lines @@
 void CopyTreeWorkItem::RollbackImpl() {
   // Normally the delete operations below should not fail unless some
   // programs like anti-virus are inspecting the files we just copied.
   // If this does happen sometimes, we may consider using Move instead of
   // Delete here. For now we just log the error and continue with the
   // rest of rollback operation.
   if (copied_to_dest_path_ && !base::DeleteFile(dest_path_, true)) {
     LOG(ERROR) << "Can not delete " << dest_path_.value();
   }
   if (moved_to_backup_) {
-    base::FilePath backup(backup_path_.path().Append(dest_path_.BaseName()));
+    base::FilePath backup(backup_path_.Append(dest_path_.BaseName()));
     if (!base::Move(backup, dest_path_)) {
       PLOG(ERROR) << "failed move " << backup.value()
                   << " to " << dest_path_.value();
     }
   }
   if (copied_to_alternate_path_ &&
       !base::DeleteFile(alternative_path_, true)) {
     LOG(ERROR) << "Can not delete " << alternative_path_.value();
   }
 }
 
 bool CopyTreeWorkItem::IsFileInUse(const base::FilePath& path) {
   if (!base::PathExists(path))
     return false;
 
   HANDLE handle = ::CreateFile(path.value().c_str(), FILE_ALL_ACCESS,
                                NULL, NULL, OPEN_EXISTING, NULL, NULL);
   if (handle  == INVALID_HANDLE_VALUE)
     return true;
 
   CloseHandle(handle);
   return false;
 }