[BlobStorage] BlobMemoryController & tests

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/strings/string_util.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";
+const char BlobDataBuilder::kFutureFileName[] = "_future_name_";
+
+/* static */
+bool BlobDataBuilder::IsFutureFileItem(const DataElement& element) {
+  const std::string prefix(kFutureFileName);
+  const std::string path = element.path().MaybeAsASCII();
+  return path.size() >= prefix.size() &&
+         base::StartsWith(path, prefix, base::CompareCase::SENSITIVE);
+}
+
+/* static */
+uint64_t BlobDataBuilder::GetFutureFileID(const DataElement& element) {
+  DCHECK(IsFutureFileItem(element));
+  uint64_t id = 0;
+  bool success =
+      base::StringToUint64(element.path().BaseName().MaybeAsASCII(), &id);

[pwnall, 2016/09/21 09:03:35]

Won't BaseName be something like "_future_name.42", which wouldn't parse?

Did you mean to create a directory and use FilePath::Append in
AppendFutureFile()?

[dmurph, 2016/09/21 23:45:52]

Yeah, this was a suggestion that didn't end up working. Fixed.

+  DCHECK(success) << element.path().Extension();
+  return id;
+}
 
 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:
@@ skipping 64 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,
+                                         uint64_t file_id) {
   CHECK_NE(length, 0ull);
   std::unique_ptr<DataElement> element(new DataElement());
-  element->SetToFilePathRange(base::FilePath::FromUTF8Unsafe(std::string(
-                                  kAppendFutureFileTemporaryFileName)),
+  element->SetToFilePathRange(base::FilePath::FromUTF8Unsafe(kFutureFileName)

[pwnall, 2016/09/21 09:03:35]

How about pulling out
base::FilePath::FromUTF8Unsafe(kFutureFileName).AddExtension(base::Uint64ToString(file_id)
into its own static method (e.g., GetFutureFilePath) and exporting that instead
of kFutureFileName[]?

This way, you could test GetFutureFilePath in conjunction with IsFutureFileItem
/ GetFutureFileID, and you could have the BlobAsyncTransportRequestBuilderTest
test use GetFutureFilePath for the expectation.

[dmurph, 2016/09/21 23:45:52]

Done.

+                                  .AddExtension(base::Uint64ToString(file_id)),
                               offset, length, base::Time());
   items_.push_back(new BlobDataItem(std::move(element)));
   return items_.size() - 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 (!IsFutureFileItem(*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);

[pwnall, 2016/09/21 09:03:35]

I think you can use [] instead of at(), because you DCHECKed the index already.
But I saw that at() was already used before (line 148). Do you happen to know
why at() is better than []? I thought it's worse because it can throw an
exception, so it triggers worse code generation.

[dmurph, 2016/09/21 23:45:52]

Done.

   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