[BlobAsync] Asynchronous Blob Construction Final Patch

storage/browser/blob/blob_storage_context.h

Index: storage/browser/blob/blob_storage_context.h
diff --git a/storage/browser/blob/blob_storage_context.h b/storage/browser/blob/blob_storage_context.h
index 54cff9b2a86d4f99d7caae178be8ab694f4080b0..6ec14eefa9d5755173d6e79e0bbc0e2357bd9c09 100644
--- a/storage/browser/blob/blob_storage_context.h
+++ b/storage/browser/blob/blob_storage_context.h
@@ -12,15 +12,16 @@
 #include <string>
 #include <vector>
 
+#include "base/callback_forward.h"
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "storage/browser/blob/blob_data_handle.h"
-#include "storage/browser/blob/blob_data_item.h"
-#include "storage/browser/blob/blob_data_snapshot.h"
+#include "storage/browser/blob/blob_storage_registry.h"
 #include "storage/browser/blob/internal_blob_data.h"
-#include "storage/browser/blob/shareable_blob_data_item.h"
 #include "storage/browser/storage_browser_export.h"
+#include "storage/common/blob_storage/blob_storage_constants.h"
 #include "storage/common/data_element.h"
 
 class GURL;
@@ -31,13 +32,16 @@ class Time;
 }
 
 namespace content {
-class BlobStorageHost;
+class BlobDispatcherHost;
+class BlobDispatcherHostTest;
 }
 
 namespace storage {
 
 class BlobDataBuilder;
-class InternalBlobData;
+class BlobDataItem;
+class BlobDataSnapshot;
+class ShareableBlobDataItem;
 
 // This class handles the logistics of blob Storage within the browser process,
 // and maintains a mapping from blob uuid to the data. The class is single
@@ -69,53 +73,49 @@ class STORAGE_EXPORT BlobStorageContext
   void RevokePublicBlobURL(const GURL& url);
 
   size_t memory_usage() const { return memory_usage_; }
-  size_t blob_count() const { return blob_map_.size(); }
+  size_t blob_count() const { return registry_.blob_count(); }
+  size_t memory_available() const {
+    return kBlobStorageMaxMemoryUsage - memory_usage_;
+  }
+
+  const BlobStorageRegistry& registry() { return registry_; }
 
  private:
-  friend class content::BlobStorageHost;
+  using BlobRegistryEntry = BlobStorageRegistry::Entry;
+  friend class content::BlobDispatcherHost;
+  friend class BlobAsyncBuilderHost;
+  friend class BlobAsyncBuilderHostTest;
   friend class BlobDataHandle::BlobDataHandleShared;
+  friend class BlobStorageContextTest;
+  FRIEND_TEST_ALL_PREFIXES(BlobStorageContextTest, IncrementDecrementRef);
+  FRIEND_TEST_ALL_PREFIXES(BlobStorageContextTest, OnCancelBuildingBlob);
+  FRIEND_TEST_ALL_PREFIXES(BlobStorageContextTest, PublicBlobUrls);
   friend class ViewBlobInternalsJob;
 
-  enum EntryFlags {
-    EXCEEDED_MEMORY = 1 << 1,
-  };
-
-  struct BlobMapEntry {
-    int refcount;
-    int flags;
-    // data and data_builder are mutually exclusive.
-    scoped_ptr<InternalBlobData> data;
-    scoped_ptr<InternalBlobData::Builder> data_builder;
-
-    BlobMapEntry();
-    BlobMapEntry(int refcount, InternalBlobData::Builder* data);
-    ~BlobMapEntry();
-
-    bool IsBeingBuilt();
-  };
-
-  typedef std::map<std::string, BlobMapEntry*> BlobMap;
-  typedef std::map<GURL, std::string> BlobURLMap;
+  // This completes construction of the blob, including resolving blob
+  // references in the builder. This also handles if the blob was previously
+  // flagged as broken or if it exceeds memory.

[michaeln, 2016/02/06 00:08:39]

the broken'ess handling is an impl detail, external callers probably should only
pass PENDING blobs in (as the method name suggests)

the three method (create complete break|cancel) names are fairly self
explanatory, i'm not sure you need any doc comments to be honest, maybe mention
that cmplete and cancel will invoke the OnConstructionComplete callbacks?

+  void CompletePendingBlob(const BlobDataBuilder& external_builder);
 
-  // Called by BlobDataHandle.
+  // Methods called by BlobDataHandle:
   scoped_ptr<BlobDataSnapshot> CreateSnapshot(const std::string& uuid);
-
-  // ### Methods called by BlobStorageHost ###
-  void StartBuildingBlob(const std::string& uuid);
-  void AppendBlobDataItem(const std::string& uuid,
-                          const DataElement& data_item);
-  void FinishBuildingBlob(const std::string& uuid, const std::string& type);
-  void CancelBuildingBlob(const std::string& uuid);
+  bool IsBroken(const std::string& uuid) const;
+  bool IsBeingBuilt(const std::string& uuid) const;
+  // Runs |done| when construction completes, with true if it was successful.
+  void RunOnConstructionComplete(const std::string& uuid,
+                                 const base::Callback<void(bool)>& done);
+
+  // Methods called by BlobStorageHost:
+  void RegisterBlobUUID(const std::string& uuid,

[michaeln, 2016/02/06 00:08:39]

harmonize... CreatePendingBlob()

[dmurph, 2016/02/09 00:52:25]

Done.

+                        const std::string& content_type,
+                        const std::string& content_disposition);
+  // Changes the state of the blob to broken. After calling this method the blob
+  // will be built and in the BROKEN state (it is not necessary to call
+  // BuildRegisteredBlob).
+  void FlagBlobAsBrokenAndBuild(const std::string& uuid,

[michaeln, 2016/02/06 00:08:40]

it'd be helpful to readers to rearrange the order to match the order in which
they're to be called.

CreatePendingBlob()
CompletePendingBlob()
CancelPendingBlob() | BreakPendingBlob()

also the file is large enough that matching order in .h and .cc files would help

[dmurph, 2016/02/09 00:52:25]

Sounds good.

+                                IPCBlobCreationCancelCode reason);
   void IncrementBlobRefCount(const std::string& uuid);
   void DecrementBlobRefCount(const std::string& uuid);
-  // #########################################
-
-  // Flags the entry for exceeding memory, and resets the builder.
-  void BlobEntryExceededMemory(BlobMapEntry* entry);
-
-  // Allocates memory to hold the given data element and copies the data over.
-  scoped_refptr<BlobDataItem> AllocateBlobItem(const std::string& uuid,
-                                               const DataElement& data_item);
 
   // Appends the given blob item to the blob builder.  The new blob
   // retains ownership of data_item if applicable, and returns false if there
@@ -124,6 +124,12 @@ class STORAGE_EXPORT BlobStorageContext
                                scoped_refptr<BlobDataItem> data_item,
                                InternalBlobData::Builder* target_blob_data);
 
+  // Allocates a shareable blob data item, with blob references resolved.  If
+  // there isn't enough memory, then a nullptr is returned.
+  scoped_refptr<ShareableBlobDataItem> AllocateShareableBlobDataItem(
+      const std::string& target_blob_uuid,
+      scoped_refptr<BlobDataItem> data_item);
+
   // Deconstructs the blob and appends it's contents to the target blob.  Items
   // are shared if possible, and copied if the given offset and length
   // have to split an item.
@@ -133,12 +139,7 @@ class STORAGE_EXPORT BlobStorageContext
                   uint64_t length,
                   InternalBlobData::Builder* target_blob_data);
 
-  bool IsInUse(const std::string& uuid);
-  bool IsBeingBuilt(const std::string& uuid);
-  bool IsUrlRegistered(const GURL& blob_url);
-
-  BlobMap blob_map_;
-  BlobURLMap public_blob_urls_;
+  BlobStorageRegistry registry_;
 
   // Used to keep track of how much memory is being utilized for blob data,
   // we count only the items of TYPE_DATA which are held in memory and not