Rework UMAHistogramHelper and StatisticsDeltaReader into [Chrome]HistogramTester.

Rework UMAHistogramHelper and StatisticsDeltaReader into [Chrome]HistogramTester.

Design Doc (Googlers Only):
https://docs.google.com/a/google.com/document/d/1RUY0BcxBppdkwFP3T8qbQmQStBRFGsLmuR4WuIXhiCA/edit

BUG=384011
TBR=timsteele@chromium.org

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

[Mike Lerman, 2014-07-14 17:03:59]

Hi Ilya,

I'm reading to start getting this reviewed. Let me know if you have any
feedback!

Thanks,
Mike

[Mike Lerman, 2014-07-14 18:45:23]

Phajdan,

I'll also get you to review at this stage, before I send these changes out for
wider review, as you own the test folders.

Thanks!

[Ilya Sherman, 2014-07-15 03:56:36]

Thanks, Mike!  This is very welcome cleanup :)

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:21: // or otherwise. Tests can use this interface
to verify that Histograms data is
nit: "Histograms" -> "histograms"

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:24: // |ChromeHistogramTester| class.
nit: Please omit this sentence -- it's actually better to use the simpler class
where feasible, IMO; and it's also awkward to refer to a higher-level source
directory from a lower-level one.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:35: virtual void FetchTestingSnapshot();
It looks like this method can be omitted from the base class, and just defined
in the //chrome subclass.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:39: // named |name| then the |expected_count| is
the diff from the snapshot.
Please update this comment -- PrepareSnapshot() is no longer a method in this
class.  Ditto for the other comments.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:49: base::HistogramBase::Count expected_count);
nit: Please fix up the alignment of these two lines.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:58: scoped_ptr<HistogramSamples>
GetBaseHistogramSamples(
nit: What does the word "base" mean in this context?

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:59: const std::string& histogram_name);
Is this method still useful, or can all clients of this class be served by the
three ExpectFoo() methods defined above?

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:69: base::HistogramSamples& samples);
Please document these two methods.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:74: std::map<std::string, HistogramSamples*>
base_snapshot_;
nit: WDYT of "initial_snapshot_" rather than "base_snapshot_"?

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
File base/test/histogram_tester_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester_unittest.cc:32: }
Please add test coverage for the other methods in the public API as well.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:64: #include
"chrome/test/base/chrome_histogram_tester.h"
nit: Can you include just the //base class, rather than the //chrome class?

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1510: base::HistogramTester*
GetHistogramTester() {
nit: This is a simple getter, so it should be named using hacker_case -- i.e.
histogram_tester().  The variable name should also match the method name.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1510: base::HistogramTester*
GetHistogramTester() {
nit: Can you return a const-reference instead?

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:2086: // Checks that we get
the right PPLT histograms_ for client redirect prerenders
nit: Please revert this change.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/search/s...
File chrome/browser/search/suggestions/blacklist_store_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/search/s...
chrome/browser/search/suggestions/blacklist_store_unittest.cc:12: #include
"chrome/test/base/chrome_histogram_tester.h"
nit: Please include the //base class rather than the //chrome class here.  Ditto
for all the other test files that don't use the //chrome-layer functionality.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/search/s...
chrome/browser/search/suggestions/blacklist_store_unittest.cc:144:
histogram_tester.ExpectBucketCount("Suggestions.LocalBlacklistSize", 2, 1);
Hmm, it's a bit unfortunate that both the second and the third parameters to
this function are of integer type -- it makes the semantics of the method call
somewhat hard to deduce at the call site.  I don't have any great ideas on how
to improve this, though -- do you?

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_host_metrics_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_host_metrics_unittest.cc:84: const char*
histogramName[] = {
nit: As long as you're here, please update this line to be "const char* const
histogram_names[] = {"

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_host_metrics_unittest.cc:96: // Start the
reader.
nit: Please update (or remove) this comment.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_view_test.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_view_test.cc:94:
histogram).release();
nit: Please rewrap this line (probably wise to run "git cl format" over this CL,
since you're editing a bunch of files in a fairly mechanical manner).

https://codereview.chromium.org/379283002/diff/300001/chrome/renderer/net/net...
File chrome/renderer/net/net_error_helper_core_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/renderer/net/net...
chrome/renderer/net/net_error_helper_core_unittest.cc:2242: }
nit: Please inline the calls to histogram_tester_.ExpectTotalCount() rather than
having this wrapper method.

https://codereview.chromium.org/379283002/diff/300001/chrome/renderer/net/net...
chrome/renderer/net/net_error_helper_core_unittest.cc:2250: private:
Note: There's no need to make the HistogramTester private if you'd just make a
protected (or public) accessor -- for FooTest classes, it's quite common to have
protected data members to limit boiler plate code.

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.cc:12: #include
"testing/gtest/include/gtest/gtest.h"
nit: Is this include needed?

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.cc:15:
base::StatisticsRecorder::Initialize();
This is already called by the base class's constructor, right?

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.cc:34: content::RunMessageLoop();
Is it possible for content::FetchHistogramsAsynchronously() to synchronously
call back into the callback?  If so, this is a race condition: You might try to
quit the message loop prior to running it, and then run it with no Quit()
callback in sight.  Using a MessageLoopRunner would be better.

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.h (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:12: #include
"base/metrics/histogram_base.h"
nit: Are these two includes needed?

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:17: // logged as intended.
Please document how this class differs from base::HistogramTester.

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:32: void FetchCallback();
nit: Please document this method.  Also, perhaps "OnFetchCompleted()" would be a
better name for this method?

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:34:
DISALLOW_COPY_AND_ASSIGN(ChromeHistogramTester);
nit: This should be the last line in the class.

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:38: histogram_snapshots;
Is this actually needed?  I don't see it being used anywhere.

[Mike Lerman, 2014-07-16 17:29:04]

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:21: // or otherwise. Tests can use this interface
to verify that Histograms data is
On 2014/07/15 03:56:34, Ilya Sherman wrote:
> nit: "Histograms" -> "histograms"

Done.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:24: // |ChromeHistogramTester| class.
On 2014/07/15 03:56:34, Ilya Sherman wrote:
> nit: Please omit this sentence -- it's actually better to use the simpler
class
> where feasible, IMO; and it's also awkward to refer to a higher-level source
> directory from a lower-level one.

Done.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:35: virtual void FetchTestingSnapshot();
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> It looks like this method can be omitted from the base class, and just defined
> in the //chrome subclass.

Done.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:39: // named |name| then the |expected_count| is
the diff from the snapshot.
On 2014/07/15 03:56:34, Ilya Sherman wrote:
> Please update this comment -- PrepareSnapshot() is no longer a method in this
> class.  Ditto for the other comments.

Done.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:49: base::HistogramBase::Count expected_count);
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> nit: Please fix up the alignment of these two lines.

Done.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:58: scoped_ptr<HistogramSamples>
GetBaseHistogramSamples(
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> nit: What does the word "base" mean in this context?

Nothing useful. Name and comment changed.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:59: const std::string& histogram_name);
On 2014/07/15 03:56:34, Ilya Sherman wrote:
> Is this method still useful, or can all clients of this class be served by the
> three ExpectFoo() methods defined above?

I had hoped so, but there is at least one test that does other math using the
snapshot's value, so I have to still expose this.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:69: base::HistogramSamples& samples);
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> Please document these two methods.

Done.

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester.h:74: std::map<std::string, HistogramSamples*>
base_snapshot_;
On 2014/07/15 03:56:34, Ilya Sherman wrote:
> nit: WDYT of "initial_snapshot_" rather than "base_snapshot_"?

There's only one snapshot, so it doesn't really need a prefix at all that
relates to timing. If any prefix is supplied, it should describe that this
contains data of all histogram samples. So perhaps histograms_snapshot_?

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
File base/test/histogram_tester_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/300001/base/test/histogram_tes...
base/test/histogram_tester_unittest.cc:32: }
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> Please add test coverage for the other methods in the public API as well.

Done. I can add tests that will succeed, (e.g. log a 1, test for a 1), but I
don't think there's a way in GTest if I want an EXPECT_* to fail (e.g. log a 2,
call "ExpectUniqueSample" looking for 1), which means I can only test happy
paths.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:64: #include
"chrome/test/base/chrome_histogram_tester.h"
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> nit: Can you include just the //base class, rather than the //chrome class?

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1510: base::HistogramTester*
GetHistogramTester() {
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> nit: This is a simple getter, so it should be named using hacker_case -- i.e.
> histogram_tester().  The variable name should also match the method name.

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1510: base::HistogramTester*
GetHistogramTester() {
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> nit: Can you return a const-reference instead?

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:2086: // Checks that we get
the right PPLT histograms_ for client redirect prerenders
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> nit: Please revert this change.

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/search/s...
File chrome/browser/search/suggestions/blacklist_store_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/search/s...
chrome/browser/search/suggestions/blacklist_store_unittest.cc:12: #include
"chrome/test/base/chrome_histogram_tester.h"
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> nit: Please include the //base class rather than the //chrome class here. 
Ditto
> for all the other test files that don't use the //chrome-layer functionality.

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/search/s...
chrome/browser/search/suggestions/blacklist_store_unittest.cc:144:
histogram_tester.ExpectBucketCount("Suggestions.LocalBlacklistSize", 2, 1);
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> Hmm, it's a bit unfortunate that both the second and the third parameters to
> this function are of integer type -- it makes the semantics of the method call
> somewhat hard to deduce at the call site.  I don't have any great ideas on how
> to improve this, though -- do you?

Use Objective-C? I agree, this is ugly. Can't think of much. Well... I've seen
unit testing libraries for Java ... we'd have an API like:

histogram_tester.ForHistogram("blah blah").ForSample(2).ExpectValue(1);
or 
histogram_tester.ForHistogram("blah blah").ForUniqueSample(2).ExpectValue(1);
or
histogram_tester.ForHistogram("foo").ExpectTotalCount(2);

I've always found that kind of syntax really wordy in Java, especially since I
was usually working in Eclipse with Javadoc & Intellisense. Here... maybe it's
the right answer? What do you think? I could sketch out the necessary classes
and signatures in the design doc.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_host_metrics_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_host_metrics_unittest.cc:84: const char*
histogramName[] = {
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> nit: As long as you're here, please update this line to be "const char* const
> histogram_names[] = {"

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_host_metrics_unittest.cc:96: // Start the
reader.
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> nit: Please update (or remove) this comment.

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_view_test.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_view_test.cc:94:
histogram).release();
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> nit: Please rewrap this line (probably wise to run "git cl format" over this
CL,
> since you're editing a bunch of files in a fairly mechanical manner).

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/renderer/net/net...
File chrome/renderer/net/net_error_helper_core_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/renderer/net/net...
chrome/renderer/net/net_error_helper_core_unittest.cc:2242: }
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> nit: Please inline the calls to histogram_tester_.ExpectTotalCount() rather
than
> having this wrapper method.

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/renderer/net/net...
chrome/renderer/net/net_error_helper_core_unittest.cc:2250: private:
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> Note: There's no need to make the HistogramTester private if you'd just make a
> protected (or public) accessor -- for FooTest classes, it's quite common to
have
> protected data members to limit boiler plate code.

It can stay private.

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.cc:12: #include
"testing/gtest/include/gtest/gtest.h"
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> nit: Is this include needed?

Nope!

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.cc:15:
base::StatisticsRecorder::Initialize();
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> This is already called by the base class's constructor, right?

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.cc:34: content::RunMessageLoop();
On 2014/07/15 03:56:35, Ilya Sherman wrote:
> Is it possible for content::FetchHistogramsAsynchronously() to synchronously
> call back into the callback?  If so, this is a race condition: You might try
to
> quit the message loop prior to running it, and then run it with no Quit()
> callback in sight.  Using a MessageLoopRunner would be better.

I don't think there's a way for it to be run synchronously. Even if
FetchTestingSnapshot() is called twice, the tasks are still posted to the
MessageLoop. But I don't know a lot about the MessageLoop - I'll look into the
MessageLoopRunner.

I think I made the change you requested - the NaCl tests still past. Please make
sure I understand how this was supposed to work correctly.

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.h (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:12: #include
"base/metrics/histogram_base.h"
On 2014/07/15 03:56:36, Ilya Sherman wrote:
> nit: Are these two includes needed?

These four aren't needed!

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:17: // logged as intended.
On 2014/07/15 03:56:36, Ilya Sherman wrote:
> Please document how this class differs from base::HistogramTester.

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:32: void FetchCallback();
On 2014/07/15 03:56:36, Ilya Sherman wrote:
> nit: Please document this method.  Also, perhaps "OnFetchCompleted()" would be
a
> better name for this method?

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:34:
DISALLOW_COPY_AND_ASSIGN(ChromeHistogramTester);
On 2014/07/15 03:56:36, Ilya Sherman wrote:
> nit: This should be the last line in the class.

Done.

https://codereview.chromium.org/379283002/diff/300001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:38: histogram_snapshots;
On 2014/07/15 03:56:36, Ilya Sherman wrote:
> Is this actually needed?  I don't see it being used anywhere.

Done.

[Ilya Sherman, 2014-07-16 18:44:47]

Thanks, Mike!

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/search/s...
File chrome/browser/search/suggestions/blacklist_store_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/search/s...
chrome/browser/search/suggestions/blacklist_store_unittest.cc:144:
histogram_tester.ExpectBucketCount("Suggestions.LocalBlacklistSize", 2, 1);
On 2014/07/16 17:29:03, Mike Lerman wrote:
> On 2014/07/15 03:56:35, Ilya Sherman wrote:
> > Hmm, it's a bit unfortunate that both the second and the third parameters to
> > this function are of integer type -- it makes the semantics of the method
call
> > somewhat hard to deduce at the call site.  I don't have any great ideas on
how
> > to improve this, though -- do you?
> 
> Use Objective-C? I agree, this is ugly. Can't think of much. Well... I've seen
> unit testing libraries for Java ... we'd have an API like:
> 
> histogram_tester.ForHistogram("blah blah").ForSample(2).ExpectValue(1);
> or 
> histogram_tester.ForHistogram("blah blah").ForUniqueSample(2).ExpectValue(1);
> or
> histogram_tester.ForHistogram("foo").ExpectTotalCount(2);
> 
> I've always found that kind of syntax really wordy in Java, especially since I
> was usually working in Eclipse with Javadoc & Intellisense. Here... maybe it's
> the right answer? What do you think? I could sketch out the necessary classes
> and signatures in the design doc.

Hmm, that's definitely an interesting thought, worth exploring more.

Another idea is to change the ExpectFoo() methods to be GetFoo() methods instead
-- then we'd have something like

EXPECT_EQ(1, histogram_tester.GetBucketCount("Suggestions.LocalBlacklistSize",
2));

That's at least somewhat clearer -- the 2 is still a little mysterious, but at
least the 1 is more clearly a count within a bucket.  WDYT?

In any case, I think it's reasonable to punt this question to a follow-up CL, so
that this one can remain a little more focused.

https://codereview.chromium.org/379283002/diff/340001/base/test/histogram_tes...
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/379283002/diff/340001/base/test/histogram_tes...
base/test/histogram_tester.h:21: // or otherwise. Tests can use this interface
to verify that Histogram data is
nit: Uppercase "Histogram" should be lowercase.

https://codereview.chromium.org/379283002/diff/340001/base/test/histogram_tes...
base/test/histogram_tester.h:33: const void ExpectUniqueSample(const
std::string& name,
"const void" doesn't make a lot of sense as a return type.  Did you mean to mark
the method as const?

https://codereview.chromium.org/379283002/diff/340001/base/test/histogram_tes...
File base/test/histogram_tester_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/340001/base/test/histogram_tes...
base/test/histogram_tester_unittest.cc:62: 
nit: Spurious newline

https://codereview.chromium.org/379283002/diff/340001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/379283002/diff/340001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1510: base::HistogramTester*
const histogram_tester() {
You're currently returning a const pointer, in the sense that the pointer value
is const.  That's not very helpful -- the pointer value is a primitive type, and
so would be copied anyway.  What you want is "const base::HistogramTester*" --
equivalently, but less idiomatically, "base::HistogramTester const * -- which
specifies that the pointed-to object is const.

Of course at that point, since the object is never null, you might as well use a
const-reference: "const base::HistogramTester&".

https://codereview.chromium.org/379283002/diff/340001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.cc (right):

https://codereview.chromium.org/379283002/diff/340001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.cc:20: 
nit: Spurious newline.

https://codereview.chromium.org/379283002/diff/340001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.h (right):

https://codereview.chromium.org/379283002/diff/340001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:12: // histogram tests.
More precisely, the Chrome subclass calls out to other processes -- like any
renderer process -- and fetches histogram data from them.  The MessageLoop is
just a synchronization tool in this case.

[Mike Lerman, 2014-07-16 19:36:36]

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/search/s...
File chrome/browser/search/suggestions/blacklist_store_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/300001/chrome/browser/search/s...
chrome/browser/search/suggestions/blacklist_store_unittest.cc:144:
histogram_tester.ExpectBucketCount("Suggestions.LocalBlacklistSize", 2, 1);
On 2014/07/16 18:44:46, Ilya Sherman wrote:
> On 2014/07/16 17:29:03, Mike Lerman wrote:
> > On 2014/07/15 03:56:35, Ilya Sherman wrote:
> > > Hmm, it's a bit unfortunate that both the second and the third parameters
to
> > > this function are of integer type -- it makes the semantics of the method
> call
> > > somewhat hard to deduce at the call site.  I don't have any great ideas on
> how
> > > to improve this, though -- do you?
> > 
> > Use Objective-C? I agree, this is ugly. Can't think of much. Well... I've
seen
> > unit testing libraries for Java ... we'd have an API like:
> > 
> > histogram_tester.ForHistogram("blah blah").ForSample(2).ExpectValue(1);
> > or 
> > histogram_tester.ForHistogram("blah
blah").ForUniqueSample(2).ExpectValue(1);
> > or
> > histogram_tester.ForHistogram("foo").ExpectTotalCount(2);
> > 
> > I've always found that kind of syntax really wordy in Java, especially since
I
> > was usually working in Eclipse with Javadoc & Intellisense. Here... maybe
it's
> > the right answer? What do you think? I could sketch out the necessary
classes
> > and signatures in the design doc.
> 
> Hmm, that's definitely an interesting thought, worth exploring more.
> 
> Another idea is to change the ExpectFoo() methods to be GetFoo() methods
instead
> -- then we'd have something like
> 
> EXPECT_EQ(1, histogram_tester.GetBucketCount("Suggestions.LocalBlacklistSize",
> 2));
> 
> That's at least somewhat clearer -- the 2 is still a little mysterious, but at
> least the 1 is more clearly a count within a bucket.  WDYT?
> 
> In any case, I think it's reasonable to punt this question to a follow-up CL,
so
> that this one can remain a little more focused.

Acknowledged.

https://codereview.chromium.org/379283002/diff/340001/base/test/histogram_tes...
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/379283002/diff/340001/base/test/histogram_tes...
base/test/histogram_tester.h:21: // or otherwise. Tests can use this interface
to verify that Histogram data is
On 2014/07/16 18:44:46, Ilya Sherman wrote:
> nit: Uppercase "Histogram" should be lowercase.

Done.

https://codereview.chromium.org/379283002/diff/340001/base/test/histogram_tes...
base/test/histogram_tester.h:33: const void ExpectUniqueSample(const
std::string& name,
On 2014/07/16 18:44:46, Ilya Sherman wrote:
> "const void" doesn't make a lot of sense as a return type.  Did you mean to
mark
> the method as const?

Ummm. Yes. Awkward.

https://codereview.chromium.org/379283002/diff/340001/base/test/histogram_tes...
File base/test/histogram_tester_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/340001/base/test/histogram_tes...
base/test/histogram_tester_unittest.cc:62: 
On 2014/07/16 18:44:46, Ilya Sherman wrote:
> nit: Spurious newline

Done.

https://codereview.chromium.org/379283002/diff/340001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/379283002/diff/340001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1510: base::HistogramTester*
const histogram_tester() {
On 2014/07/16 18:44:46, Ilya Sherman wrote:
> You're currently returning a const pointer, in the sense that the pointer
value
> is const.  That's not very helpful -- the pointer value is a primitive type,
and
> so would be copied anyway.  What you want is "const base::HistogramTester*" --
> equivalently, but less idiomatically, "base::HistogramTester const * -- which
> specifies that the pointed-to object is const.
> 
> Of course at that point, since the object is never null, you might as well use
a
> const-reference: "const base::HistogramTester&".

I'd say I hate C++ const, except Java's final isn't much better.

https://codereview.chromium.org/379283002/diff/340001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.cc (right):

https://codereview.chromium.org/379283002/diff/340001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.cc:20: 
On 2014/07/16 18:44:47, Ilya Sherman wrote:
> nit: Spurious newline.

Done.

https://codereview.chromium.org/379283002/diff/340001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.h (right):

https://codereview.chromium.org/379283002/diff/340001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:12: // histogram tests.
On 2014/07/16 18:44:47, Ilya Sherman wrote:
> More precisely, the Chrome subclass calls out to other processes -- like any
> renderer process -- and fetches histogram data from them.  The MessageLoop is
> just a synchronization tool in this case.

I had thought half the benefit, since this is used in a multi-process scenario,
is also that you're guaranteed that the other processes have done their work,
and we are in fact ready to do assert the test values.

But comment changed.

[Ilya Sherman, 2014-07-16 20:01:13]

https://codereview.chromium.org/379283002/diff/340001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.h (right):

https://codereview.chromium.org/379283002/diff/340001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:12: // histogram tests.
On 2014/07/16 19:36:36, Mike Lerman wrote:
> On 2014/07/16 18:44:47, Ilya Sherman wrote:
> > More precisely, the Chrome subclass calls out to other processes -- like any
> > renderer process -- and fetches histogram data from them.  The MessageLoop
is
> > just a synchronization tool in this case.
> 
> I had thought half the benefit, since this is used in a multi-process
scenario,
> is also that you're guaranteed that the other processes have done their work,
> and we are in fact ready to do assert the test values.

Yes, this is the entire benefit.  However, the other processes don't run on the
test's main message loop (well, unless there's some really egregious
single_process shenanigans going on).  We spin the MessageLoop essentially as a
way to wait: The MessageLoop itself isn't doing any work; it's just blocking the
rest of the test from running.  Once the Quit() closure is posted, the
MessageLoop stops the test from blocking, and allows the rest of the test code
to run.  That Quit() closure is only posted when we get a message back
indicating that the other processes are done doing their thing, so it has the
effect of serving as a synchronization tool: The test code that needs to wait on
other processes doesn't get to run until the other guys are ready.

https://codereview.chromium.org/379283002/diff/380001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/379283002/diff/380001/base/test/histogram_tes...
base/test/histogram_tester.cc:96: actual_count -=
histograms_snapshot_.at(name)->GetCount(sample);
Please use .find() rather than .at(), as .at() can throw an exception.  (It's
really quite frustrating that there's no const version of operator[]... alas.)

https://codereview.chromium.org/379283002/diff/380001/base/test/histogram_tes...
base/test/histogram_tester.cc:109: actual_count -=
histograms_snapshot_.at(name)->TotalCount();
(here too)

https://codereview.chromium.org/379283002/diff/380001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.cc (right):

https://codereview.chromium.org/379283002/diff/380001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.cc:31: }
It's interesting to note that this method does not interact with the base class
in any way.  Perhaps rather than providing a ChromeHistogramTester class, we
should just provide a free function FetchHistogramsFromChildProcesses(), which
we define in e.g. //content/public/test/browser_test_utils.h?

https://codereview.chromium.org/379283002/diff/380001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.h (right):

https://codereview.chromium.org/379283002/diff/380001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:11: // from all other processes
before performing assertions.
IMO, it would be better to move this comment to be part of the documentation for
FetchTestingSnapshot, since the other methods (from the base class) are not
affected.  If you prefer to also have a class-level comment that's fine; in that
case, please provide documentation in both places.

https://codereview.chromium.org/379283002/diff/380001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:17: 
Optional nit: I'd omit this blank line.

[Mike Lerman, 2014-07-17 15:04:18]

https://codereview.chromium.org/379283002/diff/380001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/379283002/diff/380001/base/test/histogram_tes...
base/test/histogram_tester.cc:96: actual_count -=
histograms_snapshot_.at(name)->GetCount(sample);
On 2014/07/16 20:01:12, Ilya Sherman wrote:
> Please use .find() rather than .at(), as .at() can throw an exception.  (It's
> really quite frustrating that there's no const version of operator[]... alas.)

The at() command directly follows a count() which verifies that I do have a
value in the map for my key. Since I'm doing that check, isn't it easier, rather
than declaring a std::map<std::string, HistogramSamples*>::iterator to hold the
return value of find() before I call .second->GetCount()?

https://codereview.chromium.org/379283002/diff/380001/base/test/histogram_tes...
base/test/histogram_tester.cc:109: actual_count -=
histograms_snapshot_.at(name)->TotalCount();
On 2014/07/16 20:01:12, Ilya Sherman wrote:
> (here too)

Question above

https://codereview.chromium.org/379283002/diff/380001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.cc (right):

https://codereview.chromium.org/379283002/diff/380001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.cc:31: }
On 2014/07/16 20:01:12, Ilya Sherman wrote:
> It's interesting to note that this method does not interact with the base
class
> in any way.  Perhaps rather than providing a ChromeHistogramTester class, we
> should just provide a free function FetchHistogramsFromChildProcesses(), which
> we define in e.g. //content/public/test/browser_test_utils.h?

I was hoping to have something I could add under //content rather than //chrome,
since that's the where the dependency actually is. I like this suggestion. Done.

My big concern now is usability/find-ability. Someone's going to write a
browser_test, use the HistogramTester, get inconsistent or flaky results and
will have a hard time figuring out how to fix it. A subclass like
ChromeHistogramTester might at least come up in someone's search. I think I may
need to add a comment to //base/test/histogram_tester.h for future usability,
although I know it breaks the practice of referencing files further up the
dependency tree. WDYT?

https://codereview.chromium.org/379283002/diff/380001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.h (right):

https://codereview.chromium.org/379283002/diff/380001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:11: // from all other processes
before performing assertions.
On 2014/07/16 20:01:13, Ilya Sherman wrote:
> IMO, it would be better to move this comment to be part of the documentation
for
> FetchTestingSnapshot, since the other methods (from the base class) are not
> affected.  If you prefer to also have a class-level comment that's fine; in
that
> case, please provide documentation in both places.

Done.

https://codereview.chromium.org/379283002/diff/380001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.h:17: 
On 2014/07/16 20:01:13, Ilya Sherman wrote:
> Optional nit: I'd omit this blank line.

Ya, the standard seems to be constructor-destructor without no newline between.

[Ilya Sherman, 2014-07-17 18:35:09]

Thanks, Mike!  LGTM % the remaining comments.

https://codereview.chromium.org/379283002/diff/380001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/379283002/diff/380001/base/test/histogram_tes...
base/test/histogram_tester.cc:96: actual_count -=
histograms_snapshot_.at(name)->GetCount(sample);
On 2014/07/17 15:04:18, Mike Lerman wrote:
> On 2014/07/16 20:01:12, Ilya Sherman wrote:
> > Please use .find() rather than .at(), as .at() can throw an exception. 
(It's
> > really quite frustrating that there's no const version of operator[]...
alas.)
> 
> The at() command directly follows a count() which verifies that I do have a
> value in the map for my key. Since I'm doing that check, isn't it easier,
rather
> than declaring a std::map<std::string, HistogramSamples*>::iterator to hold
the
> return value of find() before I call .second->GetCount()?

I'd still prefer to use .find() rather than .at().  While it's safe here, it's
better to just not call code that can call an exception.  Otherwise, other folks
might come along and try to write similar code, but not realize this issue, and
then accidentally introduce exceptions.

https://codereview.chromium.org/379283002/diff/380001/chrome/test/base/chrome...
File chrome/test/base/chrome_histogram_tester.cc (right):

https://codereview.chromium.org/379283002/diff/380001/chrome/test/base/chrome...
chrome/test/base/chrome_histogram_tester.cc:31: }
On 2014/07/17 15:04:18, Mike Lerman wrote:
> On 2014/07/16 20:01:12, Ilya Sherman wrote:
> > It's interesting to note that this method does not interact with the base
> class
> > in any way.  Perhaps rather than providing a ChromeHistogramTester class, we
> > should just provide a free function FetchHistogramsFromChildProcesses(),
which
> > we define in e.g. //content/public/test/browser_test_utils.h?
> 
> I was hoping to have something I could add under //content rather than
//chrome,
> since that's the where the dependency actually is. I like this suggestion.
Done.
> 
> My big concern now is usability/find-ability. Someone's going to write a
> browser_test, use the HistogramTester, get inconsistent or flaky results and
> will have a hard time figuring out how to fix it. A subclass like
> ChromeHistogramTester might at least come up in someone's search. I think I
may
> need to add a comment to //base/test/histogram_tester.h for future usability,
> although I know it breaks the practice of referencing files further up the
> dependency tree. WDYT?

I don't think the comment is needed, for two reasons:  First of all, collecting
histograms from other processes is a very rare need in tests.  Secondly, if the
test author doesn't call this code, their test won't be flaky -- it just won't
work.  There's no race condition, as there's no code that periodically
synchronizes histograms across processes in tests.  It's possible that somebody
would reimplement this method, and get it wrong, but I'm not too worried about
that possibility.

https://codereview.chromium.org/379283002/diff/400001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/379283002/diff/400001/content/public/test/bro...
content/public/test/browser_test_utils.cc:603:
content::FetchHistogramsAsynchronously(
nit: No need for the "content::" namespace prefix here or two lines up, since
this code is now being implemented in the content layer.

https://codereview.chromium.org/379283002/diff/400001/content/public/test/bro...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/379283002/diff/400001/content/public/test/bro...
content/public/test/browser_test_utils.h:193: void FetchHistogramsData();
nit: I think it's important for the function name to clarify that data is being
fetched across processes -- that was why my suggested name was lengthy.

[Mike Lerman, 2014-07-18 13:58:24]

Thanks Ilya, sending this out for wider review.

Implemented use of std::map::find as well.

https://codereview.chromium.org/379283002/diff/400001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/379283002/diff/400001/content/public/test/bro...
content/public/test/browser_test_utils.cc:603:
content::FetchHistogramsAsynchronously(
On 2014/07/17 18:35:09, Ilya Sherman wrote:
> nit: No need for the "content::" namespace prefix here or two lines up, since
> this code is now being implemented in the content layer.

Done.

https://codereview.chromium.org/379283002/diff/400001/content/public/test/bro...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/379283002/diff/400001/content/public/test/bro...
content/public/test/browser_test_utils.h:193: void FetchHistogramsData();
On 2014/07/17 18:35:09, Ilya Sherman wrote:
> nit: I think it's important for the function name to clarify that data is
being
> fetched across processes -- that was why my suggested name was lengthy.

Done.

[Mike Lerman, 2014-07-18 14:11:17]

Hello Owners,

Sending this out to review all the unit tests.

ttuttle - /c/renderer/net, net/*
vabr - c/b/ui/views/passwords, c/b/ui/passwords
msw - c/b/ui/views/profiles
shess - c/b/ui/cocia/browser
groby - c/b/spellchecker
tim - c/b/signin
mmenke - c/b/prerender
dmazzoni - c/b/accessibility

And also Brett for the GYP file changes in base.

Thanks!

[Deprecated (see juliatuttle), 2014-07-18 14:17:06]

On 2014/07/18 14:11:17, Mike Lerman wrote:
> Hello Owners,
> 
> Sending this out to review all the unit tests.
> 
> ttuttle - /c/renderer/net, net/*
> vabr - c/b/ui/views/passwords, c/b/ui/passwords
> msw - c/b/ui/views/profiles
> shess - c/b/ui/cocia/browser
> groby - c/b/spellchecker
> tim - c/b/signin
> mmenke - c/b/prerender
> dmazzoni - c/b/accessibility
> 
> And also Brett for the GYP file changes in base.
> 
> Thanks!

lgtm.

[mmenke, 2014-07-18 14:53:39]

prerender/ LGTM

https://codereview.chromium.org/379283002/diff/420001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/379283002/diff/420001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1510: const
base::HistogramTester& histogram_tester() { return histograms_; }
nit:  Should name the variable histogram_tester_ as well, to be consistent.

https://codereview.chromium.org/379283002/diff/420001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:3378: 
nit:  Remove blank line.

https://codereview.chromium.org/379283002/diff/420001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:4185: 
nit:  Remove blank line.

[Scott Hess - ex-Googler, 2014-07-18 15:23:25]

lgtm for cocoa/

[Mike Lerman, 2014-07-18 15:55:52]

https://codereview.chromium.org/379283002/diff/420001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/379283002/diff/420001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1510: const
base::HistogramTester& histogram_tester() { return histograms_; }
On 2014/07/18 14:53:39, mmenke wrote:
> nit:  Should name the variable histogram_tester_ as well, to be consistent.

Done.

https://codereview.chromium.org/379283002/diff/420001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:3378: 
On 2014/07/18 14:53:39, mmenke wrote:
> nit:  Remove blank line.

Done.

https://codereview.chromium.org/379283002/diff/420001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:4185: 
On 2014/07/18 14:53:39, mmenke wrote:
> nit:  Remove blank line.

Done.

[Ilya Sherman, 2014-07-18 16:21:09]

(Still LGTM % comment below)

https://codereview.chromium.org/379283002/diff/440001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/379283002/diff/440001/base/test/histogram_tes...
base/test/histogram_tester.cc:95: auto histogram_data =
histograms_snapshot_.find(name);
C++11 is not yet allowed in Chromium code, as not all of our compiler toolchains
support it.  You'll need to spell out the full type here.

[msw, 2014-07-18 17:30:25]

LGTM with nits and a question.

https://codereview.chromium.org/379283002/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_view_test.cc (right):

https://codereview.chromium.org/379283002/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_view_test.cc:7: #include
"base/test/histogram_tester.h"
nit: you don't need to explicitly include this here since it's in the header.

https://codereview.chromium.org/379283002/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_view_test.cc:93: return
histogram_tester_.GetHistogramSamplesSinceCreation(histogram)
nit: or make this function-local, not a class member, since it's only used here.

https://codereview.chromium.org/379283002/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc
(left):

https://codereview.chromium.org/379283002/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:99:
histograms.Fetch();
Were the NaCl tests the only ones that actually required this synchronization?
You removed this and many other Fetch() calls without adding
FetchHistogramsFromChildProcesses() calls (except for NaCl).

[Ilya Sherman, 2014-07-18 17:32:01]

https://codereview.chromium.org/379283002/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc
(left):

https://codereview.chromium.org/379283002/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:99:
histograms.Fetch();
On 2014/07/18 17:30:25, msw wrote:
> Were the NaCl tests the only ones that actually required this synchronization?
> You removed this and many other Fetch() calls without adding
> FetchHistogramsFromChildProcesses() calls (except for NaCl).

Indeed, the NaCl tests were the only ones that actually needed to fetch
histograms data from child processes.

[Mike Lerman, 2014-07-18 18:23:58]

https://codereview.chromium.org/379283002/diff/440001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/379283002/diff/440001/base/test/histogram_tes...
base/test/histogram_tester.cc:95: auto histogram_data =
histograms_snapshot_.find(name);
On 2014/07/18 16:21:09, Ilya Sherman wrote:
> C++11 is not yet allowed in Chromium code, as not all of our compiler
toolchains
> support it.  You'll need to spell out the full type here.

Shucks - done.

https://codereview.chromium.org/379283002/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_view_test.cc (right):

https://codereview.chromium.org/379283002/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_view_test.cc:7: #include
"base/test/histogram_tester.h"
On 2014/07/18 17:30:25, msw wrote:
> nit: you don't need to explicitly include this here since it's in the header.

Done.

https://codereview.chromium.org/379283002/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_view_test.cc:93: return
histogram_tester_.GetHistogramSamplesSinceCreation(histogram)
On 2014/07/18 17:30:25, msw wrote:
> nit: or make this function-local, not a class member, since it's only used
here.

The timing of the execution of the constructor is important here, as it gathers
snapshot data as a baseline for comparison. If I remove this from being a class
member, I would instead have to create it in at the beginning of each of the
test methods that calls GetSamples(), and then pass in that HistogramTester to
this object. The current implementation seems cleaner.

[groby-ooo-7-16, 2014-07-18 18:35:52]

c/b/ui/cocoa LGTM (but shess already said so ;)

[groby-ooo-7-16, 2014-07-18 18:45:33]

In that case, spellchecker LGTM ;)

[vabr (Chromium), 2014-07-21 07:45:44]

c/b/ui/views/passwords, c/b/ui/passwords LGTM
Thanks!
Vaclav

[Paweł Hajdan Jr., 2014-07-23 10:50:58]

"not lgtm"

https://codereview.chromium.org/379283002/diff/460001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/379283002/diff/460001/content/public/test/bro...
content/public/test/browser_test_utils.cc:609:
TestTimeouts::action_max_timeout() * 2);
The timeouts returned by TestTimeout should not be multiplied.

There must be some better solution than this.

[Ilya Sherman, 2014-07-23 17:25:11]

https://codereview.chromium.org/379283002/diff/460001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/379283002/diff/460001/content/public/test/bro...
content/public/test/browser_test_utils.cc:609:
TestTimeouts::action_max_timeout() * 2);
On 2014/07/23 10:50:58, Paweł Hajdan Jr. wrote:
> The timeouts returned by TestTimeout should not be multiplied.
> 
> There must be some better solution than this.

What is your objection to multiplying the timeout?  The goal is to have the test
time out if the FetchHistogramsAsynchronously call fails (also with a timeout). 
The multiplication isn't strictly necessary, I suppose; using
action_max_timeout() directly should, in theory, given the same result.

[Mike Lerman, 2014-07-29 15:03:47]

Ping - Pawel?

Also ping timsteele, brettw and dmazzoni.

[brettw, 2014-08-04 05:45:49]

base lgtm, please fix timeout.

https://codereview.chromium.org/379283002/diff/460001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/379283002/diff/460001/content/public/test/bro...
content/public/test/browser_test_utils.cc:609:
TestTimeouts::action_max_timeout() * 2);
I agree with Pawel. We shouldn't have arbitrary timeouts unless there is some
specific justification. Using action_max_timeout directly should be sufficient.
If this isn't long enough, we have a different problem.

[Mike Lerman, 2014-08-04 19:12:46]

Hi Pawel and Brett,

I've changed the timeout, the new value seems to work fine.

Hi Mathieu,

Added you as owner for components/suggestions.

Thanks!

[Mathieu, 2014-08-04 19:15:12]

On 2014/08/04 19:12:46, Mike Lerman wrote:
> Hi Pawel and Brett,
> 
> I've changed the timeout, the new value seems to work fine.
> 
> Hi Mathieu,
> 
> Added you as owner for components/suggestions.
> 
> Thanks!

lgtm

[Mike Lerman, 2014-08-04 20:43:42]

+ nikita as reviewer for c/b/chromeos

thanks!

[Nikita (slow), 2014-08-05 12:03:58]

On 2014/08/04 20:43:42, Mike Lerman wrote:
> + nikita as reviewer for c/b/chromeos

lgtm

[Nikita (slow), 2014-08-05 12:04:09]

https://codereview.chromium.org/379283002/diff/590001/chrome/browser/chromeos...
File chrome/browser/chromeos/external_metrics_unittest.cc (right):

https://codereview.chromium.org/379283002/diff/590001/chrome/browser/chromeos...
chrome/browser/chromeos/external_metrics_unittest.cc:38: HistogramTester
histogram_tester;
It seems that you need to use base::HistogramTester here (trybot shows compile
errors).

[Mike Lerman, 2014-08-06 14:12:21]

Ping Pawel

[dmazzoni, 2014-08-11 05:45:21]

lgtm

for c/b/accessibility

[Paweł Hajdan Jr., 2014-08-11 09:57:41]

LGTM

[Mike Lerman, 2014-08-21 15:14:13]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-08-21 15:16:30]

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

[Mike Lerman, 2014-08-21 19:26:07]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-08-21 19:28:54]

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

[commit-bot: I haz the power, 2014-08-21 22:00:52]

Committed patchset #27 (750001) as 291207