Translate: move UMA related code in TranslateManager to TranslateManagerMetrics

Translate: move UMA related code in TranslateManager to TranslateManagerMetrics

BUG=none
TEST=unit_tests --gtest_filter='TranslateManagerMetricsTest.*'

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

[Takashi Toyoshima, 2013-05-14 10:58:05]

Hi mad, this is a metrics clean up CL for TranslateManager side.

[MAD, 2013-05-15 01:32:48]

A few comments...

Thanks for the cleanup!

BYE
MAD

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
File chrome/browser/translate/translate_manager_metrics.cc (right):

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_manager_metrics.cc:12: // Constant string
values to indicate UMA names. All entry should have
All entry -> All entries

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_manager_metrics.cc:26: // This entry table
should be updated when new UMA item is added.
item is -> items are

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_manager_metrics.cc:54:
UMA_HISTOGRAM_COUNTS("Translate.ServerReportedUnsupportedLanguage", 1);
Oupss... Please use the variable...

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
File chrome/browser/translate/translate_manager_metrics.h (right):

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_manager_metrics.h:44: // Called when it's
turned out that translating page is in unsupported language.
it's turned -> it turns

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
File chrome/browser/translate/translate_manager_metrics_unittest.cc (right):

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_manager_metrics_unittest.cc:68:
HistogramBase::Count GetTotalCount() {
Why not make get public, and explicitly get the count we are looking for?

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_manager_metrics_unittest.cc:146: EXPECT_EQ(1,
recorder.GetTotalCount());
As suggested before, we would be better making sure we report the right metric.

[Takashi Toyoshima, 2013-05-15 07:35:04]

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
File chrome/browser/translate/translate_manager_metrics.cc (right):

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_manager_metrics.cc:12: // Constant string
values to indicate UMA names. All entry should have
Oops. This is a mistake belonging to "All your base" :(

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_manager_metrics.cc:26: // This entry table
should be updated when new UMA item is added.
On 2013/05/15 01:32:48, MAD wrote:
> item is -> items are

Done.

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_manager_metrics.cc:54:
UMA_HISTOGRAM_COUNTS("Translate.ServerReportedUnsupportedLanguage", 1);
Sorry for poor copy and paste...

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
File chrome/browser/translate/translate_manager_metrics.h (right):

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_manager_metrics.h:44: // Called when it's
turned out that translating page is in unsupported language.
thanks!

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
File chrome/browser/translate/translate_manager_metrics_unittest.cc (right):

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_manager_metrics_unittest.cc:146: EXPECT_EQ(1,
recorder.GetTotalCount());
I'n not sure I understand your comments correctly here.

Here, recorder is initialized with UMA_LANGUAGE_DETECTION_ERROR so that it
checks UMA_LANGUAGE DETECTION_ERROR in following GetTotalCount().

Here, recorder take a snapshot to know the initial count for
UMA_LANGUAGE_DETECTION_ERROR. Then, calling GetTotalCount() takes another
snapshot and subtract initial count if needed.

Do you mean I should check the reported value for the UMA is always "1"?

Here is another question.
In renderer side, I use CheckTotalCount(int) in similar tests. But I feel
GetTotalCount() way is better here because it can provide more useful log in
failures. In CheckTotalCount(int), failures will be reported that it happens
always at the same line where CheckTotalCount(int) is implemented.
CheckInitiationStatus(...) and so on have the same problem. But there pros win
cons. It can reduce code size dramatically.

[MAD, 2013-05-15 12:22:24]

LGTM!

Thanks...

BYE
MAD

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
File chrome/browser/translate/translate_manager_metrics_unittest.cc (right):

https://chromiumcodereview.appspot.com/14762014/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_manager_metrics_unittest.cc:146: EXPECT_EQ(1,
recorder.GetTotalCount());
Ho! My goof... I forgot about the constructor receiving the base name and the
other tests validating the sub names only, so you are right, this is my
misunderstanding... Sorry... :-)

As for the other question, I think it is fine as is...

Thanks!

[commit-bot: I haz the power, 2013-05-16 04:07:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/14762014/10001

[commit-bot: I haz the power, 2013-05-16 08:16:15]

Change committed as 200492