[BlobAsync] Patch 1: BlobStorageRegistry

storage/browser/blob/blob_storage_registry.h

Index: storage/browser/blob/blob_storage_registry.h
diff --git a/storage/browser/blob/blob_storage_registry.h b/storage/browser/blob/blob_storage_registry.h
new file mode 100644
index 0000000000000000000000000000000000000000..7955cf92954814febb2263812a8d9e3f77828a89
--- /dev/null
+++ b/storage/browser/blob/blob_storage_registry.h
@@ -0,0 +1,111 @@
+// Copyright 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.
+
+#ifndef STORAGE_BROWSER_BLOB_BLOB_STORAGE_REGISTRY_H_
+#define STORAGE_BROWSER_BLOB_BLOB_STORAGE_REGISTRY_H_
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "storage/browser/blob/internal_blob_data.h"
+#include "storage/browser/storage_browser_export.h"
+
+class GURL;
+
+namespace storage {
+
+// This class stores the blob data in the various states of construction, as
+// well as URL mappings to blob uuids.
+// Implementation notes:
+// * There is no implicit refcounting in this class, except for setting the
+//   refcount to 1 on registration.
+// * When removing a uuid registration, we do not check for URL mappings to that
+//   uuid. The user must keep track of these.
+class STORAGE_EXPORT BlobStorageRegistry {
+ public:
+  enum class BlobState {
+    UNKNOWN = 0,

[michaeln, 2015/10/12 19:50:13]

Thnx for flattening the helpers out.

I think there's no longer a need for the UNKNOWN state. I looks like it was
there only to provide a possible return value to the reg class helpers when
asked about entries that didn't exist. An entry proper is never actually in the
UNKNOWN state.

[dmurph, 2015/10/12 20:08:28]

This is a protection against uninitialized entries, and is a common pattern. You
put the '0' value of the enumeration as an unknown error state, and if you ever
encounter that, then that enum wasn't initialized at all.

[michaeln, 2015/10/12 22:17:14]

Is it common to start the first value at 1 instead of 0 and to not define the
enum value that should never be used?

+    // First the renderer reserves the uuid.
+    RESERVED,
+    // Second, we are asynchronously transporting data to the browser.
+    ASYNC_TRANSPORTATION,
+    // Third, we construct the blob when we have all of the data.
+    CONSTRUCTION,
+    // Finally, the blob is built.
+    ACTIVE
+  };
+  enum Flags { EXCEEDED_MEMORY = 1 << 1 };

[kinuko, 2015/10/12 14:34:58]

Looks like this one's no longer (or at least not for now) needed

[dmurph, 2015/10/12 20:08:28]

Whoops, yeah, I didn't remove it.

+
+  struct Entry {
+    size_t refcount;
+    BlobState state;
+    std::vector<base::Callback<void(bool)>> construction_complete_callbacks;
+
+    // Flags
+    bool exceeded_memory;

[kinuko, 2015/10/12 14:34:58]

at this point this bool is not really used either? maybe just remove this for
now and re-add later when we start to use this?

[dmurph, 2015/10/12 20:08:28]

This bool is definitely used right now (instead of flags) to signify if the blob
has exceeded the available memory. This is the current behavior of the system.
(I haven't rebased the later patches yet to include the removal of flags yet).

+
+    // data and data_builder are mutually exclusive.
+    scoped_ptr<InternalBlobData> data;
+    scoped_ptr<InternalBlobData::Builder> data_builder;
+
+    Entry(int refcount, BlobState state);

[kinuko, 2015/10/12 14:34:58]

maybe should we delete default constructor? (Entry() = delete)

[dmurph, 2015/10/12 20:08:28]

Done.

+    ~Entry();
+
+    // Performs a test-and-set on the state of the given blob. If the state
+    // isn't as expected, we return false. Otherwise we set the new state and
+    // return true.
+    bool TestAndSetState(BlobState expected, BlobState set);
+  };
+
+  BlobStorageRegistry();
+  ~BlobStorageRegistry();
+
+  // Creates the blob entry with a refcount of 1 and a state of RESERVED. If
+  // the blob is already in use, we return null.
+  Entry* CreateEntry(const std::string& uuid);
+
+  // Removes the blob entry with the given uuid. This does not unmap any
+  // URLs that are pointing to this uuid. Returns if the entry existed.
+  bool DeleteEntry(const std::string& uuid);
+
+  // Returns if the uuid is mapped to an entry.
+  bool HasEntry(const std::string& uuid) const;

[kinuko, 2015/10/12 14:34:58]

feels a bit duplicated with GetEntry?

[dmurph, 2015/10/12 20:08:28]

Removed.

+
+  // Gets the blob entry for the given uuid. Returns nullptr if the entry
+  // does not exist.
+  Entry* GetEntry(const std::string& uuid) const;

[kinuko, 2015/10/12 14:34:58]

returning a non-const pointer to internal map from const method is not really
recommended, could this return const ptr or the method be non-const?

[michaeln, 2015/10/12 19:50:13]

ooops, i suggested this in a earlier comment, i may have given you a bad tip. it
can't return a const ptr because the caller has to be able to alter the entry's
state and refcount.

[dmurph, 2015/10/12 20:08:28]

Done.

+
+  // Creates a url mapping from blob uuid to the given url. Returns false if
+  // the uuid isn't mapped to an entry or if there already is a map for the URL.
+  bool CreateUrlMapping(const GURL& url, const std::string& uuid);
+
+  // Removes the given URL mapping. Optionally populates a uuid string of the
+  // removed entry uuid. Returns false if the url isn't mapped.
+  bool DeleteURLMapping(const GURL& url, std::string* uuid);
+
+  // Returns if the url is mapped to a blob uuid.
+  bool IsURLMapped(const GURL& blob_url) const;
+
+  // Returns the entry from the given url, and optionally populates the uuid for
+  // that entry. Returns a nullptr if the mapping or entry doesn't exist.
+  Entry* GetEntryFromURL(const GURL& url, std::string* uuid) const;
+
+  size_t blob_count() const { return blob_map_.size(); }
+  size_t url_count() const { return url_to_uuid_.size(); }
+
+ private:
+  typedef std::map<std::string, Entry*> BlobMap;

[michaeln, 2015/10/12 19:50:13]

we might want to use ScopedPtrHashMap instead, wdyt?

[dmurph, 2015/10/12 20:08:28]

Sure.

+  typedef std::map<GURL, std::string> URLMap;

[kinuko, 2015/10/12 14:34:58]

nit: prefer using rather than typedef

[dmurph, 2015/10/12 20:08:28]

Done.

+
+  BlobMap blob_map_;
+  URLMap url_to_uuid_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlobStorageRegistry);
+};
+
+}  // namespace storage
+#endif  // STORAGE_BROWSER_BLOB_BLOB_STORAGE_REGISTRY_H_