Stylistic fixes for BrowsingDataFileSystemHelper.

The only functional change is to drop the `profile_` property from the
CannedBrowsingDataFileSystemHelper, which just stored it without ever
touching it.  I don't like keeping pointers around if I don't need them, so
I'm dropping it here.

BUG=None
TEST=Read the code. Can you understand it?


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

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

chrome/browser/browsing_data_file_system_helper.cc

@@ -11,24 +11,20 @@
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/browser/browser_thread.h"
-#include "content/browser/in_process_webkit/webkit_context.h"
-#include "third_party/WebKit/Source/WebKit/chromium/public/WebCString.h"
-#include "third_party/WebKit/Source/WebKit/chromium/public/WebSecurityOrigin.h"
-#include "third_party/WebKit/Source/WebKit/chromium/public/WebString.h"
 #include "webkit/fileapi/file_system_quota_util.h"
 #include "webkit/fileapi/file_system_context.h"
 #include "webkit/fileapi/file_system_types.h"
 #include "webkit/fileapi/sandbox_mount_point_provider.h"
-#include "webkit/glue/webkit_glue.h"
-
-using WebKit::WebSecurityOrigin;
 
 namespace {
 
+// An implementation of the BrowsingDataFileSystemHelper interface that pulls
+// data from a given |profile| and returns a list of FileSystemInfo items to a
+// client.
 class BrowsingDataFileSystemHelperImpl : public BrowsingDataFileSystemHelper {
  public:
+  // BrowsingDataFileSystemHelper implementation
   explicit BrowsingDataFileSystemHelperImpl(Profile* profile);
-
   virtual void StartFetching(
       Callback1<const std::vector<FileSystemInfo>& >::Type* callback);
   virtual void CancelNotification();
@@ -37,26 +33,36 @@ class BrowsingDataFileSystemHelperImpl : public BrowsingDataFileSystemHelper {
  private:
   virtual ~BrowsingDataFileSystemHelperImpl();
 
-  // Enumerates all filesystem files in the FILE thread.
+  // Enumerates all filesystem files, storing the resulting list into
+  // file_system_file_ for later use. This must be called on the FILE thread.
   void FetchFileSystemInfoInFileThread();
-  // Notifies the completion callback in the UI thread.
-  void NotifyInUIThread();
-  // Delete data for an origin on the FILE thread.
+
+  // Triggers the success callback as the end of a StartFetching workflow. This
+  // must be called on the UI thread.
+  void NotifyOnUIThread();
+
+  // Deletes all file systems associated with |origin|. This must be called on
+  // the FILE thread.
   void DeleteFileSystemOriginInFileThread(const GURL& origin);
 
+  // We don't own the Profile object. Clients are responsible for destroying the
+  // object when it's no longer used.
   Profile* profile_;
 
-  // This only mutates in the FILE thread.
+  // Holds the current list of file systems returned to the client after
+  // StartFetching is called. This only mutates in the FILE thread.
   std::vector<FileSystemInfo> file_system_info_;
 
-  // This only mutates on the UI thread.
+  // Holds the callback passed in at the beginning of the StartFetching workflow
+  // so that it can be triggered via NotifyOnUIThread. This only mutates on the
+  // UI thread.
   scoped_ptr<Callback1<const std::vector<FileSystemInfo>& >::Type >
       completion_callback_;
 
-  // Indicates whether or not we're currently fetching information:
-  // it's true when StartFetching() is called in the UI thread, and it's reset
-  // after we notified the callback in the UI thread.
-  // This only mutates on the UI thread.
+  // Indicates whether or not we're currently fetching information: set to true
+  // when StartFetching is called on the UI thread, and reset to false when
+  // NotifyOnUIThread triggers the success callback.
+  // This property only mutates on the UI thread.
   bool is_fetching_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowsingDataFileSystemHelperImpl);
@@ -109,7 +115,9 @@ void BrowsingDataFileSystemHelperImpl::FetchFileSystemInfoInFileThread() {
   scoped_ptr<fileapi::SandboxMountPointProvider::OriginEnumerator>
       origin_enumerator(profile_->GetFileSystemContext()->path_manager()->
           sandbox_provider()->CreateOriginEnumerator());
-  // We don't own this pointer; deleting it would be a bad idea.
+
+  // We don't own this pointer; it's a magic singleton generated by the
+  // profile's FileSystemContext. Deleting it would be a bad idea.
   fileapi::FileSystemQuotaUtil* quota_util = profile_->
       GetFileSystemContext()->GetQuotaUtil(fileapi::kFileSystemTypeTemporary);
 
@@ -119,6 +127,8 @@ void BrowsingDataFileSystemHelperImpl::FetchFileSystemInfoInFileThread() {
       // Extension state is not considered browsing data.
       continue;
     }
+    // We can call these synchronous methods as we've already verified that
+    // we're running on the FILE thread.
     int64 persistent_usage = quota_util->GetOriginUsageOnFileThread(current,
         fileapi::kFileSystemTypePersistent);
     int64 temporary_usage = quota_util->GetOriginUsageOnFileThread(current,
@@ -137,14 +147,14 @@ void BrowsingDataFileSystemHelperImpl::FetchFileSystemInfoInFileThread() {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(
-          this, &BrowsingDataFileSystemHelperImpl::NotifyInUIThread));
+          this, &BrowsingDataFileSystemHelperImpl::NotifyOnUIThread));
 }
 
-void BrowsingDataFileSystemHelperImpl::NotifyInUIThread() {
+void BrowsingDataFileSystemHelperImpl::NotifyOnUIThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(is_fetching_);
-  // Note: completion_callback_ mutates only in the UI thread, so it's safe to
-  // test it here.
+  // completion_callback_ mutates only in the UI thread, so we're safe to test
+  // it here.
   if (completion_callback_ != NULL) {
     completion_callback_->Run(file_system_info_);
     completion_callback_.reset();
@@ -182,27 +192,30 @@ BrowsingDataFileSystemHelper* BrowsingDataFileSystemHelper::Create(
 }
 
 CannedBrowsingDataFileSystemHelper::
-PendingFileSystemInfo::PendingFileSystemInfo() {
+    PendingFileSystemInfo::PendingFileSystemInfo() {
 }
 
 CannedBrowsingDataFileSystemHelper::
-PendingFileSystemInfo::PendingFileSystemInfo(const GURL& origin,
-                                             const fileapi::FileSystemType type,
-                                             int64 size)
+    PendingFileSystemInfo::PendingFileSystemInfo(
+        const GURL& origin,
+        const fileapi::FileSystemType type,
+        int64 size)
     : origin(origin),
       type(type),
       size(size) {
 }
 
 CannedBrowsingDataFileSystemHelper::
-PendingFileSystemInfo::~PendingFileSystemInfo() {
+    PendingFileSystemInfo::~PendingFileSystemInfo() {
 }
 
 CannedBrowsingDataFileSystemHelper::CannedBrowsingDataFileSystemHelper(
-    Profile* profile)
-    : profile_(profile),
-      is_fetching_(false) {
-  DCHECK(profile);
+    Profile* /* profile */)
+    : is_fetching_(false) {
+}
+
+CannedBrowsingDataFileSystemHelper::CannedBrowsingDataFileSystemHelper()
+    : is_fetching_(false) {
 }
 
 CannedBrowsingDataFileSystemHelper::~CannedBrowsingDataFileSystemHelper() {}
@@ -211,8 +224,7 @@ CannedBrowsingDataFileSystemHelper*
     CannedBrowsingDataFileSystemHelper::Clone() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   CannedBrowsingDataFileSystemHelper* clone =
-      new CannedBrowsingDataFileSystemHelper(profile_);
-
+      new CannedBrowsingDataFileSystemHelper();
   clone->pending_file_system_info_ = pending_file_system_info_;
   clone->file_system_info_ = file_system_info_;
   return clone;
@@ -242,8 +254,9 @@ void CannedBrowsingDataFileSystemHelper::StartFetching(
   completion_callback_.reset(callback);
 
   for (std::vector<PendingFileSystemInfo>::const_iterator
-       info = pending_file_system_info_.begin();
-       info != pending_file_system_info_.end(); ++info) {
+           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();
@@ -273,16 +286,13 @@ void CannedBrowsingDataFileSystemHelper::StartFetching(
   }
   pending_file_system_info_.clear();
 
-//  MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(this,
-//      &CannedBrowsingDataFileSystemHelper::Notify));
-
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(
-          this, &CannedBrowsingDataFileSystemHelper::Notify));
+          this, &CannedBrowsingDataFileSystemHelper::NotifyOnUIThread));
 }
 
-void CannedBrowsingDataFileSystemHelper::Notify() {
+void CannedBrowsingDataFileSystemHelper::NotifyOnUIThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(is_fetching_);
   if (completion_callback_ != NULL) {

chrome/browser/browsing_data_file_system_helper.h

@@ -21,16 +21,18 @@
 
 class Profile;
 
-// BrowsingDataFileSystemHelper is an interface for classes dealing with
-// aggregating and deleting browsing data stored in local filesystems.  A
-// client of this class needs to call StartFetching from the UI thread to
-// initiate the flow, and it'll be notified by the callback in its UI thread at
-// some later point.  The client must call CancelNotification() if it's
-// destroyed before the callback is notified.
+// 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
+// CancelNotification.
 class BrowsingDataFileSystemHelper
     : public base::RefCountedThreadSafe<BrowsingDataFileSystemHelper> {
  public:
-  // Contains detailed information about a filesystem
+  // Detailed information about a file system, including it's origin GURL,
+  // the presence or absence of persistent and temporary storage, and the
+  // amount of data (in bytes) each contains.
   struct FileSystemInfo {
     FileSystemInfo(
         const GURL& origin,
@@ -40,10 +42,6 @@ class BrowsingDataFileSystemHelper
         int64 usage_temporary);
     ~FileSystemInfo();
 
-    bool IsFileSchemeData() {
-      return origin.scheme() == chrome::kFileScheme;
-    }
-
     GURL origin;
     bool has_persistent;
     bool has_temporary;
@@ -51,20 +49,23 @@ class BrowsingDataFileSystemHelper
     int64 usage_temporary;
   };
 
-  // Create a BrowsingDataFileSystemHelper instance for the filesystems
+  // Create a BrowsingDataFileSystemHelper instance for the file systems
   // stored in |profile|'s user data directory.
   static BrowsingDataFileSystemHelper* Create(Profile* profile);
 
-  // Starts the fetching process, which will notify its completion via
-  // callback.
+  // 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.
   virtual void StartFetching(
       Callback1<const std::vector<FileSystemInfo>& >::Type* callback) = 0;
-  // Cancels the notification callback (i.e., the window that created it no
-  // longer exists).
-  // This must be called only in the UI thread.
+
+  // Cancels the notification callback associated with StartFetching. Clients
+  // that are destroyed before the callback is triggered must call this, and
+  // it must be called only on the UI thread.
   virtual void CancelNotification() = 0;
-  // Requests a single filesystem to be deleted in the FILE thread.
+
+  // 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.
   virtual void DeleteFileSystemOrigin(const GURL& origin) = 0;
 
  protected:
@@ -72,26 +73,31 @@ class BrowsingDataFileSystemHelper
   virtual ~BrowsingDataFileSystemHelper() {}
 };
 
-// This class is an implementation of BrowsingDataFileSystemHelper that does
-// not fetch its information from the filesystem tracker, but gets it passed
-// in as a parameter.
+// An implementation of the BrowsingDataFileSystemHelper interface that can
+// be manually populated with data, rather than fetching data from the file
+// systems created in a particular Profile.
 class CannedBrowsingDataFileSystemHelper
     : public BrowsingDataFileSystemHelper {
  public:
+  // |profile| is unused in this canned implementation, but it's the interface
+  // we're writing to, so we'll accept it, but not store it.
   explicit CannedBrowsingDataFileSystemHelper(Profile* profile);
 
-  // Return a copy of the filesystem helper. Only one consumer can use the
-  // StartFetching method at a time, so we need to create a copy of the helper
-  // everytime we instantiate a cookies tree model for it.
+  // Creates a copy of the file system helper. StartFetching can only respond
+  // 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.
   CannedBrowsingDataFileSystemHelper* Clone();
 
-  // Add a filesystem to the set of canned filesystems that is
-  // returned by this helper.
+  // Manually add 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).
   void AddFileSystem(const GURL& origin,
                      fileapi::FileSystemType type,
                      int64 size);
 
-  // Clear the list of canned filesystems.
+  // Clear this helper's list of canned filesystems.
   void Reset();
 
   // True if no filesystems are currently stored.
@@ -104,6 +110,14 @@ class CannedBrowsingDataFileSystemHelper
   virtual void DeleteFileSystemOrigin(const GURL& origin) {}
 
  private:
+  // Used by Clone() to create an object without a Profile
+  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,
@@ -116,25 +130,27 @@ class CannedBrowsingDataFileSystemHelper
     int64 size;
   };
 
-  // StartFetching's callback should be executed asynchronously, Notify handles
-  // that nicely.
-  void Notify();
-
-  virtual ~CannedBrowsingDataFileSystemHelper();
-
-  Profile* profile_;
+  // 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_;
 
+  // Holds the callback passed in at the beginning of the StartFetching workflow
+  // so that it can be triggered via NotifyOnUIThread.
   scoped_ptr<Callback1<const std::vector<FileSystemInfo>& >::Type >
       completion_callback_;
 
-  // Indicates whether or not we're currently fetching information:
-  // it's true when StartFetching() is called in the UI thread, and it's reset
-  // after we notified the callback in the UI thread.
-  // This only mutates on the UI thread.
+  // Indicates whether or not we're currently fetching information: set to true
+  // when StartFetching is called on the UI thread, and reset to false when
+  // NotifyOnUIThread triggers the success callback.
+  // This property only mutates on the UI thread.
   bool is_fetching_;
 
   DISALLOW_COPY_AND_ASSIGN(CannedBrowsingDataFileSystemHelper);