c/browser, c/common, components M-N: Change ScopedTempDir::path() to GetPath()

chrome/browser/media_galleries/fileapi/itunes_file_util_unittest.cc

 // Copyright 2013 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/browser/media_galleries/fileapi/itunes_file_util.h"
 
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 65 matching lines @@
     EXPECT_TRUE(fake_auto_add_dir_.CreateUniqueTempDir());
   }
 
   ~TestITunesDataProvider() override {}
 
   void RefreshData(const ReadyCallback& ready_callback) override {
     ready_callback.Run(true /* success */);
   }
 
   const base::FilePath& auto_add_path() const override {
-    return fake_auto_add_dir_.path();
+    return fake_auto_add_dir_path_;
   }
 
   void SetProvideAutoAddDir(bool provide_auto_add_dir) {
     if (provide_auto_add_dir) {
       if (!fake_auto_add_dir_.IsValid())
         ASSERT_TRUE(fake_auto_add_dir_.CreateUniqueTempDir());
+      fake_auto_add_dir_path_ = fake_auto_add_dir_.GetPath();
     } else {
       ASSERT_TRUE(fake_auto_add_dir_.Delete());
+      fake_auto_add_dir_path_.clear();
     }
   }
 
  private:
+  base::FilePath fake_auto_add_dir_path_;

[vabr (Chromium), 2016/09/14 16:22:17]

Before: ScopedTempDir needed to trust the code calling auto_add_path() that the
path is not used if empty.
Now: With adding fake_auto_add_dir_path_ auto_add_path() needs to trust its
callers that they won't use the path for file access if the path is empty.

In other words, there is still potential for misuse, but at least the scope is
now more limited.

[Reilly Grant (use Gerrit), 2016/09/14 18:06:19]

You could add an ASSERT_FALSE(fake_auto_add_dir_path_.empty()) to
auto_add_path().

[vabr (Chromium), 2016/09/14 20:14:39]

The trouble is that it can actually be empty:
ItunesMediaDirectoryContentsNoAutoAdd instructs the data provider not to have
the directory. The empty path is still taken from the provider in
itunes::ITunesFileUtil::ReadDirectorySync, but fortunately only to check if it
is empty, not to write files under it in the empty case.

I guess this kind of trust in the code is OK inside
chrome/browser/media_galleries/fileapi, where a single person is likely to
understand the impacted code. It is problematic in the case when
base::ScopedTempDir should be vending the empty path not knowing what the caller
is going to do with it.

   base::ScopedTempDir fake_auto_add_dir_;
 };
 
 class TestITunesFileUtil : public ITunesFileUtil {
  public:
   explicit TestITunesFileUtil(MediaPathFilter* media_path_filter,
                               ITunesDataProvider* data_provider)
       : ITunesFileUtil(media_path_filter),
         data_provider_(data_provider) {
   }
@@ skipping 27 matching lines @@
 };
 
 class ItunesFileUtilTest : public testing::Test {
  public:
   ItunesFileUtilTest()
       : io_thread_(content::BrowserThread::IO, &message_loop_) {
   }
 
   void SetUpDataProvider() {
     ASSERT_TRUE(fake_library_dir_.CreateUniqueTempDir());
-    ASSERT_EQ(
-        0,
-        base::WriteFile(
-            fake_library_dir_.path().AppendASCII(kITunesLibraryXML),
-            NULL,
-            0));
+    ASSERT_EQ(0, base::WriteFile(
+                     fake_library_dir_.GetPath().AppendASCII(kITunesLibraryXML),
+                     NULL, 0));
 
     itunes_data_provider_.reset(
-        new TestITunesDataProvider(fake_library_dir_.path()));
+        new TestITunesDataProvider(fake_library_dir_.GetPath()));
   }
 
   void SetUp() override {
     ASSERT_TRUE(profile_dir_.CreateUniqueTempDir());
     ImportedMediaGalleryRegistry::GetInstance()->Initialize();
 
     scoped_refptr<storage::SpecialStoragePolicy> storage_policy =
         new content::MockSpecialStoragePolicy();
 
     // Initialize fake ItunesDataProvider on media task runner thread.
     MediaFileSystemBackend::MediaTaskRunner()->PostTask(
         FROM_HERE,
         base::Bind(&ItunesFileUtilTest::SetUpDataProvider,
                    base::Unretained(this)));
     base::WaitableEvent event(base::WaitableEvent::ResetPolicy::MANUAL,
                               base::WaitableEvent::InitialState::NOT_SIGNALED);
     MediaFileSystemBackend::MediaTaskRunner()->PostTask(
         FROM_HERE,
         base::Bind(&base::WaitableEvent::Signal, base::Unretained(&event)));
     event.Wait();
 
     media_path_filter_.reset(new MediaPathFilter());
     ScopedVector<storage::FileSystemBackend> additional_providers;
     additional_providers.push_back(new TestMediaFileSystemBackend(
-        profile_dir_.path(),
+        profile_dir_.GetPath(),
         new TestITunesFileUtil(media_path_filter_.get(),
                                itunes_data_provider_.get())));
 
     file_system_context_ = new storage::FileSystemContext(
         base::ThreadTaskRunnerHandle::Get().get(),
         base::ThreadTaskRunnerHandle::Get().get(),
         storage::ExternalMountPoints::CreateRefCounted().get(),
         storage_policy.get(), NULL, std::move(additional_providers),
-        std::vector<storage::URLRequestAutoMountHandler>(), profile_dir_.path(),
-        content::CreateAllowFileAccessOptions());
+        std::vector<storage::URLRequestAutoMountHandler>(),
+        profile_dir_.GetPath(), content::CreateAllowFileAccessOptions());
   }
 
  protected:
   void TestNonexistentFolder(const std::string& path_append) {
     FileSystemOperation::FileEntryList contents;
     FileSystemURL url = CreateURL(path_append);
     bool completed = false;
     ReadDirectoryTestHelper(operation_runner(), url, &contents, &completed);
 
     ASSERT_FALSE(completed);
@@ skipping 143 matching lines @@
   completed = false;
   ReadDirectoryTestHelper(operation_runner(), url, &contents, &completed);
   ASSERT_TRUE(completed);
   ASSERT_EQ(1u, contents.size());
   EXPECT_FALSE(contents.front().is_directory);
   EXPECT_EQ(base::FilePath().AppendASCII("baz.ogg").value(),
             contents.front().name);
 }
 
 }  // namespace itunes