[BlobAsync] Disk support for blob storage

storage/browser/blob/blob_data_builder.cc

 // Copyright (c) 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.
 
 #include "storage/browser/blob/blob_data_builder.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 #include <utility>
 
 #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 {
 
 namespace {
 
 const static int kInvalidDiskCacheSideStreamIndex = -1;
 
 }  // namespace
 
 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() {
 }
 
 void BlobDataBuilder::AppendIPCDataElement(const DataElement& ipc_data) {
   uint64_t length = ipc_data.length();
   switch (ipc_data.type()) {
     case DataElement::TYPE_BYTES:
       DCHECK(!ipc_data.offset());
@@ skipping 63 matching lines @@
   base::CheckedNumeric<size_t> checked_end = offset;
   checked_end += length;
   if (!checked_end.IsValid() || checked_end.ValueOrDie() > element->length()) {
     DVLOG(1) << "Invalid offset or length.";
     return false;
   }
   std::memcpy(element->mutable_bytes() + offset, data, length);
   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.

 }
 
 bool BlobDataBuilder::PopulateFutureFile(
     size_t index,
     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;
 }
 
 void BlobDataBuilder::AppendFile(const base::FilePath& file_path,
                                  uint64_t offset,
                                  uint64_t length,
                                  const base::Time& expected_modification_time) {
   std::unique_ptr<DataElement> element(new DataElement());
   element->SetToFilePathRange(file_path, offset, length,
                               expected_modification_time);
@@ skipping 66 matching lines @@
       << ", content_type: " << x.content_type_
       << ", content_disposition: " << x.content_disposition_ << ", items: [";
   for (const auto& item : x.items_) {
     PrintTo(*item, os);
     *os << ", ";
   }
   *os << "]}";
 }
 
 }  // namespace storage