Refactor PrefHashStore's BeginTransaction to take a rawptr

Refactor PrefHashStore's BeginTransaction to take a rawptr

To support a new HashStoreContents which needs to be passed around in
a different manner, the PrefHashStoreTransaction now has a raw ptr
instead of owning the HashStoreContents.

BUG=624858
Committed: https://crrev.com/9164d24b054643599f0ec0a14243ff1bf956e269
Cr-Commit-Position: refs/heads/master@{#417023}

[proberge, 2016-08-31 20:24:09]

Description was changed from

==========
Refactor PrefHashStore's BeginTransaction to take a rawptr

To support a new HashStoreContents which needs to be passed around in
a different manner, the PrefHashStoreTransaction now has a raw ptr
instead of owning the HashStoreContents.

BUG=624858
==========

to

==========
Refactor PrefHashStore's BeginTransaction to take a rawptr

To support a new HashStoreContents which needs to be passed around in
a different manner, the PrefHashStoreTransaction now has a raw ptr
instead of owning the HashStoreContents.

BUG=624858
==========

[proberge, 2016-08-31 20:24:09]

proberge@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2016-09-01 20:31:50]

lg

https://codereview.chromium.org/2297373002/diff/1/components/user_prefs/track...
File components/user_prefs/tracked/pref_hash_store.h (right):

https://codereview.chromium.org/2297373002/diff/1/components/user_prefs/track...
components/user_prefs/tracked/pref_hash_store.h:25: // not outlive |storage|.
Raw pointer in Chromium always implies lack of ownership, last sentence could
simply be:

// |storage| must outlive the returned transaction.

https://codereview.chromium.org/2297373002/diff/1/components/user_prefs/track...
File components/user_prefs/tracked/pref_hash_store_impl_unittest.cc (right):

https://codereview.chromium.org/2297373002/diff/1/components/user_prefs/track...
components/user_prefs/tracked/pref_hash_store_impl_unittest.cc:20:
contents_.reset(new DictionaryHashStoreContents(&pref_store_contents_));
A chromium-dev discussion just concluded in favor of avoiding naked "new" and
using MakeUnique as much as possible instead :
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/iQgMedVA8-k/dis...

But here, even better, initialization can be done in the constructor's
initializer list and this doesn't even need to be in a unique_ptr:

PrefHashStoreImplTest() 
    : contents_(&pref_store_contents_) {}

https://codereview.chromium.org/2297373002/diff/1/components/user_prefs/track...
components/user_prefs/tracked/pref_hash_store_impl_unittest.cc:27:
std::unique_ptr<HashStoreContents> contents_;
Needs to be after |pref_store_contents_| as members are initialized in order and
destroyed in reverse order (and this one depends on |pref_store_contents_|).

Move it and add a comment like:

// Must be declared after |pref_store_contents_| as it needs to be outlived by
it.

https://codereview.chromium.org/2297373002/diff/1/components/user_prefs/track...
components/user_prefs/tracked/pref_hash_store_impl_unittest.cc:29: };
DISALLOW_COPY_AND_ASSIGN(PrefHashStoreImplTest);

(this was written in days where Chromium was sloppy about
DISALLOW_COPY_AND_ASSIGN in test fixtures but we long ago decided to enforce it
everywhere, so cleaning it up here would be nice :-))

[proberge, 2016-09-06 19:24:10]

https://codereview.chromium.org/2297373002/diff/1/components/user_prefs/track...
File components/user_prefs/tracked/pref_hash_store.h (right):

https://codereview.chromium.org/2297373002/diff/1/components/user_prefs/track...
components/user_prefs/tracked/pref_hash_store.h:25: // not outlive |storage|.
On 2016/09/01 20:31:50, gab wrote:
> Raw pointer in Chromium always implies lack of ownership, last sentence could
> simply be:
> 
> // |storage| must outlive the returned transaction.

Done.

https://codereview.chromium.org/2297373002/diff/1/components/user_prefs/track...
File components/user_prefs/tracked/pref_hash_store_impl_unittest.cc (right):

https://codereview.chromium.org/2297373002/diff/1/components/user_prefs/track...
components/user_prefs/tracked/pref_hash_store_impl_unittest.cc:20:
contents_.reset(new DictionaryHashStoreContents(&pref_store_contents_));
On 2016/09/01 20:31:50, gab wrote:
> A chromium-dev discussion just concluded in favor of avoiding naked "new" and
> using MakeUnique as much as possible instead :
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/iQgMedVA8-k/dis...
> 
> But here, even better, initialization can be done in the constructor's
> initializer list and this doesn't even need to be in a unique_ptr:
> 
> PrefHashStoreImplTest() 
>     : contents_(&pref_store_contents_) {}

Done.

https://codereview.chromium.org/2297373002/diff/1/components/user_prefs/track...
components/user_prefs/tracked/pref_hash_store_impl_unittest.cc:27:
std::unique_ptr<HashStoreContents> contents_;
On 2016/09/01 20:31:50, gab wrote:
> Needs to be after |pref_store_contents_| as members are initialized in order
and
> destroyed in reverse order (and this one depends on |pref_store_contents_|).
> 
> Move it and add a comment like:
> 
> // Must be declared after |pref_store_contents_| as it needs to be outlived by
> it.

Done.

https://codereview.chromium.org/2297373002/diff/1/components/user_prefs/track...
components/user_prefs/tracked/pref_hash_store_impl_unittest.cc:29: };
On 2016/09/01 20:31:50, gab wrote:
> DISALLOW_COPY_AND_ASSIGN(PrefHashStoreImplTest);
> 
> (this was written in days where Chromium was sloppy about
> DISALLOW_COPY_AND_ASSIGN in test fixtures but we long ago decided to enforce
it
> everywhere, so cleaning it up here would be nice :-))

Done.

[gab, 2016-09-07 15:03:18]

lgtm

[proberge, 2016-09-07 18:09:20]

The CQ bit was checked by proberge@chromium.org

[commit-bot: I haz the power, 2016-09-07 18:09:44]

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-09-07 19:16:09]

Description was changed from

==========
Refactor PrefHashStore's BeginTransaction to take a rawptr

To support a new HashStoreContents which needs to be passed around in
a different manner, the PrefHashStoreTransaction now has a raw ptr
instead of owning the HashStoreContents.

BUG=624858
==========

to

==========
Refactor PrefHashStore's BeginTransaction to take a rawptr

To support a new HashStoreContents which needs to be passed around in
a different manner, the PrefHashStoreTransaction now has a raw ptr
instead of owning the HashStoreContents.

BUG=624858
==========

[commit-bot: I haz the power, 2016-09-07 19:16:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-07 19:19:38]

Description was changed from

==========
Refactor PrefHashStore's BeginTransaction to take a rawptr

To support a new HashStoreContents which needs to be passed around in
a different manner, the PrefHashStoreTransaction now has a raw ptr
instead of owning the HashStoreContents.

BUG=624858
==========

to

==========
Refactor PrefHashStore's BeginTransaction to take a rawptr

To support a new HashStoreContents which needs to be passed around in
a different manner, the PrefHashStoreTransaction now has a raw ptr
instead of owning the HashStoreContents.

BUG=624858

Committed: https://crrev.com/9164d24b054643599f0ec0a14243ff1bf956e269
Cr-Commit-Position: refs/heads/master@{#417023}
==========

[commit-bot: I haz the power, 2016-09-07 19:19:39]

Patchset 2 (id:??) landed as
https://crrev.com/9164d24b054643599f0ec0a14243ff1bf956e269
Cr-Commit-Position: refs/heads/master@{#417023}