IDBObserver: Lifetime Management: Adding Observer

content/browser/indexed_db/indexed_db_connection.h

 // 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.
 
 #ifndef CONTENT_BROWSER_INDEXED_DB_INDEXED_DB_CONNECTION_H_
 #define CONTENT_BROWSER_INDEXED_DB_INDEXED_DB_CONNECTION_H_
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #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;
 class IndexedDBDatabaseError;
 
 class CONTENT_EXPORT IndexedDBConnection {
  public:
   IndexedDBConnection(scoped_refptr<IndexedDBDatabase> db,
                       scoped_refptr<IndexedDBDatabaseCallbacks> callbacks);
   virtual ~IndexedDBConnection();
 
   // These methods are virtual to allow subclassing in unit tests.
   virtual void ForceClose();
   virtual void Close();
   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.
   scoped_refptr<IndexedDBDatabase> database_;
 
   // 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);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_INDEXED_DB_INDEXED_DB_CONNECTION_H_