Readability review for BrowsingDataFileSystemHelper.

BUG=None
TEST=Examine the code. Is it readable?

Review URL: http://codereview.chromium.org/7104105

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@92141 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 141 insertions, 116 deletions

diff --git a/chrome/browser/browsing_data_file_system_helper.cc b/chrome/browser/browsing_data_file_system_helper.cc
index 766452a..f2870e8 100644
--- a/chrome/browser/browsing_data_file_system_helper.cc
+++ b/chrome/browser/browsing_data_file_system_helper.cc
@@ -191,26 +191,6 @@ BrowsingDataFileSystemHelper* BrowsingDataFileSystemHelper::Create(
   return new BrowsingDataFileSystemHelperImpl(profile);
 }
 
-CannedBrowsingDataFileSystemHelper::
-    PendingFileSystemInfo::PendingFileSystemInfo()
-        : type(fileapi::kFileSystemTypeUnknown),
-          size(0) {
-}
-
-CannedBrowsingDataFileSystemHelper::
-    PendingFileSystemInfo::PendingFileSystemInfo(
-        const GURL& origin,
-        const fileapi::FileSystemType type,
-        int64 size)
-    : origin(origin),
-      type(type),
-      size(size) {
-}
-
-CannedBrowsingDataFileSystemHelper::
-    PendingFileSystemInfo::~PendingFileSystemInfo() {
-}
-
 CannedBrowsingDataFileSystemHelper::CannedBrowsingDataFileSystemHelper(
     Profile* /* profile */)
     : is_fetching_(false) {
@@ -227,24 +207,53 @@ CannedBrowsingDataFileSystemHelper*
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   CannedBrowsingDataFileSystemHelper* clone =
       new CannedBrowsingDataFileSystemHelper();
-  clone->pending_file_system_info_ = pending_file_system_info_;
+  // This list only mutates on the UI thread, so it's safe to work with it here
+  // (given the DCHECK above).
   clone->file_system_info_ = file_system_info_;
   return clone;
 }
 
 void CannedBrowsingDataFileSystemHelper::AddFileSystem(
     const GURL& origin, const fileapi::FileSystemType type, const int64 size) {
-  pending_file_system_info_.push_back(
-      PendingFileSystemInfo(origin, type, size));
+
+  // This canned implementation of AddFileSystem uses an O(n^2) algorithm; which
+  // is fine, as it isn't meant for use in a high-volume context. If it turns
+  // out that we want to start using this in a context with many, many origins,
+  // we should think about reworking the implementation.
+  bool duplicate_origin = false;
+  for (std::vector<FileSystemInfo>::iterator
+           file_system = file_system_info_.begin();
+       file_system != file_system_info_.end();
+       ++file_system) {
+    if (file_system->origin == origin) {
+      if (type == fileapi::kFileSystemTypePersistent) {
+        file_system->has_persistent = true;
+        file_system->usage_persistent = size;
+      } else {
+        file_system->has_temporary = true;
+        file_system->usage_temporary = size;
+      }
+      duplicate_origin = true;
+      break;
+    }
+  }
+  if (duplicate_origin)
+    return;
+
+  file_system_info_.push_back(FileSystemInfo(
+      origin,
+      (type == fileapi::kFileSystemTypePersistent),
+      (type == fileapi::kFileSystemTypeTemporary),
+      (type == fileapi::kFileSystemTypePersistent) ? size : 0,
+      (type == fileapi::kFileSystemTypeTemporary) ? size : 0));
 }
 
 void CannedBrowsingDataFileSystemHelper::Reset() {
   file_system_info_.clear();
-  pending_file_system_info_.clear();
 }
 
 bool CannedBrowsingDataFileSystemHelper::empty() const {
-  return file_system_info_.empty() && pending_file_system_info_.empty();
+  return file_system_info_.empty();
 }
 
 void CannedBrowsingDataFileSystemHelper::StartFetching(
@@ -255,39 +264,6 @@ void CannedBrowsingDataFileSystemHelper::StartFetching(
   is_fetching_ = true;
   completion_callback_.reset(callback);
 
-  for (std::vector<PendingFileSystemInfo>::const_iterator
-           info = pending_file_system_info_.begin();
-       info != pending_file_system_info_.end();
-       ++info) {
-    bool duplicate = false;
-    for (std::vector<FileSystemInfo>::iterator
-             file_system = file_system_info_.begin();
-         file_system != file_system_info_.end();
-         ++file_system) {
-      if (file_system->origin == info->origin) {
-        if (info->type == fileapi::kFileSystemTypePersistent) {
-          file_system->has_persistent = true;
-          file_system->usage_persistent = info->size;
-        } else {
-          file_system->has_temporary = true;
-          file_system->usage_temporary = info->size;
-        }
-        duplicate = true;
-        break;
-      }
-    }
-    if (duplicate)
-      continue;
-
-    file_system_info_.push_back(FileSystemInfo(
-        info->origin,
-        (info->type == fileapi::kFileSystemTypePersistent),
-        (info->type == fileapi::kFileSystemTypeTemporary),
-        (info->type == fileapi::kFileSystemTypePersistent) ? info->size : 0,
-        (info->type == fileapi::kFileSystemTypeTemporary) ? info->size : 0));
-  }
-  pending_file_system_info_.clear();
-
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(
diff --git a/chrome/browser/browsing_data_file_system_helper.h b/chrome/browser/browsing_data_file_system_helper.h
index 8389919..4dae0d4 100644
--- a/chrome/browser/browsing_data_file_system_helper.h
+++ b/chrome/browser/browsing_data_file_system_helper.h
@@ -22,11 +22,23 @@
 class Profile;
 
 // Defines an interface for classes that deal with aggregating and deleting
-// browsing data stored in an origin's file systems. Clients of this interface
-// must call StartFetching from the UI thread to initiate the flow, and will
-// be notified via a callback at some later point (also in the UI thread). If
-// the client is destroyed before the callback is triggered, it must call
+// browsing data stored in an origin's file systems.
+// BrowsingDataFileSystemHelper instances for a specific profile should be
+// created via the static Create method. Each instance will lazily fetch file
+// system data when a client calls StartFetching from the UI thread, and will
+// notify the client via a supplied callback when the data is available.
+// Only one StartFetching task can run at a time: executing StartFetching while
+// another StartFetching task is running will DCHECK. If the client must abort
+// the process before completion (it's destroyed, for example) then it must call
 // CancelNotification.
+//
+// The client's callback is passed a vector of FileSystemInfo objects containing
+// usage information for each origin's temporary and persistent file systems.
+//
+// Clients may remove an origin's file systems at any time (even before fetching
+// data) by calling DeleteFileSystemOrigin() on the UI thread. Calling
+// DeleteFileSystemOrigin() for an origin that doesn't have any is safe; it's
+// just an expensive NOOP.
 class BrowsingDataFileSystemHelper
     : public base::RefCountedThreadSafe<BrowsingDataFileSystemHelper> {
  public:
@@ -42,20 +54,34 @@ class BrowsingDataFileSystemHelper
         int64 usage_temporary);
     ~FileSystemInfo();
 
+    // The origin for which the information is relevant.
     GURL origin;
+    // True if the origin has a persistent file system.
     bool has_persistent;
+    // True if the origin has a temporary file system.
     bool has_temporary;
+    // Persistent file system usage, in bytes.
     int64 usage_persistent;
+    // Temporary file system usage, in bytes.
     int64 usage_temporary;
   };
 
-  // Create a BrowsingDataFileSystemHelper instance for the file systems
-  // stored in |profile|'s user data directory.
+  // Creates a BrowsingDataFileSystemHelper instance for the file systems
+  // stored in |profile|'s user data directory. The BrowsingDataFileSystemHelper
+  // object will hold a reference to the Profile that's passed in, but is not
+  // responsible for destroying it.
+  //
+  // The BrowsingDataFileSystemHelper will not change the profile itself, but
+  // can modify data it contains (by removing file systems).
   static BrowsingDataFileSystemHelper* Create(Profile* profile);
 
   // Starts the process of fetching file system data, which will call |callback|
   // upon completion, passing it a constant vector of FileSystemInfo objects.
-  // This must be called only in the UI thread.
+  // StartFetching must be called only in the UI thread; the provided Callback1
+  // will likewise be executed asynchronously on the UI thread.
+  //
+  // BrowsingDataFileSystemHelper takes ownership of the Callback1, and is
+  // responsible for deleting it once it's no longer needed.
   virtual void StartFetching(
       Callback1<const std::vector<FileSystemInfo>& >::Type* callback) = 0;
 
@@ -64,12 +90,15 @@ class BrowsingDataFileSystemHelper
   // it must be called only on the UI thread.
   virtual void CancelNotification() = 0;
 
-  // Deletes all file systems associated with |origin|. The deletion will occur
-  // on the FILE thread, but this function must be called only on the UI thread.
+  // Deletes any temporary or persistent file systems associated with |origin|
+  // from the disk. Deletion will occur asynchronously on the FILE thread, but
+  // this function must be called only on the UI thread.
   virtual void DeleteFileSystemOrigin(const GURL& origin) = 0;
 
  protected:
   friend class base::RefCountedThreadSafe<BrowsingDataFileSystemHelper>;
+
+  BrowsingDataFileSystemHelper() {}
   virtual ~BrowsingDataFileSystemHelper() {}
 };
 
@@ -87,9 +116,13 @@ class CannedBrowsingDataFileSystemHelper
   // to one client at a time; we need to be able to act on multiple parallel
   // requests in certain situations (see CookiesTreeModel and its clients). For
   // these cases, simply clone the object and fire off another fetching process.
+  //
+  // Clone() is safe to call while StartFetching() is running. Clients of the
+  // newly created object must themselves execute StartFetching(), however: the
+  // copy will not have a pending fetch.
   CannedBrowsingDataFileSystemHelper* Clone();
 
-  // Manually add a filesystem to the set of canned file systems that this
+  // Manually adds a filesystem to the set of canned file systems that this
   // helper returns via StartFetching. If an origin contains both a temporary
   // and a persistent filesystem, AddFileSystem must be called twice (once for
   // each file system type).
@@ -107,6 +140,11 @@ class CannedBrowsingDataFileSystemHelper
   virtual void StartFetching(
       Callback1<const std::vector<FileSystemInfo>& >::Type* callback);
   virtual void CancelNotification();
+
+  // Note that this doesn't actually have an implementation for this canned
+  // class. It hasn't been necessary for anything that uses the canned
+  // implementation, as the canned class is only used in tests, or in read-only
+  // contexts (like the non-modal cookie dialog).
   virtual void DeleteFileSystemOrigin(const GURL& origin) {}
 
  private:
@@ -114,30 +152,10 @@ class CannedBrowsingDataFileSystemHelper
   CannedBrowsingDataFileSystemHelper();
   virtual ~CannedBrowsingDataFileSystemHelper();
 
-  // AddFileSystem doesn't store file systems directly, but holds them until
-  // StartFetching is called. At that point, the pending file system data is
-  // merged with the current file system data before being returned to the
-  // client.
-  struct PendingFileSystemInfo {
-    PendingFileSystemInfo();
-    PendingFileSystemInfo(const GURL& origin,
-                          fileapi::FileSystemType type,
-                          int64 size);
-    ~PendingFileSystemInfo();
-
-    GURL origin;
-    fileapi::FileSystemType type;
-    int64 size;
-  };
-
   // Triggers the success callback as the end of a StartFetching workflow. This
   // must be called on the UI thread.
   void NotifyOnUIThread();
 
-  // Holds file systems that have been added to the helper until StartFetching
-  // is called.
-  std::vector<PendingFileSystemInfo> pending_file_system_info_;
-
   // Holds the current list of file systems returned to the client after
   // StartFetching is called.
   std::vector<FileSystemInfo> file_system_info_;
diff --git a/chrome/browser/browsing_data_file_system_helper_unittest.cc b/chrome/browser/browsing_data_file_system_helper_unittest.cc
index fabe105..6b673d9 100644
--- a/chrome/browser/browsing_data_file_system_helper_unittest.cc
+++ b/chrome/browser/browsing_data_file_system_helper_unittest.cc
@@ -17,23 +17,35 @@
 
 namespace {
 
+// Shorter names for fileapi::* constants.
 const fileapi::FileSystemType kTemporary = fileapi::kFileSystemTypeTemporary;
 const fileapi::FileSystemType kPersistent = fileapi::kFileSystemTypePersistent;
 
+// We'll use these three distinct origins for testing, both as strings and as
+// GURLs in appropriate contexts.
 const char kTestOrigin1[] = "http://host1:1/";
-const char kTestOrigin2[] = "http://host2:1/";
-const char kTestOrigin3[] = "http://host3:1/";
+const char kTestOrigin2[] = "http://host2:2/";
+const char kTestOrigin3[] = "http://host3:3/";
 
 const GURL kOrigin1(kTestOrigin1);
 const GURL kOrigin2(kTestOrigin2);
 const GURL kOrigin3(kTestOrigin3);
 
+// TODO(mkwst): Update this size once the discussion in http://crbug.com/86114
+// is concluded.
 const int kEmptyFileSystemSize = 0;
 
 typedef std::vector<BrowsingDataFileSystemHelper::FileSystemInfo>
     FileSystemInfoVector;
 typedef scoped_ptr<FileSystemInfoVector> ScopedFileSystemInfoVector;
 
+// The FileSystem APIs are all asynchronous; this testing class wraps up the
+// boilerplate code necessary to deal with waiting for responses. In a nutshell,
+// any async call whose response we want to test ought to be followed by a call
+// to BlockUntilNotified(), which will (shockingly!) block until Notify() is
+// called. For this to work, you'll need to ensure that each async call is
+// implemented as a class method that that calls Notify() at an appropriate
+// point.
 class BrowsingDataFileSystemHelperTest : public testing::Test {
  public:
   BrowsingDataFileSystemHelperTest()
@@ -49,22 +61,41 @@ class BrowsingDataFileSystemHelperTest : public testing::Test {
     return &profile_;
   }
 
-  void FindFileSystemPathCallback(bool success,
+  // Blocks on the current MessageLoop until Notify() is called.
+  void BlockUntilNotified() {
+    MessageLoop::current()->Run();
+  }
+
+  // Unblocks the current MessageLoop. Should be called in response to some sort
+  // of async activity in a callback method.
+  void Notify() {
+    MessageLoop::current()->Quit();
+  }
+
+  // Callback that should be executed in response to
+  // fileapi::SandboxMountPointProvider::ValidateFileSystemRootAndGetURL
+  void CallbackFindFileSystemPath(bool success,
                                   const FilePath& path,
                                   const std::string& name) {
     found_file_system_ = success;
     Notify();
   }
 
+  // Calls fileapi::SandboxMountPointProvider::ValidateFileSystemRootAndGetURL
+  // to verify the existence of a file system for a specified type and origin,
+  // blocks until a response is available, then returns the result
+  // synchronously to it's caller.
   bool FileSystemContainsOriginAndType(const GURL& origin,
                                        fileapi::FileSystemType type) {
     sandbox_->ValidateFileSystemRootAndGetURL(
         origin, type, false, NewCallback(this,
-            &BrowsingDataFileSystemHelperTest::FindFileSystemPathCallback));
+            &BrowsingDataFileSystemHelperTest::CallbackFindFileSystemPath));
     BlockUntilNotified();
     return found_file_system_;
   }
 
+  // Callback that should be executed in response to StartFetching(), and stores
+  // found file systems locally so that they are available via GetFileSystems().
   void CallbackStartFetching(
       const std::vector<BrowsingDataFileSystemHelper::FileSystemInfo>&
           file_system_info_list) {
@@ -74,21 +105,25 @@ class BrowsingDataFileSystemHelperTest : public testing::Test {
     Notify();
   }
 
+  // Calls StartFetching() on the test's BrowsingDataFileSystemHelper
+  // object, then blocks until the callback is executed.
   void FetchFileSystems() {
     helper_->StartFetching(NewCallback(this,
         &BrowsingDataFileSystemHelperTest::CallbackStartFetching));
     BlockUntilNotified();
   }
 
+  // Calls StartFetching() on the test's CannedBrowsingDataFileSystemHelper
+  // object, then blocks until the callback is executed.
   void FetchCannedFileSystems() {
     canned_helper_->StartFetching(NewCallback(this,
         &BrowsingDataFileSystemHelperTest::CallbackStartFetching));
     BlockUntilNotified();
   }
 
+  // Sets up kOrigin1 with a temporary file system, kOrigin2 with a persistent
+  // file system, and kOrigin3 with both.
   virtual void PopulateTestFileSystemData() {
-    // Set up kOrigin1 with a temporary file system, kOrigin2 with a persistent
-    // file system, and kOrigin3 with both.
     sandbox_ = profile_.GetFileSystemContext()->path_manager()->
         sandbox_provider();
 
@@ -105,6 +140,8 @@ class BrowsingDataFileSystemHelperTest : public testing::Test {
     EXPECT_TRUE(FileSystemContainsOriginAndType(kOrigin3, kTemporary));
   }
 
+  // Uses the fileapi methods to create a filesystem of a given type for a
+  // specified origin.
   void CreateDirectoryForOriginAndType(const GURL& origin,
                                        fileapi::FileSystemType type) {
     FilePath target = sandbox_->ValidateFileSystemRootAndGetPathOnFileThread(
@@ -112,17 +149,12 @@ class BrowsingDataFileSystemHelperTest : public testing::Test {
     EXPECT_TRUE(file_util::DirectoryExists(target));
   }
 
+  // Returns a list of the FileSystemInfo objects gathered in the most recent
+  // call to StartFetching().
   FileSystemInfoVector* GetFileSystems() {
     return file_system_info_list_.get();
   }
 
-  void BlockUntilNotified() {
-    MessageLoop::current()->Run();
-  }
-
-  void Notify() {
-    MessageLoop::current()->Quit();
-  }
 
   // Temporary storage to pass information back from callbacks.
   bool found_file_system_;
@@ -133,7 +165,8 @@ class BrowsingDataFileSystemHelperTest : public testing::Test {
 
  private:
   // message_loop_, as well as all the threads associated with it must be
-  // defined before profile_ to prevent explosions. Oh how I love C++.
+  // defined before profile_ to prevent explosions. The threads also must be
+  // defined in the order they're listed here. Oh how I love C++.
   MessageLoopForUI message_loop_;
   BrowserThread ui_thread_;
   BrowserThread file_thread_;
@@ -146,6 +179,8 @@ class BrowsingDataFileSystemHelperTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(BrowsingDataFileSystemHelperTest);
 };
 
+// Verifies that the BrowsingDataFileSystemHelper correctly finds the test file
+// system data, and that each file system returned contains the expected data.
 TEST_F(BrowsingDataFileSystemHelperTest, FetchData) {
   PopulateTestFileSystemData();
 
@@ -159,18 +194,21 @@ TEST_F(BrowsingDataFileSystemHelperTest, FetchData) {
     BrowsingDataFileSystemHelper::FileSystemInfo info =
         file_system_info_list_->at(i);
     if (info.origin == kOrigin1) {
+        EXPECT_FALSE(test_hosts_found[0]);
         test_hosts_found[0] = true;
         EXPECT_FALSE(info.has_persistent);
         EXPECT_TRUE(info.has_temporary);
         EXPECT_EQ(0, info.usage_persistent);
         EXPECT_EQ(kEmptyFileSystemSize, info.usage_temporary);
     } else if (info.origin == kOrigin2) {
+        EXPECT_FALSE(test_hosts_found[1]);
         test_hosts_found[1] = true;
         EXPECT_TRUE(info.has_persistent);
         EXPECT_FALSE(info.has_temporary);
         EXPECT_EQ(kEmptyFileSystemSize, info.usage_persistent);
         EXPECT_EQ(0, info.usage_temporary);
     } else if (info.origin == kOrigin3) {
+        EXPECT_FALSE(test_hosts_found[2]);
         test_hosts_found[2] = true;
         EXPECT_TRUE(info.has_persistent);
         EXPECT_TRUE(info.has_temporary);
@@ -185,6 +223,8 @@ TEST_F(BrowsingDataFileSystemHelperTest, FetchData) {
   }
 }
 
+// Verifies that the BrowsingDataFileSystemHelper correctly deletes file
+// systems via DeleteFileSystemOrigin().
 TEST_F(BrowsingDataFileSystemHelperTest, DeleteData) {
   PopulateTestFileSystemData();
 
@@ -209,6 +249,8 @@ TEST_F(BrowsingDataFileSystemHelperTest, DeleteData) {
   }
 }
 
+// Verifies that the CannedBrowsingDataFileSystemHelper correctly reports
+// whether or not it currently contains file systems.
 TEST_F(BrowsingDataFileSystemHelperTest, Empty) {
   ASSERT_TRUE(canned_helper_->empty());
   canned_helper_->AddFileSystem(kOrigin1, kTemporary, 0);
@@ -217,6 +259,8 @@ TEST_F(BrowsingDataFileSystemHelperTest, Empty) {
   ASSERT_TRUE(canned_helper_->empty());
 }
 
+// Verifies that AddFileSystem correctly adds file systems, and that both
+// the type and usage metadata are reported as provided.
 TEST_F(BrowsingDataFileSystemHelperTest, CannedAddFileSystem) {
   canned_helper_->AddFileSystem(kOrigin1, kPersistent, 200);
   canned_helper_->AddFileSystem(kOrigin2, kTemporary, 100);
@@ -236,18 +280,5 @@ TEST_F(BrowsingDataFileSystemHelperTest, CannedAddFileSystem) {
   EXPECT_EQ(100, file_system_info_list_->at(1).usage_temporary);
 }
 
-TEST_F(BrowsingDataFileSystemHelperTest, CannedUnique) {
-  canned_helper_->AddFileSystem(kOrigin3, kPersistent, 200);
-  canned_helper_->AddFileSystem(kOrigin3, kTemporary, 100);
-
-  FetchCannedFileSystems();
-
-  EXPECT_EQ(1U, file_system_info_list_->size());
-  EXPECT_EQ(kOrigin3, file_system_info_list_->at(0).origin);
-  EXPECT_TRUE(file_system_info_list_->at(0).has_persistent);
-  EXPECT_TRUE(file_system_info_list_->at(0).has_temporary);
-  EXPECT_EQ(200, file_system_info_list_->at(0).usage_persistent);
-  EXPECT_EQ(100, file_system_info_list_->at(0).usage_temporary);
-}
-
 }  // namespace
+