[IndexedDB] Adding transaction and value support to observers

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 <memory>
 #include <set>
 #include <unordered_map>
@@ skipping 59 matching lines @@
 
   IndexedDBTransaction* GetTransaction(int64_t id) const;
 
   base::WeakPtr<IndexedDBTransaction> AddTransactionForTesting(
       std::unique_ptr<IndexedDBTransaction> transaction);
 
   // We ignore calls where the id doesn't exist to facilitate the AbortAll call.
   // TODO(dmurph): Change that so this doesn't need to ignore unknown ids.
   void RemoveTransaction(int64_t id);
 
+  int64_t NextObserverTransactionId();

[cmumford, 2017/01/09 21:31:25]

Comment for method.

[dmurph, 2017/01/10 00:17:38]

Done.

+
  private:
   const int32_t id_;
 
+  // The renderer-created transactions are in the right 32 bits, and the
+  // browser-created transactions are in the left 32 bits.
+  int64_t next_observer_transaction_id_ = 1ll << 32;

[pwnall, 2017/01/10 01:01:24]

I think it'd be easier to debug if you have the highest bit set on browser
transactions, and unset on renderer transactions.

I think you can have minObserverTransactionId = ~0ll - (~0ll >> 1) and then
start from -1 and decrement on the browser side, and start from 0 (or 1) and
increment on the render side.

Also, IIUC, observer transactions are triggered by renderer transactions
committing. If that's the case, how about having renderer transactions be in 1
... (~0ll >> 1), and having a browser transaction be the negative of the
renderer transaction that triggered it?

[dmurph, 2017/01/10 20:39:59]

That second suggestion wouldn't work as the the same transaction could have
multiple observers requesting transactions on the same connection, so they need
to have multiple unique ids.

I'm mostly working off the pre-existing id constraint here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/indexe...,

Where renderer ids are already constrained to the bottom 32 bits. I'd rather
initially keep the ID separation we had previously, and then if necessary do a
follow-up patch to make it nicer.

[pwnall, 2017/01/10 23:21:54]

Thank you for explaining! Sorry, I completely forgot about the multiple
observers case.
I wonder if that comment still relevant. AFAIK, ports were a WebKit thing, and
Chromium's content layer is Blink's only "port".
Works for me. Feel free to file a bug against me instead.

+  static const int64_t kMaxObserverTransactionId = -1ll;

[cmumford, 2017/01/09 21:31:25]

How about moving kMaxObserverTransactionId to the *.cc file to simplify the
interface?

[dmurph, 2017/01/10 00:17:38]

Done.

[pwnall, 2017/01/10 01:01:24]

Hm, I was going to suggest adding a static_assert to make sure that the browser
and the renderer agree on how the transaction ID space is split up.

+
   // The process id of the child process this connection is associated with.
   // Tracked for IndexedDBContextImpl::GetAllOriginsDetails and debugging.
   const int child_process_id_;
 
   // NULL in some unit tests, and after the connection is closed.
   scoped_refptr<IndexedDBDatabase> database_;
 
   // The connection owns transactions created on this connection.
   std::unordered_map<int64_t, std::unique_ptr<IndexedDBTransaction>>
       transactions_;
 
   // 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_