[BlobAsync] Disk support for blob storage

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 1397cede297e47dbb88f2f52f3fa5f24172c39b9..bae5357fa222f31ba7127cf677dba1e63385f891 100644
--- a/storage/browser/blob/blob_data_builder.cc
+++ b/storage/browser/blob/blob_data_builder.cc
@@ -12,9 +12,9 @@
 
 #include "base/numerics/safe_conversions.h"
 #include "base/numerics/safe_math.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "net/disk_cache/disk_cache.h"
-#include "storage/browser/blob/shareable_file_reference.h"
 
 namespace storage {
 
@@ -27,6 +27,17 @@ const static int kInvalidDiskCacheSideStreamIndex = -1;
 const char BlobDataBuilder::kAppendFutureFileTemporaryFileName[] =
     "kFakeFilenameToBeChangedByPopulateFutureFile";

[michaeln, 2016/08/15 22:44:43]

would be safer to use a shorter filename to avoid path length problems
"_future_name_"

[dmurph, 2016/08/19 00:18:32]

Done.

 
+bool BlobDataBuilder::IsTemporaryFileItem(const DataElement& element) {

[michaeln, 2016/08/15 22:44:43]

i think this can be simplified, element.path().BaseName() gives what precedes
the '.' in the last component of the path

return element.path().BaseName() == kFutureFilename;

[dmurph, 2016/08/19 00:18:32]

Done.

+  const std::string prefix(kAppendFutureFileTemporaryFileName);
+  const std::string path = element.path().MaybeAsASCII();
+  return path.size() >= prefix.size() &&
+         element.path().MaybeAsASCII().substr(0, prefix.size()) == prefix;

[Marijn Kruisselbrink, 2016/08/05 23:23:54]

Why call element.path().MaybeAsASCII() if you already have saved that as path
above? And isn't this just base::StartsWith(path, prefix,
base::CompareCase::SENSITIVE) (which would avoid the substring copy, and the
std::string copy of prefix since it just takes a StringPiece).

[dmurph, 2016/08/19 00:18:32]

Done.

+}
+
+std::string BlobDataBuilder::GetTemporaryFileID(const DataElement& element) {

[michaeln, 2016/08/15 22:44:43]

maybe DCHECK(IsFutureFileItem(element))

[dmurph, 2016/08/19 00:18:32]

Done.

+  return element.path().Extension();
+}
+
 BlobDataBuilder::BlobDataBuilder(const std::string& uuid) : uuid_(uuid) {
 }
 BlobDataBuilder::~BlobDataBuilder() {
@@ -110,12 +121,16 @@ bool BlobDataBuilder::PopulateFutureData(size_t index,
   return true;
 }
 
-size_t BlobDataBuilder::AppendFutureFile(uint64_t offset, uint64_t length) {
+size_t BlobDataBuilder::AppendFutureFile(uint64_t offset,
+                                         uint64_t length,
+                                         const std::string& file_id) {
   CHECK_NE(length, 0ull);
   std::unique_ptr<DataElement> element(new DataElement());
-  element->SetToFilePathRange(base::FilePath::FromUTF8Unsafe(std::string(
-                                  kAppendFutureFileTemporaryFileName)),
-                              offset, length, base::Time());
+  element->SetToFilePathRange(
+      base::FilePath::FromUTF8Unsafe(
+          std::string(kAppendFutureFileTemporaryFileName))

[Marijn Kruisselbrink, 2016/08/05 23:23:54]

Why the conversion to a std::string? FromUTF8Unsafe takes a StringPiece, so
passing the const char* directly should work fine.

[dmurph, 2016/08/19 00:18:32]

Done.

+          .AddExtension(file_id),

[Marijn Kruisselbrink, 2016/08/05 23:23:54]

Does this work on windows where FilePath::StringType is std::wstring rather than
std::string?

+      offset, length, base::Time());
   items_.push_back(new BlobDataItem(std::move(element)));
   return items_.size() - 1;

[michaeln, 2016/08/15 22:44:43]

Why does file_id need to be passed in? the class comment says it has to be
unique within the builder and the builder can most easily maintain that
constraint.

FilePath name = FormFutureFilename(items_.size());
elem->SetXYZ(name, ...);

The 'id' can be the index of the element

[dmurph, 2016/08/19 00:18:32]

The id can be reused between elements. I'm putting elements 1, 2, 3 in file 0,
and 7, 9, 10 in file 1.

 }
@@ -125,22 +140,20 @@ bool BlobDataBuilder::PopulateFutureFile(
     const scoped_refptr<ShareableFileReference>& file_reference,
     const base::Time& expected_modification_time) {
   DCHECK_LT(index, items_.size());
-  DataElement* old_element = items_.at(index)->data_element_ptr();
+  DataElement* element = items_.at(index)->data_element_ptr();
 
-  if (old_element->type() != DataElement::TYPE_FILE) {
+  if (element->type() != DataElement::TYPE_FILE) {
     DVLOG(1) << "Invalid item type.";
     return false;
-  } else if (old_element->path().AsUTF8Unsafe() !=
-             std::string(kAppendFutureFileTemporaryFileName)) {
+  } else if (!IsTemporaryFileItem(*element)) {
     DVLOG(1) << "Item not created by AppendFutureFile";
     return false;
   }
-  uint64_t length = old_element->length();
-  uint64_t offset = old_element->offset();
-  std::unique_ptr<DataElement> element(new DataElement());
+  uint64_t length = element->length();
+  uint64_t offset = element->offset();
+  items_.at(index)->data_handle_ = std::move(file_reference);
   element->SetToFilePathRange(file_reference->path(), offset, length,
                               expected_modification_time);
-  items_[index] = new BlobDataItem(std::move(element), file_reference);
   return true;
 }