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..2416c8be4075326de9409080bd84e092aff69fae 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 (uint32_t i = 0; i < pending_observers.size(); i++) {

[palmer, 2016/06/28 19:34:30]

Note that std::vector::size returns size_t, not uint32_t. But, you can just
declare |i| as auto, anyway. :)

[palakj1, 2016/06/29 23:02:40]

Done. 

+    active_observers_.push_back(std::move(pending_observers[i]));
+  }
+  pending_observers.clear();
+}
+
+void IndexedDBConnection::RemoveObservers(

[cmumford, 2016/06/28 20:26:22]

Would this function be simpler if active_observers_ was a map of id -> observer?

[palakj1, 2016/06/29 23:02:40]

Yes. But the more dominant (much more used) use case would involve iterating
over active_observers_ for determining it the observer is listening for a change
or not (part of next cl) like this:

observers = connection->GetActiveObservers()
for(const auto& observer : observers ) {
      if(observer->IsRecording(some property)){ 
           add observation
       }

I believe a vector would be faster for this case than map. But if they are
almost equivalent performance wise, I can sure switch to map.

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

I think these should be called in order, so that might make using a map wrong.
Probably best to stick with a vector.

+    const std::vector<int32_t>& observer_ids_to_remove) {
+  // TODO(palakj): Change from vector to set or create tmp vector instead of

[palmer, 2016/06/28 19:34:30]

You might also consider
http://www.cplusplus.com/reference/algorithm/set_difference/.

[palakj1, 2016/06/29 23:02:40]

Wouldn't that require sorted vectors? Is it preferred to sort the vectors and
apply set_difference than iterating over both of them.
Also, I can populate the pending_observer_id vector this way, but I'd still need
to erase the elements one by one right?

+  // erase.
+  std::vector<int32_t> pending_observer_ids;
+  for (int32_t id_to_remove : observer_ids_to_remove) {
+    bool removed = false;
+    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) {
+      pending_observer_ids.push_back(id_to_remove);
+    }
+  }
+  if (!pending_observer_ids.empty())
+    database_->RemovePendingObservers(this, pending_observer_ids);
+}
+
 }  // namespace content