Introduce a hash_of_hashes dictionary.

Introduce a hash_of_hashes dictionary.

It allows us to know whether the existing hashes dictionary can be trusted on startup (and thus whether seeding should be allowed).

Note: seeding a NULL preference is always allowed.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243776

[gab, 2014-01-07 19:49:29]

PTAL.

Thanks!
Gab

[Bernhard Bauer, 2014-01-08 09:16:22]

LGTM

https://codereview.chromium.org/126093007/diff/60001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/126093007/diff/60001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:238: ASSERT_EQ(string_value,
value_in_store);
Do we really want to assert pointer equality here?

[gab, 2014-01-08 14:33:52]

Thanks, justification to question below.

Cheers,
Gab

https://codereview.chromium.org/126093007/diff/60001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/126093007/diff/60001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:238: ASSERT_EQ(string_value,
value_in_store);
On 2014/01/08 09:16:23, Bernhard Bauer wrote:
> Do we really want to assert pointer equality here?

This is how all of the other tests in this file work.

I wrote them that way in https://codereview.chromium.org/122653005/.

All this test really cares about is that the correct Value* was passed down to
the PrefHashStore. Since Value isn't copy-constructable, we can just use the
same pointer value throughout the test (in fact stored_value() returns a void*
to emphasize the fact that this should be used to compare to another Value* yet
shouldn't be dereferenced).

This is more powerful than storing a string representation of the stored_value
and doing string comparison as was previously done:
 1) Having to turn the Value into a string to do the check is ugly
 2) It allows us to use non-string Value types -- which becomes really helpful
in upcoming CL https://codereview.chromium.org/114223002/

[commit-bot: I haz the power, 2014-01-08 14:34:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/126093007/60001

[Bernhard Bauer, 2014-01-08 14:41:12]

https://codereview.chromium.org/126093007/diff/60001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/126093007/diff/60001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:238: ASSERT_EQ(string_value,
value_in_store);
On 2014/01/08 14:33:52, gab wrote:
> On 2014/01/08 09:16:23, Bernhard Bauer wrote:
> > Do we really want to assert pointer equality here?
> 
> This is how all of the other tests in this file work.
> 
> I wrote them that way in https://codereview.chromium.org/122653005/.
> 
> All this test really cares about is that the correct Value* was passed down to
> the PrefHashStore. Since Value isn't copy-constructable, we can just use the
> same pointer value throughout the test (in fact stored_value() returns a void*
> to emphasize the fact that this should be used to compare to another Value*
yet
> shouldn't be dereferenced).
> 
> This is more powerful than storing a string representation of the stored_value
> and doing string comparison as was previously done:
>  1) Having to turn the Value into a string to do the check is ugly
>  2) It allows us to use non-string Value types -- which becomes really helpful
> in upcoming CL https://codereview.chromium.org/114223002/

I'm just somewhat worried that the test is too strict -- keeping a DeepCopy() of
the value would be perfectly acceptable IMO.

Another slight advantage of comparing, say, JSON serializations of the values
would be that it would make spotting differences easier when a test fails.

In any case, LGTM still holds.

[gab, 2014-01-08 14:55:58]

https://codereview.chromium.org/126093007/diff/60001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/126093007/diff/60001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter_unittest.cc:238: ASSERT_EQ(string_value,
value_in_store);
On 2014/01/08 14:41:12, Bernhard Bauer wrote:
> On 2014/01/08 14:33:52, gab wrote:
> > On 2014/01/08 09:16:23, Bernhard Bauer wrote:
> > > Do we really want to assert pointer equality here?
> > 
> > This is how all of the other tests in this file work.
> > 
> > I wrote them that way in https://codereview.chromium.org/122653005/.
> > 
> > All this test really cares about is that the correct Value* was passed down
to
> > the PrefHashStore. Since Value isn't copy-constructable, we can just use the
> > same pointer value throughout the test (in fact stored_value() returns a
void*
> > to emphasize the fact that this should be used to compare to another Value*
> yet
> > shouldn't be dereferenced).
> > 
> > This is more powerful than storing a string representation of the
stored_value
> > and doing string comparison as was previously done:
> >  1) Having to turn the Value into a string to do the check is ugly
> >  2) It allows us to use non-string Value types -- which becomes really
helpful
> > in upcoming CL https://codereview.chromium.org/114223002/
> 
> I'm just somewhat worried that the test is too strict -- keeping a DeepCopy()
of
> the value would be perfectly acceptable IMO.
> 
> Another slight advantage of comparing, say, JSON serializations of the values
> would be that it would make spotting differences easier when a test fails.
> 
> In any case, LGTM still holds.

sg, I'll take a look at using DeepCopy() instead for these tests today.

I agree that the failure mode is much nicer when comparing Value rather than
pointer (provided the gtest framework can print Value?).

[erikwright (departed), 2014-01-08 14:58:52]

I think that Bernhard's comment was that string serializations (the prior
implementation) would be easier to diagnose.

DeepCopy would be fine, and you could write a serializer if it doesn't
already exist, but then you add the burden of managing ownership. You have
to make a copy on insertion, and potentially free a previous value too.

At destruction you have to free all the elements in the map.

I can't see a single likely case where this is better than serialization.
Serialization works just fine unless two inequal values serialize to the
same string. That seems exceedingly unlikely in a test scenario, and
something I'm personally willing to accept.


On Wed, Jan 8, 2014 at 9:55 AM, <gab@chromium.org> wrote:

>
> https://codereview.chromium.org/126093007/diff/60001/
> chrome/browser/prefs/pref_hash_filter_unittest.cc
> File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):
>
> https://codereview.chromium.org/126093007/diff/60001/
> chrome/browser/prefs/pref_hash_filter_unittest.cc#newcode238
> chrome/browser/prefs/pref_hash_filter_unittest.cc:238:
> ASSERT_EQ(string_value, value_in_store);
> On 2014/01/08 14:41:12, Bernhard Bauer wrote:
>
>> On 2014/01/08 14:33:52, gab wrote:
>> > On 2014/01/08 09:16:23, Bernhard Bauer wrote:
>> > > Do we really want to assert pointer equality here?
>> >
>> > This is how all of the other tests in this file work.
>> >
>> > I wrote them that way in https://codereview.chromium.org/122653005/.
>> >
>> > All this test really cares about is that the correct Value* was
>>
> passed down to
>
>> > the PrefHashStore. Since Value isn't copy-constructable, we can just
>>
> use the
>
>> > same pointer value throughout the test (in fact stored_value()
>>
> returns a void*
>
>> > to emphasize the fact that this should be used to compare to another
>>
> Value*
>
>> yet
>> > shouldn't be dereferenced).
>> >
>> > This is more powerful than storing a string representation of the
>>
> stored_value
>
>> > and doing string comparison as was previously done:
>> >  1) Having to turn the Value into a string to do the check is ugly
>> >  2) It allows us to use non-string Value types -- which becomes
>>
> really helpful
>
>> > in upcoming CL https://codereview.chromium.org/114223002/
>>
>
>  I'm just somewhat worried that the test is too strict -- keeping a
>>
> DeepCopy() of
>
>> the value would be perfectly acceptable IMO.
>>
>
>  Another slight advantage of comparing, say, JSON serializations of the
>>
> values
>
>> would be that it would make spotting differences easier when a test
>>
> fails.
>
>  In any case, LGTM still holds.
>>
>
> sg, I'll take a look at using DeepCopy() instead for these tests today.
>
> I agree that the failure mode is much nicer when comparing Value rather
> than pointer (provided the gtest framework can print Value?).
>
> https://codereview.chromium.org/126093007/
>

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, 2014-01-08 15:05:41]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[gab, 2014-01-08 15:06:28]

+Alexei for histograms OWNERS.

Thanks!
Gab

[Alexei Svitkine (slow), 2014-01-08 21:18:14]

https://codereview.chromium.org/126093007/diff/270001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_store.h (right):

https://codereview.chromium.org/126093007/diff/270001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store.h:19: // debating whether an unknown value
is to be trusted, will be stored as a
Nit: clarify "will be stored", i.e. "will be stored in preferences"

https://codereview.chromium.org/126093007/diff/270001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/126093007/diff/270001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:17025: +    willing to seed blindly as
either (1) the current MACs are trusted and this
Nit: not sure what "seed blindly" means, can you clarify in the description?

[Alexei Svitkine (slow), 2014-01-08 21:19:36]

https://codereview.chromium.org/126093007/diff/300001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/126093007/diff/300001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:16850: +  <summary>Whether the hashes
dictionary for this profile is trusted.</summary>
Mention when this is logged. (On profile prefs load?)

[gab, 2014-01-08 21:36:12]

PTAL.

Thanks!
Gab

https://codereview.chromium.org/126093007/diff/270001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_store.h (right):

https://codereview.chromium.org/126093007/diff/270001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store.h:19: // debating whether an unknown value
is to be trusted, will be stored as a
On 2014/01/08 21:18:15, Alexei Svitkine wrote:
> Nit: clarify "will be stored", i.e. "will be stored in preferences"

Actually this class is fairly low-level, it doesn't really know what it's
storing into (it's currently local_state but might/will change).

I don't want to add something to a comment here that will be come false.

This is a store so saying "stored in this store's storage" is kind of redundant
too... leaving as-is.

https://codereview.chromium.org/126093007/diff/270001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/126093007/diff/270001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:17025: +    willing to seed blindly as
either (1) the current MACs are trusted and this
On 2014/01/08 21:18:15, Alexei Svitkine wrote:
> Nit: not sure what "seed blindly" means, can you clarify in the description?

Done.

[Alexei Svitkine (slow), 2014-01-08 21:39:49]

LGTM, thanks!

[commit-bot: I haz the power, 2014-01-08 21:45:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/126093007/390001

[commit-bot: I haz the power, 2014-01-09 04:51:03]

Change committed as 243776