Record language usage as UMA histograms.

Record language usage as UMA histograms.

Record accept languages and application language as UMA histograms on
browser start-up.

See also http://crosbug.com/17419

BUG=none
TEST=run the unit tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93360

[Yusuke Sato, 2011-07-13 06:52:06]

Thank you for working on this quickly.

one request:
I'm not familiar with UMA systems. Could you add one more reviewer who can
review the browser_main.cc change?

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1877:
LanguageUsageMetrics::RecordAcceptLanguages(
Question: Is it okay to use the metrics on all platforms? If not, please move
this inside the '#if defined(OS_CHROMEOS)' block below.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
File chrome/browser/language_usage_metrics.cc (right):

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.cc:44: Language language =
ToLanguage(locales.token());
nit: add const?

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.cc:57: std::string language_part =
parts.token();
ditto

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.cc:58: Language language;
ditto

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
File chrome/browser/language_usage_metrics.h (right):

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.h:18: 
nit: unnecessary vertical space?

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.h:23: // code. Country code is ignored.
That is, xx and Xx-YY are considered
s/Xx/xx/ ?

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.h:23: // code. Country code is ignored.
That is, xx and Xx-YY are considered
Your implementation seems to handle xx_YY case as well. If so, please update the
doc.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.h:28: // is a case-insensitive locale
string of either xx or xx-YY format. Only the
ditto.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.h:34: LanguageUsageMetrics() {}
I believe you can omit the implementation, '{}', in this case.

 LanguageUsageMetrics();

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.h:41: // Parses |locale| and returns the
language code.
Please mention the error return.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
File chrome/browser/language_usage_metrics_unittest.cc (right):

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics_unittest.cc:12: void ToSortedList(const
std::set<Language>& language_set,
The word "List" reminds me std::list. It's up to you but how about using "Array"
or "Vector"?

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics_unittest.cc:16:
sort(language_list->begin(), language_list->end());
std:: seems to be missing.

I'm also wondering why this line is necessary. I guess the language list is
sorted from the beginning (since std::set is an ordered set).

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics_unittest.cc:23:
TEST_F(LanguageUsageMetricsTest, ParseAcceptLanguages) {
I slightly prefer using TEST() and removing line 20-21 (since it's shorter.)

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics_unittest.cc:71: }
What about adding tests for bad inputs like ",", "en,", or ",en"?

[Yuzo, 2011-07-13 11:30:20]

Thank you for the review.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1877:
LanguageUsageMetrics::RecordAcceptLanguages(
I think it is OK because:
- these metrics is meaningful also for other platforms.
- this is run only once at start up.

On 2011/07/13 06:52:06, Yusuke Sato wrote:
> Question: Is it okay to use the metrics on all platforms? If not, please move
> this inside the '#if defined(OS_CHROMEOS)' block below.
>

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
File chrome/browser/language_usage_metrics.cc (right):

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.cc:44: Language language =
ToLanguage(locales.token());
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> nit: add const?

Done.
Also done at line 31 and 57.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.cc:57: std::string language_part =
parts.token();
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> ditto

Done.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.cc:58: Language language;
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> ditto

This is non-const because it will be changed on line 59.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
File chrome/browser/language_usage_metrics.h (right):

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.h:18: 
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> nit: unnecessary vertical space?

Done.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.h:23: // code. Country code is ignored.
That is, xx and Xx-YY are considered
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> Your implementation seems to handle xx_YY case as well. If so, please update
the
> doc.

Done.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.h:23: // code. Country code is ignored.
That is, xx and Xx-YY are considered
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> s/Xx/xx/ ?

It was intentional to express the case-insensitive'ness.
Changed to XX to avoid confusion.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.h:28: // is a case-insensitive locale
string of either xx or xx-YY format. Only the
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> ditto.

Done.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.h:34: LanguageUsageMetrics() {}
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> I believe you can omit the implementation, '{}', in this case.
> 
>  LanguageUsageMetrics();

Done.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics.h:41: // Parses |locale| and returns the
language code.
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> Please mention the error return.

Done.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
File chrome/browser/language_usage_metrics_unittest.cc (right):

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics_unittest.cc:12: void ToSortedList(const
std::set<Language>& language_set,
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> The word "List" reminds me std::list. It's up to you but how about using
"Array"
> or "Vector"?

Renamed to language_vector.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics_unittest.cc:16:
sort(language_list->begin(), language_list->end());
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> std:: seems to be missing.
> 
> I'm also wondering why this line is necessary. I guess the language list is
> sorted from the beginning (since std::set is an ordered set).
> 
> 

Removed the function as a whole. I admit I didn't know that it is always sorted.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics_unittest.cc:23:
TEST_F(LanguageUsageMetricsTest, ParseAcceptLanguages) {
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> I slightly prefer using TEST() and removing line 20-21 (since it's shorter.)

Done.

http://codereview.chromium.org/7348004/diff/1001/chrome/browser/language_usag...
chrome/browser/language_usage_metrics_unittest.cc:71: }
On 2011/07/13 06:52:06, Yusuke Sato wrote:
> What about adding tests for bad inputs like ",", "en,", or ",en"?

Done.

[Yuzo, 2011-07-13 11:33:02]

Jim,

Can you review the UMA usage of this patch if you are not on vacation?

[Yusuke Sato, 2011-07-14 01:54:04]

language_usage_metrics* LGTM

[jar (doing other things), 2011-07-18 17:46:30]

I had only a small nit/concern (see below).

My LGTM is restricted to looking the use of the histogram, and not for careful
reading of the code that parses out or acquires the languages.

http://codereview.chromium.org/7348004/diff/11001/chrome/browser/language_usa...
File chrome/browser/language_usage_metrics.cc (right):

http://codereview.chromium.org/7348004/diff/11001/chrome/browser/language_usa...
chrome/browser/language_usage_metrics.cc:16: language, NUM_LANGUAGES);
I'm always concerned with memory costs for large enumerated histograms.

I see this is currently set to 161.  I'd like us to rethink this if/when this
grows much(?) larger.  We have talked about creating a sparse-histogram (slow,
but small in memory costs when the enumeration is large).  Would you mind adding
a:

DCHECK_LE(NUM_LANGUAGES, 300);  // Rethink histogram wewoly costs as this gets
to be large(?).

Can you tell me how large NUM_LANGUAGES is?

[Yuzo, 2011-07-19 09:17:37]

Thank you for the review.

http://codereview.chromium.org/7348004/diff/11001/chrome/browser/language_usa...
File chrome/browser/language_usage_metrics.cc (right):

http://codereview.chromium.org/7348004/diff/11001/chrome/browser/language_usa...
chrome/browser/language_usage_metrics.cc:16: language, NUM_LANGUAGES);
On 2011/07/18 17:46:30, jar wrote:
> I'm always concerned with memory costs for large enumerated histograms.
> 
> I see this is currently set to 161.  I'd like us to rethink this if/when this
> grows much(?) larger.  We have talked about creating a sparse-histogram (slow,
> but small in memory costs when the enumeration is large).  Would you mind
adding
> a:
> 
> DCHECK_LE(NUM_LANGUAGES, 300);  // Rethink histogram wewoly costs as this gets
> to be large(?).

Added DCHECK below to RecordAcceptLanguages.

> 
> Can you tell me how large NUM_LANGUAGES is?

Currently 161 and it hasn't grown at least for 2 years:
http://src.chromium.org/viewvc/chrome/trunk/src/third_party/cld/languages/pro...

I believe it is unlikely that it exceeds 300.

[commit-bot: I haz the power, 2011-07-21 08:42:09]

Change committed as 93360