Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
213433003:
Add Sparse Histogram Support to metricsPrivate (Closed)
|
Created:
6 years, 9 months ago by robliao Modified:
6 years, 9 months ago CC:
skare_, rgustafson, chromium-reviews, chromium-apps-reviews_chromium.org, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd Sparse Histogram Support to metricsPrivate
BUG=356820
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259793
Patch Set 1 #Patch Set 2 : Fix Typo #
Total comments: 14
Patch Set 3 : CR Feedback #
Total comments: 4
Patch Set 4 : CR Feedback #
Total comments: 16
Patch Set 5 : CR Feedback #
Total comments: 6
Patch Set 6 : CR Feedback #
Total comments: 2
Patch Set 7 : Style and Unsigned/Signed Compare #
Messages
Total messages: 23 (0 generated)
Reviewers: Please review changes in the following files. Thanks! isherman: chrome/browser/extensions/api/metrics_private/metrics_apitest.cc chrome/browser/extensions/api/metrics_private/metrics_private_api.h chrome/browser/extensions/api/metrics_private/metrics_private_api.cc tools/metrics/histograms/histograms.xml kalman: chrome/common/extensions/api/metrics_private.json extensions/browser/extension_function_histogram_value.h
Fix a quick typo
lgtm https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:127: } you could EXPECT_EQ(r.type == base::HISTOGRAM, histogram->HasConstructionArguments(r.min, r.max, r.buckets));
https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:127: } While true, that seems to be a bit too cute for readability. And it's not just histograms, it's also LINEAR_HISTOGRAM, so it would look like EXPECT_EQ(r.type != base::SPARSE_HISTOGRAM, histogram->HasConstructionArguments(r.min, r.max, r.buckets)) On 2014/03/26 20:39:45, kalman wrote: > you could > > EXPECT_EQ(r.type == base::HISTOGRAM, > histogram->HasConstructionArguments(r.min, r.max, r.buckets));
https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:127: } On 2014/03/26 21:15:43, robliao wrote: > While true, that seems to be a bit too cute for readability. > > And it's not just histograms, it's also LINEAR_HISTOGRAM, so it would look like > > EXPECT_EQ(r.type != base::SPARSE_HISTOGRAM, > histogram->HasConstructionArguments(r.min, r.max, r.buckets)) > On 2014/03/26 20:39:45, kalman wrote: > > you could > > > > EXPECT_EQ(r.type == base::HISTOGRAM, > > histogram->HasConstructionArguments(r.min, r.max, r.buckets)); > ok. I don't think it's overly cute actually. Is there something you can assert about the sparse case to make it more obvious? like what arguments is it supposed to have...?
On 2014/03/26 21:19:35, kalman wrote: > https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... > File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): > > https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... > chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:127: } > On 2014/03/26 21:15:43, robliao wrote: > > While true, that seems to be a bit too cute for readability. > > > > And it's not just histograms, it's also LINEAR_HISTOGRAM, so it would look > like > > > > EXPECT_EQ(r.type != base::SPARSE_HISTOGRAM, > > histogram->HasConstructionArguments(r.min, r.max, r.buckets)) > > On 2014/03/26 20:39:45, kalman wrote: > > > you could > > > > > > EXPECT_EQ(r.type == base::HISTOGRAM, > > > histogram->HasConstructionArguments(r.min, r.max, r.buckets)); > > > > ok. I don't think it's overly cute actually. Is there something you can assert > about the sparse case to make it more obvious? like what arguments is it > supposed to have...? The arguments are ignored.
https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:42: {"test.sparse.2", base::SPARSE_HISTOGRAM, 0, 0, 0, 2}, Where are you testing which bucket was emitted to? https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_private_api.cc (right): https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_private_api.cc:136: UMA_HISTOGRAM_SPARSE_SLOWLY(params->metric_name, params->value); This won't work -- you can only use an UMA_HISTOGRAM_ macro if the histogram name is a runtime constant. You'll need to instead expand out the macro, as has been done elsewhere in this file. https://codereview.chromium.org/213433003/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/metrics_private.json (right): https://codereview.chromium.org/213433003/diff/20001/chrome/common/extensions... chrome/common/extensions/api/metrics_private.json:153: "description": "Adds a value to the given sparse metric.", nit: "metric" -> "histogram". I'd actually rephrase this whole sentence as "Increments the count associated with |value| in the sparse histogram defined by the |metricName|.", or something along those lines. https://codereview.chromium.org/213433003/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/metrics/test.js (right): https://codereview.chromium.org/213433003/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/metrics/test.js:49: chrome.metricsPrivate.recordSparseValue('test.sparse.2', 42); It would be nice to verify that more buckets than just bucket #42 can be emitted to.
https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:42: {"test.sparse.2", base::SPARSE_HISTOGRAM, 0, 0, 0, 2}, Can't really do this as there's no way to get the buckets out in a nice way. We should definitely test for this once it gets implemented. void SparseHistogram::GetCountAndBucketData(Count* count, int64* sum, ListValue* buckets) const { // TODO(kaiwang): Implement. (See HistogramBase::WriteJSON.) } On 2014/03/26 22:02:03, Ilya Sherman wrote: > Where are you testing which bucket was emitted to? https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_private_api.cc (right): https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_private_api.cc:136: UMA_HISTOGRAM_SPARSE_SLOWLY(params->metric_name, params->value); Why not? The macro seems to expand fine. This is already being used like this elsewhere. void BlinkPlatformImpl::histogramSparse(const char* name, int sample) { // For sparse histograms, we can use the macro, as it does not incorporate a // static. UMA_HISTOGRAM_SPARSE_SLOWLY(name, sample); } On 2014/03/26 22:02:03, Ilya Sherman wrote: > This won't work -- you can only use an UMA_HISTOGRAM_ macro if the histogram > name is a runtime constant. You'll need to instead expand out the macro, as has > been done elsewhere in this file. https://codereview.chromium.org/213433003/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/metrics_private.json (right): https://codereview.chromium.org/213433003/diff/20001/chrome/common/extensions... chrome/common/extensions/api/metrics_private.json:153: "description": "Adds a value to the given sparse metric.", On 2014/03/26 22:02:03, Ilya Sherman wrote: > nit: "metric" -> "histogram". I'd actually rephrase this whole sentence as > "Increments the count associated with |value| in the sparse histogram defined by > the |metricName|.", or something along those lines. Done. https://codereview.chromium.org/213433003/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/metrics/test.js (right): https://codereview.chromium.org/213433003/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/metrics/test.js:49: chrome.metricsPrivate.recordSparseValue('test.sparse.2', 42); On 2014/03/26 22:02:03, Ilya Sherman wrote: > It would be nice to verify that more buckets than just bucket #42 can be emitted > to. Done.
https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:42: {"test.sparse.2", base::SPARSE_HISTOGRAM, 0, 0, 0, 2}, You're already extracting a HistogramSamples object for the histogram. That has a GetCount() method that will return the count for a specific bucket. I don't think GetCountAndBucketData() is relevant -- that's for generating the chrome://histograms UI. On 2014/03/26 22:30:50, robliao wrote: > Can't really do this as there's no way to get the buckets out in a nice way. We > should definitely test for this once it gets implemented. > > void SparseHistogram::GetCountAndBucketData(Count* count, > int64* sum, > ListValue* buckets) const { > // TODO(kaiwang): Implement. (See HistogramBase::WriteJSON.) > } > > On 2014/03/26 22:02:03, Ilya Sherman wrote: > > Where are you testing which bucket was emitted to? > https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_private_api.cc (right): https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_private_api.cc:136: UMA_HISTOGRAM_SPARSE_SLOWLY(params->metric_name, params->value); Ah, you're right! Could you please include a similar comment here? I forgot that UMA_HISTOGRAM_SPARSE_SLOWLY was different from all of the other UMA_HISTOGRAM_ macros. It's nice to document in spots like this where you're taking advantage of it, so that later maintainers don't get confused. On 2014/03/26 22:30:50, robliao wrote: > Why not? The macro seems to expand fine. This is already being used like this > elsewhere. > > void BlinkPlatformImpl::histogramSparse(const char* name, int sample) { > // For sparse histograms, we can use the macro, as it does not incorporate a > // static. > UMA_HISTOGRAM_SPARSE_SLOWLY(name, sample); > } > > > On 2014/03/26 22:02:03, Ilya Sherman wrote: > > This won't work -- you can only use an UMA_HISTOGRAM_ macro if the histogram > > name is a runtime constant. You'll need to instead expand out the macro, as > has > > been done elsewhere in this file. > https://codereview.chromium.org/213433003/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/metrics_private.json (right): https://codereview.chromium.org/213433003/diff/40001/chrome/common/extensions... chrome/common/extensions/api/metrics_private.json:153: "description": "Increments the count associated with |value| in the sparse histogram defined by the |metricName|", nit: Please end the sentence with a period. https://codereview.chromium.org/213433003/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/metrics/test.js (right): https://codereview.chromium.org/213433003/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/metrics/test.js:49: chrome.metricsPrivate.recordSparseValue('test.sparse.2', 24); What I really meant was that it would be nice to verify that for a single sparse histogram, it's possible to emit to multiple buckets.
CR Feedback and adding skare@ and rgustafson@ as FYI. https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_private_api.cc (right): https://codereview.chromium.org/213433003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_private_api.cc:136: UMA_HISTOGRAM_SPARSE_SLOWLY(params->metric_name, params->value); On 2014/03/26 22:44:14, Ilya Sherman wrote: > Ah, you're right! Could you please include a similar comment here? I forgot > that UMA_HISTOGRAM_SPARSE_SLOWLY was different from all of the other > UMA_HISTOGRAM_ macros. It's nice to document in spots like this where you're > taking advantage of it, so that later maintainers don't get confused. > > On 2014/03/26 22:30:50, robliao wrote: > > Why not? The macro seems to expand fine. This is already being used like this > > elsewhere. > > > > void BlinkPlatformImpl::histogramSparse(const char* name, int sample) { > > // For sparse histograms, we can use the macro, as it does not incorporate a > > // static. > > UMA_HISTOGRAM_SPARSE_SLOWLY(name, sample); > > } > > > > > > On 2014/03/26 22:02:03, Ilya Sherman wrote: > > > This won't work -- you can only use an UMA_HISTOGRAM_ macro if the histogram > > > name is a runtime constant. You'll need to instead expand out the macro, as > > has > > > been done elsewhere in this file. > > > Done. https://codereview.chromium.org/213433003/diff/40001/chrome/common/extensions... File chrome/common/extensions/api/metrics_private.json (right): https://codereview.chromium.org/213433003/diff/40001/chrome/common/extensions... chrome/common/extensions/api/metrics_private.json:153: "description": "Increments the count associated with |value| in the sparse histogram defined by the |metricName|", On 2014/03/26 22:44:14, Ilya Sherman wrote: > nit: Please end the sentence with a period. Done. https://codereview.chromium.org/213433003/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/metrics/test.js (right): https://codereview.chromium.org/213433003/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/metrics/test.js:49: chrome.metricsPrivate.recordSparseValue('test.sparse.2', 24); On 2014/03/26 22:44:14, Ilya Sherman wrote: > What I really meant was that it would be nice to verify that for a single sparse > histogram, it's possible to emit to multiple buckets. Done.
Thanks! Lots of little nits, but otherwise LG. https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:55: int histogramValue; nit: Please use hacker_case. https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:123: const scoped_ptr<base::HistogramSamples>& samples) { nit: Why pass the scoped_ptr, rather than the pointed-to object? https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:124: for (int i = 0; i < arraysize(g_sparse_histograms); i++) { nit: ++i https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:124: for (int i = 0; i < arraysize(g_sparse_histograms); i++) { nit: "i <" -> "i <" (one fewer spaces) https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:126: if (name == sparse_histogram.name) { Comparing C-strings is a pointer comparison, which is probably not what you want. The simplest fix is to change the type of the |name| parameter to be std::string. https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:127: for (int j = 0; j < sparse_histogram.bucket_count; j++) { nit: ++j https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_private_api.cc (right): https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_private_api.cc:136: // This particular UMA_HISTOGRAM_* is okay for runtime strings. nit: "runtime strings" -> "non-runtime-constant strings". https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_private_api.cc:136: // This particular UMA_HISTOGRAM_* is okay for runtime strings. nit: "UMA_HISTOGRAM_*" -> "UMA_HISTOGRAM_ macro"
https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:55: int histogramValue; On 2014/03/26 23:52:13, Ilya Sherman wrote: > nit: Please use hacker_case. Done. https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:123: const scoped_ptr<base::HistogramSamples>& samples) { That would work too. On 2014/03/26 23:52:13, Ilya Sherman wrote: > nit: Why pass the scoped_ptr, rather than the pointed-to object? https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:124: for (int i = 0; i < arraysize(g_sparse_histograms); i++) { On 2014/03/26 23:52:13, Ilya Sherman wrote: > nit: "i <" -> "i <" (one fewer spaces) Done. https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:126: if (name == sparse_histogram.name) { Oops, too much jumping between JS and C++>. Yup, I forgot the std::string. Oddly enough, the code works (tested with bogus counts) and it shouldn't... On 2014/03/26 23:52:13, Ilya Sherman wrote: > Comparing C-strings is a pointer comparison, which is probably not what you > want. The simplest fix is to change the type of the |name| parameter to be > std::string. https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:127: for (int j = 0; j < sparse_histogram.bucket_count; j++) { On 2014/03/26 23:52:13, Ilya Sherman wrote: > nit: ++j Done. https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_private_api.cc (right): https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_private_api.cc:136: // This particular UMA_HISTOGRAM_* is okay for runtime strings. On 2014/03/26 23:52:13, Ilya Sherman wrote: > nit: "UMA_HISTOGRAM_*" -> "UMA_HISTOGRAM_ macro" Done.
LGTM with the remaining nits addressed. Thanks for shaving this yak! https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:126: if (name == sparse_histogram.name) { Sometimes optimizing compilers are *too* helpful ;) On 2014/03/27 00:02:44, robliao wrote: > Oops, too much jumping between JS and C++>. Yup, I forgot the std::string. > > Oddly enough, the code works (tested with bogus counts) and it shouldn't... > On 2014/03/26 23:52:13, Ilya Sherman wrote: > > Comparing C-strings is a pointer comparison, which is probably not what you > > want. The simplest fix is to change the type of the |name| parameter to be > > std::string. > https://codereview.chromium.org/213433003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:122: const char* name, nit: Please pass this as a const std::string&, rather than converting below. https://codereview.chromium.org/213433003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:123: const base::HistogramSamples* samples) { nit: Please pass by const-ref rather than const-pointer. https://codereview.chromium.org/213433003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_private_api.cc (right): https://codereview.chromium.org/213433003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_private_api.cc:136: // This particular UMA_HISTOGRAM_ macro is okay for runtime strings. nit: "runtime strings" -> "non-runtime-constant strings".
https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:126: if (name == sparse_histogram.name) { I was thinking that, but these strings also come from JS Land at runtime. On 2014/03/27 00:08:16, Ilya Sherman wrote: > Sometimes optimizing compilers are *too* helpful ;) > > On 2014/03/27 00:02:44, robliao wrote: > > Oops, too much jumping between JS and C++>. Yup, I forgot the std::string. > > > > Oddly enough, the code works (tested with bogus counts) and it shouldn't... > > On 2014/03/26 23:52:13, Ilya Sherman wrote: > > > Comparing C-strings is a pointer comparison, which is probably not what you > > > want. The simplest fix is to change the type of the |name| parameter to be > > > std::string. > > > https://codereview.chromium.org/213433003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:122: const char* name, On 2014/03/27 00:08:16, Ilya Sherman wrote: > nit: Please pass this as a const std::string&, rather than converting below. Done. https://codereview.chromium.org/213433003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:123: const base::HistogramSamples* samples) { This is why it was scoped_ptr<...>&. That was preferable to *(foo.get()) On 2014/03/27 00:08:16, Ilya Sherman wrote: > nit: Please pass by const-ref rather than const-pointer. Done. https://codereview.chromium.org/213433003/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/metrics_private/metrics_private_api.cc (right): https://codereview.chromium.org/213433003/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/metrics_private/metrics_private_api.cc:136: // This particular UMA_HISTOGRAM_ macro is okay for runtime strings. On 2014/03/27 00:08:16, Ilya Sherman wrote: > nit: "runtime strings" -> "non-runtime-constant strings". Done.
The CQ bit was checked by robliao@chromium.org
https://codereview.chromium.org/213433003/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:157: ValidateSparseHistogramSamples(r.name, *(snapshot.get())); nit: *(snapshot.get()) -> *snapshot
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/213433003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by robliao@chromium.org
Style and Unsigned/Signed Compare https://codereview.chromium.org/213433003/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right): https://codereview.chromium.org/213433003/diff/100001/chrome/browser/extensio... chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:157: ValidateSparseHistogramSamples(r.name, *(snapshot.get())); On 2014/03/27 00:25:34, Ilya Sherman wrote: > nit: *(snapshot.get()) -> *snapshot Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/213433003/140001
Message was sent while issue was closed.
Change committed as 259793 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
