Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873103003:
Implements logging in the new compact translate UI. (Closed)
|
Created:
3 years, 7 months ago by ramyasharma Modified:
3 years, 7 months ago CC:
agrieve+watch_chromium.org, asvitkine+watch_chromium.org, chromium-reviews, dfalcantara+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplements logging in the new compact translate UI.
1. Adds enumerated logging events.
2. Adds language code information in separate histograms
for user events, by re-using the
CLD3LanguageCode enums for logging.
BUG=720231
TBR=groby@chromium.org
Review-Url: https://codereview.chromium.org/2873103003
Cr-Commit-Position: refs/heads/master@{#471593}
Committed: https://chromium.googlesource.com/chromium/src/+/f6e95c3d37313a5de99cfadcb5ffd7a3595a1653
Patch Set 1 #
Total comments: 10
Patch Set 2 #
Total comments: 6
Patch Set 3 #
Total comments: 2
Patch Set 4 #
Total comments: 10
Patch Set 5 #Patch Set 6 #Messages
Total messages: 66 (47 generated)
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ramyasharma@chromium.org changed reviewers: + dfalcantara@chromium.org, isherman@chromium.org, pdyson@chromium.org
Adding dfalcantara@ for TranslateCompactInfobar.java Adding isherman@ for histograms.xml and enums.xml Adding pdyson@ for logging.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Implements logging in the new compact translate UI. BUG=720231 ========== to ========== Implements logging in the new compact translate UI. BUG=720231 ==========
ramyasharma@chromium.org changed reviewers: + googleo@chromium.org
https://codereview.chromium.org/2873103003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2873103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:49: "Translate.Compact.infobar_never_translate_lang"; Did you mean to add these histograms to histograms.xml? Also, why are the suffixes hacker_case rather than CamelCase? https://codereview.chromium.org/2873103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:100: INFOBAR_SNACKBAR_CANCEL_ALWAYS, INFOBAR_HISTOGRAM_BOUNDARY); Optional nit: There's a lot of repetition of these two lines in this file. I'd recommend extracting a small wrapper function for readability, that you'd call like recordInfobarAction(INFOBAR_SNACKBAR_CANCEL_ALWAYS)
Sorry, plus one more comment: https://codereview.chromium.org/2873103003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2873103003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:75434: +<histogram name="Translate.Compact" enum="TranslateCompactType"> What does "Compact" mean in this histogram name? It feels like there is a noun that's missing here.
Some quick comments https://codereview.chromium.org/2873103003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2873103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:40: private static final String INFOBAR_HISTOGRAM_TRANSLATE_LANGUAGE = Any chance moving all the definitions to Metrics.java under translate and provide a convenient util to do logging shortly? https://codereview.chromium.org/2873103003/diff/1/tools/metrics/histograms/en... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2873103003/diff/1/tools/metrics/histograms/en... tools/metrics/histograms/enums.xml:37110: +<enum name="TranslateCompactType type="int"> maybe name it as TranslateCompactUiEvent Example: https://cs.chromium.org/chromium/src/tools/metrics/histograms/enums.xml?q=too...
ramyasharma@chromium.org changed reviewers: + miguelg@chromium.org
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Thanks Ilya and Leo, ptal? https://codereview.chromium.org/2873103003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2873103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:40: private static final String INFOBAR_HISTOGRAM_TRANSLATE_LANGUAGE = On 2017/05/10 07:23:58, Leo wrote: > Any chance moving all the definitions to Metrics.java under translate and > provide a convenient util to do logging shortly? Hmmm.. as we discussed, keeping these metrics in this file, as they are not used outside this class. If required we can move it in the future. https://codereview.chromium.org/2873103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:49: "Translate.Compact.infobar_never_translate_lang"; On 2017/05/10 07:10:20, Ilya Sherman wrote: > Did you mean to add these histograms to histograms.xml? Also, why are the > suffixes hacker_case rather than CamelCase? Yes, sorry I missed this. Fixed the case thanks. https://codereview.chromium.org/2873103003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:100: INFOBAR_SNACKBAR_CANCEL_ALWAYS, INFOBAR_HISTOGRAM_BOUNDARY); On 2017/05/10 07:10:20, Ilya Sherman wrote: > Optional nit: There's a lot of repetition of these two lines in this file. I'd > recommend extracting a small wrapper function for readability, that you'd call > like recordInfobarAction(INFOBAR_SNACKBAR_CANCEL_ALWAYS) Thanks for the idea. Done. https://codereview.chromium.org/2873103003/diff/1/tools/metrics/histograms/en... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2873103003/diff/1/tools/metrics/histograms/en... tools/metrics/histograms/enums.xml:37110: +<enum name="TranslateCompactType type="int"> On 2017/05/10 07:23:58, Leo wrote: > maybe name it as TranslateCompactUiEvent > > Example: > https://cs.chromium.org/chromium/src/tools/metrics/histograms/enums.xml?q=too... Good idea, thanks. Done. https://codereview.chromium.org/2873103003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2873103003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:75434: +<histogram name="Translate.Compact" enum="TranslateCompactType"> On 2017/05/10 07:11:07, Ilya Sherman wrote: > What does "Compact" mean in this histogram name? It feels like there is a noun > that's missing here. We already have a translate UI, we are implementing a new UI which is a compact version of the translate UI. So this signifies that.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2873103003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2873103003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75407: +<histogram name="Translate.Compact.Infobar.AlwaysTranslateLang"> Optional nit: I'd name these histograms something like "Translate.CompactUI.Language.AlwaysTranslate", "Translate.CompactUI.Language.MoreLanguages", etc. (It's also fine to use "Translate.Compact.Infobar" if you think that name is clearer -- though I'd kind of expect "Compact" to always be associated with "Infobar", so I'd group it as "Translate.CompactInfobar".) https://codereview.chromium.org/2873103003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75411: + language option is clicked in the menu. Seems like this histogram could use an enum associated with it, so that users don't have to manually look up hashcode values. Ditto for the other hashcode-recording histograms.
https://codereview.chromium.org/2873103003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2873103003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:355: private void recordInfobarAction(int action) { static
Patchset #3 (id:60001) has been deleted
Description was changed from ========== Implements logging in the new compact translate UI. BUG=720231 ========== to ========== Implements logging in the new compact translate UI. 1. Adds enumerated logging events. 2. Adds language code information in separate histograms for user events, by re-using the CLD3LanguageCode enums for logging. BUG=720231 ==========
Thanks Leo and Illya, PTAL? https://codereview.chromium.org/2873103003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2873103003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:355: private void recordInfobarAction(int action) { On 2017/05/11 00:58:50, Leo wrote: > static Done. https://codereview.chromium.org/2873103003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2873103003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75407: +<histogram name="Translate.Compact.Infobar.AlwaysTranslateLang"> On 2017/05/11 00:04:10, Ilya Sherman wrote: > Optional nit: I'd name these histograms something like > > "Translate.CompactUI.Language.AlwaysTranslate", > "Translate.CompactUI.Language.MoreLanguages", > etc. > > (It's also fine to use "Translate.Compact.Infobar" if you think that name is > clearer -- though I'd kind of expect "Compact" to always be associated with > "Infobar", so I'd group it as "Translate.CompactInfobar".) Thanks for the suggestion Ilya. I have renamed these to Translate.CompactInfobar.Language. https://codereview.chromium.org/2873103003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75411: + language option is clicked in the menu. On 2017/05/11 00:04:09, Ilya Sherman wrote: > Seems like this histogram could use an enum associated with it, so that users > don't have to manually look up hashcode values. Ditto for the other > hashcode-recording histograms. Thanks. Yes, I was able to reuse the enum CLD3LanguageCode.
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Metrics LGTM, thanks. https://codereview.chromium.org/2873103003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2873103003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:374: private void recordSparseInfobarLanguageData(String histogram, String langCode) { Optional nit: "Sparse" seems like an implementation detail, so I'd omit it from this method name.
Patchset #4 (id:100001) has been deleted
Thanks Ilya. https://codereview.chromium.org/2873103003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2873103003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:374: private void recordSparseInfobarLanguageData(String histogram, String langCode) { On 2017/05/11 20:25:05, Ilya Sherman wrote: > Optional nit: "Sparse" seems like an implementation detail, so I'd omit it from > this method name. Done.
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ramyasharma@chromium.org changed reviewers: + mdjones@chromium.org
Adding mdjones@ for chrome/android/java/src/org/chromium/chrome/browser/infobar
Patchset #4 (id:120001) has been deleted
lgtm lgtm for the infobar logging changes. https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java (right): https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java:78: private Map<String, Integer> mCodeToHashCodeRepresentation; s/mCodeToHashCodeRepresentation/mCodeToUMAHashCode. Representation in this class means "Language name". https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java:263: public Integer getUMAHashCodeRepresentationFromCode(String languageCode) { remove Representation.
https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java (right): https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java:261: * @return The UMA hashcode representation of the language, or null if not found. Does this allow tracking of new languages or will they come back as null?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
infobar/ lgtm % nits https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:57: private static final Integer INFOBAR_IMPRESSION = 0; nit: why are these all "Integer" objects and not "int"? https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:124: boolean alwaysTranslate, boolean triggeredFromMenu, String[] languages, String[] codes, nit: "code" is used in a lot of names here. Can "String[] codes" be more descriptive or can you add javadoc?
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implements logging in the new compact translate UI. 1. Adds enumerated logging events. 2. Adds language code information in separate histograms for user events, by re-using the CLD3LanguageCode enums for logging. BUG=720231 ========== to ========== Implements logging in the new compact translate UI. 1. Adds enumerated logging events. 2. Adds language code information in separate histograms for user events, by re-using the CLD3LanguageCode enums for logging. BUG=720231 TBR=groby@chromium.org ==========
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:160001) has been deleted
Thanks for the review everyone. https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java (right): https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:57: private static final Integer INFOBAR_IMPRESSION = 0; On 2017/05/12 16:20:17, mdjones wrote: > nit: why are these all "Integer" objects and not "int"? Done. https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java:124: boolean alwaysTranslate, boolean triggeredFromMenu, String[] languages, String[] codes, On 2017/05/12 16:20:17, mdjones wrote: > nit: "code" is used in a lot of names here. Can "String[] codes" be more > descriptive or can you add javadoc? Done. https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java (right): https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java:78: private Map<String, Integer> mCodeToHashCodeRepresentation; On 2017/05/12 04:46:16, Leo wrote: > s/mCodeToHashCodeRepresentation/mCodeToUMAHashCode. > Representation in this class means "Language name". Thanks, good idea. Done. https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java:261: * @return The UMA hashcode representation of the language, or null if not found. On 2017/05/12 04:58:40, pdyson wrote: > Does this allow tracking of new languages or will they come back as null? It will allow tracking of new languages, null is returned only if hashcode is not available for a language, which is mostly an error case. https://codereview.chromium.org/2873103003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateOptions.java:263: public Integer getUMAHashCodeRepresentationFromCode(String languageCode) { On 2017/05/12 04:46:16, Leo wrote: > remove Representation. Done.
The CQ bit was unchecked by ramyasharma@chromium.org
The CQ bit was checked by ramyasharma@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, googleo@chromium.org, mdjones@chromium.org Link to the patchset: https://codereview.chromium.org/2873103003/#ps180001 (title: "")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ramyasharma@chromium.org
The CQ bit was checked by ramyasharma@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from googleo@chromium.org, mdjones@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2873103003/#ps200001 (title: "")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1494685920531460,
"parent_rev": "801801c769fcf5fda71384cac6861535bc1153f6", "commit_rev":
"f6e95c3d37313a5de99cfadcb5ffd7a3595a1653"}
Message was sent while issue was closed.
Description was changed from ========== Implements logging in the new compact translate UI. 1. Adds enumerated logging events. 2. Adds language code information in separate histograms for user events, by re-using the CLD3LanguageCode enums for logging. BUG=720231 TBR=groby@chromium.org ========== to ========== Implements logging in the new compact translate UI. 1. Adds enumerated logging events. 2. Adds language code information in separate histograms for user events, by re-using the CLD3LanguageCode enums for logging. BUG=720231 TBR=groby@chromium.org Review-Url: https://codereview.chromium.org/2873103003 Cr-Commit-Position: refs/heads/master@{#471593} Committed: https://chromium.googlesource.com/chromium/src/+/f6e95c3d37313a5de99cfadcb5ff... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001) as https://chromium.googlesource.com/chromium/src/+/f6e95c3d37313a5de99cfadcb5ff... |
