Improve UMA tests; improve no-op reconcilor; new reconcilor test.

Improve the no-op reconcilor so that it doesn't "hang" after the first execution, but instead successfully completes the reconciliation.
Improve the UMAHistogramHelper so that it can nicely take snapshots between tests, allowing the test writer to easily test ONLY the changes logged in "this" test.
Write a new unit test that executes the reconcilor twice, and make all reconcilor unit tests execute with and without the flag.

BUG=357693
TBR=phajdan.jr@chromium.org

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

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

[Mike Lerman, 2014-06-12 15:46:13]

Hi Reviewers, PTAL.

rogerta - all things account_reconcilor
alexei - UMAHistogramHelper changes
sky - owner for c/test/base

Thanks!

[sky, 2014-06-12 16:59:20]

LGTM

[Alexei Svitkine (slow), 2014-06-12 17:09:15]

https://codereview.chromium.org/309843002/diff/60001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/309843002/diff/60001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor_unittest.cc:36: const int
kNumHistogramsForSnapshot = 6;
You don't need this. Just use [] below and use arraysize(kHistogramsToSnapshot).

https://codereview.chromium.org/309843002/diff/60001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor_unittest.cc:37: const char*
histograms_to_snapshot[kNumHistogramsForSnapshot] = {
kHistogramsToSnapshot?

https://codereview.chromium.org/309843002/diff/60001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor_unittest.cc:134: if
(!::testing::UnitTest::GetInstance()->current_test_info()->value_param()
I don't understand this line. What's it for?

(Each test case will get its own command line, so you don't need to check if its
the first one if that's what this is intending.)

https://codereview.chromium.org/309843002/diff/60001/chrome/test/base/uma_his...
File chrome/test/base/uma_histogram_helper.cc (right):

https://codereview.chromium.org/309843002/diff/60001/chrome/test/base/uma_his...
chrome/test/base/uma_histogram_helper.cc:120: if
(histogram_snapshots.count(name)) {
Nit: How about:

int actual_count = samples.TotalCount();
if (histogram_snapshots.count(name))
  actual_count -= histogram_snapshots[name]->TotalCount();

Same above.

https://codereview.chromium.org/309843002/diff/60001/chrome/test/base/uma_his...
File chrome/test/base/uma_histogram_helper.h (right):

https://codereview.chromium.org/309843002/diff/60001/chrome/test/base/uma_his...
chrome/test/base/uma_histogram_helper.h:17: // To be called before the body of
the test code.
Comment is confusing since you can't generally call a constructor by itself.
Maybe expand to say that a new UMAHistogramHelper should be created for each
test.

https://codereview.chromium.org/309843002/diff/60001/chrome/test/base/uma_his...
chrome/test/base/uma_histogram_helper.h:19: 
Add a destructor declaration and put its implementation into the .cc file (so an
implicit one isn't generated into each object whose cc file include this
header).

https://codereview.chromium.org/309843002/diff/60001/components/signin/core/b...
File components/signin/core/browser/account_reconcilor.cc (right):

https://codereview.chromium.org/309843002/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.cc:716: VLOG(1) <<
"AccountReconcilor::MergeSessionCompleted: account_id="
Does this need to be VLOG(1)? The string will live in the Chrome binary and
contribute to bloat. Can it be DVLOG() instead? (VLOG's are generally
discouraged - per a chromium-dev thread).

https://codereview.chromium.org/309843002/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.cc:763: if
(!refresh_token.empty()) {
Nit: No {}'s

https://codereview.chromium.org/309843002/diff/60001/components/signin/core/b...
File components/signin/core/browser/account_reconcilor.h (right):

https://codereview.chromium.org/309843002/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.h:173: void
MarkAccountAsAddedToChrome(const std::string& account_id);
Is there a reason none of these methods have comments? General Chromium style is
to have 1-line comments above all methods.

[Ilya Sherman, 2014-06-12 17:24:19]

Did you see the base/test/statistics_delta_reader.h class?  It looks like it
might already do most of what you need.  Perhaps we ought to merge that class
and the UMAHistogramHelper class into one?

[Alexei Svitkine (slow), 2014-06-12 17:32:43]

Thanks Ilya, I didn't know of statistics_delta_reader.h. Merging the two
makes sense to me.


On Thu, Jun 12, 2014 at 10:24 AM, <isherman@chromium.org> wrote:

> Did you see the base/test/statistics_delta_reader.h class?  It looks like
> it
> might already do most of what you need.  Perhaps we ought to merge that
> class
> and the UMAHistogramHelper class into one?
>
> https://codereview.chromium.org/309843002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-06-12 17:50:59]

I didn't know the statistics_delta_reader existed. I think all locations
where it's used can be replaced with the new UMAHistogramHelper.
Since I need the account_reconcilor changes to land, my plan was to get
this CL in, and then do a second CL that cleans up other tests that either
have various workarounds (
https://code.google.com/p/chromium/codesearch#chromium/src/dbus/signal_sender...
as
an example) or will also rework the tests that call
statistics_delta_reader. I'll then remove the statistics_delta_reader.


On Thu, Jun 12, 2014 at 1:32 PM, Alexei Svitkine <asvitkine@chromium.org>
wrote:

> Thanks Ilya, I didn't know of statistics_delta_reader.h. Merging the two
> makes sense to me.
>
>
> On Thu, Jun 12, 2014 at 10:24 AM, <isherman@chromium.org> wrote:
>
>> Did you see the base/test/statistics_delta_reader.h class?  It looks
>> like it
>> might already do most of what you need.  Perhaps we ought to merge that
>> class
>> and the UMAHistogramHelper class into one?
>>
>> https://codereview.chromium.org/309843002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mike Lerman, 2014-06-12 18:12:16]

https://codereview.chromium.org/309843002/diff/60001/chrome/browser/signin/ac...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/309843002/diff/60001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor_unittest.cc:36: const int
kNumHistogramsForSnapshot = 6;
On 2014/06/12 17:09:11, Alexei Svitkine wrote:
> You don't need this. Just use [] below and use
arraysize(kHistogramsToSnapshot).

Done.

https://codereview.chromium.org/309843002/diff/60001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor_unittest.cc:37: const char*
histograms_to_snapshot[kNumHistogramsForSnapshot] = {
On 2014/06/12 17:09:11, Alexei Svitkine wrote:
> kHistogramsToSnapshot?

Done.

https://codereview.chromium.org/309843002/diff/60001/chrome/browser/signin/ac...
chrome/browser/signin/account_reconcilor_unittest.cc:134: if
(!::testing::UnitTest::GetInstance()->current_test_info()->value_param()
On 2014/06/12 17:09:11, Alexei Svitkine wrote:
> I don't understand this line. What's it for?
> 
> (Each test case will get its own command line, so you don't need to check if
its
> the first one if that's what this is intending.)

Some of the tests are TEST_F and some are TEST_P (only some should be executed
twice). GetParam() will not work if it's a non-parameterized test - the first
condition tests if there is a parameter or not.
e.g. if (!parameter_exists || parameter == true)

https://codereview.chromium.org/309843002/diff/60001/chrome/test/base/uma_his...
File chrome/test/base/uma_histogram_helper.cc (right):

https://codereview.chromium.org/309843002/diff/60001/chrome/test/base/uma_his...
chrome/test/base/uma_histogram_helper.cc:120: if
(histogram_snapshots.count(name)) {
On 2014/06/12 17:09:11, Alexei Svitkine wrote:
> Nit: How about:
> 
> int actual_count = samples.TotalCount();
> if (histogram_snapshots.count(name))
>   actual_count -= histogram_snapshots[name]->TotalCount();
> 
> Same above.

Slick. Done.

https://codereview.chromium.org/309843002/diff/60001/chrome/test/base/uma_his...
File chrome/test/base/uma_histogram_helper.h (right):

https://codereview.chromium.org/309843002/diff/60001/chrome/test/base/uma_his...
chrome/test/base/uma_histogram_helper.h:17: // To be called before the body of
the test code.
On 2014/06/12 17:09:14, Alexei Svitkine wrote:
> Comment is confusing since you can't generally call a constructor by itself.
> Maybe expand to say that a new UMAHistogramHelper should be created for each
> test.

Done.

https://codereview.chromium.org/309843002/diff/60001/chrome/test/base/uma_his...
chrome/test/base/uma_histogram_helper.h:19: 
On 2014/06/12 17:09:14, Alexei Svitkine wrote:
> Add a destructor declaration and put its implementation into the .cc file (so
an
> implicit one isn't generated into each object whose cc file include this
> header).

Done.

https://codereview.chromium.org/309843002/diff/60001/components/signin/core/b...
File components/signin/core/browser/account_reconcilor.cc (right):

https://codereview.chromium.org/309843002/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.cc:716: VLOG(1) <<
"AccountReconcilor::MergeSessionCompleted: account_id="
On 2014/06/12 17:09:14, Alexei Svitkine wrote:
> Does this need to be VLOG(1)? The string will live in the Chrome binary and
> contribute to bloat. Can it be DVLOG() instead? (VLOG's are generally
> discouraged - per a chromium-dev thread).

We do use these in release builds to debug what's going on in live situations,
so I'm inclined to leave it in. I checked with rogerta@, he says these are in by
design.

https://codereview.chromium.org/309843002/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.cc:763: if
(!refresh_token.empty()) {
On 2014/06/12 17:09:14, Alexei Svitkine wrote:
> Nit: No {}'s

Done.

https://codereview.chromium.org/309843002/diff/60001/components/signin/core/b...
File components/signin/core/browser/account_reconcilor.h (right):

https://codereview.chromium.org/309843002/diff/60001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.h:173: void
MarkAccountAsAddedToChrome(const std::string& account_id);
On 2014/06/12 17:09:15, Alexei Svitkine wrote:
> Is there a reason none of these methods have comments? General Chromium style
is
> to have 1-line comments above all methods.

Added comments for my new methods. I'm not confident enough that I understand
each of the other methods fully to commit to writing comments for all of them.

[Ilya Sherman, 2014-06-12 19:43:45]

How hard would it be to implement one of the test helper classes in terms of the
other?  I don't mind punting test clean up work to follow-up CLs; but I think it
would be ideal, as part of this CL, to (a) avoid duplicating implementation code
and (b) clearly mark one of the test helper classes as deprecated.  If it's a
lot of work, though, then it's probably not worth shoehorning into this CL.

In either case, please file a bug for merging the two helper classes.  It's easy
to lose track of clean-up tasks if they're punted to follow-up CLs, even with
the best of intentions.

[chromium-reviews, 2014-06-12 19:49:21]

I'll create a bug for tracking this, no problem. I'll also note that the
statistics_delta_reader should be considered deprecated as it will go away
in the near future. Is there a standard way to mark a class as deprecated?

I could implement the UMAHistogramHelper such that in uses a
statistics_delta_reader, although I'd have to make some modifications to
both classes and the interface of the statistic_delta_reader. Given neither
class is terribly large, I think for long term maintenance is will be
easier to have just one class.


On Thu, Jun 12, 2014 at 3:43 PM, <isherman@chromium.org> wrote:

> How hard would it be to implement one of the test helper classes in terms
> of the
> other?  I don't mind punting test clean up work to follow-up CLs; but I
> think it
> would be ideal, as part of this CL, to (a) avoid duplicating
> implementation code
> and (b) clearly mark one of the test helper classes as deprecated.  If
> it's a
> lot of work, though, then it's probably not worth shoehorning into this CL.
>
> In either case, please file a bug for merging the two helper classes.
>  It's easy
> to lose track of clean-up tasks if they're punted to follow-up CLs, even
> with
> the best of intentions.
>
> https://codereview.chromium.org/309843002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-06-12 19:54:15]

On 2014/06/12 19:49:21, chromium-reviews wrote:
> I'll create a bug for tracking this, no problem. I'll also note that the
> statistics_delta_reader should be considered deprecated as it will go away
> in the near future. Is there a standard way to mark a class as deprecated?

There's no super standard way that I'm aware of.  I think just a comment with
the word "deprecated" in all-caps is probably good enough.

> I could implement the UMAHistogramHelper such that in uses a
> statistics_delta_reader, although I'd have to make some modifications to
> both classes and the interface of the statistic_delta_reader. Given neither
> class is terribly large, I think for long term maintenance is will be
> easier to have just one class.

Ok, sounds like it's not worth trying to jump through hoops so that we can share
the implementation -- fair enough.  Thanks for checking.

[Mike Lerman, 2014-06-12 19:59:33]

crbug.com/384011

[Mike Lerman, 2014-06-12 20:00:33]

phajdan.jr - PTAL for comment added to statistics_delta_reader. Thanks.

[Roger Tawa OOO till Jul 10th, 2014-06-13 14:37:55]

lgtm

https://codereview.chromium.org/309843002/diff/100001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/309843002/diff/100001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor_unittest.cc:135: || GetParam() == true)
{
GetParam() == true  -->  GetParam()

https://codereview.chromium.org/309843002/diff/100001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor_unittest.cc:135: || GetParam() == true)
{
For my own info, what is the first condition
"!::testing::UnitTest::GetInstance()->current_test_info()->value_param()" for?

[Mike Lerman, 2014-06-13 15:15:27]

https://codereview.chromium.org/309843002/diff/100001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/309843002/diff/100001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor_unittest.cc:135: || GetParam() == true)
{
On 2014/06/13 14:37:55, Roger Tawa wrote:
> For my own info, what is the first condition
> "!::testing::UnitTest::GetInstance()->current_test_info()->value_param()" for?

Tests if we're in a parameterized test or not. There are a mix of TEST_F's and
TEST_P's below.

https://codereview.chromium.org/309843002/diff/100001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor_unittest.cc:135: || GetParam() == true)
{
On 2014/06/13 14:37:55, Roger Tawa wrote:
> GetParam() == true  -->  GetParam()

I can remove the true! Done.

[Mike Lerman, 2014-06-13 17:16:41]

Alexei and Ilya,

Let me know if you have other comments or concerns around this CL.

Thanks,
Mike

[Ilya Sherman, 2014-06-13 19:58:12]

I hadn't actually looked at the code in detail previously.  Since Alexei is
airborne today, I guess I can take over the review from the UMA side of things
:)

https://codereview.chromium.org/309843002/diff/120001/base/test/statistics_de...
File base/test/statistics_delta_reader.h (right):

https://codereview.chromium.org/309843002/diff/120001/base/test/statistics_de...
base/test/statistics_delta_reader.h:23: // references. crbug.com/384011
This class is actually used from //net, which cannot depend on a helper class
defined in //chrome/test/base.  So, FYI, as part of reconciling these two
classes, you'll want to move the helper class into //base/test, same as where
this file lives.

https://codereview.chromium.org/309843002/diff/120001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/309843002/diff/120001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor_unittest.cc:36: const char*
kHistogramsToSnapshot[] = {
nit: "const char* const" (MOAR const :P)

https://codereview.chromium.org/309843002/diff/120001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor_unittest.cc:170:
arraysize(kHistogramsToSnapshot));
nit: Please align the params, or have them both be on a single line.  The
easiest thing to do is to run git cl format over this.

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
File chrome/test/base/uma_histogram_helper.cc (right):

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.cc:22: for (int i = 0; i < num_histograms;
i++) {
nit: "++i"

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
File chrome/test/base/uma_histogram_helper.h (right):

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:24: void PrepareSnapshot(const char*
histogram_names[], int num_histograms);
nit: "int" -> "size_t"

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:24: void PrepareSnapshot(const char*
histogram_names[], int num_histograms);
nit: "const char*" -> "const char* const"

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:24: void PrepareSnapshot(const char*
histogram_names[], int num_histograms);
Actually, is this function needed at all?  Why not just snapshot all histograms
at the time this class is created, as the StatisticsDeltaReader does?

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:31: // should have samples. If
|PrepareSnapshot| was called for |name| histogram,
nit: "for |name| histogram" -> "for the histogram named |name|" or "for the
histogram with the given |name|" or something like that

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:32: // then the |expected_count| is the
diff from the snapshot.
Is it worth documenting what happens if PrepareSnapshot was called multiple
times?

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:65: std::map<std::string,
scoped_ptr<base::HistogramSamples> >
Hmm, I'm like 99% sure that scoped_ptrs aren't safe to store in std::map<>'s.  I
think you want either a linked_ptr<> or to just follow the same pattern used in
the StatisticsDeltaReader class.

[Mike Lerman, 2014-06-16 14:59:14]

https://codereview.chromium.org/309843002/diff/120001/base/test/statistics_de...
File base/test/statistics_delta_reader.h (right):

https://codereview.chromium.org/309843002/diff/120001/base/test/statistics_de...
base/test/statistics_delta_reader.h:23: // references. crbug.com/384011
On 2014/06/13 19:58:11, Ilya Sherman wrote:
> This class is actually used from //net, which cannot depend on a helper class
> defined in //chrome/test/base.  So, FYI, as part of reconciling these two
> classes, you'll want to move the helper class into //base/test, same as where
> this file lives.

I may, although there is code in UMAHistogramHelper which needs to live within
Chrome. This class is only referenced once within //net, so I may just make that
a special case... but that conversation can happen in the other bug :)

https://codereview.chromium.org/309843002/diff/120001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/309843002/diff/120001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor_unittest.cc:36: const char*
kHistogramsToSnapshot[] = {
On 2014/06/13 19:58:11, Ilya Sherman wrote:
> nit: "const char* const" (MOAR const :P)

Done. KKKKAAAAAHHHHHHHHNNNNNst!

https://codereview.chromium.org/309843002/diff/120001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor_unittest.cc:170:
arraysize(kHistogramsToSnapshot));
On 2014/06/13 19:58:11, Ilya Sherman wrote:
> nit: Please align the params, or have them both be on a single line.  The
> easiest thing to do is to run git cl format over this.

TIL about git cl format. Thanks!

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
File chrome/test/base/uma_histogram_helper.cc (right):

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.cc:22: for (int i = 0; i < num_histograms;
i++) {
On 2014/06/13 19:58:12, Ilya Sherman wrote:
> nit: "++i"

Done.

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
File chrome/test/base/uma_histogram_helper.h (right):

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:24: void PrepareSnapshot(const char*
histogram_names[], int num_histograms);
On 2014/06/13 19:58:12, Ilya Sherman wrote:
> nit: "const char*" -> "const char* const"

Done.

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:24: void PrepareSnapshot(const char*
histogram_names[], int num_histograms);
On 2014/06/13 19:58:12, Ilya Sherman wrote:
> nit: "int" -> "size_t"

Done.

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:24: void PrepareSnapshot(const char*
histogram_names[], int num_histograms);
On 2014/06/13 19:58:12, Ilya Sherman wrote:
> Actually, is this function needed at all?  Why not just snapshot all
histograms
> at the time this class is created, as the StatisticsDeltaReader does?

Efficiency's sake? There can be dozens if not hundreds of histograms in the UMA
system but we likely only care about a small number. I assume that only taking
snapshots of a few is faster, and speed of unit test execution is a good thing.

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:31: // should have samples. If
|PrepareSnapshot| was called for |name| histogram,
On 2014/06/13 19:58:12, Ilya Sherman wrote:
> nit: "for |name| histogram" -> "for the histogram named |name|" or "for the
> histogram with the given |name|" or something like that

Done.

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:32: // then the |expected_count| is the
diff from the snapshot.
On 2014/06/13 19:58:12, Ilya Sherman wrote:
> Is it worth documenting what happens if PrepareSnapshot was called multiple
> times?

Yes. I wrote wordy text, but would almost rather right something like "Expect
Map mechanics using the histogram's name as a key and the histogram's snapshot
as the value," as this should be clear to most programmers.

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:65: std::map<std::string,
scoped_ptr<base::HistogramSamples> >
On 2014/06/13 19:58:12, Ilya Sherman wrote:
> Hmm, I'm like 99% sure that scoped_ptrs aren't safe to store in std::map<>'s. 
I
> think you want either a linked_ptr<> or to just follow the same pattern used
in
> the StatisticsDeltaReader class.

Good to know. I'll go with a linked_ptr. Since I'm explicitly supporting
multiple calls, and for a unit test I'd rather delegate managing the memory
references to the pointer class.

[Mike Lerman, 2014-06-16 15:00:29]

Alexei, shall I remove you from the reviewer list? Since Ilya will be providing
UMA-related commentary?

[Alexei Svitkine (slow), 2014-06-16 15:20:54]

On 2014/06/16 15:00:29, Mike Lerman wrote:
> Alexei, shall I remove you from the reviewer list? Since Ilya will be
providing
> UMA-related commentary?

Up to you. I'm fine with Ilya continuing the review or you can keep me as a
second pair of eyes.

[Ilya Sherman, 2014-06-16 18:11:29]

LGTM.  Thanks, Mike!

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
File chrome/test/base/uma_histogram_helper.h (right):

https://codereview.chromium.org/309843002/diff/120001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.h:24: void PrepareSnapshot(const char*
histogram_names[], int num_histograms);
On 2014/06/16 14:59:14, Mike Lerman wrote:
> On 2014/06/13 19:58:12, Ilya Sherman wrote:
> > Actually, is this function needed at all?  Why not just snapshot all
> histograms
> > at the time this class is created, as the StatisticsDeltaReader does?
> 
> Efficiency's sake? There can be dozens if not hundreds of histograms in the
UMA
> system but we likely only care about a small number. I assume that only taking
> snapshots of a few is faster, and speed of unit test execution is a good
thing.

I'm not convinced that the small bit of efficiency is worthwhile, given that
this requires test authors to have to add a bunch of boilerplate to their test
code.  Unless you measure the performance delta, and find that there is a
significant difference, I'd prefer to go with the option that's simpler for test
authors.  But, I don't feel super strongly about this.

[Mike Lerman, 2014-06-16 19:26:39]

Ilya, I've made a note in the crbug.com for the follow-up work around this.

Alexei, I'll leave you on. If you're willing to look, the more eyes the better!

[Alexei Svitkine (slow), 2014-06-16 19:30:26]

lgtm % a couple of comments

https://codereview.chromium.org/309843002/diff/160001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/309843002/diff/160001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor_unittest.cc:124: };
Nit: Add DISALLOW_COPY_AND_ASSIGN() at the bottom.

https://codereview.chromium.org/309843002/diff/160001/chrome/test/base/uma_hi...
File chrome/test/base/uma_histogram_helper.cc (right):

https://codereview.chromium.org/309843002/diff/160001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.cc:31: histogram_snapshots[histogram_name]
= linked_ptr<base::HistogramSamples>(
Nit: make_linked_ptr and wrap after the =

[Mike Lerman, 2014-06-16 19:55:45]

Thanks Alexei.

Moving phajdan to TBD, since the only change affecting that file is a comment.

https://codereview.chromium.org/309843002/diff/160001/chrome/browser/signin/a...
File chrome/browser/signin/account_reconcilor_unittest.cc (right):

https://codereview.chromium.org/309843002/diff/160001/chrome/browser/signin/a...
chrome/browser/signin/account_reconcilor_unittest.cc:124: };
On 2014/06/16 19:30:26, Alexei Svitkine wrote:
> Nit: Add DISALLOW_COPY_AND_ASSIGN() at the bottom.

Done.

https://codereview.chromium.org/309843002/diff/160001/chrome/test/base/uma_hi...
File chrome/test/base/uma_histogram_helper.cc (right):

https://codereview.chromium.org/309843002/diff/160001/chrome/test/base/uma_hi...
chrome/test/base/uma_histogram_helper.cc:31: histogram_snapshots[histogram_name]
= linked_ptr<base::HistogramSamples>(
On 2014/06/16 19:30:26, Alexei Svitkine wrote:
> Nit: make_linked_ptr and wrap after the =

Done.

[Mike Lerman, 2014-06-16 23:50:31]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-06-16 23:53:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/309843002/180001

[commit-bot: I haz the power, 2014-06-17 01:03:39]

Change committed as 277605

[hirono, 2014-06-18 06:54:42]

A revert of this CL has been created in
https://codereview.chromium.org/338883006/ by hirono@chromium.org.

The reason for reverting is: This is causes crushes about 20 secs after
launching chromeos build on the linux.
.

[Mike Lerman, 2014-06-18 13:07:22]

On 2014/06/18 06:54:42, hirono wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/338883006/ by mailto:hirono@chromium.org.
> 
> The reason for reverting is: This is causes crushes about 20 secs after
> launching chromeos build on the linux.
> .

Note: The revert is actually https://codereview.chromium.org/344513005/

[Mike Lerman, 2014-06-18 13:53:15]

This was fixed in CL https://codereview.chromium.org/339223004/ which has since
landed.

Resubmitted to the CQ.

[Mike Lerman, 2014-06-18 13:53:34]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-06-18 13:54:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/309843002/180001

[Mike Lerman, 2014-06-18 14:06:52]

The CQ bit was unchecked by mlerman@chromium.org

[Mike Lerman, 2014-06-18 14:20:16]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-06-18 14:20:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/309843002/180001

[Mike Lerman, 2014-06-18 14:21:25]

On 2014/06/18 14:20:16, Mike Lerman wrote:
> The CQ bit was checked by mailto:mlerman@chromium.org

After discussion with Roger, we will let this CL land. Afterwards we will make a
small tweak to the account_reconcilor_unittest for the code that rogerta@ had
modified.
(https://codereview.chromium.org/344513005/diff/1/chrome/browser/signin/accoun...)

[commit-bot: I haz the power, 2014-06-18 15:33:33]

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

[commit-bot: I haz the power, 2014-06-18 15:33:34]

Failed to apply patch for chrome/browser/signin/account_reconcilor_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/signin/account_reconcilor_unittest.cc
  Hunk #1 FAILED at 19.
  Hunk #2 succeeded at 34 (offset 1 line).
  Hunk #3 succeeded at 84 (offset 1 line).
  Hunk #4 succeeded at 92 (offset 1 line).
  Hunk #5 succeeded at 121 (offset 1 line).
  Hunk #6 FAILED at 132.
  Hunk #7 succeeded at 163 (offset 1 line).
  Hunk #8 succeeded at 354 (offset 1 line).
  Hunk #9 succeeded at 383 (offset 1 line).
  Hunk #10 succeeded at 399 (offset 1 line).
  Hunk #11 succeeded at 430 (offset 1 line).
  Hunk #12 succeeded at 473 (offset 1 line).
  Hunk #13 succeeded at 506 (offset 1 line).
  Hunk #14 succeeded at 628 (offset 1 line).
  Hunk #15 succeeded at 669 (offset 1 line).
  Hunk #16 succeeded at 704 (offset 1 line).
  Hunk #17 succeeded at 736 (offset 1 line).
  2 out of 17 hunks FAILED -- saving rejects to file
chrome/browser/signin/account_reconcilor_unittest.cc.rej

Patch:       chrome/browser/signin/account_reconcilor_unittest.cc
Index: chrome/browser/signin/account_reconcilor_unittest.cc
diff --git a/chrome/browser/signin/account_reconcilor_unittest.cc
b/chrome/browser/signin/account_reconcilor_unittest.cc
index
26bdac96f60c15fe2d5926664cb46da48ace2bd0..9266b95a9391b034e2c3aecece8519dc0da0a0fb
100644
--- a/chrome/browser/signin/account_reconcilor_unittest.cc
+++ b/chrome/browser/signin/account_reconcilor_unittest.cc
@@ -19,10 +19,10 @@
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/testing_profile_manager.h"
+#include "chrome/test/base/uma_histogram_helper.h"
 #include "components/signin/core/browser/account_reconcilor.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager.h"
-//#include "components/signin/core/browser/test_signin_client.h"
 #include "components/signin/core/common/signin_switches.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "google_apis/gaia/gaia_urls.h"
@@ -33,6 +33,13 @@
 namespace {
 
 const char kTestEmail[] = "user@gmail.com";
+const char* const kHistogramsToSnapshot[] = {
+    "Signin.Reconciler.DifferentPrimaryAccounts.FirstRun",
+    "Signin.Reconciler.AddedToCookieJar.FirstRun",
+    "Signin.Reconciler.AddedToChrome.FirstRun",
+    "Signin.Reconciler.DifferentPrimaryAccounts.SubsequentRun",
+    "Signin.Reconciler.AddedToCookieJar.SubsequentRun",
+    "Signin.Reconciler.AddedToChrome.SubsequentRun"};
 
 class MockAccountReconcilor : public testing::StrictMock<AccountReconcilor> {
  public:
@@ -76,7 +83,7 @@ MockAccountReconcilor::MockAccountReconcilor(
 
 }  // namespace
 
-class AccountReconcilorTest : public testing::Test {
+class AccountReconcilorTest : public ::testing::TestWithParam<bool> {
  public:
   AccountReconcilorTest();
   virtual void SetUp() OVERRIDE;
@@ -84,6 +91,7 @@ class AccountReconcilorTest : public testing::Test {
   TestingProfile* profile() { return profile_; }
   FakeSigninManagerForTesting* signin_manager() { return signin_manager_; }
   FakeProfileOAuth2TokenService* token_service() { return token_service_; }
+  UMAHistogramHelper* histogram_helper() { return &histogram_helper_; }
 
   void SetFakeResponse(const std::string& url,
                        const std::string& data,
@@ -112,6 +120,9 @@ class AccountReconcilorTest : public testing::Test {
   MockAccountReconcilor* mock_reconcilor_;
   net::FakeURLFetcherFactory url_fetcher_factory_;
   scoped_ptr<TestingProfileManager> testing_profile_manager_;
+  UMAHistogramHelper histogram_helper_;
+
+  DISALLOW_COPY_AND_ASSIGN(AccountReconcilorTest);
 };
 
 AccountReconcilorTest::AccountReconcilorTest()
@@ -121,8 +132,12 @@ AccountReconcilorTest::AccountReconcilorTest()
       url_fetcher_factory_(NULL) {}
 
 void AccountReconcilorTest::SetUp() {
-  CommandLine::ForCurrentProcess()->AppendSwitch(
-      switches::kNewProfileManagement);
+  // If it's a non-parameterized test, or we have a parameter of true, set
flag.
+  if (!::testing::UnitTest::GetInstance()->current_test_info()->value_param()
||
+      GetParam()) {
+    CommandLine::ForCurrentProcess()->AppendSwitch(
+        switches::kNewProfileManagement);
+  }
 
   testing_profile_manager_.reset(
       new TestingProfileManager(TestingBrowserProcess::GetGlobal()));
@@ -151,6 +166,10 @@ void AccountReconcilorTest::SetUp() {
   token_service_ =
       static_cast<FakeProfileOAuth2TokenService*>(
           ProfileOAuth2TokenServiceFactory::GetForProfile(profile()));
+
+  // Take a new snapshot of the concerned histograms before each test
+  histogram_helper_.PrepareSnapshot(kHistogramsToSnapshot,
+                                    arraysize(kHistogramsToSnapshot));
 }
 
 MockAccountReconcilor* AccountReconcilorTest::GetMockReconcilor() {
@@ -338,7 +357,7 @@ TEST_F(AccountReconcilorTest,
ValidateAccountsFromTokensFailedTokenRequest) {
   ASSERT_EQ(1u, reconcilor->GetInvalidChromeAccountsForTesting().size());
 }
 
-TEST_F(AccountReconcilorTest, StartReconcileNoop) {
+TEST_P(AccountReconcilorTest, StartReconcileNoop) {
   signin_manager()->SetAuthenticatedUsername(kTestEmail);
   token_service()->UpdateCredentials(kTestEmail, "refresh_token");
 
@@ -367,6 +386,12 @@ TEST_F(AccountReconcilorTest, StartReconcileNoop) {
   base::RunLoop().RunUntilIdle();
   ASSERT_TRUE(reconcilor->AreAllRefreshTokensChecked());
   ASSERT_FALSE(reconcilor->is_reconcile_started_);
+
+  histogram_helper()->Fetch();
+  histogram_helper()->ExpectTotalCount(
+      "Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 1);
+  histogram_helper()->ExpectUniqueSample(
+      "Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 0, 1);
 }
 
 // This is test is needed until chrome changes to use gaia obfuscated id.
@@ -377,7 +402,7 @@ TEST_F(AccountReconcilorTest, StartReconcileNoop) {
 // tests makes sure that an email like "Dot.S@hmail.com", as seen by the
 // token service, will be considered the same as "dots@gmail.com" as returned
 // by gaia::ParseListAccountsData().
-TEST_F(AccountReconcilorTest, StartReconcileNoopWithDots) {
+TEST_P(AccountReconcilorTest, StartReconcileNoopWithDots) {
   signin_manager()->SetAuthenticatedUsername("Dot.S@gmail.com");
   token_service()->UpdateCredentials("Dot.S@gmail.com", "refresh_token");
 
@@ -408,9 +433,13 @@ TEST_F(AccountReconcilorTest, StartReconcileNoopWithDots) {
   base::RunLoop().RunUntilIdle();
   ASSERT_TRUE(reconcilor->AreAllRefreshTokensChecked());
   ASSERT_FALSE(reconcilor->is_reconcile_started_);
+
+  histogram_helper()->Fetch();
+  histogram_helper()->ExpectUniqueSample(
+      "Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 0, 1);
 }
 
-TEST_F(AccountReconcilorTest, StartReconcileNoopMultiple) {
+TEST_P(AccountReconcilorTest, StartReconcileNoopMultiple) {
   signin_manager()->SetAuthenticatedUsername("user@gmail.com");
   token_service()->UpdateCredentials("user@gmail.com", "refresh_token");
   token_service()->UpdateCredentials("other@gmail.com", "refresh_token");
@@ -447,9 +476,15 @@ TEST_F(AccountReconcilorTest, StartReconcileNoopMultiple) {
   base::RunLoop().RunUntilIdle();
   ASSERT_TRUE(reconcilor->AreAllRefreshTokensChecked());
   ASSERT_FALSE(reconcilor->is_reconcile_started_);
+
+  histogram_helper()->Fetch();
+  histogram_helper()->ExpectTotalCount(
+      "Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 1);
+  histogram_helper()->ExpectUniqueSample(
+      "Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 0, 1);
 }
 
-TEST_F(AccountReconcilorTest, StartReconcileAddToCookie) {
+TEST_P(AccountReconcilorTest, StartReconcileAddToCookie) {
   signin_manager()->SetAuthenticatedUsername("user@gmail.com");
   token_service()->UpdateCredentials("user@gmail.com", "refresh_token");
   token_service()->UpdateCredentials("other@gmail.com", "refresh_token");
@@ -474,9 +509,106 @@ TEST_F(AccountReconcilorTest, StartReconcileAddToCookie) {
   SimulateMergeSessionCompleted(reconcilor, "other@gmail.com",
                                 GoogleServiceAuthError::AuthErrorNone());
   ASSERT_FALSE(reconcilor->is_reconcile_started_);
+
+  histogram_helper()->Fetch();
+  histogram_helper()->ExpectUniqueSample(
+      "Signin.Reconciler.DifferentPrimaryAccounts.FirstRun", 0, 1);
+  histogram_helper()->ExpectUniqueSample(
+      "Signin.Reconciler.AddedToCookieJar.FirstRun", 1, 1);
+  histogram_helper()->ExpectUniqueSample(
+      "Signin.Reconciler.AddedToChrome.FirstRun", 0, 1);
+}
+
+TEST_P(AccountReconcilorTest, StartReconcileAddToCookieTwice) {
+  signin_manager()->SetAuthenticatedUsername("user@gmail.com");
+  token_service()->UpdateCredentials("user@gmail.com", "refresh_token");
+  token_service()->UpdateCredentials("other@gmail.com", "refresh_token");
+
+  EXPECT_CALL(*GetMockReconcilor(), PerformMergeAction("other@gmail.com"));
+  EXPECT_CALL(*GetMockReconcilor(), PerformMergeAction("third@gmail.com"));
+
+  SetFakeResponse(
+      GaiaUrls::GetInstance()->list_accounts_url().spec(),
+      "[\"f\", [[\"b\", 0, \"n\", \"user@gmail.com\", \"p\", 0, 0, 0, 0, 1]]]",
+      net::HTTP_OK,
+      net::URLRequestStatus::SUCCESS);
+  SetFakeResponse(GaiaUrls::GetInstance()->people_get_url().spec(),
+                  "{\"id\":\"foo\"}",
+                  net::HTTP_OK,
+                  net::URLRequestStatus::SUCCESS);
+
+  AccountReconcilor* reconcilor = GetMockReconcilor();
+  reconcilor->StartReconcile();
+  token_service()->IssueAllTokensForAccount(
+      "other@gmail.com",
+      "access_token",
+      base::Time::Now() + base::TimeDelta::FromHours(1));
+  token_service()->IssueAllTokensForAccount(
+      "user@gmail.com",
+      "access_token",
+      base::Time::Now() + base::TimeDelta::FromHours(1));
+
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(reconcilor->is_reconcile_started_);
+  SimulateMergeSessionCompleted(
+      reconcilor,…
(message too large)

[Mike Lerman, 2014-06-18 15:47:39]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-06-18 15:49:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/309843002/200001

[commit-bot: I haz the power, 2014-06-19 01:24:08]

Change committed as 278223

[Paweł Hajdan Jr., 2014-07-25 14:31:46]

LGTM