[BlobAsync] Patch 4: Browser Classes & Logic.

storage/browser/blob/blob_async_transport_strategy.h

Index: storage/browser/blob/blob_async_transport_strategy.h
diff --git a/storage/browser/blob/blob_async_transport_strategy.h b/storage/browser/blob/blob_async_transport_strategy.h
new file mode 100644
index 0000000000000000000000000000000000000000..7fadf9f77b86be51c8ff8398bd31a2f17c8f462b
--- /dev/null
+++ b/storage/browser/blob/blob_async_transport_strategy.h
@@ -0,0 +1,148 @@
+// 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 STORAGE_BROWSER_BLOB_BLOB_ASYNC_TRANSPORT_STRATEGY_H_
+#define STORAGE_BROWSER_BLOB_BLOB_ASYNC_TRANSPORT_STRATEGY_H_
+
+#include <map>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "storage/browser/blob/blob_data_builder.h"
+#include "storage/browser/storage_browser_export.h"
+#include "storage/common/blob_storage/blob_item_bytes_request.h"
+#include "storage/common/data_element.h"
+
+namespace storage {
+
+// This class determines and stores the strategy for asynchronously transporting
+// memory from the renderer to the browser. This does not involve using the
+// 'shortcut' method, where the data is already present, and will always give
+// the caller the full strategy for requesting all data from the renderer. The
+// returned builder will always have TYPE_BYTES_DESCRIPTION items for the future
+// memory locations.
+// This involves two transformations:
+// 1. Transforming the data storage to be efficient in-transport.  This can
+//    mean that data is consolidated from cross-blob or cross-file boundaries
+//    in the blob in a common block of shared memory or a file.
+// 2. Transforming the transport data into the browser storage representation.
+//    This can simply mirror the renderer representation, or it can be instead
+//    pointing to transport representation with offsets and sizes.
+//
+// The following information is generated:
+// 1. The total bytes size of memory items.
+// 2. The requests for memory, segmented as described above, along with their
+//    destination browser indexes and offsets
+// 3. The sizes of the shared memory and file handles being used (by handle
+//    index) in the async operation.
+// 4. A BlobDataBuilder which can be used to construct the Blob in the
+//    BlobStorageContext object, after the TYPE_BYTES_DESCRIPTION entries are
+//    turned into TYPE_BYTES entries.
+// The initial implementation of this is simple, but the protocol allows any
+// sort of flexibility, as everything has an offset and size specified.

[michaeln, 2015/11/17 21:55:29]

The comment is verbose, but honestly i'm not sure its very helpful?

+class STORAGE_EXPORT BlobAsyncTransportStrategy {
+ public:
+  enum Mode { MODE_UNKNOWN = 0, MODE_NO_DISK, MODE_DISK };
+
+  enum Error {
+    ERROR_NONE = 0,
+    ERROR_TOO_LARGE,  // This item can't fit in disk or memory
+    ERROR_INVALID_PARAMS
+  };
+
+  struct MemoryItemRequest {

[michaeln, 2015/11/17 21:55:28]

Using 'Memory' in this struct name threw me off a little since it's used for
both all xfer types (file, sharedmem, inlineipc). The src in the renderer is
memory but the dest in the browser is not necessarily. Idk if its worth word
smithing, DataRequest?

[dmurph, 2015/11/19 02:06:18]

I mean, a request for an item that's memory in the renderer (as apposed to a
file item or otherwise). I added renderer to clarify.

+    MemoryItemRequest();
+    size_t browser_item_index;

[michaeln, 2015/11/17 21:55:29]

it's not real clear what this is an index into, builder_item_index?, at least
worth a comment

[dmurph, 2015/11/19 02:06:18]

Done.

+    // Note: For files this offset should always be 0, as the file offset in
+    //       segmentation is handled by the handle_offset in the message.  This
+    //       offset is used for populating a chunk when the data comes back to
+    //       the browser.
+    size_t browser_item_offset;
+    BlobItemBytesRequest message;
+    bool received;
+  };
+
+  template <typename SizeType>
+  class BlobSegmentVisitor {

[michaeln, 2015/11/17 21:55:28]

can this be private?

[dmurph, 2015/11/19 02:06:18]

Sure.

+   public:
+    virtual ~BlobSegmentVisitor(){};
+    virtual void VisitSegment(const DataElement& element,
+                              size_t element_index,
+                              SizeType element_offset,
+                              size_t segment_index,
+                              SizeType segment_offset,
+                              SizeType size) = 0;
+    virtual void Done() = 0;
+  };
+
+  BlobAsyncTransportStrategy();
+  virtual ~BlobAsyncTransportStrategy();
+
+  void Initialize(Mode mode,

[michaeln, 2015/11/17 21:55:28]

About the packaging of this logic, feels like more of a function than a class? A
function that computes the requests that should be made for a blob's elements
given size constraints. It also seeds a builder with future elements waiting to
be populated with the requested data.

(a briefer comment like that at the top of the class might help)

ResultType ComputeDataRequestsNeeded(elements, size_constraints,
&data_requests_to_make, &seeded_builder);

[dmurph, 2015/11/19 02:06:18]

Well, the main idea of this class is that it's the data class for storing our
requests and builder, like we discussed a while ago.  I added a comment to the
top like you described, and to this method.

+                  size_t max_ipc_memory_size,
+                  size_t max_shared_memory_size,
+                  uint64_t max_file_size,
+                  size_t max_blob_in_memory_size,
+                  uint64_t disk_space_left,
+                  size_t memory_left,
+                  const std::string& uuid,
+                  const std::vector<DataElement>& blob_item_infos);
+
+  std::vector<uint64_t>& file_handles() { return file_handles_; }

[michaeln, 2015/11/17 21:55:28]

The comments and member names make it sound like the strategy class returns
actual handles.

[dmurph, 2015/11/19 02:06:18]

Done.

+
+  // The sizes will always be in a descending order (so if you create a block
+  // with the first size, it will always be greater than the later sizes)

[michaeln, 2015/11/17 21:55:28]

comment could be simpler, sizes are sorted from largest to smallest. are the
file_handle_sizes sorted too?

[dmurph, 2015/11/19 02:06:18]

Not intentionally. These are the sizes of all of the file handles and shared
memory handles that we use.  We can be creating/sending multiple handles at
once. They aren't constrained to be sorted by size, that's just how we create
them in this algorithm.

We refer to these handles by index in our requests.

I simplified the comment and added it to file handles as well.

+  std::vector<size_t>& shared_memory_handles() {
+    return shared_memory_handles_;
+  }
+
+  const std::vector<MemoryItemRequest>& requests() const { return requests_; }
+
+  BlobDataBuilder* blob_builder() { return builder_.get(); }
+
+  uint64_t total_bytes_size() const { return total_bytes_size_; }
+
+  Error error() const { return error_; }
+
+  static bool ShouldBeShortcut(const std::vector<DataElement>& items,
+                               size_t memory_available);
+
+  // This iterates of the data elements and segments the 'bytes' data into
+  // the smallest number of segments given the max_segment_size.
+  // The callback describes either:
+  // * A non-memory item
+  // * A partition of a bytes element which will be populated into a given
+  //   segment and segment offset.
+  // Assumptions: All memory items are consolidated.  As in, there are no two
+  //              'bytes' items next to eachother.
+  template <typename SizeType>
+  static void ForEachWithSegment(const std::vector<DataElement>& items,
+                                 SizeType max_segment_size,
+                                 BlobSegmentVisitor<SizeType>* visitor);
+
+ private:
+  // This simply fills the segment_sizes vector with the sizes of segments that
+  // would be used to partition a block of total_memory_size.
+  template <typename SizeType>
+  static void CreateHandleSizes(SizeType total_memory_size,

[michaeln, 2015/11/17 21:55:28]

naming suggestion: ComputeHandleSizes to avoid having CreateHandle in the name
since the class does not create handles

[dmurph, 2015/11/19 02:06:18]

Done.

+                                SizeType max_segment_size,
+                                std::vector<SizeType>* segment_sizes);
+
+  Error error_;
+
+  std::vector<uint64_t> file_handles_;

[michaeln, 2015/11/17 21:55:28]

naming suggestions: append sizes_ to these names

[dmurph, 2015/11/19 02:06:18]

Done.

+  // can be size_t, but same as file handles so we can use the same methods.
+  std::vector<size_t> shared_memory_handles_;
+
+  uint64_t total_bytes_size_;
+  std::vector<MemoryItemRequest> requests_;
+  scoped_ptr<BlobDataBuilder> builder_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlobAsyncTransportStrategy);
+};
+
+}  // namespace storage
+
+#endif  // STORAGE_BROWSER_BLOB_BLOB_ASYNC_TRANSPORT_STRATEGY_H_