IDBObserver: Lifetime Management: Adding Observer

content/browser/indexed_db/indexed_db_connection.cc

Index: content/browser/indexed_db/indexed_db_connection.cc
diff --git a/content/browser/indexed_db/indexed_db_connection.cc b/content/browser/indexed_db/indexed_db_connection.cc
index f7133a69230da18b88b9deb75e84775823d9467e..238856c2d798a106991c9518915b81279021a3a5 100644
--- a/content/browser/indexed_db/indexed_db_connection.cc
+++ b/content/browser/indexed_db/indexed_db_connection.cc
@@ -4,6 +4,9 @@
 
 #include "content/browser/indexed_db/indexed_db_connection.h"
 
+#include "base/logging.h"
+#include "base/stl_util.h"
+
 namespace content {
 
 IndexedDBConnection::IndexedDBConnection(
@@ -21,6 +24,7 @@ void IndexedDBConnection::Close() {
   if (this_obj) {
     database_ = nullptr;
     callbacks_ = nullptr;
+    active_observers_.clear();
   }
 }
 
@@ -35,6 +39,7 @@ void IndexedDBConnection::ForceClose() {
   if (this_obj) {
     database_ = nullptr;
     callbacks_ = nullptr;
+    active_observers_.clear();
   }
   callbacks->OnForcedClose();
 }
@@ -49,4 +54,36 @@ bool IndexedDBConnection::IsConnected() {
   return database_.get() != NULL;
 }
 
+void IndexedDBConnection::ActivatePendingObservers(
+    std::vector<std::unique_ptr<IndexedDBObserver>> pending_observers) {
+  for (auto& observer : pending_observers) {
+    active_observers_.push_back(std::move(observer));
+  }
+  pending_observers.clear();
+}
+
+void IndexedDBConnection::RemoveObservers(
+    const std::vector<int32_t>& observer_ids_to_remove) {
+  // TODO(palakj): Change from vector to set or create tmp vector instead of

[cmumford, 2016/07/01 18:35:00]

This seems like a TODO that can be done now - any reason why not? I only ask
because TODO's have a way of staying in the code for years (or forever), so best
to avoid.

+  // erase.
+  std::vector<int32_t> pending_observer_ids;
+  for (int32_t id_to_remove : observer_ids_to_remove) {
+    bool removed = false;

[cmumford, 2016/07/01 18:35:00]

Your implementation is fine, but using a lambda might be cleaner - that is, if
you find lambda's clean.

    const auto& it = std::find_if(
        active_observers_.begin(), active_observers_.end(),
        [&id_to_remove](const std::unique_ptr<IndexedDBObserver>& o) {
          return o->id() == id_to_remove;
        });
    if (it != active_observers_.end())
      active_observers_.erase(it);
    else
      pending_observer_ids.push_back(id_to_remove);

But passing in a std::set<> would be most efficient.

+    for (size_t j = 0; j < active_observers_.size(); j++) {
+      if (active_observers_[j]->id() == id_to_remove) {
+        active_observers_.erase(active_observers_.begin() + j);
+        removed = true;
+        break;
+      }
+    }
+    // If observer not in active_observers_ , must be in pending_observer list
+    // of one of the transactions.
+    if (!removed) {

[cmumford, 2016/07/01 18:35:00]

Nit: Curly-braces not required for single-line statements:

https://google.github.io/styleguide/cppguide.html#Conditionals

[palakj1, 2016/07/02 00:48:13]

Done.

+      pending_observer_ids.push_back(id_to_remove);
+    }
+  }
+  if (!pending_observer_ids.empty())
+    database_->RemovePendingObservers(this, pending_observer_ids);
+}
+
 }  // namespace content