Multi-strategy based tracking.

Multi-strategy based tracking.

Introducing "split preferences" which allow tracking of preferences on a per item level rather than "atomic preferences" which are tracked as a whole.

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

[gab, 2013-12-17 01:10:07]

Erik, PTAL at everything.

Alexei, PTAL at histograms.

Thanks!
Gab

[erikwright (departed), 2013-12-17 02:22:49]

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:67: { prefs::kShowHomeButton,
TRACKING_STRATEGY_ATOMIC },
Suggestion: Make the 'reporting id' a member of this struct, removing the
relationship between position and id. And thus removing the requirement of
placeholders for platform-specific values.

Also removing the need for 'strategy none'.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:126: const
PrefHashFilter::TrackedPreference tracked_preferences[],
verify whether it's more common to use [] or * in parameter types.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:152: break;
I find this a little foreign (two breaks in a case). Consider alternatives.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:156: }
Are unhandled enums handled here too? Or is it only when there's a return value
and thus a path without a return?

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:62: TrackedPreferenceMetaData(size_t
index_in, PrefTrackingStrategy strategy_in)
The fact that it was originally an index is not important here. Call it
'reporting ID' or something.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:62: TrackedPreferenceMetaData(size_t
index_in, PrefTrackingStrategy strategy_in)
consider whether the constructor adds anything over standard struct-literal
initialization.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:65: // The index of this tracked
preference (important for metrics reporting).
once the name changes, these comments are no longer required as they will be
self-documented.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:80: void StoreHashBasedOnStrategy(const
std::string& path,
BasedOn -> Using

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:85: // the tracking |strategy|.
fix comment (Updates -> Checks)

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.cc:94: case CLEARED:  // Falls
through.
Don't fall through, have a dedicated NOTREACHED for less ambiguity. Especially
with the long comment separating this from the next case.

[gab, 2013-12-17 18:08:05]

Thanks, PTAL.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:67: { prefs::kShowHomeButton,
TRACKING_STRATEGY_ATOMIC },
On 2013/12/17 02:22:50, erikwright wrote:
> Suggestion: Make the 'reporting id' a member of this struct, removing the
> relationship between position and id. And thus removing the requirement of
> placeholders for platform-specific values.
> 
> Also removing the need for 'strategy none'.

I also considered this, I don't like that it adds an implicit requirement (which
is hard to verify with a simple DCHECK) that no two TrackedPreferences have the
same ID.

It does make things cleaner though (TRACKING_STRATEGY_NONE was making switches
and tests uglier).

Conversely not having a default "bad" enum value makes the error cases for
checked_value/stored_value in the tests uglier.

Implemented for now, let me know what you prefer (I can easily go back).

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:126: const
PrefHashFilter::TrackedPreference tracked_preferences[],
On 2013/12/17 02:22:50, erikwright wrote:
> verify whether it's more common to use [] or * in parameter types.

Here are 161 cases where an array is passed as the first argument to a method:
https://code.google.com/p/chromium/codesearch#search/&q=%5BA-Za-z:%5D%2B%5C%2...

It wouldn't be fair to compare that against uses of * since most uses of *
aren't actually arrays...

But there is precedent for taking [] + count variable as input.

I like it because it makes it explicit that you want an array, not just a
pointer to single value.

There appears to only be 7 if I exclude third_party/ though :(
https://code.google.com/p/chromium/codesearch#search/&q=%5BA-Za-z:%5D%2B%5C%2...

I'm wondering whether that's still because codesearch is ignore many packages as
it was yesterday...

I found 46 instances in my local src/ (excluding third_party), using the same
regexp:
[A-Za-z:]* [A-Za-z:]*\((\n *)?const [A-Za-z:]* [a-z_]*\[\]

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:152: break;
On 2013/12/17 02:22:50, erikwright wrote:
> I find this a little foreign (two breaks in a case). Consider alternatives.

I considered switching the break; for a return; but I don't think it adds any
benefit.

I otherwise don't see how to do it cleanly...

I don't think double-break is any worse than early returns in the same scenario
outside of a switch block.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.cc:156: }
On 2013/12/17 02:22:50, erikwright wrote:
> Are unhandled enums handled here too? Or is it only when there's a return
value
> and thus a path without a return?

Unhandled enums will throw a couple error on gcc/clang.

There doesn't need to be a NOTREACHED() block after the switch to cover the
"potential path without a return" error in this case since the return type is
void.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:62: TrackedPreferenceMetaData(size_t
index_in, PrefTrackingStrategy strategy_in)
On 2013/12/17 02:22:50, erikwright wrote:
> consider whether the constructor adds anything over standard struct-literal
> initialization.

Done.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:62: TrackedPreferenceMetaData(size_t
index_in, PrefTrackingStrategy strategy_in)
On 2013/12/17 02:22:50, erikwright wrote:
> The fact that it was originally an index is not important here. Call it
> 'reporting ID' or something.

Done.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:65: // The index of this tracked
preference (important for metrics reporting).
On 2013/12/17 02:22:50, erikwright wrote:
> once the name changes, these comments are no longer required as they will be
> self-documented.

Done.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:80: void StoreHashBasedOnStrategy(const
std::string& path,
On 2013/12/17 02:22:50, erikwright wrote:
> BasedOn -> Using

Done.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:85: // the tracking |strategy|.
On 2013/12/17 02:22:50, erikwright wrote:
> fix comment (Updates -> Checks)

Done.

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/114223002/diff/80001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.cc:94: case CLEARED:  // Falls
through.
On 2013/12/17 02:22:50, erikwright wrote:
> Don't fall through, have a dedicated NOTREACHED for less ambiguity. Especially
> with the long comment separating this from the next case.

Done.

[Alexei Svitkine (slow), 2013-12-17 18:26:24]

https://codereview.chromium.org/114223002/diff/100001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/114223002/diff/100001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:44: // static blocks which aren't
compatible with dynamically generated names.
In this case, I think it's OK to use the histograms API directly to avoid
putting 20 different macro invocations here. (You can look up what
UMA_HISTOGRAM_COUNTS_100() expands to and do the same thing without the
caching.)

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

https://codereview.chromium.org/114223002/diff/100001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store.h:49: // preference  at |path|.
|invalid_keys| must initially be empty.
Nit: Remove repeated space.

https://codereview.chromium.org/114223002/diff/100001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store.h:50: // If the return value is CHANGED:
|invalid_keys| will be filled with the keys
Maybe document the valid return values here? (e.g. given that things like
CLEARED and MIGRATED shouldn't be returned by this).

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

https://codereview.chromium.org/114223002/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:16769: +<histogram
name="Settings.TrackedSplitPreferenceChangedCount.kExtensionsPref">
I suggest defining the base histogram as:

<histogram name="Settings.TrackedSplitPreferenceChangedCount">
...
</histogram>

And then using the <fieldtrial> construct to create permutations of it with
different suffixes (search for an example in this file) with affected-histograms
listing Settings.TrackedSplitPreferenceChangedCount.

[gab, 2013-12-17 22:08:08]

Alexei/Erik, PTAL.

Thanks!
Gab

https://codereview.chromium.org/114223002/diff/100001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/114223002/diff/100001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:44: // static blocks which aren't
compatible with dynamically generated names.
On 2013/12/17 18:26:24, Alexei Svitkine wrote:
> In this case, I think it's OK to use the histograms API directly to avoid
> putting 20 different macro invocations here. (You can look up what
> UMA_HISTOGRAM_COUNTS_100() expands to and do the same thing without the
> caching.) 

Awesome, this is what I had done at first, I agree it makes more sense, but had
changed it thinking you absolutely didn't want the API to be used outside of the
MACROS.

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

https://codereview.chromium.org/114223002/diff/100001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store.h:49: // preference  at |path|.
|invalid_keys| must initially be empty.
On 2013/12/17 18:26:24, Alexei Svitkine wrote:
> Nit: Remove repeated space.

Done.

https://codereview.chromium.org/114223002/diff/100001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store.h:50: // If the return value is CHANGED:
|invalid_keys| will be filled with the keys
On 2013/12/17 18:26:24, Alexei Svitkine wrote:
> Maybe document the valid return values here? (e.g. given that things like
> CLEARED and MIGRATED shouldn't be returned by this).

CLEARED can be return by this; it's when iteration over the items in the
dictionary in the implementation that CLEARED isn't expected, but this is an
implementation detail.

I don't think special documentation is otherwise required for MIGRATED here.

It currently never returns MIGRATED, but the callers shouldn't assume that.

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

https://codereview.chromium.org/114223002/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:16769: +<histogram
name="Settings.TrackedSplitPreferenceChangedCount.kExtensionsPref">
On 2013/12/17 18:26:24, Alexei Svitkine wrote:
> I suggest defining the base histogram as:
> 
> <histogram name="Settings.TrackedSplitPreferenceChangedCount">
> ...
> </histogram>
> 
> And then using the <fieldtrial> construct to create permutations of it with
> different suffixes (search for an example in this file) with
affected-histograms
> listing Settings.TrackedSplitPreferenceChangedCount.

Done, how will I be able to see the various groups on the UMA dashboards?

[Alexei Svitkine (slow), 2013-12-17 22:19:55]

https://codereview.chromium.org/114223002/diff/120001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/114223002/diff/120001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:42: void
ReportSplitPreferenceChangedCount(const std::string& path,
Add a comment. Mention that |path| should be one of the kTrackedPrefs paths.

https://codereview.chromium.org/114223002/diff/120001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:44: // Note: The factory creates and
owns the histogram.
Nit: Mention that this histogram corresponds to a specific macro invocation.

https://codereview.chromium.org/114223002/diff/120001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:160: case TRACKING_STRATEGY_SPLIT:
Nit: Either add {} around this case body, since its not trivial, or refactor the
code to not use a switch (e.g. if/else if). Same for the function above.

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

https://codereview.chromium.org/114223002/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32978: +  <affected-histogram
name="Settings.TrackedSplitPreferenceChangedCount"/>
This will result in the
"Settings.TrackedSplitPreferenceChangedCount_extensions.settings" histogram.

Hmm, I'm not sure that this is quite optimal, since the dot notation from the
pref name kind of clashes with the histogram notation and also exposes the
internal pref path, which isn't so great.

I think it should be possible to use the actual constant names (i.e.
kShowHomeButton) with some macro magic and making a field for it in the
TrackedPreference.

Basically, you can make a C macro that stringifys a name, so instead of
specifying:

 { 0, prefs::kShowHomeButton, TRACKING_STRATEGY_ATOMIC },

You can make a macro:

 TRACKED_PREF(0, kShowHomeButton, TRACKING_STRATEGY_ATOMIC),

Where the macro will then expand things to:

 { 0, "kShowHomeButton", prefs::kShowHomeButton, TRACKING_STRATEGY_ATOMIC },

And then you can use the string "kShowHomeButton" from the TrackedPreference
struct when logging this.

[gab, 2013-12-18 17:43:14]

Thanks, PTAL.

https://codereview.chromium.org/114223002/diff/120001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/114223002/diff/120001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:42: void
ReportSplitPreferenceChangedCount(const std::string& path,
On 2013/12/17 22:19:56, Alexei Svitkine wrote:
> Add a comment. Mention that |path| should be one of the kTrackedPrefs paths.

Added a comment; kTrackedPref is in the scope of the constructor only, I don't
think this comment should mention it, it doesn't depend on it.

https://codereview.chromium.org/114223002/diff/120001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:44: // Note: The factory creates and
owns the histogram.
On 2013/12/17 22:19:56, Alexei Svitkine wrote:
> Nit: Mention that this histogram corresponds to a specific macro invocation.

Done.

https://codereview.chromium.org/114223002/diff/120001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:160: case TRACKING_STRATEGY_SPLIT:
On 2013/12/17 22:19:56, Alexei Svitkine wrote:
> Nit: Either add {} around this case body, since its not trivial, or refactor
the
> code to not use a switch (e.g. if/else if). Same for the function above.

Done.

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

https://codereview.chromium.org/114223002/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32978: +  <affected-histogram
name="Settings.TrackedSplitPreferenceChangedCount"/>
On 2013/12/17 22:19:56, Alexei Svitkine wrote:
> This will result in the
> "Settings.TrackedSplitPreferenceChangedCount_extensions.settings" histogram.
> 
> Hmm, I'm not sure that this is quite optimal, since the dot notation from the
> pref name kind of clashes with the histogram notation and also exposes the
> internal pref path, which isn't so great.
> 
> I think it should be possible to use the actual constant names (i.e.
> kShowHomeButton) with some macro magic and making a field for it in the
> TrackedPreference.
> 
> Basically, you can make a C macro that stringifys a name, so instead of
> specifying:
> 
>  { 0, prefs::kShowHomeButton, TRACKING_STRATEGY_ATOMIC },
> 
> You can make a macro:
> 
>  TRACKED_PREF(0, kShowHomeButton, TRACKING_STRATEGY_ATOMIC),
> 
> Where the macro will then expand things to:
> 
>  { 0, "kShowHomeButton", prefs::kShowHomeButton, TRACKING_STRATEGY_ATOMIC },
> 
> And then you can use the string "kShowHomeButton" from the TrackedPreference
> struct when logging this.

Switched to using a '_' before the suffix instead of '.' in pref_hash_filter.cc
but I don't think avoiding the pref's name in the histogram is worth the
complexity of a macro here.

AFAIK dots after the first dot don't represent anything particular in histogram
names, right?

[Alexei Svitkine (slow), 2013-12-18 18:28:01]

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

https://codereview.chromium.org/114223002/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32978: +  <affected-histogram
name="Settings.TrackedSplitPreferenceChangedCount"/>
On 2013/12/18 17:43:14, gab wrote:
> On 2013/12/17 22:19:56, Alexei Svitkine wrote:
> > This will result in the
> > "Settings.TrackedSplitPreferenceChangedCount_extensions.settings" histogram.
> > 
> > Hmm, I'm not sure that this is quite optimal, since the dot notation from
the
> > pref name kind of clashes with the histogram notation and also exposes the
> > internal pref path, which isn't so great.
> > 
> > I think it should be possible to use the actual constant names (i.e.
> > kShowHomeButton) with some macro magic and making a field for it in the
> > TrackedPreference.
> > 
> > Basically, you can make a C macro that stringifys a name, so instead of
> > specifying:
> > 
> >  { 0, prefs::kShowHomeButton, TRACKING_STRATEGY_ATOMIC },
> > 
> > You can make a macro:
> > 
> >  TRACKED_PREF(0, kShowHomeButton, TRACKING_STRATEGY_ATOMIC),
> > 
> > Where the macro will then expand things to:
> > 
> >  { 0, "kShowHomeButton", prefs::kShowHomeButton, TRACKING_STRATEGY_ATOMIC },
> > 
> > And then you can use the string "kShowHomeButton" from the TrackedPreference
> > struct when logging this.
> 
> Switched to using a '_' before the suffix instead of '.' in
pref_hash_filter.cc
> but I don't think avoiding the pref's name in the histogram is worth the
> complexity of a macro here.
> 
> AFAIK dots after the first dot don't represent anything particular in
histogram
> names, right?

I still prefer the macro approach (which actually isn't very complex to do), but
I'll let you make the call. If we're not going with that approach, I prefer that
this uses dots. For that, you can specifiy separator="." on this <fieldtrial>
(and change the other code back to using ".").

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:31: // tracked independently.
Can you document that adding new split strategy prefs requires modifying the
histograms.xml file to list them?

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store_impl.cc:78: bool has_hashes =
HasPath(path);
Nit: const.

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store_impl.cc:80: // Treat NULL and empty the
same; otherwise we would need to store a hash
Can you document that in the header api for CheckSplitValue (that a NULL one is
treated the same as an empty one)?

[gab, 2013-12-18 18:48:02]

PTAL.

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

https://codereview.chromium.org/114223002/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32978: +  <affected-histogram
name="Settings.TrackedSplitPreferenceChangedCount"/>
On 2013/12/18 18:28:01, Alexei Svitkine wrote:
> On 2013/12/18 17:43:14, gab wrote:
> > On 2013/12/17 22:19:56, Alexei Svitkine wrote:
> > > This will result in the
> > > "Settings.TrackedSplitPreferenceChangedCount_extensions.settings"
histogram.
> > > 
> > > Hmm, I'm not sure that this is quite optimal, since the dot notation from
> the
> > > pref name kind of clashes with the histogram notation and also exposes the
> > > internal pref path, which isn't so great.
> > > 
> > > I think it should be possible to use the actual constant names (i.e.
> > > kShowHomeButton) with some macro magic and making a field for it in the
> > > TrackedPreference.
> > > 
> > > Basically, you can make a C macro that stringifys a name, so instead of
> > > specifying:
> > > 
> > >  { 0, prefs::kShowHomeButton, TRACKING_STRATEGY_ATOMIC },
> > > 
> > > You can make a macro:
> > > 
> > >  TRACKED_PREF(0, kShowHomeButton, TRACKING_STRATEGY_ATOMIC),
> > > 
> > > Where the macro will then expand things to:
> > > 
> > >  { 0, "kShowHomeButton", prefs::kShowHomeButton, TRACKING_STRATEGY_ATOMIC
},
> > > 
> > > And then you can use the string "kShowHomeButton" from the
TrackedPreference
> > > struct when logging this.
> > 
> > Switched to using a '_' before the suffix instead of '.' in
> pref_hash_filter.cc
> > but I don't think avoiding the pref's name in the histogram is worth the
> > complexity of a macro here.
> > 
> > AFAIK dots after the first dot don't represent anything particular in
> histogram
> > names, right?
> 
> I still prefer the macro approach (which actually isn't very complex to do),
but
> I'll let you make the call. If we're not going with that approach, I prefer
that
> this uses dots. For that, you can specifiy separator="." on this <fieldtrial>
> (and change the other code back to using ".").

Okay done (back to '.' before suffix + separator).

I don't agree that the macro is simple, e.g. it has to:
1) Care about the "prefs::" namespace (or otherwise keep it in the stringified
pref name)
2) Be public or tests can't build a proper array of TrackedPreferences.

and I don't think it matters at all that the histogram be suffixed after the
real pref name rather than its constant's name (either way the string is
hardcoded in histograms.xml so using the constant has no benefits IMO).

I try to avoid macros at all cost and I don't think the cost here is justified.

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:31: // tracked independently.
On 2013/12/18 18:28:02, Alexei Svitkine wrote:
> Can you document that adding new split strategy prefs requires modifying the
> histograms.xml file to list them?

Done.

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store_impl.cc:78: bool has_hashes =
HasPath(path);
On 2013/12/18 18:28:02, Alexei Svitkine wrote:
> Nit: const.

Done.

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store_impl.cc:80: // Treat NULL and empty the
same; otherwise we would need to store a hash
On 2013/12/18 18:28:02, Alexei Svitkine wrote:
> Can you document that in the header api for CheckSplitValue (that a NULL one
is
> treated the same as an empty one)?

Done.

[Alexei Svitkine (slow), 2013-12-18 20:02:09]

histograms LGTM % nit.

I'll let Erik review the broader code structure and tests.

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:31: // tracked independently.
On 2013/12/18 18:48:03, gab wrote:
> On 2013/12/18 18:28:02, Alexei Svitkine wrote:
> > Can you document that adding new split strategy prefs requires modifying the
> > histograms.xml file to list them?
> 
> Done.

Nit: I think there's value documenting it in this comment. So that someone
looking for what this means (and looking to add a new entry to the list) will
see it here.

[gab, 2013-12-18 20:15:43]

Thanks, Erik all yours.

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/140001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:31: // tracked independently.
On 2013/12/18 20:02:09, Alexei Svitkine wrote:
> On 2013/12/18 18:48:03, gab wrote:
> > On 2013/12/18 18:28:02, Alexei Svitkine wrote:
> > > Can you document that adding new split strategy prefs requires modifying
the
> > > histograms.xml file to list them?
> > 
> > Done.
> 
> Nit: I think there's value documenting it in this comment. So that someone
> looking for what this means (and looking to add a new entry to the list) will
> see it here.

Done.

[erikwright (departed), 2013-12-19 15:21:26]

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:177:
ReportSplitPreferenceChangedCount(path, invalid_keys.size());
It's a little surprising that this "Report" call is hidden here while the caller
is responsible for the standard reporting call. I'd probaby pull the other one
into this method.

result = UNKNOWN
switch (strategy) {
  case atomic:
    result = store->check
    break;
  case split:
    if (get value) {
      state, invalid_keys = store->check_split
      if (state == changed)
        report split changed count
    }
}
report result;
return result;

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:47:
PrefHashFilter(scoped_ptr<PrefHashStore> pref_hash_store,
Maybe this should be the standard constructor and the list of tracked
preferences should live permanently in chrome_pref_service_factory.cc .

In fact, if you look at my CL for unloaded profiles you will see that I stuck a
method for initializing a pref hash store into pref filter just because that was
the place with access to the list of preferences.

It would be easier to correctly place if the list of tracked preferences was
elsewhere.

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:60: struct TrackedPreferenceMetaData {
Is there a good reason to define this struct vs. just storing TrackedPreference
instances?

Sure, there is one unnecessary field (the char* pointer) but it could simplify
the code a bit, especially if you ever add extra properties (because you won't
need to double declare them or copy them over).

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

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store.h:53: // be modified in all other cases.
'in all other cases' -> 'otherwise' or 'any other cases'.

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store.h:59: // Stores hashes for the |value| of
the split preference at |path|.
Are empty/null equivalent here too?

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store_impl.cc:122: ClearPath(path);
Is it OK that this method might not be atomic?

I'm not sure exactly what DictionaryPrefUpdate does, but I assumed it was some
kind of transactional object for guaranteed atomic operations on a dictionary.

If so, you might want to consider using it. If not, there is a remote chance
that the user adds one extension and then, while you are writing out the hash
(specifically, somewhere between line 123 and line 129) the JSON file is
serialized on the IO thread and we subsequently crash. Maybe too unlikely to
worry about, but the impact would be losing some hashes and thus some
extensions..

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store_impl.cc:148: return hashed_prefs &&
hashed_prefs->GetDictionary(path, NULL);
If you are going to rely on this calling pattern for GetDictionary I suggest
augmenting comments and (if necessary) tests to protect it.

[gab, 2013-12-20 18:37:06]

Thanks, PTAL.

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:177:
ReportSplitPreferenceChangedCount(path, invalid_keys.size());
On 2013/12/19 15:21:28, erikwright wrote:
> It's a little surprising that this "Report" call is hidden here while the
caller
> is responsible for the standard reporting call. I'd probaby pull the other one
> into this method.
> 
> result = UNKNOWN
> switch (strategy) {
>   case atomic:
>     result = store->check
>     break;
>   case split:
>     if (get value) {
>       state, invalid_keys = store->check_split
>       if (state == changed)
>         report split changed count
>     }
> }
> report result;
> return result;

Done.

(I intentionally kept the NOTREACHED() on GetAsDictionary() though; not sure
whether your example was requesting this to change or being a short form to
demonstrate your other point. I think it's important to catch the case where
someone would pass a non-dictionary Value* for a split strategy.)

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:47:
PrefHashFilter(scoped_ptr<PrefHashStore> pref_hash_store,
On 2013/12/19 15:21:28, erikwright wrote:
> Maybe this should be the standard constructor and the list of tracked
> preferences should live permanently in chrome_pref_service_factory.cc .
> 
> In fact, if you look at my CL for unloaded profiles you will see that I stuck
a
> method for initializing a pref hash store into pref filter just because that
was
> the place with access to the list of preferences.
> 
> It would be easier to correctly place if the list of tracked preferences was
> elsewhere.

Good point, moved.

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:60: struct TrackedPreferenceMetaData {
On 2013/12/19 15:21:28, erikwright wrote:
> Is there a good reason to define this struct vs. just storing
TrackedPreference
> instances?
> 
> Sure, there is one unnecessary field (the char* pointer) but it could simplify
> the code a bit, especially if you ever add extra properties (because you won't
> need to double declare them or copy them over).

Yea, I was thinking about that too when I added reporting_id to
TrackedPreference as well.

Now that you mention it too I guess it makes sense, plus it's only a pointer,
not a full string extra to store.

It just feels weird to have a map for which the key is the name if the value
also contains the name.

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

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store.h:53: // be modified in all other cases.
On 2013/12/19 15:21:28, erikwright wrote:
> 'in all other cases' -> 'otherwise' or 'any other cases'.

Done.

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store.h:59: // Stores hashes for the |value| of
the split preference at |path|.
On 2013/12/19 15:21:28, erikwright wrote:
> Are empty/null equivalent here too?

Yes, clarified.

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store_impl.cc:122: ClearPath(path);
On 2013/12/19 15:21:28, erikwright wrote:
> Is it OK that this method might not be atomic?
> 
> I'm not sure exactly what DictionaryPrefUpdate does, but I assumed it was some
> kind of transactional object for guaranteed atomic operations on a dictionary.
> 
> If so, you might want to consider using it. If not, there is a remote chance
> that the user adds one extension and then, while you are writing out the hash
> (specifically, somewhere between line 123 and line 129) the JSON file is
> serialized on the IO thread and we subsequently crash. Maybe too unlikely to
> worry about, but the impact would be losing some hashes and thus some
> extensions..

DictionaryPrefUpdate is not transactional: its only purpose is to Notify
observers of the modified pref only after the modification has occurred (it
otherwise forwards all calls directly to its underlying dictionary).

https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/scoped_...

In fact, there is probably no reason to use it in this class other than to be
consistent with other pref updating code...


There is no risk of race either (unless an observer of
prefs::kProfilePreferenceHashes modifies the extension set, but that would just
be silly...) since this and all other changes to prefs happen on the UI thread.

I did change it anyways because you are right that, in a world with a
transactional model to write preferences, this method should be a transaction to
avoid the case of crashing between clear+write and the next write.

The new model makes it correct as far as the notification for
kProfilePreferenceHashes and will allow for an easy transition to a
transactional model.

https://codereview.chromium.org/114223002/diff/220001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_store_impl.cc:148: return hashed_prefs &&
hashed_prefs->GetDictionary(path, NULL);
On 2013/12/19 15:21:28, erikwright wrote:
> If you are going to rely on this calling pattern for GetDictionary I suggest
> augmenting comments and (if necessary) tests to protect it.

I'll prepare a separate CL for this.

[gab, 2014-01-08 01:53:30]

Erik, rebased this on top of other changes I decided to get in first, PTAL.

@Alexei: histograms approach remains the same.

Thanks!
Gab

[gab, 2014-01-13 18:53:44]

Erik, merged on top of hash_of_hashes; PTAL.

Cheers!
Gab

[erikwright (departed), 2014-01-13 21:54:53]

https://codereview.chromium.org/114223002/diff/1220002/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/114223002/diff/1220002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.cc:108: pref_store_contents);
You shouldn't be passing both pref_store_contents and value.

https://codereview.chromium.org/114223002/diff/1220002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.cc:118: 
Consider a pure-virtual TrackedPreference class with a Check method.

Maybe |Check| and |Store|.
Or even |Check|, |Store|, and |Act|.

You figure it out ;)

PrefHashStore should be a parameter either to the constructor or to the
method(s).

PrefStoreContents probably a method parameter.

Path a constructor parameter

The enforcement level too.

https://codereview.chromium.org/114223002/diff/1220002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.cc:169: enum {
If you extract this into a helper like GetAction(ValueState, allow_seed,
allow_migration) it will facilitate sharing it from the two separate strategy
classes.

https://codereview.chromium.org/114223002/diff/1220002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.cc:215: DCHECK(!invalid_keys.empty());
Try to coalesce all of the "reactions" together. (the stuff here and also the
storing of the correct hash value, currently done in the caller).

[gab, 2014-01-15 00:33:16]

Erik, PTAL.

I don't think a specific unit test is required for TrackedPreference as its very
well covered by PrefHashFilter's unittests (the functionality was moved without
changing the tests). I'm also not convinced that testing TrackedPreference on
its own will help reduce the complexity of PrefHashFilter's tests themselves, so
the added benefit is slim.

It's basically an implementation detail of PrefHashFilter, although I can't put
it in pref_hash_filter.cc itself IMO because it's too big.

I'm also debating having EnforcementLevel in PrefHashFilter or in
TrackedPreference (see patch set 21 and 22 respectively)? I think it's better in
TrackedPreference (patch set 22), WDYT?

The new design is much cleaner IMO, thanks for the suggestions!

Cheers,
Gab

https://codereview.chromium.org/114223002/diff/1220002/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/114223002/diff/1220002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.cc:108: pref_store_contents);
On 2014/01/13 21:54:54, erikwright wrote:
> You shouldn't be passing both pref_store_contents and value.

Done.

https://codereview.chromium.org/114223002/diff/1220002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.cc:118: 
On 2014/01/13 21:54:54, erikwright wrote:
> Consider a pure-virtual TrackedPreference class with a Check method.
> 
> Maybe |Check| and |Store|.
> Or even |Check|, |Store|, and |Act|.
> 
> You figure it out ;)
> 
> PrefHashStore should be a parameter either to the constructor or to the
> method(s).
> 
> PrefStoreContents probably a method parameter.
> 
> Path a constructor parameter
> 
> The enforcement level too.

Done, the base class isn't pure virtual, it's used to handle the common
GetAction() and reports that the subclasses need.

It's also the one handling enforcement_level.

I removed the enforcement_level for split_seeding as discussed.

https://codereview.chromium.org/114223002/diff/1220002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.cc:169: enum {
On 2014/01/13 21:54:54, erikwright wrote:
> If you extract this into a helper like GetAction(ValueState, allow_seed,
> allow_migration) it will facilitate sharing it from the two separate strategy
> classes.

Done.

https://codereview.chromium.org/114223002/diff/1220002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.cc:215: DCHECK(!invalid_keys.empty());
On 2014/01/13 21:54:54, erikwright wrote:
> Try to coalesce all of the "reactions" together. (the stuff here and also the
> storing of the correct hash value, currently done in the caller).

Done.

[gab, 2014-01-15 21:54:26]

+Robert who wants to take a look.

Cheers,
Gab

[erikwright (departed), 2014-01-15 22:27:41]

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:37: struct TrackedPreferenceMetaData {
nit: I think Metadata is more common than MetaData.

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_store_impl.h (left):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store_impl.h:8: #include <string>
I tend to think that you should include or declare things that you use, even if
they are also part of an API that you inherit from.

i.e., you don't need to include-what-you-use exclusively to declare an OVERRIDE
(since by definition it must already be included or declared) but since you have
separate uses you should.

Rationale is that not doing so is fragile if the API of the thing you are
implementing changes to no longer need the type in question.

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_store_impl.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store_impl.h:10: #include
"base/prefs/scoped_user_pref_update.h"
forward decl?

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked_preference.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:57: // Returns an action stating
whether a reset is desired (DO_RESET) based on
Consider extracting all of this into a helper class rather than using
implementation inheritance.

TrackedPreferenceHelper? TrackedPreferenceCommon?

Each of the TrackedPreference implementations could then hold an instance of
this.

[gab, 2014-01-16 01:28:01]

PTAL, thanks!

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:37: struct TrackedPreferenceMetaData {
On 2014/01/15 22:27:42, erikwright wrote:
> nit: I think Metadata is more common than MetaData.

Done.

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_store_impl.h (left):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store_impl.h:8: #include <string>
On 2014/01/15 22:27:42, erikwright wrote:
> I tend to think that you should include or declare things that you use, even
if
> they are also part of an API that you inherit from.
> 
> i.e., you don't need to include-what-you-use exclusively to declare an
OVERRIDE
> (since by definition it must already be included or declared) but since you
have
> separate uses you should.
> 
> Rationale is that not doing so is fragile if the API of the thing you are
> implementing changes to no longer need the type in question.

Ok, makes sense, done.

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_store_impl.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store_impl.h:10: #include
"base/prefs/scoped_user_pref_update.h"
On 2014/01/15 22:27:42, erikwright wrote:
> forward decl?

It's a typedef so I can't :(, right?

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked_preference.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:57: // Returns an action stating
whether a reset is desired (DO_RESET) based on
On 2014/01/15 22:27:42, erikwright wrote:
> Consider extracting all of this into a helper class rather than using
> implementation inheritance.
> 
> TrackedPreferenceHelper? TrackedPreferenceCommon?
> 
> Each of the TrackedPreference implementations could then hold an instance of
> this.

I had considered this; it's nice if you have one helper which every
TrackedPreference shares.

But in the current implementation, not every TrackedPreference uses the same
enforcement level.

Thus, unless we put the burden on the caller to also generate a
TrackedPreferenceHelper for each TrackedPreference the helper would have to be
an implementation detail of TrackedPreference; at which point implementation
inheritance is just as good.

I agree it's not ideal, but I think it works just fine in this scenario.

[robertshield, 2014-01-16 03:59:30]

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:42: bool allow_enforcement;
nit: make this an enum value too?

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store_impl.cc:92: ValueState value_state =
CheckValue(path + "." + it.key(), &it.value());
the +'s yield a fair amount of object construction in this loop. Consider using
string.append() instead (for bonus points using string::reserve if you can
reasonably estimate the final length of the string.

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store_impl.cc:129: // hashes being stored on disk
in the next startup...
Can you file / reference a bug for the transactional support?

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store_impl.cc:136: StoreHashInternal(path + "." +
it.key(), &it.value(), &update);
Same comment on object construction here.

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked_preference.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:60: // of WANTED_RESET over DO_RESET
in various scenarios.
It's not clear to me reading this comment what the difference between
WANTED_RESET and DO_RESET is. Please document them here or at the enum above.

[erikwright (departed), 2014-01-16 16:48:31]

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:67: typedef std::map<std::string,
scoped_ptr<const TrackedPreference> >
I believe it is unsafe to place scoped ponters in a map. I note that there are
no other instances in the codebase. I also note the existence of
scoped_ptr_hash_map.h in base/containers

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked_preference.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:57: // Returns an action stating
whether a reset is desired (DO_RESET) based on
On 2014/01/16 01:28:02, gab wrote:
> On 2014/01/15 22:27:42, erikwright wrote:
> > Consider extracting all of this into a helper class rather than using
> > implementation inheritance.
> > 
> > TrackedPreferenceHelper? TrackedPreferenceCommon?
> > 
> > Each of the TrackedPreference implementations could then hold an instance of
> > this.
> 
> I had considered this; it's nice if you have one helper which every
> TrackedPreference shares.
> 
> But in the current implementation, not every TrackedPreference uses the same
> enforcement level.
> 
> Thus, unless we put the burden on the caller to also generate a
> TrackedPreferenceHelper for each TrackedPreference the helper would have to be
> an implementation detail of TrackedPreference; at which point implementation
> inheritance is just as good.
> 
> I agree it's not ideal, but I think it works just fine in this scenario.

I don't think the caller needs to know about it. And they don't need to be
shared instances. This is not a virtual class I'm proposing, just basically a
utility class that would hold the few data members and the non-virtual functions
that are currently in the abstract base class. The tracked preferences would
initialize it in their initializer lists.

[gab, 2014-01-16 18:58:04]

Erik/Robert, PTAL.

Thanks!
Gab

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:42: bool allow_enforcement;
On 2014/01/16 03:59:30, robertshield wrote:
> nit: make this an enum value too?

It will soon be (see other CL I sent you; do you feel strongly it should be in
this CL?).

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:67: typedef std::map<std::string,
scoped_ptr<const TrackedPreference> >
On 2014/01/16 16:48:32, erikwright wrote:
> I believe it is unsafe to place scoped ponters in a map. I note that there are
> no other instances in the codebase. I also note the existence of
> scoped_ptr_hash_map.h in base/containers

Great finding! I was just discussing with Cait yesterday whether there was a
better way to do this :)!

Thanks!

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store_impl.cc:92: ValueState value_state =
CheckValue(path + "." + it.key(), &it.value());
On 2014/01/16 03:59:30, robertshield wrote:
> the +'s yield a fair amount of object construction in this loop. Consider
using
> string.append() instead (for bonus points using string::reserve if you can
> reasonably estimate the final length of the string. 

How about this? Out-of-scope string with "|path|." only replacing the key
inplace on every pass.

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store_impl.cc:129: // hashes being stored on disk
in the next startup...
On 2014/01/16 03:59:30, robertshield wrote:
> Can you file / reference a bug for the transactional support?

Done.

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store_impl.cc:136: StoreHashInternal(path + "." +
it.key(), &it.value(), &update);
On 2014/01/16 03:59:30, robertshield wrote:
> Same comment on object construction here.

Done.

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked_preference.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:57: // Returns an action stating
whether a reset is desired (DO_RESET) based on
On 2014/01/16 16:48:32, erikwright wrote:
> On 2014/01/16 01:28:02, gab wrote:
> > On 2014/01/15 22:27:42, erikwright wrote:
> > > Consider extracting all of this into a helper class rather than using
> > > implementation inheritance.
> > > 
> > > TrackedPreferenceHelper? TrackedPreferenceCommon?
> > > 
> > > Each of the TrackedPreference implementations could then hold an instance
of
> > > this.
> > 
> > I had considered this; it's nice if you have one helper which every
> > TrackedPreference shares.
> > 
> > But in the current implementation, not every TrackedPreference uses the same
> > enforcement level.
> > 
> > Thus, unless we put the burden on the caller to also generate a
> > TrackedPreferenceHelper for each TrackedPreference the helper would have to
be
> > an implementation detail of TrackedPreference; at which point implementation
> > inheritance is just as good.
> > 
> > I agree it's not ideal, but I think it works just fine in this scenario.
> 
> I don't think the caller needs to know about it. And they don't need to be
> shared instances. This is not a virtual class I'm proposing, just basically a
> utility class that would hold the few data members and the non-virtual
functions
> that are currently in the abstract base class. The tracked preferences would
> initialize it in their initializer lists.
> 

Done, as discussed I don't think it's much greater in this case (it's actually
more lines of code and more components to reason about), but if the general
paradigm is better for Chrome, I'm fine with using it here.

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:60: // of WANTED_RESET over DO_RESET
in various scenarios.
On 2014/01/16 03:59:30, robertshield wrote:
> It's not clear to me reading this comment what the difference between
> WANTED_RESET and DO_RESET is. Please document them here or at the enum above.

Done.

[robertshield, 2014-01-17 03:59:44]

lgtm

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/114223002/diff/1700001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:42: bool allow_enforcement;
On 2014/01/16 18:58:06, gab wrote:
> On 2014/01/16 03:59:30, robertshield wrote:
> > nit: make this an enum value too?
> 
> It will soon be (see other CL I sent you; do you feel strongly it should be in
> this CL?).

If it's in the other CL that's fine by me.

[gab, 2014-01-17 16:28:24]

Bernhard, please take a look.

Thanks!
Gab

[Bernhard Bauer, 2014-01-17 17:00:04]

We are starting to have a lot of classes in c/b/p. I think it would make sense
to move them to a subdirectory tracked/ or something.

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter_unittest.cc:71: // |Check(Split)?Hash| for
|path|. The returned pointer could since have been
Nit: CheckHash/CheckSplitHash reads a bit easier than the regular expression
here :)

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_store.h (right):

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store.h:65: // will not be mofified unless the
return value is CHANGED, in which case it
Nit: "modified"

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store_impl.cc:135: // hashes being stored on disk
in the next startup. http://crbug.com/335149
This bug is WontFix :)

[gab, 2014-01-17 19:56:48]

Done, only moved new files for now; logged a bug (http://crbug.com/335667) to
move existing ones later when the set of CLs up against them has dwindled down.

Cheers,
Gab

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter_unittest.cc:71: // |Check(Split)?Hash| for
|path|. The returned pointer could since have been
On 2014/01/17 17:00:05, Bernhard Bauer wrote:
> Nit: CheckHash/CheckSplitHash reads a bit easier than the regular expression
> here :)

Done.

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_store.h (right):

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store.h:65: // will not be mofified unless the
return value is CHANGED, in which case it
On 2014/01/17 17:00:05, Bernhard Bauer wrote:
> Nit: "modified"

mofified sounds great, no?! ;)

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_store_impl.cc:135: // hashes being stored on disk
in the next startup. http://crbug.com/335149
On 2014/01/17 17:00:05, Bernhard Bauer wrote:
> This bug is WontFix :)

WooT :)!

[gab, 2014-01-17 19:57:52]

Note: responded to nits in patch set 34; patch set 35 is a move-only patch set.

[erikwright (departed), 2014-01-17 20:22:59]

Some small comments. I need to look deeper later.

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked_preference.h (right):

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Update this in all your new files. It's 2014, baby.

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:15: // A TrackedPreference is a
pure-virtual class whose subclasses are responsible
I think it's atypical to describe a class's OO characteristics like this. More
typically you would say something like:

// A TrackedPreference tracks changes to an individual preference, reporting and
reacting to them according to preference-specific and browser-wide policies.

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:22: // Stores a new hash in
|pref_hash_store| for this preference's |value|.
The method name and comment are somewhat misleading. This could store multiple
hashes, too.

How about OnValueChanged?

// Updates |pref_hash_store| to reflect the preference's new value |value|.

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:26: // Enforces that the value of this
tracked preference in |pref_store_contents|
Enforces -> Verifies...

// ... Responds to verification failures according to preference-specific and
browser-wide policy and reports results to via UMA.

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked_preference_helper.h (right):

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference_helper.h:18: enum EnforcementLevel {
I would probably put this in its own file. It doesn't make sense for
chrome_pref_service_factory to be aware of TrackedPreferenceHelper.

[gab, 2014-01-17 21:32:00]

Done, PTAL.

Thanks!
Gab

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked_preference.h (right):

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2014/01/17 20:23:01, erikwright wrote:
> Update this in all your new files. It's 2014, baby.

Done.

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:15: // A TrackedPreference is a
pure-virtual class whose subclasses are responsible
On 2014/01/17 20:23:01, erikwright wrote:
> I think it's atypical to describe a class's OO characteristics like this. More
> typically you would say something like:
> 
> // A TrackedPreference tracks changes to an individual preference, reporting
and
> reacting to them according to preference-specific and browser-wide policies.

Done.

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:22: // Stores a new hash in
|pref_hash_store| for this preference's |value|.
On 2014/01/17 20:23:01, erikwright wrote:
> The method name and comment are somewhat misleading. This could store multiple
> hashes, too.
> 
> How about OnValueChanged?
> 
> // Updates |pref_hash_store| to reflect the preference's new value |value|.

Done.

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference.h:26: // Enforces that the value of this
tracked preference in |pref_store_contents|
On 2014/01/17 20:23:01, erikwright wrote:
> Enforces -> Verifies...
> 
> // ... Responds to verification failures according to preference-specific and
> browser-wide policy and reports results to via UMA.

Done.

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked_preference_helper.h (right):

https://codereview.chromium.org/114223002/diff/2070002/chrome/browser/prefs/t...
chrome/browser/prefs/tracked_preference_helper.h:18: enum EnforcementLevel {
On 2014/01/17 20:23:01, erikwright wrote:
> I would probably put this in its own file. It doesn't make sense for
> chrome_pref_service_factory to be aware of TrackedPreferenceHelper.

Put it back in PrefHashFilter.

[Bernhard Bauer, 2014-01-20 12:03:40]

LGTM!

[erikwright (departed), 2014-01-21 18:44:44]

LGTM with a few IWYU fixes.

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_atomic_preference.cc (right):

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_atomic_preference.cc:21:
pref_hash_store->StoreHash(pref_path_, value);
pref_hash_store.h

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_atomic_preference.h (right):

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_atomic_preference.h:26: PrefHashStore*
pref_hash_store) const OVERRIDE;
compiler_specific.h

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_atomic_preference.h:35:
DISALLOW_COPY_AND_ASSIGN(TrackedAtomicPreference);
macros.h

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_preference_helper.h (right):

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_preference_helper.h:25:
TrackedPreferenceHelper(const std::string& pref_path,
<string>

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_split_preference.cc (right):

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_split_preference.cc:29:
pref_hash_store->StoreSplitHash(pref_path_, dict_value);
pref_hash_store.h

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_split_preference.h (right):

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_split_preference.h:29: PrefHashStore*
pref_hash_store) const OVERRIDE;
compiler_specific.h

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_split_preference.h:38:
DISALLOW_COPY_AND_ASSIGN(TrackedSplitPreference);
base/macros.h

[gab, 2014-01-21 20:51:41]

Thanks, done.

Also merged with r246052 (FilterSerializeData()), PTAL and confirm that what I
did in the split pref world makes sense to you (given the separation of
TrackedPreference it's mostly as simple as calling
TrackedPreference::OnValueChanged rather than PrefHashStore::StoreHash in
FilterSerializeData() + fixup tests).

Cheers,
Gab

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_atomic_preference.cc (right):

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_atomic_preference.cc:21:
pref_hash_store->StoreHash(pref_path_, value);
On 2014/01/21 18:44:45, erikwright wrote:
> pref_hash_store.h

Done.

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_atomic_preference.h (right):

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_atomic_preference.h:26: PrefHashStore*
pref_hash_store) const OVERRIDE;
On 2014/01/21 18:44:45, erikwright wrote:
> compiler_specific.h

Done.

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_atomic_preference.h:35:
DISALLOW_COPY_AND_ASSIGN(TrackedAtomicPreference);
On 2014/01/21 18:44:45, erikwright wrote:
> macros.h

Done.

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_preference_helper.h (right):

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_preference_helper.h:25:
TrackedPreferenceHelper(const std::string& pref_path,
On 2014/01/21 18:44:45, erikwright wrote:
> <string>

Done.

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_split_preference.cc (right):

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_split_preference.cc:29:
pref_hash_store->StoreSplitHash(pref_path_, dict_value);
On 2014/01/21 18:44:45, erikwright wrote:
> pref_hash_store.h

Done.

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_split_preference.h (right):

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_split_preference.h:29: PrefHashStore*
pref_hash_store) const OVERRIDE;
On 2014/01/21 18:44:45, erikwright wrote:
> compiler_specific.h

Done.

https://codereview.chromium.org/114223002/diff/3020001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_split_preference.h:38:
DISALLOW_COPY_AND_ASSIGN(TrackedSplitPreference);
On 2014/01/21 18:44:45, erikwright wrote:
> base/macros.h

Done.

[gab, 2014-01-30 01:37:57]

PTAL at the latest patch set; merged with crrev.com/247349 (which caused a
subtle merge issue dealt with in this latest patch set, see
https://codereview.chromium.org/137383011/ for details).

I would like your re-review on the latest diff (patch set 43 to 44); no need to
re-review the entire CL (I've been holding off commit while we were solving the
perf regressions).

Cheers!
Gab

[Bernhard Bauer, 2014-01-30 10:33:35]

lgtm

[Alexei Svitkine (slow), 2014-01-30 15:12:54]

still lgtm

[erikwright (departed), 2014-01-30 15:32:17]

LGTM with two nits.

https://codereview.chromium.org/114223002/diff/3440001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_atomic_preference.cc (right):

https://codereview.chromium.org/114223002/diff/3440001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_atomic_preference.cc:16: :
pref_path_(pref_path), pref_hash_store_(pref_hash_store),
Initialize in the declaration order.

https://codereview.chromium.org/114223002/diff/3440001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_split_preference.cc (right):

https://codereview.chromium.org/114223002/diff/3440001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_split_preference.cc:19: :
pref_path_(pref_path), pref_hash_store_(pref_hash_store),
Initialize in declaration order.

[gab, 2014-01-30 15:57:17]

Thanks, done.

https://codereview.chromium.org/114223002/diff/3440001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_atomic_preference.cc (right):

https://codereview.chromium.org/114223002/diff/3440001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_atomic_preference.cc:16: :
pref_path_(pref_path), pref_hash_store_(pref_hash_store),
On 2014/01/30 15:32:18, erikwright wrote:
> Initialize in the declaration order.

Done.

https://codereview.chromium.org/114223002/diff/3440001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/tracked_split_preference.cc (right):

https://codereview.chromium.org/114223002/diff/3440001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/tracked_split_preference.cc:19: :
pref_path_(pref_path), pref_hash_store_(pref_hash_store),
On 2014/01/30 15:32:18, erikwright wrote:
> Initialize in declaration order.

Done.

[gab, 2014-02-02 04:21:23]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-02-02 04:21:38]

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

[commit-bot: I haz the power, 2014-02-02 06:17:56]

Change committed as 248393

[commit-bot: I haz the power, 2014-02-02 06:18:03]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-02 06:18:03]

CQ bit was unchecked on CL. Ignoring.