Add ComputeMac and ComputeSplitMacs methods to pref_hash_store.

Add a ComputeMac and ComputeSplitMacs method to pref_hash_store.

This will be used to precompute macs for use in a callback without
going through a transaction.

BUG=624858
Committed: https://crrev.com/2f1a799b1b834987bef165a80b092867fb8a10cb
Cr-Commit-Position: refs/heads/master@{#417081}

[proberge, 2016-09-01 14:44:26]

Description was changed from

==========
Add a ComputeMac and ComputeSplitMacs method to pref_hash_store.

This will be used to precompute macs for use in a callback without
going through a transaction.

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

to

==========
Add a ComputeMac and ComputeSplitMacs method to pref_hash_store.

This will be used to precompute macs for use in a callback without
going through a transaction.

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

[proberge, 2016-09-01 14:44:26]

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

[gab, 2016-09-01 21:21:56]

lgtm % nits

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_store.h (right):

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store.h:10: #include "base/values.h"
fwd-decl instead:

namespace base {
class DictionaryValue;
class Value;
}

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store.h:33: virtual std::string
ComputeMac(const std::string& path,
#include <string>

(can't fwd-decl lib types)

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_store_impl.cc (left):

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store_impl.cc:13: #include
"base/values.h"
Keep this now that it's only fwd-decl'ed in headers.

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_store_impl.h (right):

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store_impl.h:17: class
PrefHashStoreTransaction;
A bit of cleanup...

Anything that is only used by an override (and thus has to be
fwd-declared/included by interface header shouldn't be re-fwd-declared in impl
header).

So here, instead of adding new fwd-decls from pref_hash_store.h I'd say remove
these two fwd-decls as well as the include of <memory>

As-is we're kind of in an in-between where this CL is doing the right thing on
its own but creating a situation where we're nor re-fwd-declaring everything,
nor correctly not doing so...

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_store_impl_unittest.cc (right):

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store_impl_unittest.cc:38: // complete
tests.
Also

EXPECT_EQ(computed_mac_1, pref_hash_store.ComputeMac("path1", &string_1));

(i.e. same result twice in a row)

[proberge, 2016-09-06 18:43:39]

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_store.h (right):

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store.h:10: #include "base/values.h"
On 2016/09/01 21:21:56, gab wrote:
> fwd-decl instead:
> 
> namespace base {
> class DictionaryValue;
> class Value;
> }

Done. This whole include vs fwd-decl is still very nebulous to me. Can you point
me to some documentation which explains when to include vs when to fwd-decl?

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store.h:33: virtual std::string
ComputeMac(const std::string& path,
On 2016/09/01 21:21:56, gab wrote:
> #include <string>
> 
> (can't fwd-decl lib types)

Done.

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_store_impl.cc (left):

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store_impl.cc:13: #include
"base/values.h"
On 2016/09/01 21:21:56, gab wrote:
> Keep this now that it's only fwd-decl'ed in headers.

Done.

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_store_impl.h (right):

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store_impl.h:17: class
PrefHashStoreTransaction;
On 2016/09/01 21:21:56, gab wrote:
> A bit of cleanup...
> 
> Anything that is only used by an override (and thus has to be
> fwd-declared/included by interface header shouldn't be re-fwd-declared in impl
> header).
> 
> So here, instead of adding new fwd-decls from pref_hash_store.h I'd say remove
> these two fwd-decls as well as the include of <memory>
> 
> As-is we're kind of in an in-between where this CL is doing the right thing on
> its own but creating a situation where we're nor re-fwd-declaring everything,
> nor correctly not doing so...

Done.

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_store_impl_unittest.cc (right):

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store_impl_unittest.cc:38: // complete
tests.
On 2016/09/01 21:21:56, gab wrote:
> Also
> 
> EXPECT_EQ(computed_mac_1, pref_hash_store.ComputeMac("path1", &string_1));
> 
> (i.e. same result twice in a row)

Done.

[proberge, 2016-09-06 18:44:16]

The CQ bit was checked by proberge@chromium.org

[proberge, 2016-09-06 18:44:16]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2304573002/#ps40001
(title: "Clean up imports")

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

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-06 18:55:46]

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

[commit-bot: I haz the power, 2016-09-06 18:55:47]

Exceeded global retry quota

[proberge, 2016-09-06 19:33:48]

The CQ bit was checked by proberge@chromium.org

[proberge, 2016-09-06 19:33:49]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2304573002/#ps60001
(title: "64 => 64U for EXPECT_EQ with size()")

[commit-bot: I haz the power, 2016-09-06 19:34:28]

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-06 19:44:18]

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

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

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

[gab, 2016-09-06 20:08:04]

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_store.h (right):

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store.h:10: #include "base/values.h"
On 2016/09/06 18:43:38, proberge wrote:
> On 2016/09/01 21:21:56, gab wrote:
> > fwd-decl instead:
> > 
> > namespace base {
> > class DictionaryValue;
> > class Value;
> > }
> 
> Done. This whole include vs fwd-decl is still very nebulous to me. Can you
point
> me to some documentation which explains when to include vs when to fwd-decl?

The concept is known as IWUY (include-what-you-use). The idea is that every .cc
should include what it uses (i.e. not rely on included headers to do so for it
-- making refactorings easier).

There is an exception when the matching .h already has that include.

That exception extends to a .h with overrides (by recursive IWUY: includes from
the overridden interface must have been included by its header so those includes
would be redundant -- only include them if needed by non-overrides of your
class, in other words "overrides" don't count in "what you use").


Now forward-decls versus includes:

In headers you often only need to tell the compiler that the type exists (not
its full definition). This is true for example with pointers and const&.
Basically ask yourself whether the compiler needs to know the size of the object
and/or know how to construct/destroy it when parsing your .h, if not, a
forward-declaration is enough.

STL types (and templates) can't be forward declared though.


So overall, in headers: include-what-you-use (things you "use" are usually:
non-pointer members, return values and parameters passed by value) and
forward-declare-the-rest.

http://stackoverflow.com/a/17499222 is a good read on why you should include
(not fwd-decl) for return values and params passed by value -- fwd-decl is
sufficient for the definition of the method but e.g. making your users have to
include type only to have the destructor when ignoring the return value isn't
cool.

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store.h:40: virtual
std::unique_ptr<base::DictionaryValue> ComputeSplitMacs(
Also, based on above discussion I just realized that this and
PrefHashStoreTransaction should be includes not fwd-decls, per being return
values (not a big deal, but to be pedantic -- so back to values.h here and not
in .cc if you're with me :-)).

[proberge, 2016-09-07 18:00:56]

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_store.h (right):

https://codereview.chromium.org/2304573002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_store.h:40: virtual
std::unique_ptr<base::DictionaryValue> ComputeSplitMacs(
On 2016/09/06 20:08:04, gab wrote:
> Also, based on above discussion I just realized that this and
> PrefHashStoreTransaction should be includes not fwd-decls, per being return
> values (not a big deal, but to be pedantic -- so back to values.h here and not
> in .cc if you're with me :-)).

Done.

[proberge, 2016-09-07 18:00:59]

The CQ bit was checked by proberge@chromium.org

[proberge, 2016-09-07 18:01:00]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2304573002/#ps100001
(title: "rm import in .cc moved to .h")

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

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 20:07:27]

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

[commit-bot: I haz the power, 2016-09-07 20:07:29]

Try jobs failed on following builders:
  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...)

[proberge, 2016-09-07 21:19:15]

The CQ bit was checked by proberge@chromium.org

[proberge, 2016-09-07 21:19:16]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2304573002/#ps120001
(title: "rebase-update'd")

[commit-bot: I haz the power, 2016-09-07 21:19: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 22:12:35]

Description was changed from

==========
Add a ComputeMac and ComputeSplitMacs method to pref_hash_store.

This will be used to precompute macs for use in a callback without
going through a transaction.

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

to

==========
Add a ComputeMac and ComputeSplitMacs method to pref_hash_store.

This will be used to precompute macs for use in a callback without
going through a transaction.

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

[commit-bot: I haz the power, 2016-09-07 22:12:36]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-07 22:14:42]

Description was changed from

==========
Add a ComputeMac and ComputeSplitMacs method to pref_hash_store.

This will be used to precompute macs for use in a callback without
going through a transaction.

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

to

==========
Add a ComputeMac and ComputeSplitMacs method to pref_hash_store.

This will be used to precompute macs for use in a callback without
going through a transaction.

BUG=624858

Committed: https://crrev.com/2f1a799b1b834987bef165a80b092867fb8a10cb
Cr-Commit-Position: refs/heads/master@{#417081}
==========

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

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