Implement the renderer side of the mojo based local storage implementation.

content/renderer/dom_storage/local_storage_cached_area.cc

Index: content/renderer/dom_storage/local_storage_cached_area.cc
diff --git a/content/renderer/dom_storage/local_storage_cached_area.cc b/content/renderer/dom_storage/local_storage_cached_area.cc
index 45336e2afcc8efc058f122edb3cf918840edfe72..94e03135e10eed3ab21999b4fc3e13ae4bb9ac7b 100644
--- a/content/renderer/dom_storage/local_storage_cached_area.cc
+++ b/content/renderer/dom_storage/local_storage_cached_area.cc
@@ -4,11 +4,37 @@
 
 #include "content/renderer/dom_storage/local_storage_cached_area.h"
 
+#include "base/bind.h"
+#include "base/strings/string_split.h"
+#include "content/common/dom_storage/dom_storage_map.h"
 #include "content/common/storage_partition_service.mojom.h"
+#include "content/renderer/dom_storage/local_storage_area.h"
 #include "content/renderer/dom_storage/local_storage_cached_areas.h"
+#include "mojo/common/common_type_converters.h"
+#include "third_party/WebKit/public/platform/WebURL.h"
+#include "third_party/WebKit/public/web/WebStorageEventDispatcher.h"
+#include "url/gurl.h"
 
 namespace content {
 
+// These methods are used to pack and unpack the page_url/storage_area_id into
+// source strings to/from the browser.
+std::string PackSource(const GURL& page_url,
+                       const std::string& storage_area_id) {
+  return page_url.spec() + "\n" + storage_area_id;

[dcheng, 2016/03/18 06:35:29]

Can GURL::spec() contain raw newlines?

[jam, 2016/03/18 16:48:33]

no (that's why i used it. it's one of the characters that aren't allowed in a
valid url, just like space, tab etc)

+}
+
+void UnpackSource(const mojo::String& source,
+                  GURL* page_url,
+                  std::string* storage_area_id) {
+  std::vector<std::string> result = base::SplitString(
+      source.To<std::string>(), "\n", base::KEEP_WHITESPACE,
+      base::SPLIT_WANT_ALL);
+  DCHECK_EQ(result.size(), 2u);
+  *page_url = GURL(result[0]);
+  *storage_area_id = result[1];
+}
+
 LocalStorageCachedArea::LocalStorageCachedArea(
     const url::Origin& origin,
     StoragePartitionService* storage_partition_service,
@@ -25,51 +51,173 @@ LocalStorageCachedArea::~LocalStorageCachedArea() {
 
 unsigned LocalStorageCachedArea::GetLength() {
   EnsureLoaded();
-  return 0u;
+  return map_->Length();
 }
 
 base::NullableString16 LocalStorageCachedArea::GetKey(unsigned index) {
   EnsureLoaded();
-  return base::NullableString16();
+  return map_->Key(index);
 }
 
 base::NullableString16 LocalStorageCachedArea::GetItem(
     const base::string16& key) {
   EnsureLoaded();
-  return base::NullableString16();
+  return map_->GetItem(key);
 }

[michaeln, 2016/03/18 01:05:16]

do we care about retaining this behavior?

  // Sudden termination is disabled when there are callbacks pending
  // to more reliably commit changes during shutdown.

unless theres reason not to, i'd like to.

[jam, 2016/03/18 16:48:33]

good question. i should have called it out that i didn't port that part
explicitly. 

the patchset that added it is
https://chromiumcodereview.appspot.com/10383123/#ps45003, and the comment was
"Darin brought up a good point about having to ensure that changes get flushed
during shutdown, so I'll be poking at the DomStorageDispatcher some more to see
to that."

It wasn't clear to me why we need to keep the renderer alive, since it's kept
alive just until it gets confirmation from the leveldb service. but the calls to
it have already been sent out, which means they're already being applied to the
database. i.e. i don't see how keeping the renderer would help ensure they're
flushed.

[michaeln, 2016/03/18 22:01:14]

per our hallway discussion... nevermind :)

mojo has a very nice characteristic i this regard, upon return from a method
call the data may not yet have been read by the remote side, but it will
eventaully be delivered even if the sender crashes on the next line

 
 bool LocalStorageCachedArea::SetItem(const base::string16& key,
                                      const base::string16& value,
-                                     const GURL& page_url) {
+                                     const GURL& page_url,
+                                     const std::string& storage_area_id) {
+  // A quick check to reject obviously overbudget items to avoid priming the
+  // cache.
+  if (key.length() + value.length() > kPerStorageAreaQuota)
+    return false;
+
   EnsureLoaded();
-  return false;
+  base::NullableString16 unused;
+  if (!map_->SetItem(key, value, &unused))
+    return false;
+
+  // Ignore mutations to 'key' until OnSetItemComplete.
+  ignore_key_mutations_[key]++;
+  leveldb_->Put(mojo::Array<uint8_t>::From(key),
+                mojo::Array<uint8_t>::From(value),
+                PackSource(page_url, storage_area_id),
+                base::Bind(&LocalStorageCachedArea::OnSetItemComplete,
+                           base::Unretained(this), key));
+  return true;
 }
 
 void LocalStorageCachedArea::RemoveItem(const base::string16& key,
-                                        const GURL& page_url) {
+                                        const GURL& page_url,
+                                        const std::string& storage_area_id) {
   EnsureLoaded();
+  base::string16 unused;
+  if (!map_->RemoveItem(key, &unused))
+    return;
+
+  // Ignore mutations to 'key' until OnRemoveItemComplete.
+  ignore_key_mutations_[key]++;
+  leveldb_->Delete(mojo::Array<uint8_t>::From(key),
+                   PackSource(page_url, storage_area_id),
+                   base::Bind(&LocalStorageCachedArea::OnRemoveItemComplete,
+                              base::Unretained(this), key));
 }
 
-void LocalStorageCachedArea::Clear(const GURL& page_url) {
+void LocalStorageCachedArea::Clear(const GURL& page_url,
+                                   const std::string& storage_area_id) {
   // No need to prime the cache in this case.
 
+  Reset();
+  map_ = new DOMStorageMap(kPerStorageAreaQuota);
   binding_.Close();
-  // TODO:
-  // binding_.CreateInterfacePtrAndBind()
+
+  leveldb_->DeleteAll(binding_.CreateInterfacePtrAndBind(),
+                      PackSource(page_url, storage_area_id),
+                      base::Bind(&LocalStorageCachedArea::OnClearComplete,
+                                  base::Unretained(this)));
+}
+
+void LocalStorageCachedArea::LocalStorageAreaCreated(LocalStorageArea* area) {
+  areas_[area->id()] = area;
+}
+
+void LocalStorageCachedArea::LocalStorageAreaDestroyed(LocalStorageArea* area) {
+  areas_.erase(area->id());

[michaeln, 2016/03/18 01:05:17]

Where is the LocalStorageCachedArea deleted?

I think adhoc manual refcounting is more error prone then using primitives
custom built for it (case in point:). A LocalStorageCachedArea is shared by
multiple areas, refcounting is a good fit for this class imho. Please consider
retaining that.

[jam, 2016/03/18 16:48:33]

ah I forgot to add that (hard to remember this stuff since I can't test this
code now until the other pieces exist). fixed.
in this case i removed the automatic refcounting because i needed explicit calls
from the LocalStorageArea to LocalStorageCachedArea to tell it about
construction/destruction. i felt it was a bit redundant to have both these calls
expliclity, and implicit references using scoped_refptr. but looking back at the
changes i dont feel strongly either way.
i don't feel strongly about this. seems like you prefer it, so i added it back

 }
 
 void LocalStorageCachedArea::KeyChanged(mojo::Array<uint8_t> key,
                                         mojo::Array<uint8_t> new_value,
                                         mojo::Array<uint8_t> old_value,
                                         const mojo::String& source) {
+  GURL page_url;
+  std::string storage_area_id;
+  UnpackSource(source, &page_url, &storage_area_id);
+
+  base::string16 key_string = key.To<base::string16>();
+  base::string16 new_value_string = new_value.To<base::string16>();
+
+  blink::WebStorageArea* originating_area = nullptr;
+  if (areas_.find(storage_area_id) != areas_.end()) {
+    // The source storage area is in this process.
+    originating_area = areas_[storage_area_id];
+  } else {
+    // This was from another process, so apply it to our cache if we haven't
+    // already changed it and are waiting for the confirmation callback.

[michaeln, 2016/03/18 01:05:17]

i'm not sure this holds true if the originating was closed after sent but prior
to completion?

Here's the old code...

  WebStorageAreaImpl* originating_area = NULL;
  if (params.connection_id) {
    originating_area = WebStorageAreaImpl::FromConnectionId(
        params.connection_id);
  } else {
    DOMStorageCachedArea* cached_area = proxy_->LookupCachedArea(
        params.namespace_id, params.origin);
    if (cached_area)
      cached_area->ApplyMutation(params.key, params.new_value);
  }


In that world, the browser sends 0 for a connection_id to all but the
originating process. The criteria for same process is not an area with a
matching id.

Oh... we could PackSource(pageurl, area_id, cache_area_id) so we could detect of
the change originated in |this| object.

[jam, 2016/03/18 16:48:33]

this is covered by the fact that observers are called before the callback (see
comments i added in leveldb_wrapper_impl.cc). so ignore_key_mutations_ will
still have the key and the mutation won't be applied.

[michaeln, 2016/03/18 22:01:14]

ah, yes, i see!

ok, please update the comment to not suggest the change must have come from a
different renderer

[jam, 2016/03/18 22:15:01]

Done.

+    if (ignore_key_mutations_.find(key_string) != ignore_key_mutations_.end()) {
+      // We turn off quota checking here to accomodate the over budget allowance
+      // that's provided in the browser process.
+      base::NullableString16 unused;
+      map_->set_quota(std::numeric_limits<int32_t>::max());
+      map_->SetItem(key_string, new_value_string, &unused);
+      map_->set_quota(kPerStorageAreaQuota);
+    }
+  }
+
+  blink::WebStorageEventDispatcher::dispatchLocalStorageEvent(
+      key_string, old_value.To<base::string16>(), new_value_string,
+      GURL(origin_.Serialize()), page_url, originating_area);
 }
 
 void LocalStorageCachedArea::KeyDeleted(mojo::Array<uint8_t> key,
+                                        mojo::Array<uint8_t> old_value,
                                         const mojo::String& source) {
+  GURL page_url;
+  std::string storage_area_id;
+  UnpackSource(source, &page_url, &storage_area_id);
+
+  base::string16 key_string = key.To<base::string16>();
+
+  blink::WebStorageArea* originating_area = nullptr;
+  if (areas_.find(storage_area_id) != areas_.end()) {
+    // The source storage area is in this process.
+    originating_area = areas_[storage_area_id];
+  } else {
+    // This was from another process, so remove it from our cache if we haven't
+    // already changed it and are waiting for the confirmation callback.
+    if (ignore_key_mutations_.find(key_string) != ignore_key_mutations_.end()) {
+      base::string16 unused;
+      map_->RemoveItem(key_string, &unused);
+    }
+  }
+
+  blink::WebStorageEventDispatcher::dispatchLocalStorageEvent(
+      key_string, old_value.To<base::string16>(), base::NullableString16(),
+      GURL(origin_.Serialize()), page_url, originating_area);
 }
 
 void LocalStorageCachedArea::AllDeleted(const mojo::String& source) {
+  GURL page_url;
+  std::string storage_area_id;
+  UnpackSource(source, &page_url, &storage_area_id);
+
+  blink::WebStorageArea* originating_area = nullptr;
+  if (areas_.find(storage_area_id) != areas_.end()) {
+    // The source storage area is in this process.
+    originating_area = areas_[storage_area_id];
+  } else {
+    scoped_refptr<DOMStorageMap> old = map_;
+    map_ = new DOMStorageMap(kPerStorageAreaQuota);
+
+    // We have to retain local additions which happened after this clear
+    // operation from another process.
+    std::map<base::string16, int>::iterator iter =
+        ignore_key_mutations_.begin();
+    while (iter != ignore_key_mutations_.end()) {
+      base::NullableString16 value = old->GetItem(iter->first);
+      if (!value.is_null()) {
+        base::NullableString16 unused;
+        map_->SetItem(iter->first, value.string(), &unused);
+      }
+      ++iter;
+    }
+  }
+
+  blink::WebStorageEventDispatcher::dispatchLocalStorageEvent(
+      base::NullableString16(), base::NullableString16(),
+      base::NullableString16(), GURL(origin_.Serialize()), page_url,
+      originating_area);
 }
 
 void LocalStorageCachedArea::EnsureLoaded() {
@@ -80,6 +228,47 @@ void LocalStorageCachedArea::EnsureLoaded() {
   leveldb::DatabaseError status = leveldb::DatabaseError::OK;
   mojo::Array<content::KeyValuePtr> data;
   leveldb_->GetAll(binding_.CreateInterfacePtrAndBind(), &status, &data);
+
+  DOMStorageValuesMap values;
+  for (size_t i = 0; i < data.size(); ++i) {
+    values[data[i]->key.To<base::string16>()] =
+        base::NullableString16(data[i]->value.To<base::string16>(), false);
+  }
+
+  map_ = new DOMStorageMap(kPerStorageAreaQuota);
+  map_->SwapValues(&values);

[michaeln, 2016/03/18 01:05:17]

wdyt about creating parallel TimeToPrime uma stats so we can compare apples to
apples?

[jam, 2016/03/18 16:48:33]

good idea, done

+}
+
+void LocalStorageCachedArea::OnSetItemComplete(const base::string16& key,
+                                               leveldb::DatabaseError result) {
+  if (result != leveldb::DatabaseError::OK) {
+    Reset();
+    return;
+  }
+  std::map<base::string16, int>::iterator found =

[michaeln, 2016/03/18 01:05:17]

maybe use auto here for readability?

[jam, 2016/03/18 16:48:33]

Done.

+      ignore_key_mutations_.find(key);
+  DCHECK(found != ignore_key_mutations_.end());
+  if (--found->second == 0)
+    ignore_key_mutations_.erase(found);
+}
+
+void LocalStorageCachedArea::OnRemoveItemComplete(

[michaeln, 2016/03/18 01:05:17]

looks like we could share code for OnSet and OnRemove complete?

[jam, 2016/03/18 16:48:33]

it's 3 lines ignoring the dcheck; i didn't think it was worth it to put that
into a function?

+    const base::string16& key, leveldb::DatabaseError result) {
+  DCHECK_EQ(result, leveldb::DatabaseError::OK);
+  std::map<base::string16, int>::iterator found =
+      ignore_key_mutations_.find(key);
+  DCHECK(found != ignore_key_mutations_.end());
+  if (--found->second == 0)
+    ignore_key_mutations_.erase(found);
+}
+
+void LocalStorageCachedArea::OnClearComplete(leveldb::DatabaseError result) {

[michaeln, 2016/03/18 01:05:17]

maybe we don't need this callback, i think recreating the binding solves the
problem this callback was for

[jam, 2016/03/18 16:48:33]

i kept it just so we can have a DCHECK during testing..

+  DCHECK_EQ(result, leveldb::DatabaseError::OK);
+}
+
+void LocalStorageCachedArea::Reset() {
+  map_ = NULL;
+  ignore_key_mutations_.clear();
 }
 
 }  // namespace content