Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(397)

Issue 15311006: Added and replaced some UMAs (Closed)

Created:
7 years, 7 months ago by hajimehoshi
Modified:
7 years, 6 months ago
CC:
chromium-reviews, MAD, Ilya Sherman, jar (doing other things), jshin+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Added and replaced some UMAs. 1) Deprecated the UMA key "Translate.ServerReportedUnsupportedLanguage" (the internal histograms.xml should be modified later). 2) Added the UMA key "Translate.UnavailableLanguage" which is sent when the language is reported by the server but not available for the user locale. [We decided to implement a new UMA instead of 2) in another CL] 3) Added the UMA key "Translate.UnsupportedLanguageAtInitiation" which is sent when the language was unsupported during initialization. BUG=242142 TEST=unit_tests Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202836

Patch Set 1 #

Total comments: 30

Patch Set 2 : (Rebasing) #

Patch Set 3 : Removed the nits and moved ToLanguageCode #

Patch Set 4 : Removed unnecessary lines #

Patch Set 5 : Moved ToLanguageCode back #

Total comments: 6

Patch Set 6 : Modified the comments in histograms.xml #

Total comments: 4

Patch Set 7 : Replaced the comment #

Patch Set 8 : (Rebasing) #

Patch Set 9 : (Rebasing) #

Total comments: 2

Patch Set 10 : DISALLOW_IMPLICIT_CONSTRUCTORS #

Patch Set 11 : (Rebasing) #

Patch Set 12 : Renamed GetCountInternal to GetCountWithoutSnapshot #

Patch Set 13 : (Rebasing) #

Patch Set 14 : Removed UMA_UNAVAILABLE_LANGUAGE #

Patch Set 15 : Removed nits #

Patch Set 16 : (Rebasing) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -35 lines) Patch
M chrome/browser/language_usage_metrics.h View 1 2 3 4 5 6 7 8 9 3 chunks +9 lines, -11 lines 0 comments Download
M chrome/browser/language_usage_metrics.cc View 1 2 3 4 2 chunks +13 lines, -13 lines 0 comments Download
M chrome/browser/translate/translate_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/translate/translate_manager_metrics.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/translate/translate_manager_metrics.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/translate/translate_manager_metrics_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +27 lines, -11 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 25 (0 generated)
hajimehoshi
Can you take a look?
7 years, 7 months ago (2013-05-21 09:47:50 UTC) #1
Takashi Toyoshima
lgtm with nits for browser/translate. Please ask other OWNERS reviews. Also, other guys will suggest ...
7 years, 7 months ago (2013-05-21 11:11:43 UTC) #2
Ilya Sherman
https://codereview.chromium.org/15311006/diff/1/chrome/common/metrics/metrics_util.h File chrome/common/metrics/metrics_util.h (right): https://codereview.chromium.org/15311006/diff/1/chrome/common/metrics/metrics_util.h#newcode25 chrome/common/metrics/metrics_util.h:25: int ToLanguageCode(const std::string &locale); This is not an appropriate ...
7 years, 7 months ago (2013-05-21 22:53:59 UTC) #3
hajimehoshi
Thank you! https://codereview.chromium.org/15311006/diff/1/chrome/browser/language_usage_metrics.cc File chrome/browser/language_usage_metrics.cc (right): https://codereview.chromium.org/15311006/diff/1/chrome/browser/language_usage_metrics.cc#newcode10 chrome/browser/language_usage_metrics.cc:10: #include "base/string_util.h" On 2013/05/21 11:11:43, Takashi Toyoshima ...
7 years, 7 months ago (2013-05-22 04:19:27 UTC) #4
hajimehoshi
Can you guys take a look at this CL? Thank you in advance. isherman: I ...
7 years, 7 months ago (2013-05-22 04:29:28 UTC) #5
Ilya Sherman
https://codereview.chromium.org/15311006/diff/1/chrome/common/metrics/metrics_util.h File chrome/common/metrics/metrics_util.h (right): https://codereview.chromium.org/15311006/diff/1/chrome/common/metrics/metrics_util.h#newcode25 chrome/common/metrics/metrics_util.h:25: int ToLanguageCode(const std::string &locale); This code doesn't belong under ...
7 years, 7 months ago (2013-05-22 07:51:36 UTC) #6
hajimehoshi
Thank you. +sky, *.gypi was rolled back and you no longer have to review this ...
7 years, 7 months ago (2013-05-22 08:08:36 UTC) #7
Ilya Sherman
https://codereview.chromium.org/15311006/diff/22001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/15311006/diff/22001/tools/metrics/histograms/histograms.xml#newcode8614 tools/metrics/histograms/histograms.xml:8614: but used by the translate server. I don't understand ...
7 years, 7 months ago (2013-05-22 08:34:31 UTC) #8
hajimehoshi
+toyoshim, would you give me an advice later? On 2013/05/22 08:34:31, Ilya Sherman wrote: > ...
7 years, 7 months ago (2013-05-23 00:59:33 UTC) #9
Takashi Toyoshima
Ilya: I answered. Does it make sense? Hajime: Does it help you to reword them? ...
7 years, 7 months ago (2013-05-23 01:26:56 UTC) #10
hajimehoshi
Thank you! https://chromiumcodereview.appspot.com/15311006/diff/22001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/15311006/diff/22001/tools/metrics/histograms/histograms.xml#newcode8614 tools/metrics/histograms/histograms.xml:8614: but used by the translate server. On ...
7 years, 7 months ago (2013-05-23 02:30:00 UTC) #11
Ilya Sherman
LGTM, thanks. https://chromiumcodereview.appspot.com/15311006/diff/28001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/15311006/diff/28001/tools/metrics/histograms/histograms.xml#newcode8618 tools/metrics/histograms/histograms.xml:8618: that of translation server may differ. Thanks ...
7 years, 7 months ago (2013-05-23 08:30:52 UTC) #12
hajimehoshi
Thank you! https://chromiumcodereview.appspot.com/15311006/diff/28001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/15311006/diff/28001/tools/metrics/histograms/histograms.xml#newcode8618 tools/metrics/histograms/histograms.xml:8618: that of translation server may differ. On ...
7 years, 7 months ago (2013-05-23 08:49:22 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/15311006/38001
7 years, 7 months ago (2013-05-23 09:05:53 UTC) #14
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=4569
7 years, 7 months ago (2013-05-23 09:14:59 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/15311006/49001
7 years, 7 months ago (2013-05-24 02:21:00 UTC) #16
hajimehoshi
+jshin, can you take a look at ui/base/*? +sky, can you take a look at ...
7 years, 7 months ago (2013-05-24 02:34:10 UTC) #17
sky
LGTM https://chromiumcodereview.appspot.com/15311006/diff/49001/chrome/browser/language_usage_metrics.h File chrome/browser/language_usage_metrics.h (right): https://chromiumcodereview.appspot.com/15311006/diff/49001/chrome/browser/language_usage_metrics.h#newcode39 chrome/browser/language_usage_metrics.h:39: LanguageUsageMetrics(); DISALLOW_IMPLICIT_CONSTRUCTORS.
7 years, 7 months ago (2013-05-24 15:28:27 UTC) #18
hajimehoshi
Thank you! https://chromiumcodereview.appspot.com/15311006/diff/49001/chrome/browser/language_usage_metrics.h File chrome/browser/language_usage_metrics.h (right): https://chromiumcodereview.appspot.com/15311006/diff/49001/chrome/browser/language_usage_metrics.h#newcode39 chrome/browser/language_usage_metrics.h:39: LanguageUsageMetrics(); On 2013/05/24 15:28:28, sky wrote: > ...
7 years, 7 months ago (2013-05-27 00:55:22 UTC) #19
hajimehoshi
ping +jshin On 2013/05/27 00:55:22, hajimehoshi wrote: > Thank you! > > https://chromiumcodereview.appspot.com/15311006/diff/49001/chrome/browser/language_usage_metrics.h > File ...
7 years, 7 months ago (2013-05-28 01:25:41 UTC) #20
hajimehoshi
Sorry for confusion, but we decided to change the direction. We are going to implement ...
7 years, 6 months ago (2013-05-29 07:36:39 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/15311006/83001
7 years, 6 months ago (2013-05-29 07:43:07 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/15311006/86001
7 years, 6 months ago (2013-05-29 08:04:08 UTC) #23
commit-bot: I haz the power
Change committed as 202836
7 years, 6 months ago (2013-05-29 11:26:56 UTC) #24
hajimehoshi
7 years, 6 months ago (2013-05-30 07:11:16 UTC) #25
Message was sent while issue was closed.
I noticed that 1) is not done because I was confused by rebasing or something.
I'll do that at https://codereview.chromium.org/15987004/

On 2013/05/29 11:26:56, I haz the power (commit-bot) wrote:
> Change committed as 202836

Powered by Google App Engine
This is Rietveld 408576698