Create blobs from Disk Cache entries.

content/browser/loader/upload_data_stream_builder.cc

Index: content/browser/loader/upload_data_stream_builder.cc
diff --git a/content/browser/loader/upload_data_stream_builder.cc b/content/browser/loader/upload_data_stream_builder.cc
index c497c16c0a769ea50c20469ba1fc44e47708dc31..df672b4cb4d30aa78a68f56d87be4323867cb77b 100644
--- a/content/browser/loader/upload_data_stream_builder.cc
+++ b/content/browser/loader/upload_data_stream_builder.cc
@@ -4,16 +4,23 @@
 
 #include "content/browser/loader/upload_data_stream_builder.h"
 
+#include <utility>
+
 #include "base/logging.h"
 #include "content/browser/fileapi/upload_file_system_file_element_reader.h"
 #include "content/common/resource_request_body.h"
 #include "net/base/elements_upload_data_stream.h"
 #include "net/base/upload_bytes_element_reader.h"
+#include "net/base/upload_disk_cache_entry_element_reader.h"
 #include "net/base/upload_file_element_reader.h"
 #include "storage/browser/blob/blob_data_handle.h"
 #include "storage/browser/blob/blob_data_snapshot.h"
 #include "storage/browser/blob/blob_storage_context.h"
 
+namespace disk_cache {
+class Entry;
+}
+
 namespace content {
 namespace {
 
@@ -60,11 +67,35 @@ class FileElementReader : public net::UploadFileElementReader {
   DISALLOW_COPY_AND_ASSIGN(FileElementReader);
 };
 
+class DiskCacheElementReader : public net::UploadDiskCacheEntryElementReader {
+ public:
+  DiskCacheElementReader(ResourceRequestBody* resource_request_body,
+                         disk_cache::Entry* disk_cache_entry,
+                         int disk_cache_stream_index,
+                         const ResourceRequestBody::Element& element)
+      : net::UploadDiskCacheEntryElementReader(disk_cache_entry,
+                                               disk_cache_stream_index,
+                                               element.offset(),
+                                               element.length()),
+        resource_request_body_(resource_request_body) {
+    DCHECK_EQ(ResourceRequestBody::Element::TYPE_DISK_CACHE_ENTRY,
+              element.type());
+  }
+
+  ~DiskCacheElementReader() override {}
+
+ private:
+  scoped_refptr<ResourceRequestBody> resource_request_body_;
+
+  DISALLOW_COPY_AND_ASSIGN(DiskCacheElementReader);
+};
+
 void ResolveBlobReference(
     ResourceRequestBody* body,
     storage::BlobStorageContext* blob_context,
     const ResourceRequestBody::Element& element,
-    std::vector<const ResourceRequestBody::Element*>* resolved_elements) {
+    std::vector<std::pair<const ResourceRequestBody::Element*,
+    const storage::BlobDataItem*>>* resolved_elements) {

[michaeln, 2015/06/15 22:17:23]

needs some indentation

[gavinp, 2015/06/16 16:07:47]

Done.

   DCHECK(blob_context);
   scoped_ptr<storage::BlobDataHandle> handle =
       blob_context->GetBlobDataFromUUID(element.blob_uuid());
@@ -84,7 +115,8 @@ void ResolveBlobReference(
   // Append the elements in the referenced blob data.
   for (const auto& item : snapshot->items()) {
     DCHECK_NE(storage::DataElement::TYPE_BLOB, item->type());
-    resolved_elements->push_back(item->data_element_ptr());
+    resolved_elements->push_back(
+        std::make_pair(item->data_element_ptr(), item.get()));
   }
   const void* key = snapshot.get();
   body->SetUserData(key, snapshot.release());
@@ -97,19 +129,22 @@ scoped_ptr<net::UploadDataStream> UploadDataStreamBuilder::Build(
     storage::BlobStorageContext* blob_context,
     storage::FileSystemContext* file_system_context,
     base::TaskRunner* file_task_runner) {
+
   // Resolve all blob elements.
-  std::vector<const ResourceRequestBody::Element*> resolved_elements;
+  std::vector<std::pair<const ResourceRequestBody::Element*,
+                        const storage::BlobDataItem*>> resolved_elements;
   for (size_t i = 0; i < body->elements()->size(); ++i) {
     const ResourceRequestBody::Element& element = (*body->elements())[i];
     if (element.type() == ResourceRequestBody::Element::TYPE_BLOB)
       ResolveBlobReference(body, blob_context, element, &resolved_elements);
     else
-      resolved_elements.push_back(&element);
+      resolved_elements.push_back(std::make_pair(&element, nullptr));

[michaeln, 2015/06/16 21:52:53]

I see, you're saying "what if" element here is of TYPE_DISK_CACHE_ENTRY? I think
I'd vote to...

else (type() != TYPE_DISK_CACHE_ENTRY)
  resolved_elements/push_back(...)
else
  NOTREACHED();

[gavinp, 2015/06/16 22:28:01]

Done.

   }
 
   ScopedVector<net::UploadElementReader> element_readers;
-  for (size_t i = 0; i < resolved_elements.size(); ++i) {
-    const ResourceRequestBody::Element& element = *resolved_elements[i];
+  for (const auto& element_and_blob_item_pair : resolved_elements) {
+    const ResourceRequestBody::Element& element =
+        *element_and_blob_item_pair.first;
     switch (element.type()) {
       case ResourceRequestBody::Element::TYPE_BYTES:
         element_readers.push_back(new BytesElementReader(body, element));
@@ -135,6 +170,15 @@ scoped_ptr<net::UploadDataStream> UploadDataStreamBuilder::Build(
         // TODO(dmurph): Create blob reader and store the snapshot in there.
         NOTREACHED();
         break;
+      case ResourceRequestBody::Element::TYPE_DISK_CACHE_ENTRY: {
+        // TODO(gavinp): What can we do if there's no blob_context?

[michaeln, 2015/06/15 22:17:23]

Does that matter?  We're making upload readers for blob items?

[gavinp, 2015/06/16 16:07:48]

This comment is a bit wrong; the real problem is that this occurs when we call
Build() with an element that is TYPE_DISK_CACHE_ENTRY. We actually NEED to be
called with an element of TYPE_BLOB here, so that we call the resolve function,
which is the only way to get the BlobDataItem pointer, which is required to find
the disk_cache::Entry*...

I don't know enough about the situations in which this code is called; how often
will it be with a DataElement of TYPE_DISK_CACHE_ENTRY vs TYPE_BLOB?

I'm guessing it's always TYPE_BLOB. I think your comment above confirms this?
I'm clearing up my comment in my next upload, but I'm hoping we can just remove
this worry. Should we be testing for |item| being null? There's a DCHECK on this
stuff in ResolveBlobReference() so I figured it's OK to crash...

[michaeln, 2015/06/16 21:52:53]

The real problem is that DataElement (in storage/common/data_element.h) is being
used for two different purposes. (a bit of a hack).

Primarily its a data structure used for an IPC represention of data provided by
blink to be uploaded or assembled into a blob. This is why it's in the /common
directory and resource_messages.h defines IPC::ParamTraits<> for it. I don't
believe this representation is used to send data from the browser process to a
renderer.

Secondarily, its used in the internal representation of a blob's data as stored
by the browser process, within BlobDataItem in storage/browser/blob.

TYPE_DISK_CACHE_ENTRY only makes sense in the context of the second use case.
This kind of data element is more or less a wrapper around a disk_cache::Entry*
which has no business being in a renderer process. Maybe the best way to 'fix'
this would be to use a different struct for the blob systems internal
representation...

class StoredBlobDataElement : public storage::DataElement {
  // add members and methods to support TYPE_DISK_CACHE_ENTRY including a
  // place to put the disk_cache::Entry*.
};

Where BlobDataItem keeps a scoped_ptr to one of those.

???

[gavinp, 2015/06/16 22:28:01]

This last sentence is what has been keeping me up at night. Thanks for saying
the thing that calms me down. Thanks.
I think we're safe if it's never used to move data from a browser process to a
renderer, because we can only ever have an element of TYPE_DISK_CACHE_ENTRY in a
browser process, so we'll never hit our IPC guards.

Given that, let's avoid adding another layer of indirection here. Does that make
sense?

[michaeln, 2015/06/17 00:57:36]

sgtm, and if/when we ever decide to refactor, thats for another cl

+        const storage::BlobDataItem* item = element_and_blob_item_pair.second;
+        element_readers.push_back(
+            new DiskCacheElementReader(body, item->disk_cache_entry(),
+                                       item->disk_cache_stream_index(),
+                                       element));
+        break;
+        }
       case ResourceRequestBody::Element::TYPE_UNKNOWN:
         NOTREACHED();
         break;