[BlobAsync] Patch 3: Renderer Classes & Logic

content/child/blob_storage/blob_transport_temporary_holder.h

+// Copyright 2015 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.
+
+#ifndef CONTENT_CHILD_BLOB_STORAGE_BLOB_TRANSPORT_TEMPORARY_HOLDER_H_
+#define CONTENT_CHILD_BLOB_STORAGE_BLOB_TRANSPORT_TEMPORARY_HOLDER_H_
+
+#include <string>
+#include <vector>
+
+#include "base/containers/hash_tables.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/shared_memory.h"
+#include "content/common/content_export.h"
+#include "ipc/ipc_platform_file.h"
+#include "storage/common/data_element.h"
+
+template <typename T>
+struct DefaultSingletonTraits;
+
+namespace base {
+class SingleThreadTaskRunner;
+}
+
+namespace storage {
+struct BlobItemBytesRequest;
+struct BlobItemBytesResponse;
+}
+
+namespace content {
+
+class BlobConsolidation;
+
+// This class holds all of the blob data that is in the process of being
+// transported to the Browser process. It
+//  * Creates the blob item descriptions for the browser.
+//  * Includes shortcut data in the descriptions.
+//  * Generates responses to blob memory requests.
+class CONTENT_EXPORT BlobTransportTemporaryHolder {

[michaeln, 2015/08/18 02:50:35]

This class is mostly just a map. The GetDescriptions() and GetResponses()
methods seem like they'd fit very well on the Consolidation class itself, those
methods describe and then extract the data elements of the consolidation.

I'd vote for one less class and put the  map of consolidations directly in
the transport controller.

[dmurph, 2015/10/07 00:32:36]

As we discussed, I would prefer to keep all the logic here in one class,
especially because all of the disk logic will go here.  Perhaps we can have
better names though.

What about:
BlobTransportControl -> BlobChildTransportRouter or
BlobTransportControllerSingleton
BlobTransportTemporaryHolder -> BlobTransportController

[michaeln, 2015/10/21 02:22:24]

Maybe this for names?

BlobMessageFilter (just like you have it now)
- this does the ipc message cracking and sending
- there are not many messages in the protocol so it's a very small class

BlobTransportController (just as you have it now)
- this is one step removed from ipc but otherwise has a method per ipc, called
by the filter upon msg receipt
- also, importantly, it has a lifeteime that is distinct from the filter which
is owned by the ipc system
- there are not many messages in the protocol so it's a very small class

BlobTransportLayer || BlobTransport[er] (what your currently calling
BlobTransportTemporaryHolder)
- a map of blob consolidations + methods to extract descriptions and then data
fitting those descriptions from an entry in its map

BlobConsolidation (just as you have it now)
- a vector of consolidated items comprising a blob with a method that allow
memory items to be read/visited.
- composed on other threads and handed off to the blobware on the IPC thread for
transporting

I think understand the division of labor your working on with these classes, but
some of it feels like over kill. I think having a hard time coming up with class
names might be a sign of that. Seems like you could merge the 'transportlayer'
into either the 'controller' or the 'consolidation' and have one less moving
part. Since you're no longer using Singleton<> for the controller (which can be
a pita for testing) is there any reason to not merge the 'controller' with the
'transporter'?

+ public:
+  enum class ResponsesStatus {
+    UNKNOWN,
+    BLOB_NOT_FOUND,
+    INVALID_ITEM_INDEX,
+    INVALID_DATA_RANGE,
+    INVALID_ITEM,
+    INVALID_HANDLE_INDEX,
+    SHARED_MEMORY_MAP_FAILED,
+    SUCCESS
+  };
+
+  BlobTransportTemporaryHolder();
+  virtual ~BlobTransportTemporaryHolder();
+
+  // Gives the blob data to this class to hold.
+  // Returns false if there already exists a blob with that uuid.
+  bool HoldBlobConsolidation(const std::string& uuid,
+                             scoped_ptr<BlobConsolidation> consolidation);
+
+  // Returns false if the blob doesn't exist.
+  bool GetDescriptions(const std::string& uuid,
+                       size_t max_data_population,
+                       std::vector<storage::DataElement>* out);
+
+  ResponsesStatus GetResponses(
+      const std::string& uuid,
+      const std::vector<storage::BlobItemBytesRequest>& requests,
+      std::vector<base::SharedMemoryHandle>* memory_handles,
+      const std::vector<IPC::PlatformFileForTransit>& file_handles,
+      std::vector<storage::BlobItemBytesResponse>* output);
+
+  void ReleaseBlob(const std::string& uuid);
+
+ private:
+  BlobConsolidation* GetConsolidation(const std::string& uuid);
+
+  base::hash_map<std::string, BlobConsolidation*> blob_storage_;

[michaeln, 2015/08/18 02:50:35]

mostly this map will be empty, maybe use a std::map<> to be more space efficient

[dmurph, 2015/10/07 00:32:36]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(BlobTransportTemporaryHolder);
+};
+
+}  // namespace content
+#endif  // CONTENT_CHILD_BLOB_STORAGE_BLOB_TRANSPORT_TEMPORARY_HOLDER_H_