[BlobAsync] Patch 1: BlobStorageRegistry

storage/browser/blob/blob_storage_registry.h

+// 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 "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 {

[michaeln, 2015/08/18 02:25:46]

Terminology nit that i think may be worth reconciling?

In existing storage/browser code, the term 'registered' applies to only to
public url mappings, the urls are registered and then later revoked. Blobs
themselves are 'built' incrementally or 'added' in complete form, they exist or
they don't. The mixing-and-matching of different terminology makes the code a
little more difficult to follow). I'd vote to be consistent about terminology in
the storage/browser code.

[dmurph, 2015/09/15 00:54:34]

Hm. I thought 'Registry' did a pretty good job of describing the function of the
class, which is to store mappings from key to data.  What about:
 * BlobStorageBackend
 * BlobStorageMap
 * BlobStorageStorage (kidding)

+ public:
+  enum class BlobState {
+    UNKNOWN = 0,
+    // First the renderer reserves the uuid.
+    RESERVED,
+    // Second, the we are asynchronously transporting data to the browser.

[kinuko, 2015/08/14 10:12:18]

nit: 'the we' -> 'we'

[dmurph, 2015/09/15 00:54:34]

Done.

+    ASYNC_TRANSPORTATION,
+    // Third, we construct the blob when we have all of the data

[kinuko, 2015/08/14 10:12:18]

nit: period at the end of comments

[dmurph, 2015/09/15 00:54:34]

Done.

+    CONSTRUCTION,
+    // Finally, the blob is built.
+    ACTIVE
+  };
+  enum BlobFlags { EXCEEDED_MEMORY = 1 << 1 };

[kinuko, 2015/08/14 10:12:19]

nit: I'd call this just Flags, or rename the following field/methods to use
'blob_flags' and '{Set,Unset,Is}BlobFlag' so that it becomes easier to associate

[dmurph, 2015/09/15 00:54:34]

Done.

+
+  struct BlobRegistryEntry {

[michaeln, 2015/08/18 02:25:46]

since we're in a class named BlobRegistry the prefix on this class name is
redundant, we could probably get away with 'Entry' here

[dmurph, 2015/09/15 00:54:34]

Sounds great, done.

+    size_t refcount;
+    BlobState state;
+    int flags;
+    std::vector<base::Callback<void(bool)>> construction_complete_callbacks;
+
+    // data and data_builder are mutually exclusive.
+    scoped_ptr<InternalBlobData> data;
+    scoped_ptr<InternalBlobData::Builder> data_builder;
+
+    BlobRegistryEntry(int refcount, BlobState state);
+    ~BlobRegistryEntry();
+
+    bool IsFlagSet(int flag);
+    void SetFlag(int flag);
+    void UnsetFlag(int flag);

[kinuko, 2015/08/14 10:12:18]

No one's calling these for now? If so I'd just drop these from this CL

[dmurph, 2015/09/15 00:54:34]

No one is calling anything in this patch haha.  These (and everything else from
these 4 patches) will be called from the hookup patch here:
https://codereview.chromium.org/1234813004

I'm updating the descriptions to include links to all patches.

+  };
+
+  BlobStorageRegistry();
+  virtual ~BlobStorageRegistry();
+
+  // Registers the blob entry with a refcount of 1 and a state of RESERVED. If
+  // the blob is already in use, we return null.
+  BlobRegistryEntry* RegisterBlobUUID(const std::string& uuid);

[michaeln, 2015/08/18 02:25:47]

Here's a stab not applying 'register' as a verb to the blob proper (or to the
url mappings either for that matter). wdyt?

Entry* CreateEntry(const string& uuid);
Entry* GetEntry(const string& uuid);
void DeleteEntry(const string& uuid);

bool CreateUrlMapping(...);
bool DeleteUrlMapping(...);
bool HasUrlMapping(...);
Entry* GetEntryForUrl(...);

[dmurph, 2015/09/15 00:54:34]

Sounds good.

+
+  // Removes the blob entry with the given uuid. This does not unregister any
+  // URLs that are pointing to this uuid. Returns if the entry existed.
+  bool RemoveBlobEntry(const std::string& uuid);
+
+  // Returns if the uuid is currently registered.
+  bool IsUUIDRegistered(const std::string& uuid) const;
+
+  // Gets the blob entry for the given uuid. Returns nullptr if the entry
+  // does not exist.
+  BlobRegistryEntry* GetBlobEntry(const std::string& uuid);
+
+  // Gets the state for the given uuid. Returns BlobState::UNKNOWN if the entry
+  // does not exist.

[kinuko, 2015/08/14 10:12:18]

Just returning UNKNOWN doesn't tell if it exists or not, would it be ok? If we
will be calling this only when we're sure IsUUIDRegistered() is true we could
just DCHECK if given uuid doesn't exist, and it could make things simpler some
times.

[michaeln, 2015/08/18 02:25:47]

Is this helper really needed at all? If i understand where the design is going,
the BlobStorageContext is the only real consumer of this class and it should
only be trying to twiddle these bits on blobs that are in the act of being
built.

[dmurph, 2015/09/15 00:54:34]

So this actually helps me avoid an extra map lookup in calling code.  There are
many instances where I want to check to is 'does the entry not exist or is it in
an unknown state', and this help that.

Otherwise I'm doing this pattern:
if (!r.IsUUIDRegistered(...)) {
  return
}

State state = r.GetBlobState(...);
if (state == UNKNOWN)
  return
// do logic

Instead, I can just do
State state = r.GetBlobState(...);
if (state == unknown) {
  // error handling
  return;
}

[michaeln, 2015/10/09 02:16:14]

Personally, i'm not a fan of the convenience methods. Why have fooExists() at
all, just get it and use it?

Entry* e = r.GetEntry(uuid);
if (!e || e->state == unknown)
  return;  // error handling

+  BlobState GetBlobState(const std::string& uuid) const;
+
+  // Performs a test-and-set on the state of the given blob. If the blob doesn't
+  // exist, or the state isn't as expected, we return false. Otherwise we set
+  // the new state and return true.
+  bool TestAndSetState(const std::string& uuid,

[michaeln, 2015/08/18 02:25:47]

This helper could go on the Entry class itself.

[dmurph, 2015/09/15 00:54:34]

It's more useful like this currently, as it removes the code that checks to see
if it exists or not.

+                       BlobState expected,
+                       BlobState set);
+
+  // Registers a url mapping from blob uuid to the given url. Returns false if
+  // the uuid isn't registered or if there already is a map for the URL.
+  bool RegisterURLMapping(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 RemoveURLMapping(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.
+  BlobRegistryEntry* GetBlobEntryFromURL(const GURL& url, std::string* uuid);
+
+  size_t blob_count() const { return blob_map_.size(); }
+  size_t url_count() const { return url_to_uuid_.size(); }
+
+ private:
+  friend class ViewBlobInternalsJob;

[kinuko, 2015/08/14 10:12:19]

nit: this friend's not necessary yet?

[dmurph, 2015/09/15 00:54:34]

Not yet....

+  typedef std::map<std::string, BlobRegistryEntry*> BlobMap;
+  typedef std::map<GURL, std::string> URLMap;
+
+  BlobMap blob_map_;
+  URLMap url_to_uuid_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlobStorageRegistry);
+};
+
+}  // namespace storage
+#endif  // STORAGE_BROWSER_BLOB_BLOB_STORAGE_REGISTRY_H_