IDBObserver: Lifetime Management: Adding Observer

content/browser/indexed_db/indexed_db_connection.cc

 // Copyright 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.
 
 #include "content/browser/indexed_db/indexed_db_connection.h"
 
+#include "base/logging.h"
+#include "base/stl_util.h"
+
 namespace content {
 
 IndexedDBConnection::IndexedDBConnection(
     scoped_refptr<IndexedDBDatabase> database,
     scoped_refptr<IndexedDBDatabaseCallbacks> callbacks)
     : database_(database), callbacks_(callbacks), weak_factory_(this) {}
 
 IndexedDBConnection::~IndexedDBConnection() {}
 
 void IndexedDBConnection::Close() {
   if (!callbacks_.get())
     return;
   base::WeakPtr<IndexedDBConnection> this_obj = weak_factory_.GetWeakPtr();
   database_->Close(this, false /* forced */);
   if (this_obj) {
     database_ = nullptr;
     callbacks_ = nullptr;
+    active_observers_.clear();
   }
 }
 
 void IndexedDBConnection::ForceClose() {
   if (!callbacks_.get())
     return;
 
   // IndexedDBDatabase::Close() can delete this instance.
   base::WeakPtr<IndexedDBConnection> this_obj = weak_factory_.GetWeakPtr();
   scoped_refptr<IndexedDBDatabaseCallbacks> callbacks(callbacks_);
   database_->Close(this, true /* forced */);
   if (this_obj) {
     database_ = nullptr;
     callbacks_ = nullptr;
+    active_observers_.clear();
   }
   callbacks->OnForcedClose();
 }
 
 void IndexedDBConnection::VersionChangeIgnored() {
   if (!database_.get())
     return;
   database_->VersionChangeIgnored();
 }
 
 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