Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2400503002:
[Translate] Integrate TranslateEventProto UMA logging into TranslateManager. (Closed)
|
Created:
4 years, 2 months ago by Roger McFarlane (Chromium) Modified:
4 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, asvitkine+watch_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntegrate TranslateEventProto UMA logging into TranslateManager.
This CL integrates the TranslateEventProto into the TranaslateManager in order to report these events via UMA.
BUG=634961, 653700
Committed: https://crrev.com/2847cf85bbbd9dcfc7f9bcc3b6add85d709155ce
Cr-Commit-Position: refs/heads/master@{#432416}
Patch Set 1 : review candidate #
Total comments: 2
Patch Set 2 : rebase and plumb through to uma metrics provider #
Total comments: 4
Patch Set 3 : fix bad interaction with UMA macro magic #Patch Set 4 : hamelphi@'s comments #
Total comments: 52
Patch Set 5 : use unique_ptr #
Total comments: 8
Patch Set 6 : change signature of ShowTranslateBubble #Patch Set 7 : log bubble requested but not shown #Patch Set 8 : fix trybots #Patch Set 9 : moar ShowTranslateBubbleResult plumbing #
Total comments: 6
Patch Set 10 : plumb through to TranslateBubbleUiEvent UMA metric #
Total comments: 6
Patch Set 11 : nits #Patch Set 12 : remove un-needed include #Patch Set 13 : fix trybots #Messages
Total messages: 119 (90 generated)
Description was changed from ========== [WORK IN PROGRESS] Integrate translate event proto logging. BUG= ========== to ========== Integrate TranslateEventProto UMA logging into TranslateManager. This CL integrates the TranslateEventProto into the TranaslateManager in order to report these events via UMA. BUG=634961, 653700 ==========
rogerm@chromium.org changed reviewers: + hamelphi@chromium.org
The CQ bit was checked by rogerm@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: Exceeded global retry quota
https://codereview.chromium.org/2400503002/diff/40001/components/translate/co... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2400503002/diff/40001/components/translate/co... components/translate/core/browser/translate_manager.cc:369: translate_event_.set_target_language(target_lang); Why not call InitTranslateEvent here?
The CQ bit was checked by rogerm@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 #1 (id:1) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by rogerm@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rogerm@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rogerm@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rogerm@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: Exceeded global retry quota
The CQ bit was checked by rogerm@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Patchset #2 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
The CQ bit was checked by rogerm@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rogerm@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 #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
rogerm@chromium.org changed reviewers: + groby@chromium.org
Thanks. +groby@ https://codereview.chromium.org/2400503002/diff/40001/components/translate/co... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2400503002/diff/40001/components/translate/co... components/translate/core/browser/translate_manager.cc:369: translate_event_.set_target_language(target_lang); On 2016/10/07 21:41:26, hamelphi wrote: > Why not call InitTranslateEvent here? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rogerm@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...
Is it possible to add LANGUAGE_DISABLED_BY_AUTO_BLACKLIST logging here: https://cs.chromium.org/chromium/src/chrome/browser/translate/chrome_translat... and also other 'leaks' mentioned in https://groups.google.com/a/google.com/forum/#!topic/chrome-ranker/4gXbGfWHbuU possibly as UNKNOWN event types. This would give us an idea of the frequency of ShowBubble being dropped. https://codereview.chromium.org/2400503002/diff/220001/components/translate/c... File components/translate/core/browser/BUILD.gn (right): https://codereview.chromium.org/2400503002/diff/220001/components/translate/c... components/translate/core/browser/BUILD.gn:71: "//components/metrics/proto", This dependency is also in deps. I suppose we only need it once. If I understand public_deps correctly, you can remove the deps here and in unit_test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rogerm@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 checked by rogerm@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 #4 (id:260001) has been deleted
Thanks. Another look? Added and captured new event types for heuristic and window-state-based suppressions of the UI. https://codereview.chromium.org/2400503002/diff/220001/components/translate/c... File components/translate/core/browser/BUILD.gn (right): https://codereview.chromium.org/2400503002/diff/220001/components/translate/c... components/translate/core/browser/BUILD.gn:71: "//components/metrics/proto", On 2016/10/13 15:13:56, hamelphi wrote: > This dependency is also in deps. I suppose we only need it once. If I understand > public_deps correctly, you can remove the deps here and in unit_test. fixed the public_deps is needed: public_deps is transitive, deps is not. public_deps is needed because the translate event proto header is included in translate_manager.h, which is included elsewhere. I could remove the public_deps by holding the proto via a unique_ptr, but the lifecycle of the proto and the translate manager are exactly the same, so it makes more sense for it to be a direct member.
On 2016/10/13 19:30:17, Roger McFarlane (Chromium) wrote: > Thanks. Another look? > > Added and captured new event types for heuristic and window-state-based > suppressions of the UI. > > https://codereview.chromium.org/2400503002/diff/220001/components/translate/c... > File components/translate/core/browser/BUILD.gn (right): > > https://codereview.chromium.org/2400503002/diff/220001/components/translate/c... > components/translate/core/browser/BUILD.gn:71: "//components/metrics/proto", > On 2016/10/13 15:13:56, hamelphi wrote: > > This dependency is also in deps. I suppose we only need it once. If I > understand > > public_deps correctly, you can remove the deps here and in unit_test. > > fixed > > the public_deps is needed: public_deps is transitive, deps is not. > > public_deps is needed because the translate event proto header is included in > translate_manager.h, which is included elsewhere. > > I could remove the public_deps by holding the proto via a unique_ptr, but the > lifecycle of the proto and the translate manager are exactly the same, so it > makes more sense for it to be a direct member. Personal preference: Let's not expose things all over the place. If we must have the event on TranslateManager, let's make it a unique_ptr<> - but maybe we can keep it on the Ranker object instead (see comments)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2400503002/diff/220001/components/translate/c... File components/translate/core/browser/BUILD.gn (right): https://codereview.chromium.org/2400503002/diff/220001/components/translate/c... components/translate/core/browser/BUILD.gn:71: "//components/metrics/proto", On 2016/10/13 19:30:16, Roger McFarlane (Chromium) wrote: > On 2016/10/13 15:13:56, hamelphi wrote: > > This dependency is also in deps. I suppose we only need it once. If I > understand > > public_deps correctly, you can remove the deps here and in unit_test. > > fixed > > the public_deps is needed: public_deps is transitive, deps is not. > > public_deps is needed because the translate event proto header is included in > translate_manager.h, which is included elsewhere. > > I could remove the public_deps by holding the proto via a unique_ptr, but the > lifecycle of the proto and the translate manager are exactly the same, so it > makes more sense for it to be a direct member. My point was that you probably can remove "//components/metrics/proto" in the 'deps' section since you add it to public_deps. But I might be wrong. In any case, I don't feel strongly.
The CQ bit was checked by rogerm@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: This issue passed the CQ dry run.
Thanks! Another look? +sky +rkaplow https://codereview.chromium.org/2400503002/diff/220001/components/translate/c... File components/translate/core/browser/BUILD.gn (right): https://codereview.chromium.org/2400503002/diff/220001/components/translate/c... components/translate/core/browser/BUILD.gn:71: "//components/metrics/proto", On 2016/10/13 20:47:02, hamelphi wrote: > On 2016/10/13 19:30:16, Roger McFarlane (Chromium) wrote: > > On 2016/10/13 15:13:56, hamelphi wrote: > > > This dependency is also in deps. I suppose we only need it once. If I > > understand > > > public_deps correctly, you can remove the deps here and in unit_test. > > > > fixed > > > > the public_deps is needed: public_deps is transitive, deps is not. > > > > public_deps is needed because the translate event proto header is included in > > translate_manager.h, which is included elsewhere. > > > > I could remove the public_deps by holding the proto via a unique_ptr, but the > > lifecycle of the proto and the translate manager are exactly the same, so it > > makes more sense for it to be a direct member. > > My point was that you probably can remove "//components/metrics/proto" in the > 'deps' section since you add it to public_deps. But I might be wrong. In any > case, I don't feel strongly. ah... yeah, that's what I did. I thought you were suggesting the opposite. sorry for the confusion.
rogerm@chromium.org changed reviewers: + rkaplow@chromium.org, sky@chromium.org
+sky +rkaplow for realz this time
https://codereview.chromium.org/2400503002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2400503002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:1262: ChromeTranslateClient* translate_client = Can this code move to browser_commands so that it's picked up by mac? You could make ShowTranslateBubble return a enum indicating what happened, eg: enum class ShowTranslateBubbleResult { SUCCESS, RENDER_VIEW_HOST_EDITABLE, }; Notice I don't have minimized. You should be able to check that case in browser_commands.
https://codereview.chromium.org/2400503002/diff/300001/components/metrics/pro... File components/metrics/proto/translate_event.proto (right): https://codereview.chromium.org/2400503002/diff/300001/components/metrics/pro... components/metrics/proto/translate_event.proto:123: // The focus in on an editable field. not sure what this last enum is really trying to say
https://codereview.chromium.org/2400503002/diff/300001/components/metrics/pro... File components/metrics/proto/translate_event.proto (right): https://codereview.chromium.org/2400503002/diff/300001/components/metrics/pro... components/metrics/proto/translate_event.proto:117: // The web context for the translate is no longer active. Please describe explicitly what happened with Translate UI (i.e. shown or not shown) in the descriptions. This will help understand the event type without understanding the details of these specific situations. E.g. The translate UI was not shown because the web context for the translate is no longer active
Thanks. Another look? https://codereview.chromium.org/2400503002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2400503002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:1262: ChromeTranslateClient* translate_client = On 2016/10/14 23:26:59, sky wrote: > Can this code move to browser_commands so that it's picked up by mac? You could > make ShowTranslateBubble return a enum indicating what happened, eg: > > enum class ShowTranslateBubbleResult { > SUCCESS, > RENDER_VIEW_HOST_EDITABLE, > }; > > Notice I don't have minimized. You should be able to check that case in > browser_commands. How does the browser_commands Translate get triggered? I'm trying to capture the outcome of events where Chrome has decided to show the translate UI to the user and the outcome of the user's interactions with the translate UI. IIUC, Chrome's auto-initiation of the translate UI doesn't pass through browser_commmands. Once the UI is shown, whether via the auto-initiation of manual activation by the user, the outcome is captures by the translate UI delegate. I added additional translate bubble ui event to capture the bubble failing to be shown, separate form the translate event proto capturing the outcome of auto-initiated translate UI prompts (and the user's choices made when interacting with the UI in general). https://codereview.chromium.org/2400503002/diff/300001/components/metrics/pro... File components/metrics/proto/translate_event.proto (right): https://codereview.chromium.org/2400503002/diff/300001/components/metrics/pro... components/metrics/proto/translate_event.proto:117: // The web context for the translate is no longer active. On 2016/10/17 16:14:40, hamelphi wrote: > Please describe explicitly what happened with Translate UI (i.e. shown or not > shown) in the descriptions. This will help understand the event type without > understanding the details of these specific situations. > > E.g. > The translate UI was not shown because the web context for the translate is no > longer active Done. https://codereview.chromium.org/2400503002/diff/300001/components/metrics/pro... components/metrics/proto/translate_event.proto:123: // The focus in on an editable field. On 2016/10/17 14:48:20, rkaplow wrote: > not sure what this last enum is really trying to say Done.
The CQ bit was checked by rogerm@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2400503002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2400503002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:1262: ChromeTranslateClient* translate_client = On 2016/10/24 19:01:52, Roger McFarlane (Chromium) wrote: > On 2016/10/14 23:26:59, sky wrote: > > Can this code move to browser_commands so that it's picked up by mac? You > could > > make ShowTranslateBubble return a enum indicating what happened, eg: > > > > enum class ShowTranslateBubbleResult { > > SUCCESS, > > RENDER_VIEW_HOST_EDITABLE, > > }; > > > > Notice I don't have minimized. You should be able to check that case in > > browser_commands. > > How does the browser_commands Translate get triggered? From the bubble? I could be wrong, but this seems like the only place that calls ShowTranslateBubble. Maybe TranslateBubbleFactory does too? > I'm trying to capture the outcome of events where Chrome has decided to show the > translate UI to the user and the outcome of the user's interactions with the > translate UI. > > IIUC, Chrome's auto-initiation of the translate UI doesn't pass through > browser_commmands. Once the UI is shown, whether via the auto-initiation of > manual activation by the user, the outcome is captures by the translate UI > delegate. > > I added additional translate bubble ui event to capture the bubble failing to be > shown, separate form the translate event proto capturing the outcome of > auto-initiated translate UI prompts (and the user's choices made when > interacting with the UI in general). I was hoping browser_commands was the only place that called ShowTranslateBubble. If that isn't the case then I agree this code has to handle the error cases.
The CQ bit was checked by rogerm@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm histogram lg, didn't review the rest
On 2016/10/24 20:45:00, sky wrote: > https://codereview.chromium.org/2400503002/diff/300001/chrome/browser/ui/view... > File chrome/browser/ui/views/frame/browser_view.cc (right): > > https://codereview.chromium.org/2400503002/diff/300001/chrome/browser/ui/view... > chrome/browser/ui/views/frame/browser_view.cc:1262: ChromeTranslateClient* > translate_client = > On 2016/10/24 19:01:52, Roger McFarlane (Chromium) wrote: > > On 2016/10/14 23:26:59, sky wrote: > > > Can this code move to browser_commands so that it's picked up by mac? You > > could > > > make ShowTranslateBubble return a enum indicating what happened, eg: > > > > > > enum class ShowTranslateBubbleResult { > > > SUCCESS, > > > RENDER_VIEW_HOST_EDITABLE, > > > }; > > > > > > Notice I don't have minimized. You should be able to check that case in > > > browser_commands. > > > > How does the browser_commands Translate get triggered? > > From the bubble? I could be wrong, but this seems like the only place that calls > ShowTranslateBubble. Maybe TranslateBubbleFactory does too? Yes, TranslateBubbleFactory calls it from https://cs.chromium.org/chromium/src/chrome/browser/ui/translate/translate_bu... The browser_commands invokation is, IIUC, only triggered when the user clicks on the chip in the omnibox. I'm not sure it's triggerable on mac (since mac uses the infobar, not the bubble). > > I'm trying to capture the outcome of events where Chrome has decided to show > the > > translate UI to the user and the outcome of the user's interactions with the > > translate UI. > > > > IIUC, Chrome's auto-initiation of the translate UI doesn't pass through > > browser_commmands. Once the UI is shown, whether via the auto-initiation of > > manual activation by the user, the outcome is captures by the translate UI > > delegate. > > > > I added additional translate bubble ui event to capture the bubble failing to > be > > shown, separate form the translate event proto capturing the outcome of > > auto-initiated translate UI prompts (and the user's choices made when > > interacting with the UI in general). > > I was hoping browser_commands was the only place that called > ShowTranslateBubble. If that isn't the case then I agree this code has to handle > the error cases.
Ugh. It seems I had a whole bunch of messages sitting in my drafts. I'll republish unreviewed, but feel free to dismiss them if signs are significantly out of date. https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:202: if (translate_manager_) { Should never be NULL - just DCHECK if you want to verify, but you can also safely ignore. (See below) https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:333: if (translate_manager_ && Is there a specific reason you're checking translate_manager_? It should be a class invariant that this is not NULL. So if this happens when you test, we might have a bug. If it's just to ensure the invariant, I'd suggest just a DCHECK at the top of the function. https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:334: step == translate::TRANSLATE_STEP_BEFORE_TRANSLATE) { Why is this just recorded for BEFORE_TRANSLATE? https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:350: metrics::TranslateEventProto::BROSWER_WINDOW_NOT_ACTIVE); See above :) https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:352: return; On a wider scale, I'm wondering it this should return a reason for every early-out point, and we then centralize the recording at the call site (or in a wrapper) https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:1265: translate_client->GetTranslateManager(); Just DCHECK https://codereview.chromium.org/2400503002/diff/280001/components/metrics/pro... File components/metrics/proto/translate_event.proto (right): https://codereview.chromium.org/2400503002/diff/280001/components/metrics/pro... components/metrics/proto/translate_event.proto:114: // The previous page was in the same language, so the translate UI was I'm sorry for missing this originally - this seems somewhat related to TranslateBubbleUiEvent. Can we combine the two, and then hook ranker into ReportUiAction, or is that a bad idea? https://codereview.chromium.org/2400503002/diff/280001/components/metrics/pro... components/metrics/proto/translate_event.proto:145: } Odd - is this a diff because there's whitespace at the end? https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:221: InitTranslateEvent(language_code, target_lang, *translate_prefs); I'm a little bit concerned with having a long-lived parameter via a member variable. Can translate_event be a local instead? https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:250: metrics::TranslateEventProto::LANGUAGE_DISABLED_BY_USER_CONFIG); We _definitely_ should centralize our reporting :) https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:306: translate_event_.set_ranker_version(translate_ranker->GetModelVersion()); Why not set timestamp and version on init/record? https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:579: void TranslateManager::InitTranslateEvent(const std::string& src_lang, I wonder if the following two functions really belong on TranslateRanker https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:152: if (translate_manager_) { Looks like it's an invariant as well - no need for if, here and elsewhere. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:154: ->set_modified_source_language(language_code); Do you really want to call this every time the language is changed, as opposed to when the user has decided on a language? My suspicion is that you'll record a lot of accidental misclicks that way. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:180: translate_manager_->mutable_translate_event() see above https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:304: // Note that we don't capture a language being unblocked... which is not Super-annoying nit: Usually, Chrome comments avoid "we" or "I" statements because it's unclear whom/what they refer to. Can you change that comment to make it more clear? https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:351: // Capture that in the metrics. Note that we don't capture a language being see above re: "we"
On 2016/10/25 18:49:59, Roger McFarlane (Chromium) wrote: > On 2016/10/24 20:45:00, sky wrote: > > > https://codereview.chromium.org/2400503002/diff/300001/chrome/browser/ui/view... > > File chrome/browser/ui/views/frame/browser_view.cc (right): > > > > > https://codereview.chromium.org/2400503002/diff/300001/chrome/browser/ui/view... > > chrome/browser/ui/views/frame/browser_view.cc:1262: ChromeTranslateClient* > > translate_client = > > On 2016/10/24 19:01:52, Roger McFarlane (Chromium) wrote: > > > On 2016/10/14 23:26:59, sky wrote: > > > > Can this code move to browser_commands so that it's picked up by mac? You > > > could > > > > make ShowTranslateBubble return a enum indicating what happened, eg: > > > > > > > > enum class ShowTranslateBubbleResult { > > > > SUCCESS, > > > > RENDER_VIEW_HOST_EDITABLE, > > > > }; > > > > > > > > Notice I don't have minimized. You should be able to check that case in > > > > browser_commands. > > > > > > How does the browser_commands Translate get triggered? > > > > From the bubble? I could be wrong, but this seems like the only place that > calls > > ShowTranslateBubble. Maybe TranslateBubbleFactory does too? > > Yes, TranslateBubbleFactory calls it from > https://cs.chromium.org/chromium/src/chrome/browser/ui/translate/translate_bu... > > The browser_commands invokation is, IIUC, only triggered when the user clicks on > the chip in the omnibox. I believe that means you should move the logging back to BrowserView, right? > > I'm not sure it's triggerable on mac (since mac uses the infobar, not the > bubble). > > > > I'm trying to capture the outcome of events where Chrome has decided to show > > the > > > translate UI to the user and the outcome of the user's interactions with the > > > translate UI. > > > > > > IIUC, Chrome's auto-initiation of the translate UI doesn't pass through > > > browser_commmands. Once the UI is shown, whether via the auto-initiation of > > > manual activation by the user, the outcome is captures by the translate UI > > > delegate. > > > > > > I added additional translate bubble ui event to capture the bubble failing > to > > be > > > shown, separate form the translate event proto capturing the outcome of > > > auto-initiated translate UI prompts (and the user's choices made when > > > interacting with the UI in general). > > > > I was hoping browser_commands was the only place that called > > ShowTranslateBubble. If that isn't the case then I agree this code has to > handle > > the error cases.
Thanks! Lots of great discussion! I had some questions/comments before I push up a new patchset. https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:202: if (translate_manager_) { On 2016/10/25 18:53:10, groby wrote: > Should never be NULL - just DCHECK if you want to verify, but you can also > safely ignore. (See below) translate_manager_ is reset to NULL on WebContentsDestroyed(). https://cs.chromium.org/chromium/src/chrome/browser/translate/chrome_translat... I observed a couple of other places that were checking for NULL before dereferencing translate_manager_, presumably for the same reason, and followed suit. Happy to change these to a DCHECK at the top of the method is you're sure this will never be called after, or in contention with, WebContentsDestroyed. https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:333: if (translate_manager_ && On 2016/10/25 18:53:10, groby wrote: > Is there a specific reason you're checking translate_manager_? It should be a > class invariant that this is not NULL. So if this happens when you test, we > might have a bug. > > If it's just to ensure the invariant, I'd suggest just a DCHECK at the top of > the function. Acknowledged. See previous commentary re: WebContentsDestroyed(). https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:334: step == translate::TRANSLATE_STEP_BEFORE_TRANSLATE) { On 2016/10/25 18:53:10, groby wrote: > Why is this just recorded for BEFORE_TRANSLATE? We're capturing the outcome of showing the translate prompt to the user. All of the other steps occur after the user has interacted with the prompt in an affirmative manner (which will have already been captured). I.e., BEFORE_TRANSLATE is the state in which the user interacts. The others are informative states to the user (translate in progress, translate complete). https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:350: metrics::TranslateEventProto::BROSWER_WINDOW_NOT_ACTIVE); On 2016/10/25 18:53:10, groby wrote: > See above :) Acknowledged. See previous commentary re: WebContentsDestroyed(). https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:352: return; On 2016/10/25 18:53:10, groby wrote: > On a wider scale, I'm wondering it this should return a reason for every > early-out point, and we then centralize the recording at the call site (or in a > wrapper) In the most recent patchset, I've done something similar with the downstream "Show" functionality. I could continue to percolate a result up the chain. https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:1265: translate_client->GetTranslateManager(); On 2016/10/25 18:53:11, groby wrote: > Just DCHECK Acknowledged. https://codereview.chromium.org/2400503002/diff/280001/components/metrics/pro... File components/metrics/proto/translate_event.proto (right): https://codereview.chromium.org/2400503002/diff/280001/components/metrics/pro... components/metrics/proto/translate_event.proto:114: // The previous page was in the same language, so the translate UI was On 2016/10/25 18:53:11, groby wrote: > I'm sorry for missing this originally - this seems somewhat related to > TranslateBubbleUiEvent. Can we combine the two, and then hook ranker into > ReportUiAction, or is that a bad idea? I'm not sure. My initial thoughts... ReportUiAction is more fine-grained about the ui interactions it captures. The logging we are aiming for is more to capture broad user-preference as opposed to individual clicks (i.e., we capture the selected target language rather than that the target language menu was clicked, and we don't capture it as a seperate event, rather we update the event attriubutes and save the event when the user ultimately accepts or declines the translation. We could combine it, but I would think we'd want to go the other way: Expand the context taken by ReportUiAction and move it to TranslateManager or some other stateful class and centralize all translate event logging to it. Some events would just generate a histogram event, others would also log the accumulated context into a TranslateEventProto. Would also need to integrate ReportUiAction into the translate ui delegate flow to capture inforbar interactions. https://codereview.chromium.org/2400503002/diff/280001/components/metrics/pro... components/metrics/proto/translate_event.proto:145: } On 2016/10/25 18:53:11, groby wrote: > Odd - is this a diff because there's whitespace at the end? Maybe a trailing newline? Not sure. I've let 'git cl format' do its thing. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:221: InitTranslateEvent(language_code, target_lang, *translate_prefs); On 2016/10/25 18:53:11, groby wrote: > I'm a little bit concerned with having a long-lived parameter via a member > variable. Can translate_event be a local instead? Not really, it needs to be created at the time the translation context is established (here) and needs to still be available whenever the translation event finishes happening, which could be here, but could, more interestingly, be when the user interacts with the UI. In particular, we want to remember what we thought the src/dest language pair was and what the translate ranker recommended we do. We could alternatively create the event on the heap and pass it via unique_ptr to the UI. This was my original approach, but it generated some pretty invasive changes to the interfaces of a lot of intermediate methods and classes, making a mess of the abstractions and the abstraction layering. Passing ownership of the event also makes it difficult to remember the context on a revert. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:250: metrics::TranslateEventProto::LANGUAGE_DISABLED_BY_USER_CONFIG); On 2016/10/25 18:53:11, groby wrote: > We _definitely_ should centralize our reporting :) +1 Once this is in, ReportInitiationStatus will be redundant, but we'll need to build some synthetic metrics to be able to visualize the data on the UMA dashboards before we remove it. The TranslateEventProto is more cohesive than the various UMA histograms we currently have. Right now, we can't reconstruct the context of a given event because, by design, disparate UMA metrics are uncorrelatable... so deriving training and validation from UMA histogram logs doesn't work. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:306: translate_event_.set_ranker_version(translate_ranker->GetModelVersion()); On 2016/10/25 18:53:11, groby wrote: > Why not set timestamp and version on init/record? If the ranker isn't enabled, then the ranker request doesn't happen and the ranker may not have a model version, in which case those fields are left unset in the event proto. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:579: void TranslateManager::InitTranslateEvent(const std::string& src_lang, On 2016/10/25 18:53:11, groby wrote: > I wonder if the following two functions really belong on TranslateRanker Perhaps... I'm not particularly sold on TranslateRanker implementing the RecordTranslateEvent binding to the UMA metrics provider, since the TranslateRanker can otherwise be cleanly considered disabled. In it's current state, the TranslateRanker can be disabled, but will still be instantiated for event logging :( TranslateRanker would also need to be broken into two parts, the ranker model cache and a per-web-context ranker model instance. That would move the translate_event_ member and these two methods, but that's about it. Clients would still go through the manager to get to the ranker to get to the event. It's an extra layer of abstraction that I don't see buying us much. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:152: if (translate_manager_) { On 2016/10/25 18:53:11, groby wrote: > Looks like it's an invariant as well - no need for if, here and elsewhere. Acknowledged. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:154: ->set_modified_source_language(language_code); On 2016/10/25 18:53:11, groby wrote: > Do you really want to call this every time the language is changed, as opposed > to when the user has decided on a language? My suspicion is that you'll record a > lot of accidental misclicks that way. This just tracks the state. We only record once the user has decided. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:180: translate_manager_->mutable_translate_event() On 2016/10/25 18:53:11, groby wrote: > see above Acknowledged. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:304: // Note that we don't capture a language being unblocked... which is not On 2016/10/25 18:53:11, groby wrote: > Super-annoying nit: Usually, Chrome comments avoid "we" or "I" statements > because it's unclear whom/what they refer to. Can you change that comment to > make it more clear? Will do, here, above and below... and will keep in mind for future CLs. :) https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:351: // Capture that in the metrics. Note that we don't capture a language being On 2016/10/25 18:53:11, groby wrote: > see above re: "we" Acknowledged.
Hopefully answered the questions. A lot more design feedback (what could go where etc), please let me know what you think https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:202: if (translate_manager_) { On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > On 2016/10/25 18:53:10, groby wrote: > > Should never be NULL - just DCHECK if you want to verify, but you can also > > safely ignore. (See below) > > translate_manager_ is reset to NULL on WebContentsDestroyed(). > > https://cs.chromium.org/chromium/src/chrome/browser/translate/chrome_translat... > > I observed a couple of other places that were checking for NULL before > dereferencing translate_manager_, presumably for the same reason, and followed > suit. We really shouldn't have to - ChromeTranslateClient is WebContentsUserData, which means its lifetime is scoped to the lifetime of the WebContents. WebContentsDestroyed is only there to clean up resources allocated. > > Happy to change these to a DCHECK at the top of the method is you're sure this > will never be called after, or in contention with, WebContentsDestroyed. If it would be called after WebContentsDestroyed, we'd have a lifetime issue - somebody is holding on to this instead of looking it up from the WebContents. If it's called in contention with WCD, I'd be very sad, because this should all happen on the same thread. So yes, please DCHECK only. https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:334: step == translate::TRANSLATE_STEP_BEFORE_TRANSLATE) { On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > On 2016/10/25 18:53:10, groby wrote: > > Why is this just recorded for BEFORE_TRANSLATE? > > We're capturing the outcome of showing the translate prompt to the user. All of > the other steps occur after the user has interacted with the prompt in an > affirmative manner (which will have already been captured). > > I.e., BEFORE_TRANSLATE is the state in which the user interacts. The others are > informative states to the user (translate in progress, translate complete). If you don't mind, could you add a comment here? Because I know I'll forget, and ask the same question again three months from here :) https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:352: return; On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > On 2016/10/25 18:53:10, groby wrote: > > On a wider scale, I'm wondering it this should return a reason for every > > early-out point, and we then centralize the recording at the call site (or in > a > > wrapper) > > In the most recent patchset, I've done something similar with the downstream > "Show" functionality. I could continue to percolate a result up the chain. Is it worth doing in this patchset, or should we postpone this to stage two? https://codereview.chromium.org/2400503002/diff/280001/components/metrics/pro... File components/metrics/proto/translate_event.proto (right): https://codereview.chromium.org/2400503002/diff/280001/components/metrics/pro... components/metrics/proto/translate_event.proto:114: // The previous page was in the same language, so the translate UI was On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > On 2016/10/25 18:53:11, groby wrote: > > I'm sorry for missing this originally - this seems somewhat related to > > TranslateBubbleUiEvent. Can we combine the two, and then hook ranker into > > ReportUiAction, or is that a bad idea? > > I'm not sure. > > My initial thoughts... > > ReportUiAction is more fine-grained about the ui interactions it captures. The > logging we are aiming for is more to capture broad user-preference as opposed to > individual clicks (i.e., we capture the selected target language rather than > that the target language menu was clicked, and we don't capture it as a seperate > event, rather we update the event attriubutes and save the event when the user > ultimately accepts or declines the translation. > > We could combine it, but I would think we'd want to go the other way: Expand the > context taken by ReportUiAction and move it to TranslateManager or some other > stateful class and centralize all translate event logging to it. Some events > would just generate a histogram event, others would also log the accumulated > context into a TranslateEventProto. > > Would also need to integrate ReportUiAction into the translate ui delegate flow > to capture inforbar interactions. I think overall this makes sense - we have many different places interested in what happened in the TranslateUI, so centralizing reporting somewhere would make it a little bit harder to miss changes. https://codereview.chromium.org/2400503002/diff/280001/components/metrics/pro... components/metrics/proto/translate_event.proto:145: } On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > On 2016/10/25 18:53:11, groby wrote: > > Odd - is this a diff because there's whitespace at the end? > > Maybe a trailing newline? Not sure. > > I've let 'git cl format' do its thing. Ack. ¯\_(ツ)_/¯ https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:221: InitTranslateEvent(language_code, target_lang, *translate_prefs); On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > On 2016/10/25 18:53:11, groby wrote: > > I'm a little bit concerned with having a long-lived parameter via a member > > variable. Can translate_event be a local instead? > > Not really, it needs to be created at the time the translation context is > established (here) and needs to still be available whenever the translation > event finishes happening, which could be here, but could, more interestingly, be > when the user interacts with the UI. > > In particular, we want to remember what we thought the src/dest language pair > was and what the translate ranker recommended we do. > > We could alternatively create the event on the heap and pass it via unique_ptr > to the UI. This was my original approach, but it generated some pretty invasive > changes to the interfaces of a lot of intermediate methods and classes, making a > mess of the abstractions and the abstraction layering. > > Passing ownership of the event also makes it difficult to remember the context > on a revert. > Hm. Let's keep it a member for now, then. But we've got a couple more places where we really just want an event that lives from language detection until the user made a decision. Might be worth teasing out in a later refactoring... https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:250: metrics::TranslateEventProto::LANGUAGE_DISABLED_BY_USER_CONFIG); On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > On 2016/10/25 18:53:11, groby wrote: > > We _definitely_ should centralize our reporting :) > > +1 > > Once this is in, ReportInitiationStatus will be redundant, but we'll need to > build some synthetic metrics to be able to visualize the data on the UMA > dashboards before we remove it. > > The TranslateEventProto is more cohesive than the various UMA histograms we > currently have. Right now, we can't reconstruct the context of a given event > because, by design, disparate UMA metrics are uncorrelatable... so deriving > training and validation from UMA histogram logs doesn't work. Yep. I'm just thinking that we can centralize all the event recording in one place, see above. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:306: translate_event_.set_ranker_version(translate_ranker->GetModelVersion()); On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > On 2016/10/25 18:53:11, groby wrote: > > Why not set timestamp and version on init/record? > > If the ranker isn't enabled, then the ranker request doesn't happen and the > ranker may not have a model version, in which case those fields are left unset > in the event proto. I suppose so.... This bugs me, but I'm currently having a hard time explaining why. Ignore me for now, I'll chime back in if I have more than "that feels strange" :) https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:579: void TranslateManager::InitTranslateEvent(const std::string& src_lang, On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > On 2016/10/25 18:53:11, groby wrote: > > I wonder if the following two functions really belong on TranslateRanker > > Perhaps... > > I'm not particularly sold on TranslateRanker implementing the > RecordTranslateEvent binding to the UMA metrics provider, since the > TranslateRanker can otherwise be cleanly considered disabled. In it's current > state, the TranslateRanker can be disabled, but will still be instantiated for > event logging :( Can we then move the entire event logging... elsewhere? (This goes, I think, with the idea of unifying the logging) Possibly a static function instead, because I don't think the logger needs to keep a lot of state. > > TranslateRanker would also need to be broken into two parts, the ranker model > cache and a per-web-context ranker model instance. That kind of neatly matches to TranslateDownloadManager and TranslateClient (which are global download cache and a per-web-context instance) > translate_event_ member and these two methods, but that's about it. Clients > would still go through the manager to get to the ranker to get to the event. > It's an extra layer of abstraction that I don't see buying us much. It's mostly about mirroring the existing architecture. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:154: ->set_modified_source_language(language_code); On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > On 2016/10/25 18:53:11, groby wrote: > > Do you really want to call this every time the language is changed, as opposed > > to when the user has decided on a language? My suspicion is that you'll record > a > > lot of accidental misclicks that way. > > This just tracks the state. We only record once the user has decided. You still could pull that at decision time instead. (It's not a big deal, but that might get it out of the UI delegate)
The CQ bit was checked by rogerm@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...
Sorry for the long delay. :( I've plumbed the ShowTranslateBubbleResult up and applied most of groby's other suggestions, punting the larger refactorings to another CL. Another look? https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:202: if (translate_manager_) { On 2016/10/25 23:17:21, groby wrote: > On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > > On 2016/10/25 18:53:10, groby wrote: > > > Should never be NULL - just DCHECK if you want to verify, but you can also > > > safely ignore. (See below) > > > > translate_manager_ is reset to NULL on WebContentsDestroyed(). > > > > > https://cs.chromium.org/chromium/src/chrome/browser/translate/chrome_translat... > > > > I observed a couple of other places that were checking for NULL before > > dereferencing translate_manager_, presumably for the same reason, and followed > > suit. > > We really shouldn't have to - ChromeTranslateClient is WebContentsUserData, > which means its lifetime is scoped to the lifetime of the WebContents. > > WebContentsDestroyed is only there to clean up resources allocated. > > > > > Happy to change these to a DCHECK at the top of the method is you're sure this > > will never be called after, or in contention with, WebContentsDestroyed. > > If it would be called after WebContentsDestroyed, we'd have a lifetime issue - > somebody is holding on to this instead of looking it up from the WebContents. > > If it's called in contention with WCD, I'd be very sad, because this should all > happen on the same thread. > > So yes, please DCHECK only. Done. https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:334: step == translate::TRANSLATE_STEP_BEFORE_TRANSLATE) { On 2016/10/25 23:17:21, groby wrote: > On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > > On 2016/10/25 18:53:10, groby wrote: > > > Why is this just recorded for BEFORE_TRANSLATE? > > > > We're capturing the outcome of showing the translate prompt to the user. All > of > > the other steps occur after the user has interacted with the prompt in an > > affirmative manner (which will have already been captured). > > > > I.e., BEFORE_TRANSLATE is the state in which the user interacts. The others > are > > informative states to the user (translate in progress, translate complete). > > If you don't mind, could you add a comment here? Because I know I'll forget, and > ask the same question again three months from here :) Done. https://codereview.chromium.org/2400503002/diff/280001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:352: return; On 2016/10/25 23:17:21, groby wrote: > On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > > On 2016/10/25 18:53:10, groby wrote: > > > On a wider scale, I'm wondering it this should return a reason for every > > > early-out point, and we then centralize the recording at the call site (or > in > > a > > > wrapper) > > > > In the most recent patchset, I've done something similar with the downstream > > "Show" functionality. I could continue to percolate a result up the chain. > > Is it worth doing in this patchset, or should we postpone this to stage two? Done. https://codereview.chromium.org/2400503002/diff/280001/components/metrics/pro... File components/metrics/proto/translate_event.proto (right): https://codereview.chromium.org/2400503002/diff/280001/components/metrics/pro... components/metrics/proto/translate_event.proto:114: // The previous page was in the same language, so the translate UI was On 2016/10/25 23:17:21, groby wrote: > On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > > On 2016/10/25 18:53:11, groby wrote: > > > I'm sorry for missing this originally - this seems somewhat related to > > > TranslateBubbleUiEvent. Can we combine the two, and then hook ranker into > > > ReportUiAction, or is that a bad idea? > > > > I'm not sure. > > > > My initial thoughts... > > > > ReportUiAction is more fine-grained about the ui interactions it captures. The > > logging we are aiming for is more to capture broad user-preference as opposed > to > > individual clicks (i.e., we capture the selected target language rather than > > that the target language menu was clicked, and we don't capture it as a > seperate > > event, rather we update the event attriubutes and save the event when the user > > ultimately accepts or declines the translation. > > > > We could combine it, but I would think we'd want to go the other way: Expand > the > > context taken by ReportUiAction and move it to TranslateManager or some other > > stateful class and centralize all translate event logging to it. Some events > > would just generate a histogram event, others would also log the accumulated > > context into a TranslateEventProto. > > > > Would also need to integrate ReportUiAction into the translate ui delegate > flow > > to capture inforbar interactions. > > I think overall this makes sense - we have many different places interested in > what happened in the TranslateUI, so centralizing reporting somewhere would make > it a little bit harder to miss changes. Ok, I'll look at centralizing this. But I think that would be better done in another CL. I am (along with hamelphi@) looking at generalizing logging for contextual assists, including but not limited to translate, over the next quarter or two and this would naturally fit into this effort. Hopefully, I'll have a more concrete design doc/plan to share with you by the time we come visit you. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... File components/translate/core/browser/translate_manager.cc (right): https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:221: InitTranslateEvent(language_code, target_lang, *translate_prefs); On 2016/10/25 23:17:21, groby wrote: > On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > > On 2016/10/25 18:53:11, groby wrote: > > > I'm a little bit concerned with having a long-lived parameter via a member > > > variable. Can translate_event be a local instead? > > > > Not really, it needs to be created at the time the translation context is > > established (here) and needs to still be available whenever the translation > > event finishes happening, which could be here, but could, more interestingly, > be > > when the user interacts with the UI. > > > > In particular, we want to remember what we thought the src/dest language pair > > was and what the translate ranker recommended we do. > > > > We could alternatively create the event on the heap and pass it via unique_ptr > > to the UI. This was my original approach, but it generated some pretty > invasive > > changes to the interfaces of a lot of intermediate methods and classes, making > a > > mess of the abstractions and the abstraction layering. > > > > Passing ownership of the event also makes it difficult to remember the context > > on a revert. > > > > Hm. Let's keep it a member for now, then. > > But we've got a couple more places where we really just want an event that lives > from language detection until the user made a decision. Might be worth teasing > out in a later refactoring... Acknowledged. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:250: metrics::TranslateEventProto::LANGUAGE_DISABLED_BY_USER_CONFIG); On 2016/10/25 23:17:21, groby wrote: > On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > > On 2016/10/25 18:53:11, groby wrote: > > > We _definitely_ should centralize our reporting :) > > > > +1 > > > > Once this is in, ReportInitiationStatus will be redundant, but we'll need to > > build some synthetic metrics to be able to visualize the data on the UMA > > dashboards before we remove it. > > > > The TranslateEventProto is more cohesive than the various UMA histograms we > > currently have. Right now, we can't reconstruct the context of a given event > > because, by design, disparate UMA metrics are uncorrelatable... so deriving > > training and validation from UMA histogram logs doesn't work. > > Yep. I'm just thinking that we can centralize all the event recording in one > place, see above. Acknowledged. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:306: translate_event_.set_ranker_version(translate_ranker->GetModelVersion()); On 2016/10/25 23:17:21, groby wrote: > On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > > On 2016/10/25 18:53:11, groby wrote: > > > Why not set timestamp and version on init/record? > > > > If the ranker isn't enabled, then the ranker request doesn't happen and the > > ranker may not have a model version, in which case those fields are left unset > > in the event proto. > > I suppose so.... This bugs me, but I'm currently having a hard time explaining > why. Ignore me for now, I'll chime back in if I have more than "that feels > strange" :) Acknowledged. https://codereview.chromium.org/2400503002/diff/280001/components/translate/c... components/translate/core/browser/translate_manager.cc:579: void TranslateManager::InitTranslateEvent(const std::string& src_lang, On 2016/10/25 23:17:21, groby wrote: > On 2016/10/25 20:07:56, Roger McFarlane (Chromium) wrote: > > On 2016/10/25 18:53:11, groby wrote: > > > I wonder if the following two functions really belong on TranslateRanker > > > > Perhaps... > > > > I'm not particularly sold on TranslateRanker implementing the > > RecordTranslateEvent binding to the UMA metrics provider, since the > > TranslateRanker can otherwise be cleanly considered disabled. In it's current > > state, the TranslateRanker can be disabled, but will still be instantiated for > > event logging :( > Can we then move the entire event logging... elsewhere? (This goes, I think, > with the idea of unifying the logging) > > Possibly a static function instead, because I don't think the logger needs to > keep a lot of state. > > > > > TranslateRanker would also need to be broken into two parts, the ranker model > > cache and a per-web-context ranker model instance. > > That kind of neatly matches to TranslateDownloadManager and TranslateClient > (which are global download cache and a per-web-context instance) > > > translate_event_ member and these two methods, but that's about it. Clients > > would still go through the manager to get to the ranker to get to the event. > > It's an extra layer of abstraction that I don't see buying us much. > > It's mostly about mirroring the existing architecture. Fair enough. As we look toward generalizing the contextual assist model to more than just translate, can we punt this to a more general RankerDownloadManager and RankerClient (or something similar) in future CLs? https://codereview.chromium.org/2400503002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2400503002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:1262: ChromeTranslateClient* translate_client = On 2016/10/24 20:45:00, sky wrote: > On 2016/10/24 19:01:52, Roger McFarlane (Chromium) wrote: > > On 2016/10/14 23:26:59, sky wrote: > > > Can this code move to browser_commands so that it's picked up by mac? You > > could > > > make ShowTranslateBubble return a enum indicating what happened, eg: > > > > > > enum class ShowTranslateBubbleResult { > > > SUCCESS, > > > RENDER_VIEW_HOST_EDITABLE, > > > }; > > > > > > Notice I don't have minimized. You should be able to check that case in > > > browser_commands. > > > > How does the browser_commands Translate get triggered? > > From the bubble? I could be wrong, but this seems like the only place that calls > ShowTranslateBubble. Maybe TranslateBubbleFactory does too? > > > I'm trying to capture the outcome of events where Chrome has decided to show > the > > translate UI to the user and the outcome of the user's interactions with the > > translate UI. > > > > IIUC, Chrome's auto-initiation of the translate UI doesn't pass through > > browser_commmands. Once the UI is shown, whether via the auto-initiation of > > manual activation by the user, the outcome is captures by the translate UI > > delegate. > > > > I added additional translate bubble ui event to capture the bubble failing to > be > > shown, separate form the translate event proto capturing the outcome of > > auto-initiated translate UI prompts (and the user's choices made when > > interacting with the UI in general). > > I was hoping browser_commands was the only place that called > ShowTranslateBubble. If that isn't the case then I agree this code has to handle > the error cases. browser_commands responds to the user bringing up the translate ui manually when the browser detects the language and decides to offer the translate ui, it goes through the TranslateBubbleFactory.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #9 (id:380001) has been deleted
groby@ and sky@: Anything else that needs to go into this CL before I can land it?
https://codereview.chromium.org/2400503002/diff/400001/chrome/browser/ui/brow... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2400503002/diff/400001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:833: if (result != ShowTranslateBubbleResult::SUCCESS) { no {} https://codereview.chromium.org/2400503002/diff/400001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:834: translate::ReportUiAction(translate::BUBBLE_REQUESTED_BUT_NOT_SHOWN); How come you don't pass through result here? https://codereview.chromium.org/2400503002/diff/400001/chrome/browser/ui/brow... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/2400503002/diff/400001/chrome/browser/ui/brow... chrome/browser/ui/browser_window.h:76: WEBCONTENTS_NOT_ACTIVE, WEB_CONTENTS
Patchset #10 (id:420001) has been deleted
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Thanks. Another look? https://codereview.chromium.org/2400503002/diff/400001/chrome/browser/ui/brow... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2400503002/diff/400001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:833: if (result != ShowTranslateBubbleResult::SUCCESS) { On 2016/11/11 19:07:40, sky wrote: > no {} Done. https://codereview.chromium.org/2400503002/diff/400001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:834: translate::ReportUiAction(translate::BUBBLE_REQUESTED_BUT_NOT_SHOWN); On 2016/11/11 19:07:40, sky wrote: > How come you don't pass through result here? Done. This required expanding the translate::TranslateBubbleUiEvent enum and corresponding histograms.xml declaration. https://codereview.chromium.org/2400503002/diff/400001/chrome/browser/ui/brow... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/2400503002/diff/400001/chrome/browser/ui/brow... chrome/browser/ui/browser_window.h:76: WEBCONTENTS_NOT_ACTIVE, On 2016/11/11 19:07:40, sky wrote: > WEB_CONTENTS Done.
LGTM
LGTM modula a comment nit. There's also some angsting over mapping enums, but either decision is probably fine. Enum mappings are horrible. https://codereview.chromium.org/2400503002/diff/440001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2400503002/diff/440001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:221: case ShowTranslateBubbleResult::BROWSER_WINDOW_NOT_VALID: I am deeply saddened we have two enums we need to map to each other :( Given that the alternative is littering every place consuming/generating the result with the proto definition, I suppose it is what it is. I still haz a sad. Part of me wants to at least make this code more readable via a macro.... #define RECORD_IF(X) case ShowTranslateBubbleResult::X: translate_manager_->RecordTranslateEvent(metrics::TranslateEventProto::X); break Or maybe RecordTranslateEvent does the mapping? It's a huge chunk of code here that doesn't really have anything to do with the translateclient. RECORD_IF(BROWSER_WINDOW_NOT_VALID); RECORD_IF(BROWSER_WINDOW_MINIMIZED); ... https://codereview.chromium.org/2400503002/diff/440001/chrome/browser/ui/brow... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2400503002/diff/440001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:125: ShowTranslateBubbleResult result) { The same thing applies here - I don't know if the mapping code should be part of the consumer, and if we shouldn't just pass a ShowTranslateBubbleResult to translate::ReportUiAction https://codereview.chromium.org/2400503002/diff/440001/chrome/browser/ui/brow... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/2400503002/diff/440001/chrome/browser/ui/brow... chrome/browser/ui/browser_window.h:73: BROWSER_WINDOW_NOT_VALID, Probably worth a quick comment that all cases except SUCCESS are actual failure cases?
The CQ bit was checked by rogerm@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...
Thanks. https://codereview.chromium.org/2400503002/diff/440001/chrome/browser/transla... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2400503002/diff/440001/chrome/browser/transla... chrome/browser/translate/chrome_translate_client.cc:221: case ShowTranslateBubbleResult::BROWSER_WINDOW_NOT_VALID: On 2016/11/15 01:15:43, groby wrote: > I am deeply saddened we have two enums we need to map to each other :( > > Given that the alternative is littering every place consuming/generating the > result with the proto definition, I suppose it is what it is. I still haz a sad. > > Part of me wants to at least make this code more readable via a macro.... > > #define RECORD_IF(X) case ShowTranslateBubbleResult::X: > translate_manager_->RecordTranslateEvent(metrics::TranslateEventProto::X); break > > Or maybe RecordTranslateEvent does the mapping? It's a huge chunk of code here > that doesn't really have anything to do with the translateclient. > > RECORD_IF(BROWSER_WINDOW_NOT_VALID); > RECORD_IF(BROWSER_WINDOW_MINIMIZED); > ... I pulled the mapping out to a helper function in the anonymous namespace. https://codereview.chromium.org/2400503002/diff/440001/chrome/browser/ui/brow... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2400503002/diff/440001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:125: ShowTranslateBubbleResult result) { On 2016/11/15 01:15:43, groby wrote: > The same thing applies here - I don't know if the mapping code should be part of > the consumer, and if we shouldn't just pass a ShowTranslateBubbleResult to > translate::ReportUiAction Or maybe get rid of ShowTranslateBubbleResult and have the ShowTranslateBubble call chain return a TranslateBubbleUiEvent. I'll look at doing that in a follow-on. Maybe if I make it an enum class we can avoid having to include the enum values all over the place. https://codereview.chromium.org/2400503002/diff/440001/chrome/browser/ui/brow... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/2400503002/diff/440001/chrome/browser/ui/brow... chrome/browser/ui/browser_window.h:73: BROWSER_WINDOW_NOT_VALID, On 2016/11/15 01:15:43, groby wrote: > Probably worth a quick comment that all cases except SUCCESS are actual failure > cases? Done.
The CQ bit was checked by rogerm@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_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 rogerm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hamelphi@chromium.org, rkaplow@chromium.org, groby@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2400503002/#ps500001 (title: "fix trybots")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Integrate TranslateEventProto UMA logging into TranslateManager. This CL integrates the TranslateEventProto into the TranaslateManager in order to report these events via UMA. BUG=634961, 653700 ========== to ========== Integrate TranslateEventProto UMA logging into TranslateManager. This CL integrates the TranslateEventProto into the TranaslateManager in order to report these events via UMA. BUG=634961, 653700 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== Integrate TranslateEventProto UMA logging into TranslateManager. This CL integrates the TranslateEventProto into the TranaslateManager in order to report these events via UMA. BUG=634961, 653700 ========== to ========== Integrate TranslateEventProto UMA logging into TranslateManager. This CL integrates the TranslateEventProto into the TranaslateManager in order to report these events via UMA. BUG=634961, 653700 Committed: https://crrev.com/2847cf85bbbd9dcfc7f9bcc3b6add85d709155ce Cr-Commit-Position: refs/heads/master@{#432416} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/2847cf85bbbd9dcfc7f9bcc3b6add85d709155ce Cr-Commit-Position: refs/heads/master@{#432416} |
