[IndexedDB] Refactoring to remove ref ptrs and host transaction ids.

[IndexedDB] Refactoring to remove ref ptrs and host transaction ids.

R=cmumford
BUG=457449, 662246
Committed: https://crrev.com/9d00e05d1070176eb8a13f7b83d73f067946f958
Cr-Commit-Position: refs/heads/master@{#435749}

[dmurph, 2016-11-04 00:42:02]

Hey Chris!

This is a refactoring CL to make the transaction ids no longer have a
host-component. I also make the transaction lifetime managed by the connection
object (which is so much cleaner).

There's a lot more cleanup here that can be done, but I wanted to minimize extra
changes in favor of getting the lifetime changes in.

Next patch will update unittests (I've only been running layout tests - and they
all pass)

PTAL!
Thanks :)

Daniel

[dmurph, 2016-11-04 01:15:16]

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

[commit-bot: I haz the power, 2016-11-04 01:16:05]

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, 2016-11-04 01:25:40]

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

[commit-bot: I haz the power, 2016-11-04 01:25:41]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jsbell, 2016-11-04 17:48:17]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2016-11-04 17:48:19]

Some notes from a quick skim. I didn't thoroughly review, though.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:4365:
IndexedDBBackingStore::Transaction* transaction_ptr_;
Nit: comment that this is only for ASYNC_TRACE macros.

(Can we make it void* to make it obvious this is not to be used for anything
real?)

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

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.cc:124:
DCHECK(transactions_.find(transaction->id()) == transactions_.end());
base::ContainsKey

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

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:83: const int
child_process_id_;
Can you add a comment about why we track the process id?

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:83: const int
child_process_id_;
Can this be base::ProcessId rather than int?

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:87:
std::unordered_map<int64_t, std::unique_ptr<IndexedDBTransaction>>
Is there a good reason to use unordered_map vs. map? This means the order of
aborts will be non-deterministic; does that matter?

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:87:
std::unordered_map<int64_t, std::unique_ptr<IndexedDBTransaction>>
Can you add comments to members as you touch them?

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:437: while
(!iterator.IsAtEnd()) {
We can't use range-based for here? :(

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:441:
connection->database()->Abort(
As an aside (maybe a TODO?) - maybe we want to copy a pattern used in other
storage backends where various types (backing store, database) get a "doomed"
flag, set when we're rudely terminating connections, which e.g. prevents
aborting a TX from starting the next TX in line.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_transaction.h (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.h:169: url::Origin origin_;
Can we replace origin_ with connection_->origin() ? Storing this per-TX seems
redundant.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_transaction_coordinator.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction_coordinator.cc:71:
result.push_back(transaction);
Now that both types are raw pointers can this be result.insert(result.end(),
started_transactions_.begin(), started_transactions_.end()) ? Or does the const
prevent that?

[dmurph, 2016-11-04 22:51:38]

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

[dmurph, 2016-11-04 22:52:25]

Thanks for the comments Josh!

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:4365:
IndexedDBBackingStore::Transaction* transaction_ptr_;
On 2016/11/04 17:48:19, jsbell wrote:
> Nit: comment that this is only for ASYNC_TRACE macros.
> 
> (Can we make it void* to make it obvious this is not to be used for anything
> real?)

Done.

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

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.cc:124:
DCHECK(transactions_.find(transaction->id()) == transactions_.end());
On 2016/11/04 17:48:19, jsbell wrote:
> base::ContainsKey

Done.

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

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:83: const int
child_process_id_;
On 2016/11/04 17:48:19, jsbell wrote:
> Can this be base::ProcessId rather than int?

Well, we're given an 'int' down the whole stack, so I was just using that.
(starting from the constructor of IndexedDBDispatcherHost, where the
RenderProcessHostImpl only gives an int.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:83: const int
child_process_id_;
On 2016/11/04 17:48:19, jsbell wrote:
> Can you add a comment about why we track the process id?

Done.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:87:
std::unordered_map<int64_t, std::unique_ptr<IndexedDBTransaction>>
On 2016/11/04 17:48:19, jsbell wrote:
> Is there a good reason to use unordered_map vs. map? This means the order of
> aborts will be non-deterministic; does that matter?
>  

Huh. I don't know. I don't think so.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:87:
std::unordered_map<int64_t, std::unique_ptr<IndexedDBTransaction>>
On 2016/11/04 17:48:19, jsbell wrote:
> Can you add comments to members as you touch them?

Done.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:437: while
(!iterator.IsAtEnd()) {
On 2016/11/04 17:48:19, jsbell wrote:
> We can't use range-based for here? :(

We got rid of the extra map, and this map doesn't let us do range-based :(

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:441:
connection->database()->Abort(
On 2016/11/04 17:48:19, jsbell wrote:
> As an aside (maybe a TODO?) - maybe we want to copy a pattern used in other
> storage backends where various types (backing store, database) get a "doomed"
> flag, set when we're rudely terminating connections, which e.g. prevents
> aborting a TX from starting the next TX in line.

Acknowledged.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_transaction.h (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.h:169: url::Origin origin_;
On 2016/11/04 17:48:19, jsbell wrote:
> Can we replace origin_ with connection_->origin() ? Storing this per-TX seems
> redundant.

Done.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_transaction_coordinator.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction_coordinator.cc:71:
result.push_back(transaction);
On 2016/11/04 17:48:19, jsbell wrote:
> Now that both types are raw pointers can this be result.insert(result.end(),
> started_transactions_.begin(), started_transactions_.end()) ? Or does the
const
> prevent that?

Done.

[commit-bot: I haz the power, 2016-11-04 22:52:26]

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, 2016-11-04 22:57:10]

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

[commit-bot: I haz the power, 2016-11-04 22:57:11]

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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[cmumford, 2016-11-04 23:33:13]

Lookin' good.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:4364:
base::WeakPtr<IndexedDBBackingStore::Transaction> transaction_;
What about the other places in this file where transaction_ is used w/o checking
for null. If not required then adding a comment, and DCHECK, would help.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:4365:
IndexedDBBackingStore::Transaction* transaction_ptr_;
Also, maybe make it:

  const IndexedDBBackingStore::Transaction* const transaction_ptr_;

To deter use by a future maintainer.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:4395: void
IndexedDBBackingStore::Transaction::Rollback() {
Not your change, but on line 4407 you can simplify:

  if (transaction_.get() == NULL)

to:

  if (!transaction_)

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

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.cc:136:
std::unordered_map<int64_t, std::unique_ptr<IndexedDBTransaction>> temp_map;
Couldn't you also do this?

  const auto temp_map = std::move(transactions_);

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

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:77: transactions() {
make method const.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_cursor.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_cursor.cc:42:
callbacks->OnError(IndexedDBDatabaseError(
nit: IndexedDBDatabaseError is created three times with same data. A helper
function would simplify this code slightly.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database_unittest.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database_unittest.cc:372: connection_ =
base::MakeUnique<IndexedDBConnection>(kFakeChildProcessId,
If you switch to base::ProcessId can you also use base::kNullProcessId instead
of kFakeChildProcessId?

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:570:
transaction->set_origin(database_origin_map_[params.ipc_database_id]);
Why aren't we passing in the origin to CreateTransaction?

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.h (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.h:90: void
FinishTransaction(url::Origin transaction_origin, bool committed);
It looks like the Transaction from which this origin came is still OK. Can you
use:

  const url::Origin&

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory_unittest.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory_unittest.cc:436:
IndexedDBTransaction* transaction() { return transaction_; }
const

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

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.cc:198: // This destroys our
object.
Nit: avoid first person language. Maybe something like "EraseTransaction will
delete |this|".

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_transaction.h (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.h:49: IndexedDBTransaction(
The only reason this is public is so that base::WrapUnique can be used. Can you
return the constructor to the private section and manually create the
unique_ptr? This'll prevent future maintainers from bypassing
IndexedDBConnection::CreateTransaction() by accident.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_transaction_coordinator.h (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction_coordinator.h:13: #include
<unordered_map>
Unused.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/mock_indexed_db_database_callbacks.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/mock_indexed_db_database_callbacks.cc:19: 
If I'm correct, the only time this is called is as so:

 OnAbort(*this, ...)

Why not just make this accept a pointer?

[jsbell, 2016-11-05 00:01:48]

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

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:83: const int
child_process_id_;
On 2016/11/04 22:52:24, dmurph wrote:
> On 2016/11/04 17:48:19, jsbell wrote:
> > Can this be base::ProcessId rather than int?
> 
> Well, we're given an 'int' down the whole stack, so I was just using that.
> (starting from the constructor of IndexedDBDispatcherHost, where the
> RenderProcessHostImpl only gives an int.

If it's given to us as an `int` that's fine (I didn't trace it back that far).
It's just odd to see a plain `int` these days. :)

[dmurph, 2016-11-07 20:03:28]

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

[commit-bot: I haz the power, 2016-11-07 20:04:03]

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

[dmurph, 2016-11-07 20:05:23]

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:4364:
base::WeakPtr<IndexedDBBackingStore::Transaction> transaction_;
On 2016/11/04 23:33:12, cmumford wrote:
> What about the other places in this file where transaction_ is used w/o
checking
> for null. If not required then adding a comment, and DCHECK, would help.

I'm not quite sure what you mean here. In this specific case, we have a task
scheduled that's separate from the regular lifecycle, and since the Transaction*
is owned by the IDBTransaction, this can be destructed.

So it looks like we have a Transaction* in Cursor, is that what you mean? That
doesn't have weakptrs but I can add a factory. BUT we already guarentee we don't
touch cursor operations if the transaction is dead in the dispatcher host.

So what do you prefer here?

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:4365:
IndexedDBBackingStore::Transaction* transaction_ptr_;
On 2016/11/04 23:33:12, cmumford wrote:
> Also, maybe make it:
> 
>   const IndexedDBBackingStore::Transaction* const transaction_ptr_;
> 
> To deter use by a future maintainer.

Done.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:4395: void
IndexedDBBackingStore::Transaction::Rollback() {
On 2016/11/04 23:33:12, cmumford wrote:
> Not your change, but on line 4407 you can simplify:
> 
>   if (transaction_.get() == NULL)
> 
> to:
> 
>   if (!transaction_)

Done.

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

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.cc:136:
std::unordered_map<int64_t, std::unique_ptr<IndexedDBTransaction>> temp_map;
On 2016/11/04 23:33:12, cmumford wrote:
> Couldn't you also do this?
> 
>   const auto temp_map = std::move(transactions_);

I could, but that leaves transactions_ in a technically undefined state. It
probably will behave correctly but I think we want to avoid using anything other
than a smart pointer after a std::move, and I don't want to introduce that
constraint into this class -- "don't use 'transactions_' after calling this
method" - It's cleaner to me to just have it be empty w/ a standard method.

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

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:77: transactions() {
On 2016/11/04 23:33:12, cmumford wrote:
> make method const.

Done.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_cursor.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_cursor.cc:42:
callbacks->OnError(IndexedDBDatabaseError(
On 2016/11/04 23:33:12, cmumford wrote:
> nit: IndexedDBDatabaseError is created three times with same data. A helper
> function would simplify this code slightly.

Done.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database_unittest.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database_unittest.cc:372: connection_ =
base::MakeUnique<IndexedDBConnection>(kFakeChildProcessId,
On 2016/11/04 23:33:12, cmumford wrote:
> If you switch to base::ProcessId can you also use base::kNullProcessId instead
> of kFakeChildProcessId?

Can't due to population stack (it's given as int).

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:570:
transaction->set_origin(database_origin_map_[params.ipc_database_id]);
On 2016/11/04 23:33:12, cmumford wrote:
> Why aren't we passing in the origin to CreateTransaction?

No longer needed now that we store it on the connection.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.h (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.h:90: void
FinishTransaction(url::Origin transaction_origin, bool committed);
On 2016/11/04 23:33:12, cmumford wrote:
> It looks like the Transaction from which this origin came is still OK. Can you
> use:
> 
>   const url::Origin&

Done.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory_unittest.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory_unittest.cc:436:
IndexedDBTransaction* transaction() { return transaction_; }
On 2016/11/04 23:33:12, cmumford wrote:
> const

Done.

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

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.cc:198: // This destroys our
object.
On 2016/11/04 23:33:13, cmumford wrote:
> Nit: avoid first person language. Maybe something like "EraseTransaction will
> delete |this|".

Done.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_transaction.h (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.h:49: IndexedDBTransaction(
On 2016/11/04 23:33:13, cmumford wrote:
> The only reason this is public is so that base::WrapUnique can be used. Can
you
> return the constructor to the private section and manually create the
> unique_ptr? This'll prevent future maintainers from bypassing
> IndexedDBConnection::CreateTransaction() by accident.

Done.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_transaction_coordinator.h (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction_coordinator.h:13: #include
<unordered_map>
On 2016/11/04 23:33:13, cmumford wrote:
> Unused.

Done.

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
File content/browser/indexed_db/mock_indexed_db_database_callbacks.cc (right):

https://codereview.chromium.org/2472213003/diff/20001/content/browser/indexed...
content/browser/indexed_db/mock_indexed_db_database_callbacks.cc:19: 
On 2016/11/04 23:33:13, cmumford wrote:
> If I'm correct, the only time this is called is as so:
> 
>  OnAbort(*this, ...)
> 
> Why not just make this accept a pointer?

There's no reason that the callbacks need to modify the transaction. I'm
defaulting to the most constrained method declaration that we need to prevent
unwanted use. const ref unless we explicitly need modification.

[commit-bot: I haz the power, 2016-11-07 20:07:35]

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

[commit-bot: I haz the power, 2016-11-07 20:07:36]

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

[jsbell, 2016-11-08 00:42:53]

more drive-by comments...

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

https://codereview.chromium.org/2472213003/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:79: // below.
above?

https://codereview.chromium.org/2472213003/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:94: // The origin that this
connection was created from.
Now that I think about it, this can be derived from:

database_->identifier().first

So... do we need to hold on to it in IndexedDBConnection, or should we replace
it with an accessor on IndexedDBDatabase?

(sorry, I know I just suggested moving it up from IndexedDBTransaction. :P)

https://codereview.chromium.org/2472213003/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2472213003/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:369: void
IndexedDBDispatcherHost::FinishTransaction(
The only purpose of this method now is to poke the quota system via the
contextimpl. We could possibly rename this, and even only call if we committed
to simplify, e.g. make it NotifyTransactionCommitted(origin)

https://codereview.chromium.org/2472213003/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:895:
parent_->context()->TaskRunner(), GURL(connection->origin().Serialize()),
This can be replaced by connection->origin().GetURL() now (recently added).
(Should probably do that in a separate CL, I guess.)

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

https://codereview.chromium.org/2472213003/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.cc:86: size_(0),
Initialize in the header file?

(I guess consistency is more important; other simple initializers could move
too)

[dmurph, 2016-11-09 00:26:34]

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

[dmurph, 2016-11-09 00:27:32]

Rebased w/ Reilly's patch. +cc reillyg as an FYI

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

https://codereview.chromium.org/2472213003/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:79: // below.
On 2016/11/08 00:42:52, jsbell wrote:
> above?

Clarified.

https://codereview.chromium.org/2472213003/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_connection.h:94: // The origin that this
connection was created from.
On 2016/11/08 00:42:52, jsbell wrote:
> Now that I think about it, this can be derived from:
> 
> database_->identifier().first
> 
> So... do we need to hold on to it in IndexedDBConnection, or should we replace
> it with an accessor on IndexedDBDatabase?
> 
> (sorry, I know I just suggested moving it up from IndexedDBTransaction. :P)

Done.

https://codereview.chromium.org/2472213003/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2472213003/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:369: void
IndexedDBDispatcherHost::FinishTransaction(
On 2016/11/08 00:42:52, jsbell wrote:
> The only purpose of this method now is to poke the quota system via the
> contextimpl. We could possibly rename this, and even only call if we committed
> to simplify, e.g. make it NotifyTransactionCommitted(origin)
> 

Done.

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

https://codereview.chromium.org/2472213003/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_transaction.cc:86: size_(0),
On 2016/11/08 00:42:52, jsbell wrote:
> Initialize in the header file?
> 
> (I guess consistency is more important; other simple initializers could move
> too)

Done.

[commit-bot: I haz the power, 2016-11-09 00:27:33]

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

[dmurph, 2016-11-09 00:28:20]

+Reilly as an FYI

[Reilly Grant (use Gerrit), 2016-11-09 00:40:20]

reillyg@chromium.org changed reviewers:
+ reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-11-09 00:40:21]

Thanks for the FYI. It looks like we might want to make transactions Mojo
objects at some point in the future.

[chromium-reviews, 2016-11-09 00:43:06]

Possibly - There might be a need to keep them alive a little bit if the
renderer dies to complete pending transactions - especially with the
observers feature - so I'm not sure if mojo will facilitate that.

On Tue, Nov 8, 2016 at 4:40 PM <reillyg@chromium.org> wrote:

> Thanks for the FYI. It looks like we might want to make transactions Mojo
> objects at some point in the future.
>
> https://codereview.chromium.org/2472213003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-11-09 00:44:40]

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

[commit-bot: I haz the power, 2016-11-09 00:44:41]

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

[Reilly Grant (use Gerrit), 2016-11-09 00:45:33]

On 2016/11/09 at 00:43:06, chromium-reviews wrote:
> Possibly - There might be a need to keep them alive a little bit if the
> renderer dies to complete pending transactions - especially with the
> observers feature - so I'm not sure if mojo will facilitate that.

You can have an object bound to a Mojo pipe that continues to exist after the
pipe is closed. The pipe closure could just kick off the abort operation and
free the object when that is done.

[dmurph, 2016-11-09 00:58:43]

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

[commit-bot: I haz the power, 2016-11-09 00:59: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, 2016-11-09 01:21:27]

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

[commit-bot: I haz the power, 2016-11-09 01:21:28]

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

[dmurph, 2016-11-09 18:11:21]

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

[commit-bot: I haz the power, 2016-11-09 18:11:53]

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, 2016-11-09 18:38:51]

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

[commit-bot: I haz the power, 2016-11-09 18:38:52]

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

[dmurph, 2016-11-09 19:28:09]

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

[commit-bot: I haz the power, 2016-11-09 19:28:30]

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

[dmurph, 2016-11-09 19:35:29]

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

[commit-bot: I haz the power, 2016-11-09 19:35:52]

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, 2016-11-09 20:02:11]

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

[commit-bot: I haz the power, 2016-11-09 20:02:12]

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

[dmurph, 2016-11-10 21:03:24]

FYI - it looks like I need to do a refactor before this change to cleanup the
error pathways for transaction operations. This should clean up a bunch of code
and actually fix bugs. Hooray! So look for that patch in a bit.

[dmurph, 2016-11-10 21:03:25]

FYI - it looks like I need to do a refactor before this change to cleanup the
error pathways for transaction operations. This should clean up a bunch of code
and actually fix bugs. Hooray! So look for that patch in a bit.

[dmurph, 2016-11-29 23:28:05]

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

[dmurph, 2016-11-29 23:28:47]

@cmumford, @jsbell can you please take a look?

This is rebased and I fixed some error handling from the previous patch.

[commit-bot: I haz the power, 2016-11-29 23:29: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, 2016-11-30 01:31:45]

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

[commit-bot: I haz the power, 2016-11-30 01:31:46]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Reilly Grant (use Gerrit), 2016-11-30 16:32:26]

The rebase on top of my changes lgtm. Thanks for removing size tracking from
IndexedDBDispatcherHost.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:241:
callbacks->set_host_transaction_id(transaction_id);
This isn't necessary anymore now that OnUpgradeNeeded gets a reference to the
transaction.

[jsbell, 2016-11-30 21:30:14]

overall lgtm

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/cursor_impl.h (right):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/cursor_impl.h:8: #include "base/memory/ref_counted.h"
nit: include <memory> too

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/database_impl.cc (left):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/database_impl.cc:492: DLOG(ERROR) << "Duplicate
host_transaction_id.";
Can you add the extra logging details into the DCHECK in
IndexedDBConnection::CreateTransaction ?

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:2271:
callback_->Run(false, false);
Comment these args, e.g. /* name */

Or maybe consider adding enums. callback_->Run(BlobWriteCallback::kFailed,
BlobWriteCallback::kAsynchronous) would be more readable.

(I don't feel strongly re: comment vs. enum)

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:2293:
callback_->Run(true, false);
ditto

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:2297:
callback_->Run(false, false);
ditto

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:4339: leveldb::Status
result = callback_->Run(succeeded, false);
sync_call ?

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

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:73:
base::WeakPtr<IndexedDBTransaction> StoreTransactionForTesting(
AddTransactionForTesting ?

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:79: void
EraseTransaction(int64_t id);
RemoveTransaction ?

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:81: const
std::unordered_map<int64_t, std::unique_ptr<IndexedDBTransaction>>&
Since this complex type appears twice, add a using TransactionMap = ... ?

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:82: transactions() const {
nit: single line? (but whatever clang format prefers)

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:95: // The connection owns
transactions created on this connection.
nit: blank line before

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_cursor.cc (right):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_cursor.cc:23: return
IndexedDBDatabaseError(blink::WebIDBDatabaseExceptionUnknownError,
Maybe add a comment that this should never be script visible: the cursor should
either be closed when it hits the end of the range (and script throws an error
before the call could be made), if the transaction has finished (ditto), or if
there's an incoming request from the front end but the transaction has aborted
on the back end; in that case the tx will already have sent an abort to the
request so this would be ignored.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_cursor.cc:36:
IndexedDBTransaction::Operation BindWeakOperation(
Add a comment on usage here.

As an alternative to this, InvokeOrSucceed could take
std::unique_ptr<base::WeakPtr<IndexedDBCursor>>

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

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:73: // The database will be
closed during this call. This should NOT be used in an
Databases don't have a "closed" state. Can you clarify?

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:80: if (status.IsCorruption())
{
nit: no need for {} here

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:1797: DCHECK_LE(0,
transaction_count_);
DCHECK_GT(transaction_count_, 0) ? Maybe I just think backwards.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database_callbacks.cc (right):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database_callbacks.cc:91:
dispatcher_host_->NotifyTransactionCommitted(
How about just calling dispatcher_host_->context()->TransactionComplete()
directly?

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_fake_backing_store.cc (right):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_fake_backing_store.cc:221:
callback->Run(true, true);
comments (or enum)

[dmurph, 2016-11-30 23:07:45]

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

[commit-bot: I haz the power, 2016-11-30 23:08:02]

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

[dmurph, 2016-11-30 23:12:25]

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

[commit-bot: I haz the power, 2016-11-30 23:12:48]

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

[dmurph, 2016-11-30 23:13:07]

I replaced the booleans with a single enum, which should be a little more
verbose.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/cursor_impl.h (right):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/cursor_impl.h:8: #include "base/memory/ref_counted.h"
On 2016/11/30 21:30:13, jsbell wrote:
> nit: include <memory> too

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/database_impl.cc (left):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/database_impl.cc:492: DLOG(ERROR) << "Duplicate
host_transaction_id.";
On 2016/11/30 21:30:13, jsbell wrote:
> Can you add the extra logging details into the DCHECK in
> IndexedDBConnection::CreateTransaction ?

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:2271:
callback_->Run(false, false);
On 2016/11/30 21:30:13, jsbell wrote:
> Comment these args, e.g. /* name */
> 
> Or maybe consider adding enums. callback_->Run(BlobWriteCallback::kFailed,
> BlobWriteCallback::kAsynchronous) would be more readable.
> 
> (I don't feel strongly re: comment vs. enum)

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:2293:
callback_->Run(true, false);
On 2016/11/30 21:30:13, jsbell wrote:
> ditto

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:2297:
callback_->Run(false, false);
On 2016/11/30 21:30:13, jsbell wrote:
> ditto

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:4339: leveldb::Status
result = callback_->Run(succeeded, false);
On 2016/11/30 21:30:13, jsbell wrote:
> sync_call ?

Done.

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

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:73:
base::WeakPtr<IndexedDBTransaction> StoreTransactionForTesting(
On 2016/11/30 21:30:13, jsbell wrote:
> AddTransactionForTesting ?

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:79: void
EraseTransaction(int64_t id);
On 2016/11/30 21:30:13, jsbell wrote:
> RemoveTransaction ?

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:82: transactions() const {
On 2016/11/30 21:30:14, jsbell wrote:
> nit: single line? (but whatever clang format prefers)

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:95: // The connection owns
transactions created on this connection.
On 2016/11/30 21:30:14, jsbell wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_cursor.cc (right):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_cursor.cc:23: return
IndexedDBDatabaseError(blink::WebIDBDatabaseExceptionUnknownError,
On 2016/11/30 21:30:14, jsbell wrote:
> Maybe add a comment that this should never be script visible: the cursor
should
> either be closed when it hits the end of the range (and script throws an error
> before the call could be made), if the transaction has finished (ditto), or if
> there's an incoming request from the front end but the transaction has aborted
> on the back end; in that case the tx will already have sent an abort to the
> request so this would be ignored.
> 

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_cursor.cc:36:
IndexedDBTransaction::Operation BindWeakOperation(
On 2016/11/30 21:30:14, jsbell wrote:
> Add a comment on usage here.
> 
> As an alternative to this, InvokeOrSucceed could take
> std::unique_ptr<base::WeakPtr<IndexedDBCursor>> 

Done.

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

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:73: // The database will be
closed during this call. This should NOT be used in an
On 2016/11/30 21:30:14, jsbell wrote:
> Databases don't have a "closed" state. Can you clarify? 

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:80: if (status.IsCorruption())
{
On 2016/11/30 21:30:14, jsbell wrote:
> nit: no need for {} here

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:1797: DCHECK_LE(0,
transaction_count_);
On 2016/11/30 21:30:14, jsbell wrote:
> DCHECK_GT(transaction_count_, 0) ? Maybe I just think backwards.

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database_callbacks.cc (right):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database_callbacks.cc:91:
dispatcher_host_->NotifyTransactionCommitted(
On 2016/11/30 21:30:14, jsbell wrote:
> How about just calling dispatcher_host_->context()->TransactionComplete()
> directly?

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:241:
callbacks->set_host_transaction_id(transaction_id);
On 2016/11/30 16:32:26, Reilly Grant wrote:
> This isn't necessary anymore now that OnUpgradeNeeded gets a reference to the
> transaction.

Done.

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_fake_backing_store.cc (right):

https://codereview.chromium.org/2472213003/diff/180001/content/browser/indexe...
content/browser/indexed_db/indexed_db_fake_backing_store.cc:221:
callback->Run(true, true);
On 2016/11/30 21:30:14, jsbell wrote:
> comments (or enum)

Done.

[commit-bot: I haz the power, 2016-11-30 23:56:10]

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

[commit-bot: I haz the power, 2016-11-30 23:56:11]

Dry run: This issue passed the CQ dry run.

[cmumford, 2016-12-01 19:14:51]

Nothing major - all small stuff.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:4341: case
IndexedDBBackingStore::BlobWriteResult::FAIILURE_ASYNC:
Since this is a nested class inside of IndexedDBBackingStore you can remove the
IndexedDBBackingStore:: namespace prefix to make a bit simpler.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.h:95: FAIILURE_ASYNC,
You have two I's in FAIILURE_ASYNC.

Also clang-format wants this to be a single line - I don't care much, so your
call on this.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store_unittest.cc (right):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:380:
switch(result) {
Add space before (

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:384: case
IndexedDBBackingStore::BlobWriteResult::SUCCESS_ASYNC:
Nit: Could replace the switch with:

  succeeded = (result !=
IndexedDBBackingStore::BlobWriteResult::FAIILURE_ASYNC);

Your call if you prefer as-is.

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

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:216: IndexedDBTransaction*
transaction,
transaction unused in this function.

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

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:16: #include "url/origin.h"
origin.h is unused, but you can include <set>.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:74: IndexedDBTransaction*
GetTransaction(int64_t id);
Function should be const.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:83: const TransactionMap&
transactions() const { return transactions_; }
transactions() is unused.

Also, now that there is only place where TransactionMap is used (transactions_)
consider deleting that typedef. Your call - I'm an acknowledged typedef bigot
;-)

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_cursor.cc (left):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_cursor.cc:242: void
IndexedDBCursor::Close() {
Any reason to move Close()? git-blame works better if it's left at previous
location.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_cursor.h (left):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_cursor.h:70: // Must be destroyed before
transaction_.
Should Close() assert that cursor_ is null?

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database.cc (left):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:878: IndexedDBTransaction*
transaction = GetTransaction(transaction_id);
It looks like callers (like IDBThreadHelper) already check for a null
transaction. Should we define a contract and either delete those other checks,
or turn these into DCHECK's?

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

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:1418: ReportError(s, origin(),
factory_.get(), error);
Why can't we still use backing_store_->origin()?

All other calls to ReportError in this file also pass in the default origin.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:23: #include
"content/browser/indexed_db/indexed_db_transaction.h"
indexed_db_transaction.h not needed.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_pending_connection.h (right):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_pending_connection.h:15: #include
"url/origin.h"
Don't see where url/origin.h is needed.

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

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_transaction.cc:388: // EraseTransaction
will delete |this|.
s/EraseTransaction/RemoveTransaction/

down below too.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_transaction.h (left):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_transaction.h:25: #include
"third_party/WebKit/public/platform/modules/indexeddb/WebIDBTypes.h"
origin.h needed?

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_transaction_unittest.cc (right):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_transaction_unittest.cc:452:
EXPECT_TRUE(transaction);
ASSERT_TRUE to avoid test crash if transaction is null.

[dmurph, 2016-12-01 21:11:45]

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

[dmurph, 2016-12-01 21:12:24]

Thanks for the review! I only had one question - which origin() do you want me
to use? There's one in the identifier in indexed_db_database, and one in the
backing store.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:4341: case
IndexedDBBackingStore::BlobWriteResult::FAIILURE_ASYNC:
On 2016/12/01 19:14:50, cmumford wrote:
> Since this is a nested class inside of IndexedDBBackingStore you can remove
the
> IndexedDBBackingStore:: namespace prefix to make a bit simpler.

Done.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.h:95: FAIILURE_ASYNC,
On 2016/12/01 19:14:50, cmumford wrote:
> You have two I's in FAIILURE_ASYNC.
> 
> Also clang-format wants this to be a single line - I don't care much, so your
> call on this.

Done. I just upload whatever 'git cl format' does.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store_unittest.cc (right):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:380:
switch(result) {
On 2016/12/01 19:14:51, cmumford wrote:
> Add space before (

Done.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:384: case
IndexedDBBackingStore::BlobWriteResult::SUCCESS_ASYNC:
On 2016/12/01 19:14:51, cmumford wrote:
> Nit: Could replace the switch with:
> 
>   succeeded = (result !=
> IndexedDBBackingStore::BlobWriteResult::FAIILURE_ASYNC);
> 
> Your call if you prefer as-is.

I'd rather have the switch statement so that it fails to compile if anyone
changes the enum.

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

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:216: IndexedDBTransaction*
transaction,
On 2016/12/01 19:14:51, cmumford wrote:
> transaction unused in this function.

Done.

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

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:16: #include "url/origin.h"
On 2016/12/01 19:14:51, cmumford wrote:
> origin.h is unused, but you can include <set>.

Done.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:74: IndexedDBTransaction*
GetTransaction(int64_t id);
On 2016/12/01 19:14:51, cmumford wrote:
> Function should be const.

Done.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_connection.h:83: const TransactionMap&
transactions() const { return transactions_; }
On 2016/12/01 19:14:51, cmumford wrote:
> transactions() is unused.
> 
> Also, now that there is only place where TransactionMap is used
(transactions_)
> consider deleting that typedef. Your call - I'm an acknowledged typedef bigot
> ;-)

haha ok done ;)

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_cursor.cc (left):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_cursor.cc:242: void
IndexedDBCursor::Close() {
On 2016/12/01 19:14:51, cmumford wrote:
> Any reason to move Close()? git-blame works better if it's left at previous
> location.

Just trying to put it in header order. Moved back.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_cursor.h (left):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_cursor.h:70: // Must be destroyed before
transaction_.
On 2016/12/01 19:14:51, cmumford wrote:
> Should Close() assert that cursor_ is null?

No, as the IndexedDBTransaction calls it in CloseAllCursors(). So we need it to
reset the cursor in that case.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database.cc (left):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:878: IndexedDBTransaction*
transaction = GetTransaction(transaction_id);
On 2016/12/01 19:14:51, cmumford wrote:
> It looks like callers (like IDBThreadHelper) already check for a null
> transaction. Should we define a contract and either delete those other checks,
> or turn these into DCHECK's?

I'll do DCHECKs everywhere. It's a safe assumption that a pointer is not null.

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

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:1418: ReportError(s, origin(),
factory_.get(), error);
On 2016/12/01 19:14:51, cmumford wrote:
> Why can't we still use backing_store_->origin()?
> 
> All other calls to ReportError in this file also pass in the default origin.

... aren't they the same? Which would you rather me use?

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:23: #include
"content/browser/indexed_db/indexed_db_transaction.h"
On 2016/12/01 19:14:51, cmumford wrote:
> indexed_db_transaction.h not needed.

Done.

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

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_transaction.cc:388: // EraseTransaction
will delete |this|.
On 2016/12/01 19:14:51, cmumford wrote:
> s/EraseTransaction/RemoveTransaction/
> 
> down below too.

Done.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_transaction.h (left):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_transaction.h:25: #include
"third_party/WebKit/public/platform/modules/indexeddb/WebIDBTypes.h"
On 2016/12/01 19:14:51, cmumford wrote:
> origin.h needed?

Done.

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_transaction_unittest.cc (right):

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_transaction_unittest.cc:452:
EXPECT_TRUE(transaction);
On 2016/12/01 19:14:51, cmumford wrote:
> ASSERT_TRUE to avoid test crash if transaction is null.

Done.

[commit-bot: I haz the power, 2016-12-01 21:12:25]

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

[cmumford, 2016-12-01 22:25:45]

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

https://codereview.chromium.org/2472213003/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:1418: ReportError(s, origin(),
factory_.get(), error);
On 2016/12/01 21:12:23, dmurph wrote:
> On 2016/12/01 19:14:51, cmumford wrote:
> > Why can't we still use backing_store_->origin()?
> > 
> > All other calls to ReportError in this file also pass in the default origin.
> 
> ... aren't they the same? Which would you rather me use?

If the other callers to ReportError have no origin, then I'm certainly OK with
passing in origin(), but at least this function should pass in
backing_store_->origin() - or are you saying that's also a default value?

[commit-bot: I haz the power, 2016-12-01 22:46:32]

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

[commit-bot: I haz the power, 2016-12-01 22:46:33]

Dry run: This issue passed the CQ dry run.

[cmumford, 2016-12-01 22:54:15]

lgtm

[dmurph, 2016-12-01 22:55:29]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2016-12-01 22:55:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/2472213003/#ps240001
(title: "comments")

[commit-bot: I haz the power, 2016-12-01 22:55:48]

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, 2016-12-01 23:00:38]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1480632929005390,
"parent_rev": "f4058fa238343622913206abfec8a2269c69a12a", "commit_rev":
"f20510c77a8712093b5601c53afaf98d196d1d75"}

[commit-bot: I haz the power, 2016-12-01 23:01:10]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-12-01 23:03:50]

Description was changed from

==========
[IndexedDB] Refactoring to remove ref ptrs and host transaction ids.

R=cmumford
BUG=457449,662246
==========

to

==========
[IndexedDB] Refactoring to remove ref ptrs and host transaction ids.

R=cmumford
BUG=457449,662246

Committed: https://crrev.com/9d00e05d1070176eb8a13f7b83d73f067946f958
Cr-Commit-Position: refs/heads/master@{#435749}
==========

[commit-bot: I haz the power, 2016-12-01 23:03:52]

Patchset 13 (id:??) landed as
https://crrev.com/9d00e05d1070176eb8a13f7b83d73f067946f958
Cr-Commit-Position: refs/heads/master@{#435749}

[sof, 2016-12-05 20:00:18]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-12-05 20:00:20]

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

https://codereview.chromium.org/2472213003/diff/240001/content/browser/indexe...
content/browser/indexed_db/indexed_db_transaction_coordinator.cc:69:
result.insert(result.end(), started_transactions_.begin(),
Somewhat odd, but I'm running into compilation breakage with the use of these
list_set<> iterators in insert():

In file included from
../../content/browser/indexed_db/indexed_db_transaction_coordinator.cc:5:
In file included from
../../content/browser/indexed_db/indexed_db_transaction_coordinator.h:10:
In file included from
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/map:60:
In file included from
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/bits/stl_tree.h:63:
In file included from
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/bits/stl_algobase.h:69:
In file included from
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/debug.h:138:
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/functions.h:137:61:
error: indirection requires pointer operand ('const
list_set<content::IndexedDBTransaction *>::const_iterator' invalid)
      return __foreign_iterator_aux4(__it, std::__addressof(*__other));
                                                            ^~~~~~~~
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/functions.h:181:14:
note: in instantiation of function template specialization
'__gnu_debug::__foreign_iterator_aux3<__gnu_cxx::__normal_iterator<const
content::IndexedDBTransaction *const *, std::__cxx1998::vector<const
content::IndexedDBTransaction *, std::allocator<const
content::IndexedDBTransaction *> > >, std::__debug::vector<const
content::IndexedDBTransaction *, std::allocator<const
content::IndexedDBTransaction *> >, list_set<content::IndexedDBTransaction
*>::const_iterator>' requested here
      return __foreign_iterator_aux3(__it, __other, __other_end, __tag());
             ^
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/functions.h:201:5:
note: in instantiation of function template specialization
'__gnu_debug::__foreign_iterator_aux2<__gnu_cxx::__normal_iterator<const
content::IndexedDBTransaction *const *, std::__cxx1998::vector<const
content::IndexedDBTransaction *, std::allocator<const
content::IndexedDBTransaction *> > >, std::__debug::vector<const
content::IndexedDBTransaction *, std::allocator<const
content::IndexedDBTransaction *> >, list_set<content::IndexedDBTransaction
*>::const_iterator>' requested here
        || __foreign_iterator_aux2(__it, std::__miter_base(__other),
           ^
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/functions.h:212:14:
note: in instantiation of function template specialization
'__gnu_debug::__foreign_iterator_aux<__gnu_cxx::__normal_iterator<const
content::IndexedDBTransaction *const *, std::__cxx1998::vector<const
content::IndexedDBTransaction *, std::allocator<const
content::IndexedDBTransaction *> > >, std::__debug::vector<const
content::IndexedDBTransaction *, std::allocator<const
content::IndexedDBTransaction *> >, list_set<content::IndexedDBTransaction
*>::const_iterator>' requested here
      return __foreign_iterator_aux(__it, __other, __other_end, _Integral());
             ^
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/vector:587:4:
note: in instantiation of function template specialization
'__gnu_debug::__foreign_iterator<__gnu_cxx::__normal_iterator<const
content::IndexedDBTransaction *const *, std::__cxx1998::vector<const
content::IndexedDBTransaction *, std::allocator<const
content::IndexedDBTransaction *> > >, std::__debug::vector<const
content::IndexedDBTransaction *, std::allocator<const
content::IndexedDBTransaction *> >, list_set<content::IndexedDBTransaction
*>::const_iterator>' requested here
          __glibcxx_check_insert_range(__position, __first, __last, __dist);
          ^
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/macros.h:116:36:
note: expanded from macro '__glibcxx_check_insert_range'
_GLIBCXX_DEBUG_VERIFY(__gnu_debug::__foreign_iterator(_Position,_First,_Last),
                                   ^
../../content/browser/indexed_db/indexed_db_transaction_coordinator.cc:69:10:
note: in instantiation of function template specialization
'std::__debug::vector<const content::IndexedDBTransaction *,
std::allocator<const content::IndexedDBTransaction *>
>::insert<list_set<content::IndexedDBTransaction *>::const_iterator, void>'
requested here
  result.insert(result.end(), started_transactions_.begin(),
         ^
1 error generated.
ninja: build stopped: subcommand failed.

Same happens with the g++ 4.9 STL headers on a different workstation (both with
the default is_clang=true) -- no one else has reported similar issues and/or
known cause?

[jsbell, 2016-12-06 00:00:47]

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

https://codereview.chromium.org/2472213003/diff/240001/content/browser/indexe...
content/browser/indexed_db/indexed_db_transaction_coordinator.cc:69:
result.insert(result.end(), started_transactions_.begin(),
On 2016/12/05 20:00:20, sof wrote:
> Somewhat odd, but I'm running into compilation breakage with the use of these
> list_set<> iterators in insert():
> 
> In file included from
> ../../content/browser/indexed_db/indexed_db_transaction_coordinator.cc:5:
> In file included from
> ../../content/browser/indexed_db/indexed_db_transaction_coordinator.h:10:
> In file included from
> /usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/map:60:
> In file included from
>
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/bits/stl_tree.h:63:
> In file included from
>
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/bits/stl_algobase.h:69:
> In file included from
>
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/debug.h:138:
>
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/functions.h:137:61:
> error: indirection requires pointer operand ('const
> list_set<content::IndexedDBTransaction *>::const_iterator' invalid)
>       return __foreign_iterator_aux4(__it, std::__addressof(*__other));
>                                                             ^~~~~~~~
>
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/functions.h:181:14:
> note: in instantiation of function template specialization
> '__gnu_debug::__foreign_iterator_aux3<__gnu_cxx::__normal_iterator<const
> content::IndexedDBTransaction *const *, std::__cxx1998::vector<const
> content::IndexedDBTransaction *, std::allocator<const
> content::IndexedDBTransaction *> > >, std::__debug::vector<const
> content::IndexedDBTransaction *, std::allocator<const
> content::IndexedDBTransaction *> >, list_set<content::IndexedDBTransaction
> *>::const_iterator>' requested here
>       return __foreign_iterator_aux3(__it, __other, __other_end, __tag());
>              ^
>
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/functions.h:201:5:
> note: in instantiation of function template specialization
> '__gnu_debug::__foreign_iterator_aux2<__gnu_cxx::__normal_iterator<const
> content::IndexedDBTransaction *const *, std::__cxx1998::vector<const
> content::IndexedDBTransaction *, std::allocator<const
> content::IndexedDBTransaction *> > >, std::__debug::vector<const
> content::IndexedDBTransaction *, std::allocator<const
> content::IndexedDBTransaction *> >, list_set<content::IndexedDBTransaction
> *>::const_iterator>' requested here
>         || __foreign_iterator_aux2(__it, std::__miter_base(__other),
>            ^
>
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/functions.h:212:14:
> note: in instantiation of function template specialization
> '__gnu_debug::__foreign_iterator_aux<__gnu_cxx::__normal_iterator<const
> content::IndexedDBTransaction *const *, std::__cxx1998::vector<const
> content::IndexedDBTransaction *, std::allocator<const
> content::IndexedDBTransaction *> > >, std::__debug::vector<const
> content::IndexedDBTransaction *, std::allocator<const
> content::IndexedDBTransaction *> >, list_set<content::IndexedDBTransaction
> *>::const_iterator>' requested here
>       return __foreign_iterator_aux(__it, __other, __other_end, _Integral());
>              ^
>
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/vector:587:4:
> note: in instantiation of function template specialization
> '__gnu_debug::__foreign_iterator<__gnu_cxx::__normal_iterator<const
> content::IndexedDBTransaction *const *, std::__cxx1998::vector<const
> content::IndexedDBTransaction *, std::allocator<const
> content::IndexedDBTransaction *> > >, std::__debug::vector<const
> content::IndexedDBTransaction *, std::allocator<const
> content::IndexedDBTransaction *> >, list_set<content::IndexedDBTransaction
> *>::const_iterator>' requested here
>           __glibcxx_check_insert_range(__position, __first, __last, __dist);
>           ^
>
/usr/lib/gcc/x86_64-linux-gnu/6.2.0/../../../../include/c++/6.2.0/debug/macros.h:116:36:
> note: expanded from macro '__glibcxx_check_insert_range'
> _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__foreign_iterator(_Position,_First,_Last),
>                                    ^
> ../../content/browser/indexed_db/indexed_db_transaction_coordinator.cc:69:10:
> note: in instantiation of function template specialization
> 'std::__debug::vector<const content::IndexedDBTransaction *,
> std::allocator<const content::IndexedDBTransaction *>
> >::insert<list_set<content::IndexedDBTransaction *>::const_iterator, void>'
> requested here
>   result.insert(result.end(), started_transactions_.begin(),
>          ^
> 1 error generated.
> ninja: build stopped: subcommand failed.
> 
> Same happens with the g++ 4.9 STL headers on a different workstation (both
with
> the default is_clang=true) -- no one else has reported similar issues and/or
> known cause?

Weird - I haven't heard any other reports. 

list_set isn't too fancy - fixes welcome!