Switch to using the async DomStorage IPC messages and add a caching layer …

content/renderer/dom_storage/dom_storage_dispatcher.cc

Index: content/renderer/dom_storage/dom_storage_dispatcher.cc
===================================================================
--- content/renderer/dom_storage/dom_storage_dispatcher.cc	(revision 138129)
+++ content/renderer/dom_storage/dom_storage_dispatcher.cc	(working copy)
@@ -4,16 +4,241 @@
 
 #include "content/renderer/dom_storage/dom_storage_dispatcher.h"
 
+#include <list>
+#include <map>
+
+#include "base/string_number_conversions.h"
+#include "base/synchronization/lock.h"
 #include "content/common/dom_storage_messages.h"
 #include "content/renderer/dom_storage/webstoragearea_impl.h"
 #include "content/renderer/dom_storage/webstoragenamespace_impl.h"
 #include "content/renderer/render_thread_impl.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebStorageEventDispatcher.h"
+#include "webkit/dom_storage/dom_storage_cached_area.h"
+#include "webkit/dom_storage/dom_storage_proxy.h"
+#include "webkit/dom_storage/dom_storage_types.h"
 
+using dom_storage::DomStorageCachedArea;
+using dom_storage::DomStorageProxy;
+using dom_storage::ValuesMap;
+
+namespace {
+class MessageThrottlingFilter : public IPC::ChannelProxy::MessageFilter {
+ public:
+  explicit MessageThrottlingFilter(RenderThreadImpl* sender)
+      : pending_count_(0), sender_(sender) {}
+
+  void SendThrottled(IPC::Message* message);
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+
+ private:
+  virtual ~MessageThrottlingFilter() {}
+
+  int IncrementPendingCount(int increment) {
+    base::AutoLock locker(lock_);
+    pending_count_ += increment;
+    return pending_count_;
+  }
+
+  base::Lock lock_;
+  int pending_count_;
+  RenderThreadImpl* sender_;
+};
+
+void MessageThrottlingFilter::SendThrottled(IPC::Message* message) {
+  bool need_to_flush = IncrementPendingCount(1) > 1000;
+  sender_->Send(message);
+  if (need_to_flush) {
+    sender_->Send(new DOMStorageHostMsg_FlushMessages);
+    DCHECK_EQ(0, IncrementPendingCount(0));
+  }
+}
+
+bool MessageThrottlingFilter::OnMessageReceived(const IPC::Message& message) {
+  if (message.type() == DOMStorageMsg_AsyncOperationComplete::ID)
+    IncrementPendingCount(-1);
+  return false;
+}
+}  // namespace
+
+// An implementation of the DomStorageProxy interface in terms of IPC.
+// This class also manages the collection of cached areas and pending
+// operations awaiting completion callbacks.
+class DomStorageDispatcher::ProxyImpl : public DomStorageProxy {
+ public:
+  explicit ProxyImpl(RenderThreadImpl* sender);
+
+  // Methods for use by DomStorageDispatcher directly.
+  scoped_refptr<DomStorageCachedArea> OpenCachedArea(
+      int64 namespace_id, const GURL& origin);
+  void CloseCachedArea(DomStorageCachedArea* area);
+  DomStorageCachedArea* LookupCachedArea(

[ericu, 2012/05/22 23:23:35]

Why does this return a raw pointer, whereas OpenCachedArea returns a
scoped_refptr?

[michaeln, 2012/05/23 00:38:51]

The open method may create a new instance whereas the lookup pointer will never
do that. The different return types is a (probably poor) attempt to make a
distinction between those two cases.

Given that this class actually holds a reference to the newly created instance,
and that this method is only accessible by DomStorageDispatcher which just
passes it up as a return value... i'll switch to raw ptrs for the both of them.

[michaeln, 2012/05/23 22:37:28]

Done.

[ericu, 2012/05/30 00:44:11]

I'd probably have gone the other way, and switched both to refptrs.  At least
there should be documentation of the lifetime of a DOMStorageCachedArea.  In
your current usage, the WebStorageAreaImpl's keeping a refptr, so it's safe, but
it's also not referencing the object after calling CloseCachedArea, so it'd be
safe with a raw pointer as well.  It would make sense to require [or at least
return] refptrs, and not let on that there's an extra reference held internally.
 It would also make sense instead to require that callers not use the pointer
after calling CloseCachedArea.  But the current state's a little weird.

What do you think?

+      int64 namespace_id, const GURL& origin);
+  void Shutdown();
+  void CompleteAsyncOperation(bool success);
+
+  // DomStorageProxy interface for use by DomStorageCachedArea.
+  virtual void LoadArea(int connection_id, ValuesMap* values,
+                        const AsyncOperationCallback& callback) OVERRIDE;
+  virtual void SetItem(int connection_id, const string16& key,
+                       const string16& value, const GURL& page_url,
+                       const AsyncOperationCallback& callback) OVERRIDE;
+  virtual void RemoveItem(int connection_id, const string16& key,
+                          const GURL& page_url,
+                          const AsyncOperationCallback& callback) OVERRIDE;
+  virtual void ClearArea(int connection_id,
+                        const GURL& page_url,
+                        const AsyncOperationCallback& callback) OVERRIDE;
+
+ private:
+  // Struct to hold references to our contained areas and
+  // to keep track of how many tabs have a given area open.
+  struct CachedAreaHolder {
+    scoped_refptr<DomStorageCachedArea> area_;
+    int open_count_;
+    CachedAreaHolder() : open_count_(0) {}
+    CachedAreaHolder(DomStorageCachedArea* area, int count)
+        : area_(area), open_count_(count) {}
+  };
+  typedef std::map<std::string, CachedAreaHolder> CachedAreaMap;
+  typedef std::list<AsyncOperationCallback> OperationList;
+
+  virtual ~ProxyImpl() {
+  }
+
+  std::string GetCachedAreaKey(int64 namespace_id, const GURL& origin) {
+    return base::Int64ToString(namespace_id) + origin.spec();
+  }
+
+  CachedAreaHolder* GetAreaHolder(const std::string& key) {
+    CachedAreaMap::iterator found = cached_areas_.find(key);
+    if (found == cached_areas_.end())
+      return NULL;
+    return &(found->second);
+  }
+
+  RenderThreadImpl* sender_;
+  CachedAreaMap cached_areas_;
+  OperationList pending_operations_;
+  scoped_refptr<MessageThrottlingFilter> throttling_filter_;
+};
+
+DomStorageDispatcher::ProxyImpl::ProxyImpl(RenderThreadImpl* sender)
+    : sender_(sender),
+      throttling_filter_(new MessageThrottlingFilter(sender)) {
+  sender_->AddFilter(throttling_filter_);
+}
+
+scoped_refptr<DomStorageCachedArea>
+DomStorageDispatcher::ProxyImpl::OpenCachedArea(
+    int64 namespace_id, const GURL& origin) {
+  std::string key = GetCachedAreaKey(namespace_id, origin);
+  if (CachedAreaHolder* holder = GetAreaHolder(key)) {
+    ++(holder->open_count_);
+    return holder->area_;
+  }
+  scoped_refptr<DomStorageCachedArea> area =
+      new DomStorageCachedArea(namespace_id, origin, this);
+  cached_areas_[key] = CachedAreaHolder(area, 1);
+  return area;
+}
+
+void DomStorageDispatcher::ProxyImpl::CloseCachedArea(
+    DomStorageCachedArea* area) {
+  std::string key = GetCachedAreaKey(area->namespace_id(), area->origin());
+  CachedAreaHolder* holder = GetAreaHolder(key);
+  DCHECK(holder);
+  DCHECK_EQ(holder->area_.get(), area);

[ericu, 2012/05/22 23:23:35]

DCHECK_GT(holder->open_count_, 0) ?

[michaeln, 2012/05/23 22:37:28]

Done.

+  if (--(holder->open_count_) == 0) {
+    cached_areas_.erase(key);
+  }
+}
+
+DomStorageCachedArea* DomStorageDispatcher::ProxyImpl::LookupCachedArea(
+    int64 namespace_id, const GURL& origin) {
+  std::string key = GetCachedAreaKey(namespace_id, origin);
+  CachedAreaHolder* holder = GetAreaHolder(key);
+  if (!holder)
+    return NULL;
+  return holder->area_.get();
+}
+
+void DomStorageDispatcher::ProxyImpl::Shutdown() {
+  sender_->RemoveFilter(throttling_filter_);
+  sender_ = NULL;
+  cached_areas_.clear();
+  pending_operations_.clear();
+}
+
+void DomStorageDispatcher::ProxyImpl::CompleteAsyncOperation(bool success) {
+  pending_operations_.front().Run(success);
+  pending_operations_.pop_front();
+}
+
+void DomStorageDispatcher::ProxyImpl::LoadArea(
+    int connection_id, ValuesMap* values,
+    const AsyncOperationCallback& callback) {
+  pending_operations_.push_back(callback);
+  sender_->Send(new DOMStorageHostMsg_LoadStorageArea(
+      connection_id, values));

[ericu, 2012/05/22 23:23:35]

Nit: It would be kind of nice to use the throttling_filter_ for every message,
and have it do the right thing, if that's not too messy.  It would keep you from
accidentally sending through the wrong sender when you change the code some day.

[michaeln, 2012/05/23 00:38:51]

sgtm, should be simple enough to determine 'needsThrottling' based on
msg->type() or msg->isSync() or something

[michaeln, 2012/05/23 22:37:28]

Done... kindof...
* added DCHECKs in SendThrottled to assert that only messages that will be ack'd
are sent thru it
* also added DCHECKs to assert that pending_count never dips below zero (this
should catch the mistake of neglecting to send a message that will be ack'd thru
the throttler

+}
+
+void DomStorageDispatcher::ProxyImpl::SetItem(
+    int connection_id, const string16& key,
+    const string16& value, const GURL& page_url,
+    const AsyncOperationCallback& callback) {
+  pending_operations_.push_back(callback);
+  throttling_filter_->SendThrottled(new DOMStorageHostMsg_SetItem(
+      connection_id, key, value, page_url));
+}
+
+void DomStorageDispatcher::ProxyImpl::RemoveItem(
+    int connection_id, const string16& key,  const GURL& page_url,
+    const AsyncOperationCallback& callback) {
+  pending_operations_.push_back(callback);
+  throttling_filter_->SendThrottled(new DOMStorageHostMsg_RemoveItem(
+      connection_id, key, page_url));
+}
+
+void DomStorageDispatcher::ProxyImpl::ClearArea(int connection_id,
+                      const GURL& page_url,
+                      const AsyncOperationCallback& callback) {
+  pending_operations_.push_back(callback);
+  throttling_filter_->SendThrottled(new DOMStorageHostMsg_Clear(
+      connection_id, page_url));
+}
+
+// DomStorageDispatcher ------------------------------------------------
+
+DomStorageDispatcher::DomStorageDispatcher()
+    : proxy_(new ProxyImpl(RenderThreadImpl::current())) {
+}
+
+DomStorageDispatcher::~DomStorageDispatcher() {
+  proxy_->Shutdown();
+}
+
+scoped_refptr<DomStorageCachedArea> DomStorageDispatcher::OpenCachedArea(
+    int connection_id, int64 namespace_id, const GURL& origin) {
+  RenderThreadImpl::current()->Send(
+      new DOMStorageHostMsg_OpenStorageArea(
+          connection_id, namespace_id, origin));
+  return proxy_->OpenCachedArea(namespace_id, origin);
+}
+
+void DomStorageDispatcher::CloseCachedArea(
+    int connection_id, DomStorageCachedArea* area) {
+  RenderThreadImpl::current()->Send(
+      new DOMStorageHostMsg_CloseStorageArea(connection_id));
+  proxy_->CloseCachedArea(area);
+}
+
 bool DomStorageDispatcher::OnMessageReceived(const IPC::Message& msg) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(DomStorageDispatcher, msg)
     IPC_MESSAGE_HANDLER(DOMStorageMsg_Event, OnStorageEvent)
+    IPC_MESSAGE_HANDLER(DOMStorageMsg_AsyncOperationComplete,
+                        OnAsyncOperationComplete)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -28,6 +253,13 @@
   if (originated_in_process) {
     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.old_value,
+                                 params.new_value);
+    }
   }
 
   if (params.namespace_id == dom_storage::kLocalStorageNamespaceId) {
@@ -39,10 +271,7 @@
         params.page_url,
         originating_area,
         originated_in_process);
-  } else if (originated_in_process) {
-    // TODO(michaeln): For now, we only raise session storage events into the
-    // process which caused the event to occur. However there are cases where
-    // sessions can span process boundaries, so there are correctness issues.
+  } else {
     WebStorageNamespaceImpl
         session_namespace_for_event_dispatch(params.namespace_id);
     WebKit::WebStorageEventDispatcher::dispatchSessionStorageEvent(
@@ -56,3 +285,7 @@
         originated_in_process);
   }
 }
+
+void DomStorageDispatcher::OnAsyncOperationComplete(bool success) {
+  proxy_->CompleteAsyncOperation(success);
+}