IDBObserver: Lifetime Management: Adding Observer

content/browser/indexed_db/indexed_db_connection.h

Index: content/browser/indexed_db/indexed_db_connection.h
diff --git a/content/browser/indexed_db/indexed_db_connection.h b/content/browser/indexed_db/indexed_db_connection.h
index 4d828e5e6ff5e28a138f19c79959f5a928ab165e..dae6fb25c450a0cf1c4dcf293dce49d15fde1523 100644
--- a/content/browser/indexed_db/indexed_db_connection.h
+++ b/content/browser/indexed_db/indexed_db_connection.h
@@ -10,6 +10,7 @@
 #include "base/memory/weak_ptr.h"
 #include "content/browser/indexed_db/indexed_db_database.h"
 #include "content/browser/indexed_db/indexed_db_database_callbacks.h"
+#include "content/browser/indexed_db/indexed_db_observer.h"
 
 namespace content {
 class IndexedDBCallbacks;
@@ -27,9 +28,19 @@ class CONTENT_EXPORT IndexedDBConnection {
   virtual bool IsConnected();
 
   void VersionChangeIgnored();
+  void ActivatePendingObservers(

[Marijn Kruisselbrink, 2016/06/28 18:58:31]

I think some comments here could be helpful too. For example why is this called
ActivatePendingObservers rather than just AddObservers?

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

Thanks. This was really important. Added some comments, let me know if they
throw sufficient light.

+      std::vector<std::unique_ptr<IndexedDBObserver>> pending_observers);
+  void RemoveObservers(const std::vector<int32_t>& remove_observer_ids);
 
   IndexedDBDatabase* database() const { return database_.get(); }
   IndexedDBDatabaseCallbacks* callbacks() const { return callbacks_.get(); }
+  const std::vector<std::unique_ptr<IndexedDBObserver>>& GetActiveObservers()

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

This is a "getter" so you can name it active_observers().

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

Is the current GetActiveObservers usage wrong? Changing it all the same.

+      const {
+    return active_observers_;
+  }
+  base::WeakPtr<IndexedDBConnection> GetWeakPtr() {

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

I think this is the "old" way. Instead consider deriving IndexedDBConnection
from base::SupportsWeakPtr<IndexedDBConnection>, and then you can delete
GetWeakPtr() and use AsWeakPtr() to mint a new WeakPtr.

BTW, are you sure you're obeying the threading rules in order to use a WeakPtr
to this class? Read comment in weak_ptr.h.

[Marijn Kruisselbrink, 2016/06/28 20:37:30]

The "old" way? I think in general a WeakPtrFactory is preferred over inheriting
from base::SupportsWeakPtr. See the comment for SupportsWeakPtr "since
SupportsWeakPtr's destructor won't invalidate weak pointers to the class until
after the derived class' members have been destroyed, its use can lead to subtle
use-after-destroy issues.". There even was some discussion back in 2013 to
completely kill SupportsWeakPtr in favor of WeakPtrFactory
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/N6w_CkKEwvE/evs2p...)

[palakj1, 2016/06/30 18:11:44]

Not really sure about thread safety. The transaction that holds weakptr to
connection, is supposed to die before the connection does. Not certain if the
weakptr is always " dereferenced and invalidated on the same
SequencedTaskRunner" as mentioned in weak_ptr.h
Do you feel there's a problem here?

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

Wow! Thanks for pointing that out Marijn.

+    return weak_factory_.GetWeakPtr();
+  }
 
  private:
   // NULL in some unit tests, and after the connection is closed.
@@ -38,7 +49,7 @@ class CONTENT_EXPORT IndexedDBConnection {
   // The callbacks_ member is cleared when the connection is closed.
   // May be NULL in unit tests.
   scoped_refptr<IndexedDBDatabaseCallbacks> callbacks_;
-
+  std::vector<std::unique_ptr<IndexedDBObserver>> active_observers_;
   base::WeakPtrFactory<IndexedDBConnection> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(IndexedDBConnection);