Pull implementation of CertStore out to be reused.

content/browser/renderer_data_memoizing_store.h

+// Copyright (c) 2013 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 CONTENT_BROWSER_RENDERER_DATA_MEMOIZING_STORE_H_
+#define CONTENT_BROWSER_RENDERER_DATA_MEMOIZING_STORE_H_
+
+#include <map>
+
+#include "base/bind.h"
+#include "base/stl_util.h"
+#include "base/synchronization/lock.h"
+#include "content/browser/renderer_host/render_process_host_impl.h"
+#include "content/browser/renderer_host/render_view_host_impl.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
+
+namespace content {
+
+// RendererDataMemoizingStore is a thread-safe container that retains reference
+// counted objects that are associated with one or more render processes.
+// Objects
+// are identified by an int and only a single instance of a given object is

[wtc, 2013/11/14 22:04:58]

Nit: a single instance => a single reference?

[alcutter, 2013/11/15 13:33:07]

Done.

+// retained. RendererDataMemoizingStore watches for render process termination
+// and
+// releases objects that are no longer associated with any render process.

[wtc, 2013/11/14 22:04:58]

The comment block should be reformatted. For example, lines 25 and 28 have only
one word.

[alcutter, 2013/11/15 13:33:07]

Not sure what happened there, done.

+template <typename T>
+class RendererDataMemoizingStore : public NotificationObserver {

[wtc, 2013/11/14 22:04:58]

Is it better to define the class inline in the header file? In general it's
better to have a .cc file with the method definitions.

[agl, 2013/11/14 22:09:28]

I think, because it's a template, it must be in the header.

[alcutter, 2013/11/15 13:33:07]

As Adams says, it has to be in the header because it's a template.

+ private:

[wtc, 2013/11/14 22:04:58]

The three "private" sections should be merged into one, and be listed after the
"public" section. This is recommended by the Style Guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

[alcutter, 2013/11/15 13:33:07]

Done.

+  template <typename M>
+  struct MatchSecond {
+    explicit MatchSecond(const M& t) : value(t) {}
+
+    template <typename Pair>
+    bool operator()(const Pair& p) const {
+      return (value == p.second);
+    }
+    M value;

[wtc, 2013/11/14 22:04:58]

Nit: add a blank line before this line. (I know you copied this from the
original code.)

[alcutter, 2013/11/15 13:33:07]

Done.

+  };
+
+ public:
+  RendererDataMemoizingStore() : next_item_id_(1) {
+    if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+      RegisterForNotification();
+    } else {
+      BrowserThread::PostTask(
+          BrowserThread::UI,
+          FROM_HERE,
+          base::Bind(&RendererDataMemoizingStore::RegisterForNotification,
+                     base::Unretained(this)));
+    }
+  }
+
+  // Store adds |item| to this collection, associates it with the given render
+  // process id and returns an opaque identifier for it. If |item| is already
+  // known, the same identifier will be returned.
+  int Store(T* item, int process_id) {
+    DCHECK(item);
+    base::AutoLock auto_lock(lock_);
+
+    int item_id;
+
+    // Do we already know this item?
+    typename ReverseItemMap::iterator item_iter = item_to_id_.find(item);

[wtc, 2013/11/14 22:04:58]

Is it necessary to use "typename" here?

(There are other occurrences of "typename" in this file. I won't repeat this
question.)

[alcutter, 2013/11/15 13:33:07]

Yes, because ReverseItemMap (==std::map<T*, int, T::LessThan>) is dependent on
T.

Same answer for the other occurrences too.

+    if (item_iter == item_to_id_.end()) {
+      item_id = next_item_id_++;
+      // We use 0 as an invalid item_id value.  In the unlikely event that
+      // next_item_id_ wraps around, we reset it to 1.
+      if (next_item_id_ == 0)
+        next_item_id_ = 1;
+      item->AddRef();
+      id_to_item_[item_id] = item;
+      item_to_id_[item] = item_id;
+    } else {
+      item_id = item_iter->second;
+    }
+
+    // Let's update process_id_to_item_id_.
+    std::pair<IDMap::iterator, IDMap::iterator> process_ids =
+        process_id_to_item_id_.equal_range(process_id);
+    if (std::find_if(process_ids.first,
+                     process_ids.second,

[wtc, 2013/11/14 22:04:58]

Nit: I would list the first two arguments on the same line as the original code
does, unless you have strong preference for this style. (Similarly for lines
93-94 and 152-153.)

[alcutter, 2013/11/15 13:33:07]

Done.

+                     MatchSecond<int>(item_id)) == process_ids.second) {
+      process_id_to_item_id_.insert(std::make_pair(process_id, item_id));
+    }
+
+    // And item_id_to_process_id_.
+    std::pair<IDMap::iterator, IDMap::iterator> item_ids =
+        item_id_to_process_id_.equal_range(item_id);
+    if (std::find_if(item_ids.first,
+                     item_ids.second,
+                     MatchSecond<int>(process_id)) == item_ids.second) {
+      item_id_to_process_id_.insert(std::make_pair(item_id, process_id));
+    }
+
+    return item_id;
+  }
+
+  // Retrieve fetches a previously Stored() item, identified by |item_id|.
+  // If |item_id| is recognized, |item| will be updated and Retrieve() will
+  // return true, it will otherwise return false.
+  bool Retrieve(int item_id, scoped_refptr<T>* item) {
+    base::AutoLock auto_lock(lock_);
+
+    typename ItemMap::iterator iter = id_to_item_.find(item_id);
+    if (iter == id_to_item_.end())
+      return false;
+    if (item)
+      *item = iter->second;
+    return true;
+  }
+
+ private:
+  void RegisterForNotification() {
+    // We watch for RenderProcess termination, as this is how we clear
+    // items for now.
+    // TODO(jcampan): we should be listening to events such as resource cached/
+    //                removed from cache, and remove the item when we know it
+    //                is not used anymore.
+
+    registrar_.Add(this,
+                   NOTIFICATION_RENDERER_PROCESS_TERMINATED,
+                   NotificationService::AllBrowserContextsAndSources());
+    registrar_.Add(this,
+                   NOTIFICATION_RENDERER_PROCESS_CLOSED,
+                   NotificationService::AllBrowserContextsAndSources());
+  }
+
+  void RemoveInternal(int item_id) {

[wtc, 2013/11/14 22:04:58]

Please copy the original comment (after updating it to remove references to
"cert" and to use the new member names):

  // Remove the specified cert from id_to_cert_ and cert_to_id_.
  // NOTE: the caller (RemoveCertsForRenderProcesHost) must hold cert_lock_.

[alcutter, 2013/11/15 13:33:07]

Whoops, done.

+    typename ItemMap::iterator item_iter = id_to_item_.find(item_id);
+    DCHECK(item_iter != id_to_item_.end());
+
+    typename ReverseItemMap::iterator id_iter =
+        item_to_id_.find(item_iter->second.get());
+    DCHECK(id_iter != item_to_id_.end());
+    item_to_id_.erase(id_iter);
+
+    item_iter->second->Release();
+    id_to_item_.erase(item_iter);
+  }
+
+  void RemoveForRenderProcessHost(int process_id) {

[wtc, 2013/11/14 22:04:58]

Please copy the original comment (after updating it to remove references to
"certs"):

  // Removes all the certs associated with the specified process from the store.

[alcutter, 2013/11/15 13:33:07]

Done.

+    base::AutoLock auto_lock(lock_);
+
+    // We iterate through all the item ids for that process.
+    std::pair<IDMap::iterator, IDMap::iterator> process_ids =
+        process_id_to_item_id_.equal_range(process_id);
+    for (IDMap::iterator ids_iter = process_ids.first;
+         ids_iter != process_ids.second;
+         ++ids_iter) {
+      int item_id = ids_iter->second;
+      // Find all the processes referring to this item id in
+      // item_id_to_process_id_, then locate the process being removed within
+      // that range.
+      std::pair<IDMap::iterator, IDMap::iterator> item_ids =
+          item_id_to_process_id_.equal_range(item_id);
+      IDMap::iterator proc_iter = std::find_if(
+          item_ids.first, item_ids.second, MatchSecond<int>(process_id));
+      DCHECK(proc_iter != item_ids.second);
+
+      // Before removing, determine if no other processes refer to the current
+      // item id. If |proc_iter| (the current process) is the lower bound of
+      // processes containing the current item id and if |next_proc_iter| is the
+      // upper bound (the first process that does not), then only one process,
+      // the one being removed, refers to the item id.
+      IDMap::iterator next_proc_iter = proc_iter;
+      ++next_proc_iter;
+      bool last_process_for_item_id =
+          (proc_iter == item_ids.first && next_proc_iter == item_ids.second);
+      item_id_to_process_id_.erase(proc_iter);
+
+      if (last_process_for_item_id) {
+        // The current item id is not referenced by any other processes, so
+        // remove it from id_to_item_ and item_to_id_.
+        RemoveInternal(item_id);
+      }
+    }
+    if (process_ids.first != process_ids.second)
+      process_id_to_item_id_.erase(process_ids.first, process_ids.second);
+  }
+
+  void Observe(int type,

[wtc, 2013/11/14 22:04:58]

Add the comment
    // NotificationObserver implementation.
before this method.

This is the convention used in the Chromium source tree.

[alcutter, 2013/11/15 13:33:07]

Done.

+               const NotificationSource& source,
+               const NotificationDetails& details) {

[wtc, 2013/11/14 22:04:58]

Add "OVERRIDE" between ')' and '{'.

[alcutter, 2013/11/15 13:33:07]

Done.

+    DCHECK(type == NOTIFICATION_RENDERER_PROCESS_TERMINATED ||
+           type == NOTIFICATION_RENDERER_PROCESS_CLOSED);
+    RenderProcessHost* rph = Source<RenderProcessHost>(source).ptr();
+    DCHECK(rph);
+    RemoveForRenderProcessHost(rph->GetID());
+  }
+
+ private:
+  typedef std::multimap<int, int> IDMap;
+  typedef std::map<int, scoped_refptr<T> > ItemMap;
+  typedef std::map<T*, int, typename T::LessThan> ReverseItemMap;

[wtc, 2013/11/14 22:04:58]

I am not familiar with templates. Is it necessary to use 'typename' here?

[alcutter, 2013/11/15 13:33:07]

Yep, same story here (it's because someone could create a class where "LessThan"
was, e.g. a member variable or something unexpected, and pass that in to the
template as T. Having this typedef shows the intent that it should be restricted
to types only, and in fact it won't compile without it.)

+
+  // Is only used on the UI Thread.
+  NotificationRegistrar registrar_;
+
+  IDMap process_id_to_item_id_;
+  IDMap item_id_to_process_id_;
+  ItemMap id_to_item_;
+  ReverseItemMap item_to_id_;
+
+  int next_item_id_;
+
+  // This lock protects: process_to_item_id_, item_id_to_process_id_,

[wtc, 2013/11/14 22:04:58]

Typo: process_to_item_id_ => process_id_to_item_id_

[alcutter, 2013/11/15 13:33:07]

Good catch!
Done.

+  //                     id_to_item_, and item_to_id_.
+  base::Lock lock_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_RENDERER_DATA_MEMOIZING_STORE_H_