[BlobAsync] Asynchronous Blob Construction Final Patch

storage/browser/blob/blob_async_builder_host.h

Index: storage/browser/blob/blob_async_builder_host.h
diff --git a/storage/browser/blob/blob_async_builder_host.h b/storage/browser/blob/blob_async_builder_host.h
index c84f2b5d34869c84e35198d5a00c8415a1220cc5..5ff4719e93714595f38fadd7cea204a3834c18c2 100644
--- a/storage/browser/blob/blob_async_builder_host.h
+++ b/storage/browser/blob/blob_async_builder_host.h
@@ -13,12 +13,15 @@
 #include <vector>
 
 #include "base/callback.h"
+#include "base/files/file.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/shared_memory_handle.h"
-#include "storage/browser/blob/blob_async_transport_strategy.h"
+#include "base/memory/weak_ptr.h"
+#include "storage/browser/blob/blob_async_transport_request_builder.h"
 #include "storage/browser/blob/blob_data_builder.h"
+#include "storage/browser/blob/blob_transport_result.h"
 #include "storage/browser/storage_browser_export.h"
 #include "storage/common/blob_storage/blob_item_bytes_request.h"
 #include "storage/common/blob_storage/blob_item_bytes_response.h"
@@ -30,49 +33,69 @@ class SharedMemory;
 }
 
 namespace storage {
-
-// This class holds all blobs that are currently being built asynchronously for
-// a child process. It sends memory request, cancel, and done messages through
-// the given callbacks.
-// This also includes handling 'shortcut' logic, where the host will use the
-// initial data in the description instead of requesting for data if we have
-// enough immediate space.
+class BlobStorageContext;
+
+// This class
+// * holds all blobs that are currently being built asynchronously for a child
+//   process,
+// * sends memory requests through the given callback in |StartBuildingBlob|,
+//   and uses the BlobTransportResult return value to signify other results,
+// * includes all logic for deciding which async transport strategy to use, and
+// * handles all blob construction communication with the BlobStorageContext.
+// The method |ClearAndBreakPendingBlobConstruction| must be called by the
+// consumer, it is not called on destruction.
 class STORAGE_EXPORT BlobAsyncBuilderHost {
  public:
+  // Files are non-const so we can take the file reference.
+  using RequestMemoryCallback =
+      base::Callback<void(const std::vector<storage::BlobItemBytesRequest>&,
+                          const std::vector<base::SharedMemoryHandle>&,
+                          std::vector<base::File>*)>;
   BlobAsyncBuilderHost();
-  virtual ~BlobAsyncBuilderHost();
+  ~BlobAsyncBuilderHost();
+
+  bool RegisterBlobUUID(const std::string& uuid,
+                        const std::string& content_type,
+                        const std::string& content_disposition,
+                        BlobStorageContext* context);
 
   // This method begins the construction of the blob given the descriptions.
-  // After this method is called, either of the following can happen.
-  // * The |done| callback is triggered immediately because we can shortcut the
-  //   construction.
-  // * The |request_memory| callback is called to request memory from the
-  //   renderer. This class waits for calls to OnMemoryResponses to continue.
-  //   The last argument in the callback corresponds to file handle sizes.
-  //   When all memory is recieved, the |done| callback is triggered.
-  // * The |cancel| callback is triggered if there is an error at any point. All
-  //   state for the cancelled blob is cleared before |cancel| is called.
-  // Returns if the arguments are valid and we have a good IPC message.
-  bool StartBuildingBlob(
+  // When we return:
+  // * DONE: The blob is finished transfering right away, and is now
+  //   successfully saved in the context.
+  // * PENDING_RESPONSES: The async builder host is waiting for responses from
+  //   the renderer. It has called |request_memory| for these responses.
+  // * CANCEL_*: We have to cancel the blob construction. This function clears
+  //   the blob's internal state and marks the blob as broken in the context
+  //   before returning.
+  // * BAD_IPC: The arguments were invalid/bad. This marks the blob as broken in
+  //   the context before returning.
+  BlobTransportResult StartBuildingBlob(
       const std::string& uuid,
-      const std::string& type,
-      const std::vector<DataElement>& descriptions,
+      const std::vector<DataElement>& elements,
       size_t memory_available,
-      const base::Callback<
-          void(const std::vector<storage::BlobItemBytesRequest>&,
-               const std::vector<base::SharedMemoryHandle>&,
-               const std::vector<uint64_t>&)>& request_memory,
-      const base::Callback<void(const BlobDataBuilder&)>& done,
-      const base::Callback<void(IPCBlobCreationCancelCode)>& cancel);
+      BlobStorageContext* context,
+      const RequestMemoryCallback& request_memory);
 
   // This is called when we have responses from the Renderer to our calls to
-  // the request_memory callback above.
-  // Returns if the arguments are valid and we have a good IPC message.
-  bool OnMemoryResponses(const std::string& uuid,
-                         const std::vector<BlobItemBytesResponse>& responses);
-
-  // This erases the blob building state.
-  void StopBuildingBlob(const std::string& uuid);
+  // the request_memory callback above. See above for return value meaning.
+  BlobTransportResult OnMemoryResponses(
+      const std::string& uuid,
+      const std::vector<BlobItemBytesResponse>& responses,
+      BlobStorageContext* context);
+
+  // This removes the BlobBuildingState from our map and flags the blob as
+  // broken in the context. This can be called both from our own logic to cancel
+  // the blob, or from the DispatcherHost (Renderer). If the blob isn't being
+  // built then we do nothing.
+  void MaybeCancelBuildingBlob(const std::string& uuid,

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

I think you remove the 'maybe' from the name, at each callsite, we know its
being built and know we want to cancel it.

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

Done.

+                               IPCBlobCreationCancelCode code,
+                               BlobStorageContext* context);
+
+  // This clears this object of pending construction. It also handles marking
+  // blobs that haven't been fully constructed as broken in the context if there
+  // are any references being held by anyone.
+  void ClearAndBreakPendingBlobConstruction(BlobStorageContext* context);

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

"ClearAndBreak" here has the same meaning as "Cancel" in the previous method
name. I'd vote to be consistent and use the simpler and more concise term in
both places.

void CancelAll();

Better yet, it would probably be nice to have context_ be a member. The public
method would no longer need be needed and the dtor could take care of it.

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

The main issue with that is that context_ would either have to be a weak ptr or
a raw ptr. Neither of these could be initialized on the construction of the
BlobDispatcherHost, as the initialization of BlobStorageContext in the
ChromeBlobStorageContext is actually a posted task, instead of creating it right
away. So this is null initially. So either we have some sort of piping to set
the context in the async builder after it's initialized (not sure the best way
to do this), which would add more complexity, or we just pass the pointer in the
methods it's needed. So I opted for this.

 
   size_t blob_building_count() const { return async_blob_map_.size(); }
 
@@ -87,11 +110,12 @@ class STORAGE_EXPORT BlobAsyncBuilderHost {
 
  private:
   struct BlobBuildingState {
-    BlobBuildingState();
+    BlobBuildingState(const std::string& uuid);
     ~BlobBuildingState();
 
-    std::string type;
-    BlobAsyncTransportStrategy transport_strategy;
+    BlobAsyncTransportRequestBuilder request_builder;
+    BlobDataBuilder builder;
+    std::vector<bool> request_received;
     size_t next_request;
     size_t num_fulfilled_requests;
     scoped_ptr<base::SharedMemory> shared_memory_block;
@@ -102,21 +126,14 @@ class STORAGE_EXPORT BlobAsyncBuilderHost {
     // Only relevant if num_shared_memory_requests is > 0
     size_t current_shared_memory_handle_index;
 
-    base::Callback<void(const std::vector<storage::BlobItemBytesRequest>&,
-                        const std::vector<base::SharedMemoryHandle>&,
-                        const std::vector<uint64_t>&)> request_memory_callback;
-    base::Callback<void(const BlobDataBuilder&)> done_callback;
-    base::Callback<void(IPCBlobCreationCancelCode)> cancel_callback;
+    BlobAsyncBuilderHost::RequestMemoryCallback request_memory_callback;
   };
 
   typedef std::map<std::string, scoped_ptr<BlobBuildingState>> AsyncBlobMap;
 
   // This is the 'main loop' of our memory requests to the renderer.
-  void ContinueBlobMemoryRequests(const std::string& uuid);
-
-  void CancelAndCleanup(const std::string& uuid,
-                        IPCBlobCreationCancelCode code);
-  void DoneAndCleanup(const std::string& uuid);
+  BlobTransportResult ContinueBlobMemoryRequests(const std::string& uuid,
+                                                 BlobStorageContext* context);
 
   AsyncBlobMap async_blob_map_;