[IndexedDB] Adding transaction and value support to observers

content/browser/indexed_db/indexed_db_database.cc

 // Copyright (c) 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_database.h"
 
 #include <math.h>
 
 #include <limits>
 #include <set>
@@ skipping 788 matching lines @@
 
 void IndexedDBDatabase::AddPendingObserver(
     IndexedDBTransaction* transaction,
     int32_t observer_id,
     const IndexedDBObserver::Options& options) {
   DCHECK(transaction);
   transaction->AddPendingObserver(observer_id, options);
 }
 
 // TODO(palakj): Augment the function with IDBValue later. Issue
 // crbug.com/609934.

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

Is this comment still relevant?

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

Done.

 void IndexedDBDatabase::FilterObservation(IndexedDBTransaction* transaction,
                                           int64_t object_store_id,
                                           blink::WebIDBOperationType type,
-                                          const IndexedDBKeyRange& key_range) {
+                                          const IndexedDBKeyRange& key_range,
+                                          IndexedDBValue* value) {
   for (auto* connection : connections_) {
     bool recorded = false;
     for (const auto& observer : connection->active_observers()) {
       if (!observer->IsRecordingType(type) ||
           !observer->IsRecordingObjectStore(object_store_id))
         continue;
       if (!recorded) {
         auto observation = ::indexed_db::mojom::Observation::New();
         observation->object_store_id = object_store_id;
         observation->type = type;
         if (type != blink::WebIDBClear)
           observation->key_range = key_range;
         transaction->AddObservation(connection->id(), std::move(observation));
         recorded = true;
       }
-      transaction->RecordObserverForLastObservation(connection->id(),
-                                                    observer->id());
+      auto* changes_ptr =
+          transaction->GetPendingChangesForConnection(connection->id());
+      DCHECK(changes_ptr);

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

We don't generally check for NULL ptrs just before use. The browser will crash
in two lines anyhow.

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

Done.

+      ::indexed_db::mojom::ObserverChangesPtr& changes = *changes_ptr;
+
+      changes->observation_index_map[observer->id()].push_back(
+          changes->observations.size() - 1);
+      if (observer->include_transaction() &&
+          changes->transaction_map.find(observer->id()) ==

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

I think this is simpler:

 if (!base::ContainsKey(changes->transaction_map, observer->id()))

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

Done.

+              changes->transaction_map.end()) {
+        IndexedDBTransaction* observer_transaction =
+            connection->CreateTransaction(
+                connection->NextObserverTransactionId(),
+                observer->object_store_ids(),
+                blink::WebIDBTransactionModeReadOnly,
+                new IndexedDBBackingStore::Transaction(backing_store_.get()));
+        transaction_coordinator_.DidCreateObserverTransaction(
+            observer_transaction);
+        observer_transaction->Start();
+        transaction_count_++;

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

Start posts a task. Should transaction_count_ be incremented first?

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

Done.

+
+        auto mojo_transaction = ::indexed_db::mojom::ObserverTransaction::New();
+        mojo_transaction->id = observer_transaction->id();
+        for (int64_t object_store : observer->object_store_ids()) {

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

s/object_store/id/ or maybe s/object_store/object_store_id/

or use STL to append:

mojo_transaction->scope.insert(
  mojo_transaction->scope.end(),
  observer->object_store_ids().begin(),
  observer->object_store_ids().end());

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

Done.

+          mojo_transaction->scope.push_back(object_store);
+        }
+        changes->transaction_map[observer->id()] = std::move(mojo_transaction);
+      }
+      if (value && observer->values() && !changes->observations.back()->value) {
+        IndexedDBValue copy = *value;
+        changes->observations.back()->value =
+            IndexedDBCallbacks::ConvertValue(&copy);
+      }
     }
   }
 }
 
 void IndexedDBDatabase::SendObservations(
     std::map<int32_t, ::indexed_db::mojom::ObserverChangesPtr> changes_map) {
   for (auto* conn : connections_) {
     auto it = changes_map.find(conn->id());
-    if (it != changes_map.end())
-      conn->callbacks()->OnDatabaseChange(std::move(it->second));
+    if (it == changes_map.end())

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

FWIW, the previous version seems mildly easier to parse. Can you please share
why you made the change, so I can code more like everyone else?

[dmurph, 2017/01/10 20:40:00]

I think they're both similar, this was most likely a result of a rewrite after
this being more complex (I think I initially was creating transactions here).

I can't find where I read this in the code style, maybe it was a TotT entry, but
this is attempting to do the "escape nesting ASAP" approach. The idea is to
avoid doing too much if-nesting, and if possible do if-tests where it results in
a 'return', 'break', or 'continue' first, as then it doesn't need an 'else' and
the next lines are no longer nested in an if-statement branch.

But in this case, I'll change it back.

+      continue;
+
+    // Create our transactions if applicable.
+    conn->callbacks()->OnDatabaseChange(std::move(it->second));
   }
 }
 
 void IndexedDBDatabase::GetAll(IndexedDBTransaction* transaction,
                                int64_t object_store_id,
                                int64_t index_id,
                                std::unique_ptr<IndexedDBKeyRange> key_range,
                                bool key_only,
                                int64_t max_count,
                                scoped_refptr<IndexedDBCallbacks> callbacks) {
@@ skipping 483 matching lines @@
   }
   {
     IDB_TRACE1("IndexedDBDatabase::PutOperation.Callbacks", "txn.id",
                transaction->id());
     params->callbacks->OnSuccess(*key);
   }
   FilterObservation(transaction, params->object_store_id,
                     params->put_mode == blink::WebIDBPutModeAddOnly
                         ? blink::WebIDBAdd
                         : blink::WebIDBPut,
-                    IndexedDBKeyRange(*key));
+                    IndexedDBKeyRange(*key), &params->value);
   return s;
 }
 
 void IndexedDBDatabase::SetIndexKeys(
     IndexedDBTransaction* transaction,
     int64_t object_store_id,
     std::unique_ptr<IndexedDBKey> primary_key,
     const std::vector<IndexedDBIndexKeys>& index_keys) {
   DCHECK(transaction);
   IDB_TRACE1("IndexedDBDatabase::SetIndexKeys", "txn.id", transaction->id());
@@ skipping 293 matching lines @@
   IDB_TRACE1("IndexedDBDatabase::DeleteRangeOperation", "txn.id",
              transaction->id());
   size_t delete_count = 0;
   leveldb::Status s =
       backing_store_->DeleteRange(transaction->BackingStoreTransaction(), id(),
                                   object_store_id, *key_range, &delete_count);
   if (!s.ok())
     return s;
   callbacks->OnSuccess();
   FilterObservation(transaction, object_store_id, blink::WebIDBDelete,
-                    *key_range);
+                    *key_range, nullptr);
   return s;
 }
 
 void IndexedDBDatabase::Clear(IndexedDBTransaction* transaction,
                               int64_t object_store_id,
                               scoped_refptr<IndexedDBCallbacks> callbacks) {
   DCHECK(transaction);
   IDB_TRACE1("IndexedDBDatabase::Clear", "txn.id", transaction->id());
   DCHECK_NE(transaction->mode(), blink::WebIDBTransactionModeReadOnly);
 
   if (!ValidateObjectStoreId(object_store_id))
     return;
 
   transaction->ScheduleTask(base::Bind(
       &IndexedDBDatabase::ClearOperation, this, object_store_id, callbacks));
 }
 
 leveldb::Status IndexedDBDatabase::ClearOperation(
     int64_t object_store_id,
     scoped_refptr<IndexedDBCallbacks> callbacks,
     IndexedDBTransaction* transaction) {
   IDB_TRACE1("IndexedDBDatabase::ClearOperation", "txn.id", transaction->id());
   leveldb::Status s = backing_store_->ClearObjectStore(
       transaction->BackingStoreTransaction(), id(), object_store_id);
   if (!s.ok())
     return s;
   callbacks->OnSuccess();
 
   FilterObservation(transaction, object_store_id, blink::WebIDBClear,
-                    IndexedDBKeyRange());
+                    IndexedDBKeyRange(), nullptr);
   return s;
 }
 
 leveldb::Status IndexedDBDatabase::DeleteObjectStoreOperation(
     int64_t object_store_id,
     IndexedDBTransaction* transaction) {
   IDB_TRACE1("IndexedDBDatabase::DeleteObjectStoreOperation",
              "txn.id",
              transaction->id());
 
@@ skipping 213 matching lines @@
   if (status.IsCorruption()) {
     IndexedDBDatabaseError error(blink::WebIDBDatabaseExceptionUnknownError,
                                  message);
     factory_->HandleBackingStoreCorruption(backing_store_->origin(), error);
   } else {
     factory_->HandleBackingStoreFailure(backing_store_->origin());
   }
 }
 
 }  // namespace content