Expand PrefHashBrowserTest to ensure unloaded profile initialization produces proper hashes.

Expand PrefHashBrowserTest to ensure unloaded profile initialization produces proper hashes.

This is done by ensuring that only Unchanged stats are generated when explicitly loading the second profile.

This is a regression test for a regression that was about to be introduced in patch set 43 of https://codereview.chromium.org/114223002/#ps3400001 (basically the merge made it such that PrefHashFilter::Initialize -- which was just introduced after writting that CL -- would now use pref_hash_store_ directly to StoreHash where as this is wrong for split prefs; I will make this impossible in a following patch set of the afore-mentioned CL).

The technique used to inspect histograms is akin to the one that was removed from the initial pref_metrics_service_unittest.cc in https://codereview.chromium.org/90563003.

BUG=329078, 336478
TEST=New test passes on trunk, but fails on top of patch set 43 of afore-mentioned CL.

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

[gab, 2014-01-30 00:53:18]

Erik/Bernhard PTAL.

Thanks!
Gab

[Bernhard Bauer, 2014-01-30 10:36:52]

lgtm

https://codereview.chromium.org/137383011/diff/20001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/137383011/diff/20001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_browsertest.cc:70: int
GetTrackedPrefHistogramCount(const char* const histogram_name,
Do we actually need both of these const's? I think |histogram_name| itself (i.e.
the variable, not the data behind it) doesn't really need to be const, as it's
local to the method anyway.

[gab, 2014-01-30 14:52:12]

Thanks, done.

https://codereview.chromium.org/137383011/diff/20001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/137383011/diff/20001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_browsertest.cc:70: int
GetTrackedPrefHistogramCount(const char* const histogram_name,
On 2014/01/30 10:36:53, Bernhard Bauer wrote:
> Do we actually need both of these const's? I think |histogram_name| itself
(i.e.
> the variable, not the data behind it) doesn't really need to be const, as it's
> local to the method anyway.

Sure, I like to put const as much as possible, but in this case I agree that the
second const would be like taking "const int" as a parameter which we don't
typically do as it's passed-by-value.

Done.

[erikwright (departed), 2014-01-30 15:38:25]

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

https://codereview.chromium.org/137383011/diff/60001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_browsertest.cc:147: 
I'm confused. I would have thought there would be a bunch of initialized MACs at
this point, since starting the browser up presumably had that effect.

[gab, 2014-01-30 15:58:52]

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

https://codereview.chromium.org/137383011/diff/60001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_browsertest.cc:147: 
On 2014/01/30 15:38:25, erikwright wrote:
> I'm confused. I would have thought there would be a bunch of initialized MACs
at
> this point, since starting the browser up presumably had that effect.

They were initialized in the PRE_* tests; now they should be Unchanged.

[erikwright (departed), 2014-01-30 16:05:57]

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

https://codereview.chromium.org/137383011/diff/60001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_browsertest.cc:147: 
On 2014/01/30 15:58:52, gab wrote:
> On 2014/01/30 15:38:25, erikwright wrote:
> > I'm confused. I would have thought there would be a bunch of initialized
MACs
> at
> > this point, since starting the browser up presumably had that effect.
> 
> They were initialized in the PRE_* tests; now they should be Unchanged.

At the very end of the previous test we Reset them. And verified that there were
only two entries in the Local State hashes dictionary.

AFAICT, one of two things has happened:
(1) Initialization of the reset profile actually occurred some time during
shutdown of the PRE_ test.
(2) Initialization of the reset profile happened (as expected) during launch of
the final test but before we started collecting histograms?

[gab, 2014-01-30 16:19:18]

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

https://codereview.chromium.org/137383011/diff/60001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_browsertest.cc:147: 
On 2014/01/30 16:05:58, erikwright wrote:
> On 2014/01/30 15:58:52, gab wrote:
> > On 2014/01/30 15:38:25, erikwright wrote:
> > > I'm confused. I would have thought there would be a bunch of initialized
> MACs
> > at
> > > this point, since starting the browser up presumably had that effect.
> > 
> > They were initialized in the PRE_* tests; now they should be Unchanged.
> 
> At the very end of the previous test we Reset them. And verified that there
were
> only two entries in the Local State hashes dictionary.
> 
> AFAICT, one of two things has happened:
> (1) Initialization of the reset profile actually occurred some time during
> shutdown of the PRE_ test.
> (2) Initialization of the reset profile happened (as expected) during launch
of
> the final test but before we started collecting histograms?

Oh you're referring to the initialization of the unloaded profile; Initialize()
does initialization directly without reporting (as desired) -- so no
Settings.TrackedPreference* histogram is expected from initializing the unloaded
profile -- only Settings.TrackedPreferencesInitializedForUnloadedProfile::true
is reported for the entire initialization (in fact I could check that this was
indeed reported here to make sure that this phase indeed did the initialization
(and not the shutdown of the previous phase).

[erikwright (departed), 2014-01-30 16:27:47]

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

https://codereview.chromium.org/137383011/diff/60001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_browsertest.cc:147: 
On 2014/01/30 16:19:18, gab wrote:
> On 2014/01/30 16:05:58, erikwright wrote:
> > On 2014/01/30 15:58:52, gab wrote:
> > > On 2014/01/30 15:38:25, erikwright wrote:
> > > > I'm confused. I would have thought there would be a bunch of initialized
> > MACs
> > > at
> > > > this point, since starting the browser up presumably had that effect.
> > > 
> > > They were initialized in the PRE_* tests; now they should be Unchanged.
> > 
> > At the very end of the previous test we Reset them. And verified that there
> were
> > only two entries in the Local State hashes dictionary.
> > 
> > AFAICT, one of two things has happened:
> > (1) Initialization of the reset profile actually occurred some time during
> > shutdown of the PRE_ test.
> > (2) Initialization of the reset profile happened (as expected) during launch
> of
> > the final test but before we started collecting histograms?
> 
> Oh you're referring to the initialization of the unloaded profile;
Initialize()
> does initialization directly without reporting (as desired) -- so no
> Settings.TrackedPreference* histogram is expected from initializing the
unloaded
> profile -- only Settings.TrackedPreferencesInitializedForUnloadedProfile::true
> is reported for the entire initialization (in fact I could check that this was
> indeed reported here to make sure that this phase indeed did the
initialization
> (and not the shutdown of the previous phase).

Ah, right.

Yeah, that would be a good idea, I think.

LGTM, Would LEBTM if you added a check for that.

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

Perfect, added a check for
Settings.TrackedPreferencesInitializedForUnloadedProfile in all 3 phases.

Used the same method and checked for a count of 1 (true is reported once as
1...); I guess I could split the methods as this one doesn't really care about
reporting_ids, but didn't feel it mattered for at test; WDYT?

Cheers,
Gab

[commit-bot: I haz the power, 2014-01-30 17:16:02]

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

[commit-bot: I haz the power, 2014-01-30 21:15:52]

Change committed as 248032

[commit-bot: I haz the power, 2014-01-30 21:15:55]

CQ bit was unchecked on CL. Ignoring.