[BlobAsync] Patch 2: Common Constants

storage/browser/blob/blob_data_builder.cc

Index: storage/browser/blob/blob_data_builder.cc
diff --git a/storage/browser/blob/blob_data_builder.cc b/storage/browser/blob/blob_data_builder.cc
index 94fb9e50388633ba5bfcff901338603efc5039b6..555c9859b534b2a5f9cc27e4c75cabd53ba321bc 100644
--- a/storage/browser/blob/blob_data_builder.cc
+++ b/storage/browser/blob/blob_data_builder.cc
@@ -15,6 +15,44 @@ BlobDataBuilder::BlobDataBuilder(const std::string& uuid) : uuid_(uuid) {
 BlobDataBuilder::~BlobDataBuilder() {
 }
 
+void BlobDataBuilder::AppendDataElement(const DataElement& ipc_data) {
+  scoped_refptr<BlobDataItem> blob_item;
+
+  uint64 length = ipc_data.length();
+  scoped_ptr<DataElement> element(new DataElement());
+  switch (ipc_data.type()) {
+    case DataElement::TYPE_BYTES:

[michaeln, 2015/10/20 01:16:14]

There's code duplication. Do you intend to remove AppendData, AppendFile,
AppendFileSystemFile, and AppendBlob methods in favor of this one? It'd be nice
to not have duplication in the end.

[dmurph, 2015/10/20 18:28:27]

I made this method call the Append* methods.  It's useful to have this for the
places where we're adding an IPC-given DataElement, as otherwise I'm duplicating
switch statement and couple DCHECKs. I renamed it to be more clear. IS that
better?

+      DCHECK(!ipc_data.offset());
+      element->SetToBytes(ipc_data.bytes(), length);
+      blob_item = new BlobDataItem(element.Pass());
+      break;
+    case DataElement::TYPE_FILE:
+      element->SetToFilePathRange(ipc_data.path(), ipc_data.offset(), length,
+                                  ipc_data.expected_modification_time());
+      blob_item = new BlobDataItem(
+          element.Pass(), ShareableFileReference::Get(ipc_data.path()));
+      break;
+    case DataElement::TYPE_FILE_FILESYSTEM:
+      element->SetToFileSystemUrlRange(ipc_data.filesystem_url(),
+                                       ipc_data.offset(), length,
+                                       ipc_data.expected_modification_time());
+      blob_item = new BlobDataItem(element.Pass());
+      break;
+    case DataElement::TYPE_BLOB:
+      // This is a temporary item that will be deconstructed later.
+      element->SetToBlobRange(ipc_data.blob_uuid(), ipc_data.offset(),
+                              ipc_data.length());
+      blob_item = new BlobDataItem(element.Pass());
+      break;
+    case DataElement::TYPE_BYTES_DESCRIPTION:
+    case DataElement::TYPE_UNKNOWN:
+    case DataElement::TYPE_DISK_CACHE_ENTRY:  // This type can't be sent by IPC.
+      NOTREACHED();
+      break;
+  }
+  items_.push_back(blob_item.Pass());
+}
+
 void BlobDataBuilder::AppendData(const char* data, size_t length) {
   if (!length)
     return;
@@ -23,6 +61,40 @@ void BlobDataBuilder::AppendData(const char* data, size_t length) {
   items_.push_back(new BlobDataItem(element.Pass()));
 }
 
+void BlobDataBuilder::AppendFutureData(size_t length) {
+  if (!length)
+    return;
+  scoped_ptr<DataElement> element(new DataElement());
+  element->SetToBytesDescription(length);
+  items_.push_back(new BlobDataItem(element.Pass()));
+}
+
+bool BlobDataBuilder::PopulateFutureData(size_t index,
+                                         const char* data,
+                                         size_t offset,
+                                         size_t length) {
+  if (!length)
+    return true;

[kinuko, 2015/10/20 10:06:22]

I'm not fully sure why this returns true?

[dmurph, 2015/10/20 18:28:27]

I probably shouldn't exit early, you're right.

+  if (!data)
+    return false;
+  auto& item = items_.at(index);

[michaeln, 2015/10/20 01:16:14]

maybe be explicit about what item is
scoped_refptr<BlobDataItem>& item = items_.at(index);

[dmurph, 2015/10/20 18:28:27]

Removed, as we don't need it.

+  DataElement* element = item->data_element_ptr();
+  // On the first call, we are description, and allocate then.

[michaeln, 2015/10/20 01:16:14]

I don't understand the comment. Is the first call the AppendFutureData call? Ah,
a comment that first explains a future data element can be populated
incrementally would have helped.

[dmurph, 2015/10/20 18:28:27]

I added more documentation.

+  if (element->type() == DataElement::TYPE_BYTES_DESCRIPTION) {
+    element->SetToAllocatedBytes(element->length());

[kinuko, 2015/10/20 10:06:22]

Um... should we return here?  Otherwise if the element type is
TYPE_BYTES_DESCRIPTION we call SetToAllocateBytes() then errors out online 88??

[dmurph, 2015/10/20 18:28:27]

It changes to TYPE_BYTES (this is lazy allocation). I added a comment to
clarify.

+  }
+  if (element->type() != DataElement::TYPE_BYTES) {
+    LOG(ERROR) << "Invalid item type.";

[michaeln, 2015/10/20 01:16:14]

Switch these to DVLOG(1)?
http://www.chromium.org/developers/coding-style

[dmurph, 2015/10/20 18:28:27]

Done.

+    return false;
+  }
+  if (offset + length > element->length()) {
+    LOG(ERROR) << "Invalid offset or length.";
+    return false;
+  }
+  std::memcpy(element->mutable_bytes() + offset, data, length);
+  return true;
+}
+
 void BlobDataBuilder::AppendFile(const base::FilePath& file_path,
                                  uint64_t offset,
                                  uint64_t length,
@@ -73,4 +145,23 @@ void BlobDataBuilder::AppendDiskCacheEntry(
                        disk_cache_stream_index));
 }
 
+void BlobDataBuilder::Clear() {
+  items_.clear();
+  content_disposition_.clear();
+  content_type_.clear();
+  uuid_.clear();
+}
+
+void PrintTo(const BlobDataBuilder& x, std::ostream* os) {
+  DCHECK(os);
+  *os << "<BlobDataBuilder>{uuid: " << x.uuid()
+      << ", content_type: " << x.content_type_
+      << ", content_disposition: " << x.content_disposition_ << ", items: [";
+  for (const auto& item : x.items_) {
+    PrintTo(*item, os);
+    *os << ", ";
+  }
+  *os << "]}";
+}
+
 }  // namespace storage