[IndexedDB] Adding transaction and value support to observers

[IndexedDB] Adding transaction and value support to observers

BUG=662248, 662246, 609934
R=pwnall@chromium.org,jsbell@chromium.org,cmumford@chromium.org

Review-Url: https://codereview.chromium.org/2601983002
Cr-Commit-Position: refs/heads/master@{#444514}
Committed: https://chromium.googlesource.com/chromium/src/+/a1319a867978d9700653ecc5a6592d24b6367a2c

[dmurph, 2017-01-05 23:20:24]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 23:21:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 23:33:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 23:33:55]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dmurph, 2017-01-07 01:04:20]

Description was changed from

==========
[IndexedDB] Adding transaction and value support to observers

BUG=662248,662246
==========

to

==========
[IndexedDB] Adding transaction and value support to observers

BUG=662248,662246
R=pwnall@chromium.org,jsbell@chromium.org,cmumford@chromium.org
==========

[dmurph, 2017-01-07 01:07:13]

dmurph@chromium.org changed reviewers:
+ cmumford@chromium.org, jsbell@chromium.org, pwnall@chromium.org

[dmurph, 2017-01-07 01:07:14]

Hello Josh, Victor, and Chris!

Can you all PTAL at this patch? I've added 'value' and 'transaction' support to
IDBObservers, and I'm hoping that I did this correctly, especially w/
transactions and their lifecycle.

One potential problem that I'm trying to avoid - if we have an observer on
object stores A and B, and two transactions are happening (at the same time?),
one on A, one on B. When A finished first, I create the observer transaction
over A and B. This is a snapshot, right? And would it also possibly include B's
changes? Or am I safe.

Thanks!
Daniel

[dmurph, 2017-01-07 01:15:53]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 01:16:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 02:32:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-07 02:32:28]

Dry run: This issue passed the CQ dry run.

[cmumford, 2017-01-09 21:31:26]

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_connection.cc (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.cc:161: int64_t
IndexedDBConnection::NextObserverTransactionId() {
nit: "Next*" might be interpreted as a read-only operation. What do you think
about AllocObserverTransactionId()?

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_connection.h (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:80: int64_t
NextObserverTransactionId();
Comment for method.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:88: static const int64_t
kMaxObserverTransactionId = -1ll;
How about moving kMaxObserverTransactionId to the *.cc file to simplify the
interface?

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:809: // crbug.com/609934.
Is this comment still relevant?

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:832: DCHECK(changes_ptr);
We don't generally check for NULL ptrs just before use. The browser will crash
in two lines anyhow.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:838:
changes->transaction_map.find(observer->id()) ==
I think this is simpler:

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

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:849: transaction_count_++;
Start posts a task. Should transaction_count_ be incremented first?

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:853: for (int64_t object_store
: observer->object_store_ids()) {
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());

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.h:146: IndexedDBValue* value);
Value isn't modified, so make it const. Also, add a comment above to explain
that a null value is allowed.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_transaction.cc (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.cc:370: std::set<int64_t>
object_stores;
object_store_ids

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.cc:375:
pending_observer->set_object_store_ids(std::move(object_stores));
Won't this only set the first pending observer's set of object store ID's. After
that object_stores will be empty right?

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.cc:523:
::indexed_db::mojom::ObserverChangesPtr*
function should be const, *and* return a const ObserverChangesPtr.

https://codereview.chromium.org/2601983002/diff/80001/content/child/indexed_d...
File content/child/indexed_db/indexed_db_database_callbacks_impl.cc (right):

https://codereview.chromium.org/2601983002/diff/80001/content/child/indexed_d...
content/child/indexed_db/indexed_db_database_callbacks_impl.cc:55:
observer_transactions;
Is observer_transactions ever set?

[cmumford, 2017-01-09 21:33:02]

https://codereview.chromium.org/2601983002/diff/80001/content/child/indexed_d...
File content/child/indexed_db/indexed_db_database_callbacks_impl.cc (right):

https://codereview.chromium.org/2601983002/diff/80001/content/child/indexed_d...
content/child/indexed_db/indexed_db_database_callbacks_impl.cc:55:
observer_transactions;
On 2017/01/09 21:31:26, cmumford wrote:
> Is observer_transactions ever set?

Nevermind (doh!) I see it.

[dmurph, 2017-01-09 23:23:46]

Description was changed from

==========
[IndexedDB] Adding transaction and value support to observers

BUG=662248,662246
R=pwnall@chromium.org,jsbell@chromium.org,cmumford@chromium.org
==========

to

==========
[IndexedDB] Adding transaction and value support to observers

BUG=662248,662246,609934
R=pwnall@chromium.org,jsbell@chromium.org,cmumford@chromium.org
==========

[dmurph, 2017-01-10 00:16:30]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 00:17:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Thanks! Replied to comments. I also now disallow observing on a versionchange
transaction, as per github issue
https://github.com/WICG/indexed-db-observers/issues/37.

PTAL!

Thanks,
Daniel

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_connection.cc (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.cc:161: int64_t
IndexedDBConnection::NextObserverTransactionId() {
On 2017/01/09 21:31:25, cmumford wrote:
> nit: "Next*" might be interpreted as a read-only operation. What do you think
> about AllocObserverTransactionId()?

NewObserverTransactionId?

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_connection.h (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:80: int64_t
NextObserverTransactionId();
On 2017/01/09 21:31:25, cmumford wrote:
> Comment for method.

Done.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:88: static const int64_t
kMaxObserverTransactionId = -1ll;
On 2017/01/09 21:31:25, cmumford wrote:
> How about moving kMaxObserverTransactionId to the *.cc file to simplify the
> interface?

Done.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:809: // crbug.com/609934.
On 2017/01/09 21:31:25, cmumford wrote:
> Is this comment still relevant?

Done.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:832: DCHECK(changes_ptr);
On 2017/01/09 21:31:25, cmumford wrote:
> We don't generally check for NULL ptrs just before use. The browser will crash
> in two lines anyhow.

Done.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:838:
changes->transaction_map.find(observer->id()) ==
On 2017/01/09 21:31:25, cmumford wrote:
> I think this is simpler:
> 
>  if (!base::ContainsKey(changes->transaction_map, observer->id()))

Done.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:849: transaction_count_++;
On 2017/01/09 21:31:25, cmumford wrote:
> Start posts a task. Should transaction_count_ be incremented first?

Done.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:853: for (int64_t object_store
: observer->object_store_ids()) {
On 2017/01/09 21:31:25, cmumford wrote:
> 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());

Done.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.h:146: IndexedDBValue* value);
On 2017/01/09 21:31:25, cmumford wrote:
> Value isn't modified, so make it const. Also, add a comment above to explain
> that a null value is allowed.

Done.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_transaction.cc (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.cc:370: std::set<int64_t>
object_stores;
On 2017/01/09 21:31:25, cmumford wrote:
> object_store_ids

removed.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.cc:375:
pending_observer->set_object_store_ids(std::move(object_stores));
On 2017/01/09 21:31:26, cmumford wrote:
> Won't this only set the first pending observer's set of object store ID's.
After
> that object_stores will be empty right?

Removed this feature as per
https://github.com/WICG/indexed-db-observers/issues/37 (oops forgot to add you
on there). But yeah, this is now impossible.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.cc:523:
::indexed_db::mojom::ObserverChangesPtr*
On 2017/01/09 21:31:26, cmumford wrote:
> function should be const, *and* return a const ObserverChangesPtr.

Neither can be const, as I need to modify the return value here to populate
transactions and values (in indexed_db_database).

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

Comments on your previous patch.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_connection.h (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:87: int64_t
next_observer_transaction_id_ = 1ll << 32;
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?

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:88: static const int64_t
kMaxObserverTransactionId = -1ll;
On 2017/01/09 21:31:25, cmumford wrote:
> How about moving kMaxObserverTransactionId to the *.cc file to simplify the
> interface?

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.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:871: if (it ==
changes_map.end())
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?

[pwnall, 2017-01-10 01:41:12]

Here are a few more small comments.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/observer.html (right):

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/observer.html:1: <html>
Please add <!doctype html> at the beginning of this.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js
(right):

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:5:
function compareChanges(actual, expected) {
would it be fair to say this is assertChangesEqual?

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:10:
var storesStr = Array.from(actual.records.keys()).join(", ");
storeNamesString?

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:14:
var actual_obsv = actual.records.get(key);
camelCase instead of snake_case?

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:24:
assert_not_equals(actual.type, 'kDelete');
Why is this here?

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:25:
if (actual.type == 'clear') {
I'd prefer === because it makes things slightly easier to reason about. In this
case it's pretty clear that no type coercion will happen, but it's still nice
not to have to think that through.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:39:
assert_equals(actual.key.lower, expected.key, 'Observed operation key lower
bound should match operation performed');
There doesn't seem to be any checking for add vs put. Would it make sense to
have that here? I imagine that, at the very least, you could check that the
change type is 'add' or 'put'.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer-tests.js
(right):

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer-tests.js:100:
assert_true(connection != null);
Can you add a description for the meaning of this failure? Based on what I'm
reading below, it seems like you're testing that the observer callback is only
called after the open call succeeded?

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer-tests.js:121:
'store2': [{type: 'add', key: 'z'}]
comma at the end of the line, to minimize future churn

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer-tests.js:291:
assert_true(connection != null, "connection is null");
The comment appears to be redundant with the condition.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js
(right):

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:9: var
obs = new IDBObserver(t.step_func(function() {
I think you can save 2 lines by using fat arrows here.

var obs = new IDBObserver(t.step_func(() => { done(); }));

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBDatabaseCallbacks.h (right):

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBDatabaseCallbacks.h:60: const
std::unordered_map<int32_t,
Can you alias this type to a meaningful name?

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:56:
DCHECK(!scope.isEmpty()) << "Observer transactions should operate on a "
how about using "must" instead of "should" just like below?

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:122:
DCHECK_EQ(m_state, Active);
This seems a bit excessive, given that you're initializing the state right
above.

[commit-bot: I haz the power, 2017-01-10 02:19:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 02:19:54]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[dmurph, 2017-01-10 20:38:15]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 20:39:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2017-01-10 20:40:08]

Thanks!

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_connection.h (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:87: int64_t
next_observer_transaction_id_ = 1ll << 32;
On 2017/01/10 01:01:24, pwnall wrote:
> 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?

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.

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:871: if (it ==
changes_map.end())
On 2017/01/10 01:01:24, pwnall wrote:
> 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?

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.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/observer.html (right):

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/observer.html:1: <html>
On 2017/01/10 01:41:11, pwnall wrote:
> Please add <!doctype html> at the beginning of this.

Done.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js
(right):

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:5:
function compareChanges(actual, expected) {
On 2017/01/10 01:41:11, pwnall wrote:
> would it be fair to say this is assertChangesEqual?

Done.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:10:
var storesStr = Array.from(actual.records.keys()).join(", ");
On 2017/01/10 01:41:11, pwnall wrote:
> storeNamesString?

Done.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:14:
var actual_obsv = actual.records.get(key);
On 2017/01/10 01:41:11, pwnall wrote:
> camelCase instead of snake_case?

Done.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:24:
assert_not_equals(actual.type, 'kDelete');
On 2017/01/10 01:41:11, pwnall wrote:
> Why is this here?

Done.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:25:
if (actual.type == 'clear') {
On 2017/01/10 01:41:11, pwnall wrote:
> I'd prefer === because it makes things slightly easier to reason about. In
this
> case it's pretty clear that no type coercion will happen, but it's still nice
> not to have to think that through.

Done.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/generic-idb-operations.js:39:
assert_equals(actual.key.lower, expected.key, 'Observed operation key lower
bound should match operation performed');
On 2017/01/10 01:41:11, pwnall wrote:
> There doesn't seem to be any checking for add vs put. Would it make sense to
> have that here? I imagine that, at the very least, you could check that the
> change type is 'add' or 'put'.

That's basically done above, in the assert_equals(actual.type, expected.type);

That would fail if the operation is not what we expect it to be. Here we're just
special casing what we compare, as it changes based on the type.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer-tests.js
(right):

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer-tests.js:100:
assert_true(connection != null);
On 2017/01/10 01:41:11, pwnall wrote:
> Can you add a description for the meaning of this failure? Based on what I'm
> reading below, it seems like you're testing that the observer callback is only
> called after the open call succeeded?

Done.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer-tests.js:121:
'store2': [{type: 'add', key: 'z'}]
On 2017/01/10 01:41:11, pwnall wrote:
> comma at the end of the line, to minimize future churn

Done.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer-tests.js:291:
assert_true(connection != null, "connection is null");
On 2017/01/10 01:41:11, pwnall wrote:
> The comment appears to be redundant with the condition.

Done.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js
(right):

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/observer.js:9: var
obs = new IDBObserver(t.step_func(function() {
On 2017/01/10 01:41:11, pwnall wrote:
> I think you can save 2 lines by using fat arrows here.
> 
> var obs = new IDBObserver(t.step_func(() => { done(); }));

Done.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBDatabaseCallbacks.h (right):

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBDatabaseCallbacks.h:60: const
std::unordered_map<int32_t,
On 2017/01/10 01:41:11, pwnall wrote:
> Can you alias this type to a meaningful name?

Done.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:56:
DCHECK(!scope.isEmpty()) << "Observer transactions should operate on a "
On 2017/01/10 01:41:11, pwnall wrote:
> how about using "must" instead of "should" just like below?

Done.

https://codereview.chromium.org/2601983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:122:
DCHECK_EQ(m_state, Active);
On 2017/01/10 01:41:11, pwnall wrote:
> This seems a bit excessive, given that you're initializing the state right
> above.

Done.

[commit-bot: I haz the power, 2017-01-10 21:48:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 21:48:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_connection.h (right):

https://codereview.chromium.org/2601983002/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:87: int64_t
next_observer_transaction_id_ = 1ll << 32;
On 2017/01/10 20:39:59, dmurph wrote:
> On 2017/01/10 01:01:24, pwnall wrote:
> > 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?
> 
> 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.

Thank you for explaining! Sorry, I completely forgot about the multiple
observers case.

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

I wonder if that comment still relevant. AFAIK, ports were a WebKit thing, and
Chromium's content layer is Blink's only "port".

> 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.

Works for me. Feel free to file a bug against me instead.

[dmurph, 2017-01-12 01:31:38]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 01:32:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2017-01-12 01:34:07]

As per the test failure, I created a test that specifically tests the data
consistency of transactions when changes are done in another context (worker).
This found the bug, and I fixed it - I needed to explicitly call 'begin' on the
internal Transaction, instead of relying on it being called on a scheduled task,
and I needed to create / start the transactions when I send the observations
instead of during the operations.

QUESTION: Should I also defer the IndexedDBTransaction object creation to the
SendObservations method? I could just generate the ID and stuff first, and then
wait until SendObservations.... This would also help with possible transaction
aborting... I may just do this.

PTAL! Thanks :)

[commit-bot: I haz the power, 2017-01-12 01:46:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 01:46:15]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[dmurph, 2017-01-12 21:57:54]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 21:58:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2017-01-13 00:14:12]

Feedback on the content/ side - I'll look at the tests/blink part tomorrow.

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:134: /* static */
::indexed_db::mojom::ValuePtr IndexedDBCallbacks::ConvertValue(
nit: usual commenting convention is:

// static
::indexed_db::mojom::ValuePtr IndexedDBCallbacks::ConvertValue(...)

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_connection.cc (right):

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.cc:162: // If we ever overflow,
just reset the ID.
At 1000 transaction/sec that's 285000 years. But good to be safe. :)

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.cc:163: if
(next_observer_transaction_id_ == kMaxObserverTransactionId) {
nit: no need for {}

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.cc:166: return
next_observer_transaction_id_++;
Maybe DCHECK that the high bits are not 0 ?

(which is the critical thing here)

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:830:
::indexed_db::mojom::ObserverChangesPtr& changes = *changes_ptr;
Why have changes_ptr instead of doing this all in one line?

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_transaction.cc (right):

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_transaction.cc:490: CHECK_NE(mode(),
blink::WebIDBTransactionModeVersionChange);
It seems like DCHECK_NE would be fine here?

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_transaction_coordinator.cc (right):

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_transaction_coordinator.cc:30: void
IndexedDBTransactionCoordinator::DidCreateObserverTransaction(
Maybe comment here that observer txs jump to the head of the queue?

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
File content/browser/indexed_db/list_set.h (right):

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/list_set.h:36: void insert_front(const T& elem) {
Add test case to list_set_unittest.cc ?

https://codereview.chromium.org/2601983002/diff/160001/content/child/indexed_...
File content/child/indexed_db/indexed_db_callbacks_impl.cc (right):

https://codereview.chromium.org/2601983002/diff/160001/content/child/indexed_...
content/child/indexed_db/indexed_db_callbacks_impl.cc:77: /* static */ void
IndexedDBCallbacksImpl::ConvertValue(
Nit: 

// static
....

[commit-bot: I haz the power, 2017-01-13 01:45:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 01:46:00]

Dry run: This issue passed the CQ dry run.

[dmurph, 2017-01-13 01:46:44]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 01:47:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2017-01-13 01:50:28]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[dmurph, 2017-01-13 01:50:43]

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:134: /* static */
::indexed_db::mojom::ValuePtr IndexedDBCallbacks::ConvertValue(
On 2017/01/13 00:14:12, jsbell wrote:
> nit: usual commenting convention is:
> 
> // static
> ::indexed_db::mojom::ValuePtr IndexedDBCallbacks::ConvertValue(...)

Done.

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_connection.cc (right):

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.cc:162: // If we ever overflow,
just reset the ID.
On 2017/01/13 00:14:12, jsbell wrote:
> At 1000 transaction/sec that's 285000 years. But good to be safe. :)

haha sgtm.

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.cc:163: if
(next_observer_transaction_id_ == kMaxObserverTransactionId) {
On 2017/01/13 00:14:12, jsbell wrote:
> nit: no need for {}

Done.

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.cc:166: return
next_observer_transaction_id_++;
On 2017/01/13 00:14:12, jsbell wrote:
> Maybe DCHECK that the high bits are not 0 ?
> 
> (which is the critical thing here)

Done.

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:830:
::indexed_db::mojom::ObserverChangesPtr& changes = *changes_ptr;
On 2017/01/13 00:14:12, jsbell wrote:
> Why have changes_ptr instead of doing this all in one line?

Done.

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_transaction.cc (right):

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_transaction.cc:490: CHECK_NE(mode(),
blink::WebIDBTransactionModeVersionChange);
On 2017/01/13 00:14:12, jsbell wrote:
> It seems like DCHECK_NE would be fine here?

Sure.

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_transaction_coordinator.cc (right):

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/indexed_db_transaction_coordinator.cc:30: void
IndexedDBTransactionCoordinator::DidCreateObserverTransaction(
On 2017/01/13 00:14:12, jsbell wrote:
> Maybe comment here that observer txs jump to the head of the queue?

Done.

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
File content/browser/indexed_db/list_set.h (right):

https://codereview.chromium.org/2601983002/diff/160001/content/browser/indexe...
content/browser/indexed_db/list_set.h:36: void insert_front(const T& elem) {
On 2017/01/13 00:14:12, jsbell wrote:
> Add test case to list_set_unittest.cc ?

Done.

https://codereview.chromium.org/2601983002/diff/160001/content/child/indexed_...
File content/child/indexed_db/indexed_db_callbacks_impl.cc (right):

https://codereview.chromium.org/2601983002/diff/160001/content/child/indexed_...
content/child/indexed_db/indexed_db_callbacks_impl.cc:77: /* static */ void
IndexedDBCallbacksImpl::ConvertValue(
On 2017/01/13 00:14:12, jsbell wrote:
> Nit: 
> 
> // static
> ....

Done.

[commit-bot: I haz the power, 2017-01-13 01:50:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 03:58:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 03:58:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cmumford, 2017-01-17 20:58:11]

lgtm

https://codereview.chromium.org/2601983002/diff/200001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_connection.cc (right):

https://codereview.chromium.org/2601983002/diff/200001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.cc:18: static const int64_t
kMaxObserverTransactionId = -1ll;
Add newline before.

https://codereview.chromium.org/2601983002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right):

https://codereview.chromium.org/2601983002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:36:
observationIndex);
Are these really observerIndices?

[dmurph, 2017-01-17 23:26:31]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 23:26:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2017-01-17 23:28:03]

dmurph@chromium.org changed reviewers:
+ palmer@chromium.org

[dmurph, 2017-01-17 23:28:04]

+palmer for security review of content/common/indexed_db/indexed_db.mojom


After chatting with Victor I simplified the ID generation for observer
transaction IDs.

https://codereview.chromium.org/2601983002/diff/200001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_connection.cc (right):

https://codereview.chromium.org/2601983002/diff/200001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.cc:18: static const int64_t
kMaxObserverTransactionId = -1ll;
On 2017/01/17 20:58:10, cmumford wrote:
> Add newline before.

Done.

Actually, as per working with Victor, we removed all of this and simplified the
ID incrementing system to just have a uint32_t that then we shift by 32 before
returning.

https://codereview.chromium.org/2601983002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp (right):

https://codereview.chromium.org/2601983002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObserverChanges.cpp:36:
observationIndex);
On 2017/01/17 20:58:11, cmumford wrote:
> Are these really observerIndices?

Yeah, they are the indexes in the observations array that apply to this
observer. So.... observationIndex? I'll make it observationIndices.

[commit-bot: I haz the power, 2017-01-17 23:29:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 23:29:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dmurph, 2017-01-18 01:39:48]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 01:40:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 03:27:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 03:27:51]

Dry run: This issue passed the CQ dry run.

[dmurph, 2017-01-18 19:39:39]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 19:40:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[palmer, 2017-01-18 19:56:01]

LGTM from an IPC security perspective. Some code style nits and comments.

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_connection.cc (right):

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.cc:166: return
static_cast<int64_t>(next_observer_transaction_id_++) << 32;
Yeah, here is a place where that struct would result in cleaner code:

  TransactionID id;
  id.browser_created_id = next_browser_created_id++;
  return id;

Or perhaps even:

  return {0, next_browser_created_id++};

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_connection.h (right):

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:87: // The renderer-created
transactions are in the right 32 bits, and the
Nit: Clarify: "...the right 32 bits of a transaction ID..."

But, it sounds like TransactionID should be of a type that documents itself
better?

struct TransactionID {
  int64_t renderer_created_id : 32;
  int64_t browser_created_id : 32;
};

or, if you ever need to refer to the whole 64-bit object at once:

struct TransationID {
  union {
    int64_t storage;
    int32_t renderer_created_id;
    int32_t browser_created_id;
  };
};

or similarly:

struct TransationID {
  union {
    int64_t storage;
    int64_t renderer_created_id : 32;
    int64_t browser_created_id : 32;
  };
};

...or any number of other ideas.

And then you can say:

int32_t next_browser_created_id = 1;

without needing any comments.

(Note also the int32_t vs. uint32_t mismatch, and the terminology mismatch:
"browser-created" vs. "observer" — pick 1 and use it consistently.)

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:827:
changes->observations.size() - 1);
Why - 1 ?

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:838: IndexedDBValue copy =
*value;
Yeah, here's the copy you were talking about, right? Its sole purpose seems to
be to make ConvertValue work, so if this is the only call site, maybe you could
refactor ConvertValue to do the copy. Then maybe you could get both cleaner
semantics and not-worse performance? Your call; I don't know this code like you
do.

[dmurph, 2017-01-18 21:00:54]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[dmurph, 2017-01-18 21:00:58]

Thanks!

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_connection.cc (right):

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.cc:166: return
static_cast<int64_t>(next_observer_transaction_id_++) << 32;
On 2017/01/18 19:56:01, palmer wrote:
> Yeah, here is a place where that struct would result in cleaner code:
> 
>   TransactionID id;
>   id.browser_created_id = next_browser_created_id++;
>   return id;
> 
> Or perhaps even:
> 
>   return {0, next_browser_created_id++};

Acknowledged.

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_connection.h (right):

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:87: // The renderer-created
transactions are in the right 32 bits, and the
On 2017/01/18 19:56:01, palmer wrote:
> Nit: Clarify: "...the right 32 bits of a transaction ID..."
> 
> But, it sounds like TransactionID should be of a type that documents itself
> better?
> 
> struct TransactionID {
>   int64_t renderer_created_id : 32;
>   int64_t browser_created_id : 32;
> };
> 
> or, if you ever need to refer to the whole 64-bit object at once:
> 
> struct TransationID {
>   union {
>     int64_t storage;
>     int32_t renderer_created_id;
>     int32_t browser_created_id;
>   };
> };
> 
> or similarly:
> 
> struct TransationID {
>   union {
>     int64_t storage;
>     int64_t renderer_created_id : 32;
>     int64_t browser_created_id : 32;
>   };
> };
> 
> ...or any number of other ideas.
> 
> And then you can say:
> 
> int32_t next_browser_created_id = 1;
> 
> without needing any comments.
> 
> (Note also the int32_t vs. uint32_t mismatch, and the terminology mismatch:
> "browser-created" vs. "observer" — pick 1 and use it consistently.)

I tried to clarify. I'm going to add a bug for updating the Transaction id
scheme, we have it as an int64 everywhere and it would be nice to change it. 
https://bugs.chromium.org/p/chromium/issues/detail?id=682378

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:827:
changes->observations.size() - 1);
On 2017/01/18 19:56:01, palmer wrote:
> Why - 1 ?

We want the last index.

https://codereview.chromium.org/2601983002/diff/240001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:838: IndexedDBValue copy =
*value;
On 2017/01/18 19:56:01, palmer wrote:
> Yeah, here's the copy you were talking about, right? Its sole purpose seems to
> be to make ConvertValue work, so if this is the only call site, maybe you
could
> refactor ConvertValue to do the copy. Then maybe you could get both cleaner
> semantics and not-worse performance? Your call; I don't know this code like
you
> do.

I added a bug mention here - we want to avoid IndexedDBValue copies when
avoidable. Bug has solution idea.

[dmurph, 2017-01-18 21:01:10]

The CQ bit was unchecked by dmurph@chromium.org

[dmurph, 2017-01-18 21:01:18]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2017-01-18 21:01:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
cmumford@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/2601983002/#ps280001
(title: "added comments and bug link")

[commit-bot: I haz the power, 2017-01-18 21:02:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 22:29:48]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1484773278057530,
"parent_rev": "3d9dba1c2096a55778307a3d5ad322e187ab13f3", "commit_rev":
"a1319a867978d9700653ecc5a6592d24b6367a2c"}

[commit-bot: I haz the power, 2017-01-18 22:30:38]

Description was changed from

==========
[IndexedDB] Adding transaction and value support to observers

BUG=662248,662246,609934
R=pwnall@chromium.org,jsbell@chromium.org,cmumford@chromium.org
==========

to

==========
[IndexedDB] Adding transaction and value support to observers

BUG=662248,662246,609934
R=pwnall@chromium.org,jsbell@chromium.org,cmumford@chromium.org

Review-Url: https://codereview.chromium.org/2601983002
Cr-Commit-Position: refs/heads/master@{#444514}
Committed:
https://chromium.googlesource.com/chromium/src/+/a1319a867978d9700653ecc5a659...
==========

[commit-bot: I haz the power, 2017-01-18 22:30:40]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/a1319a867978d9700653ecc5a659...