IndexedDB: WrapUnique(new T(args..)) -> MakeUnique<T>(args...)

IndexedDB: WrapUnique(new T(args...)) -> MakeUnique<T>(args...)

Get on the base::MakeUnique bandwagon, both replacing
bad uses of WrapUnique and eliminating bare `new` invocations
Also change several factory methods to return unique_ptr or
scoped_refptr instead of raw pointers, and replace some
manually managed pointers in maps with smart pointers.

WrapUnique() is used in cases with private constructors.

R=cmumford@chromium.org

Committed: https://crrev.com/7017da4baf8aff83269ab72a34737143c6d9d96b
Cr-Commit-Position: refs/heads/master@{#412222}

[jsbell, 2016-08-10 21:16:41]

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

[jsbell, 2016-08-10 21:17:17]

Description was changed from

==========
IndexedDB: WrapUnique(new T(args)...) -> MakeUnique<T>(args...)

Get on the base::MakeUnique bandwagon.

R=cmumford@chromium.org
==========

to

==========
IndexedDB: WrapUnique(new T(args...)) -> MakeUnique<T>(args...)

Get on the base::MakeUnique bandwagon.

R=cmumford@chromium.org
==========

[jsbell, 2016-08-10 21:17:29]

cmumford@ - please take a look?

[commit-bot: I haz the power, 2016-08-10 21:17:53]

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

[cmumford, 2016-08-10 23:03:47]

There are some other places, like MakeIndexWriters, which are creating
unique_ptr's, but weren't using WrapUnique. Do you want to convert those in this
CL?

https://codereview.chromium.org/2233153002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/leveldb/leveldb_write_batch.cc (right):

https://codereview.chromium.org/2233153002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/leveldb/leveldb_write_batch.cc:15: return
base::WrapUnique(new LevelDBWriteBatch());
It's too bad MakeUnique can't handle private methods.

[commit-bot: I haz the power, 2016-08-11 00:03:43]

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

[commit-bot: I haz the power, 2016-08-11 00:03:44]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jsbell, 2016-08-11 23:02:56]

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

[commit-bot: I haz the power, 2016-08-11 23:04:06]

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-08-12 00:10:17]

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

[commit-bot: I haz the power, 2016-08-12 00:10:18]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-08-12 21:28:34]

Description was changed from

==========
IndexedDB: WrapUnique(new T(args...)) -> MakeUnique<T>(args...)

Get on the base::MakeUnique bandwagon.

R=cmumford@chromium.org
==========

to

==========
IndexedDB: WrapUnique(new T(args...)) -> MakeUnique<T>(args...)

Get on the base::MakeUnique bandwagon, both replacing
bad uses of WrapUnique and eliminating bare `new` invocations
Also change several factory methods to return unique_ptr or
scoped_refptr instead of raw pointers, and replace some 
manually managed pointers in maps with smart pointers.

R=cmumford@chromium.org
==========

[jsbell, 2016-08-12 21:29:54]

Description was changed from

==========
IndexedDB: WrapUnique(new T(args...)) -> MakeUnique<T>(args...)

Get on the base::MakeUnique bandwagon, both replacing
bad uses of WrapUnique and eliminating bare `new` invocations
Also change several factory methods to return unique_ptr or
scoped_refptr instead of raw pointers, and replace some 
manually managed pointers in maps with smart pointers.

R=cmumford@chromium.org
==========

to

==========
IndexedDB: WrapUnique(new T(args...)) -> MakeUnique<T>(args...)

Get on the base::MakeUnique bandwagon, both replacing
bad uses of WrapUnique and eliminating bare `new` invocations
Also change several factory methods to return unique_ptr or
scoped_refptr instead of raw pointers, and replace some 
manually managed pointers in maps with smart pointers.

WrapUnique() is used in cases with private constructors.

R=cmumford@chromium.org
==========

[jsbell, 2016-08-12 21:52:21]

cmumford@ - please take a look?

asan/lsan clean for content_unittests and content_browsertests

I filed https://crbug.com/637019 for one follow-up.

[cmumford, 2016-08-13 00:08:39]

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

https://codereview.chromium.org/2233153002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:1121:
std::unique_ptr<LevelDBComparator> comparator(base::MakeUnique<Comparator>());
I know this is nothing new in your CL, but comparator.get() is passed to
LevelDBDatabase::Open(), which is assigned to ComparatorAdapter::comparator_ as
a raw pointer. I'm not seeing how this is *not* a dangling pointer. Do you see
what I'm missing?

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

https://codereview.chromium.org/2233153002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.h:318: virtual
std::unique_ptr<Cursor> Clone() = 0;
Now that you're changing Clone's signature, why not make it a const method?

https://codereview.chromium.org/2233153002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right):

https://codereview.chromium.org/2233153002/diff/20001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:31: void
LevelDBTransaction::Clear() {
Now only used in LevelDBTransaction::Rollback(). Could simplify - but only if
you feel it's simpler.

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

https://codereview.chromium.org/2233153002/diff/20001/content/browser/indexed...
content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.h:49:
IndexedDBTransaction* CreateIndexedDBTransaction(
It'd be nice, in a future commit, to make this a smart pointer too.

[jsbell, 2016-08-16 03:05:05]

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

[jsbell, 2016-08-16 03:05:28]

Thanks - one more look cmumford@ ?

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

https://codereview.chromium.org/2233153002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:1121:
std::unique_ptr<LevelDBComparator> comparator(base::MakeUnique<Comparator>());
On 2016/08/13 00:08:38, cmumford wrote:
> I know this is nothing new in your CL, but comparator.get() is passed to
> LevelDBDatabase::Open(), which is assigned to ComparatorAdapter::comparator_
as
> a raw pointer. I'm not seeing how this is *not* a dangling pointer. Do you see
> what I'm missing?

The comparator is passed through (as a unique_ptr via std::move) into the
IndexedDBBackingStore constructor (via the Create call below), and the backing
store manages the lifetime of both the LevelDBComparator and the LevelDBDatabase
- see IndexedDBBackingStore's ctor/dtor.

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

https://codereview.chromium.org/2233153002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.h:318: virtual
std::unique_ptr<Cursor> Clone() = 0;
On 2016/08/13 00:08:38, cmumford wrote:
> Now that you're changing Clone's signature, why not make it a const method?

Done.

https://codereview.chromium.org/2233153002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right):

https://codereview.chromium.org/2233153002/diff/20001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:31: void
LevelDBTransaction::Clear() {
On 2016/08/13 00:08:38, cmumford wrote:
> Now only used in LevelDBTransaction::Rollback(). Could simplify - but only if
> you feel it's simpler.

Done.

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

https://codereview.chromium.org/2233153002/diff/20001/content/browser/indexed...
content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.h:49:
IndexedDBTransaction* CreateIndexedDBTransaction(
On 2016/08/13 00:08:39, cmumford wrote:
> It'd be nice, in a future commit, to make this a smart pointer too.

Agreed. I was worried about doing it in this patch because it was getting scary
complex already.

[commit-bot: I haz the power, 2016-08-16 03:05:31]

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-08-16 03:55:11]

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

[commit-bot: I haz the power, 2016-08-16 03:55:12]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-08-16 05:22:19]

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

https://codereview.chromium.org/2233153002/diff/20001/content/browser/indexed...
content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.h:49:
IndexedDBTransaction* CreateIndexedDBTransaction(
On 2016/08/16 03:05:28, jsbell (slow thru Aug 18) wrote:
> On 2016/08/13 00:08:39, cmumford wrote:
> > It'd be nice, in a future commit, to make this a smart pointer too.
> 
> Agreed. I was worried about doing it in this patch because it was getting
scary
> complex already.

To be more precise: this function is WEIRD since (as noted in comments) the
transaction ctor ensures it is "owned" (at least one ref) by the tx coordinator,
so the recipient of the raw pointer never needs to take a ref (and in fact
doesn't). I was worried that there might be some weird case where taking a ref
would delay destruction, so didn't just make the change.

This should probably be untangled a bit, and (like everything in this directory)
we should look for opportunities to eliminate refcounting since it makes
lifetimes more complicated.

[cmumford, 2016-08-16 12:10:31]

lgtm

[jsbell, 2016-08-16 13:03:36]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-08-16 13:03:47]

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-08-16 13:07:30]

Description was changed from

==========
IndexedDB: WrapUnique(new T(args...)) -> MakeUnique<T>(args...)

Get on the base::MakeUnique bandwagon, both replacing
bad uses of WrapUnique and eliminating bare `new` invocations
Also change several factory methods to return unique_ptr or
scoped_refptr instead of raw pointers, and replace some 
manually managed pointers in maps with smart pointers.

WrapUnique() is used in cases with private constructors.

R=cmumford@chromium.org
==========

to

==========
IndexedDB: WrapUnique(new T(args...)) -> MakeUnique<T>(args...)

Get on the base::MakeUnique bandwagon, both replacing
bad uses of WrapUnique and eliminating bare `new` invocations
Also change several factory methods to return unique_ptr or
scoped_refptr instead of raw pointers, and replace some 
manually managed pointers in maps with smart pointers.

WrapUnique() is used in cases with private constructors.

R=cmumford@chromium.org
==========

[commit-bot: I haz the power, 2016-08-16 13:07:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-16 13:09:07]

Description was changed from

==========
IndexedDB: WrapUnique(new T(args...)) -> MakeUnique<T>(args...)

Get on the base::MakeUnique bandwagon, both replacing
bad uses of WrapUnique and eliminating bare `new` invocations
Also change several factory methods to return unique_ptr or
scoped_refptr instead of raw pointers, and replace some 
manually managed pointers in maps with smart pointers.

WrapUnique() is used in cases with private constructors.

R=cmumford@chromium.org
==========

to

==========
IndexedDB: WrapUnique(new T(args...)) -> MakeUnique<T>(args...)

Get on the base::MakeUnique bandwagon, both replacing
bad uses of WrapUnique and eliminating bare `new` invocations
Also change several factory methods to return unique_ptr or
scoped_refptr instead of raw pointers, and replace some
manually managed pointers in maps with smart pointers.

WrapUnique() is used in cases with private constructors.

R=cmumford@chromium.org

Committed: https://crrev.com/7017da4baf8aff83269ab72a34737143c6d9d96b
Cr-Commit-Position: refs/heads/master@{#412222}
==========

[commit-bot: I haz the power, 2016-08-16 13:09:08]

Patchset 3 (id:??) landed as
https://crrev.com/7017da4baf8aff83269ab72a34737143c6d9d96b
Cr-Commit-Position: refs/heads/master@{#412222}