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..0e125cecf487a658d773a21d92e9ddfeb1f30a9b 100644
--- a/content/browser/indexed_db/indexed_db_connection.cc
+++ b/content/browser/indexed_db/indexed_db_connection.cc
@@ -4,6 +4,8 @@
 
 #include "content/browser/indexed_db/indexed_db_connection.h"
 
+#include "base/stl_util.h"
+
 namespace content {
 
 IndexedDBConnection::IndexedDBConnection(
@@ -21,6 +23,7 @@ void IndexedDBConnection::Close() {
   if (this_obj) {
     database_ = nullptr;
     callbacks_ = nullptr;
+    active_observers_.clear();
   }
 }
 
@@ -35,6 +38,7 @@ void IndexedDBConnection::ForceClose() {
   if (this_obj) {
     database_ = nullptr;
     callbacks_ = nullptr;
+    active_observers_.clear();
   }
   callbacks->OnForcedClose();
 }
@@ -49,4 +53,35 @@ bool IndexedDBConnection::IsConnected() {
   return database_.get() != NULL;
 }
 
+void IndexedDBConnection::ActivatePendingObservers(
+    std::vector<std::unique_ptr<IndexedDBObserver>>* pending_observers) {
+  for (uint32_t i = 0; i < pending_observers->size(); i++) {
+    active_observers_.push_back(std::move(pending_observers->at(i)));

[Marijn Kruisselbrink, 2016/06/24 00:48:08]

I think it's a bit of a weird API that ActivatePendingObservers changes
|pending_observers| to be a vector full of nullptrs. It might at least make
senes to have a pending_observers->clear() at the end of this method?

Or this method could take pending_observers by value rather than by pointer,
with the caller using std::move to cheaply move the vector.

[palakj1, 2016/06/27 20:19:22]

For the second option, doesn't the chromium convention say, that we pass an
argument either as a pointer or const ref.
Am I allowed to pass a value that's not const?

+  }
+}
+
+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
+  // erase.
+  std::vector<int32_t> pending_observer_ids;
+  for (size_t i = 0; i < observer_ids_to_remove.size(); i++) {

[Marijn Kruisselbrink, 2016/06/24 00:48:08]

nit: You're not really using the index |i|, so you could also write this as
for (int32_t id_to_remove : observer_ids_to_remove) { ... }

[palakj1, 2016/06/27 20:19:22]

Done.

+    bool removed = false;
+    for (size_t j = 0; j < active_observers_.size(); j++) {

[Marijn Kruisselbrink, 2016/06/24 00:48:08]

You could rewrite the body of the outer loop as (untested):

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

Although that isn't really any shorter, and arguably not any clearer than what
you have now either, so probably fine/better to just leave what you have now.

[palakj1, 2016/06/27 20:19:22]

this is nice. But, I'm not really comfortable with lambda functions. So, leaving
it as is, unless it offers a performance advantage.

+      if (active_observers_[j]->id() == observer_ids_to_remove[i]) {
+        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) {
+      pending_observer_ids.push_back(observer_ids_to_remove[i]);
+    }
+  }
+  if (!pending_observer_ids.empty())
+    database_->RemovePendingObservers(this, pending_observer_ids);
+}
+
 }  // namespace content