Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
227083007:
UMA for New Profile Management. Track User-Menu Tutorial actions with Histograms. (Closed)
|
Created:
6 years, 8 months ago by Mike Lerman Modified:
6 years, 7 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUMA for New Profile Management. Track User-Menu Tutorial actions with Histograms.
Specifically tracking Tutorial Display, Tutorial Accept
and Tutorial Dismissal.
Googlers:
View the Enrollment section of the design doc for details.
https://docs.google.com/a/google.com/document/d/1ybiN1r2xAlMj0PBABH63PJIyVWhsM409xPpZI4zlmck
BUG=357693
TEST=Interact with the user menu and the tutorial card. Verify
that UMA Histograms reflect the actual behaviour.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267716
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268094
Patch Set 1 #Patch Set 2 : Change name of "Promo" histogram to "Enrollment" #Patch Set 3 : Rebase #
Total comments: 10
Patch Set 4 : Apply metrics to correct cards; add to histograms.xml #Patch Set 5 : Exclude enroll-confirm from counts of the promo card. #
Total comments: 10
Patch Set 6 : Rename metrics constants. #Patch Set 7 : Rebase #
Total comments: 4
Patch Set 8 : Rename constants; track tutorial card only once. #
Total comments: 19
Patch Set 9 : Nits and update for unit tests; update histograms.xml #
Total comments: 4
Patch Set 10 : More browsertest cleanup. #
Total comments: 24
Patch Set 11 : Histogram naming; Nits; Simplify BrowserTest; Comments. #
Total comments: 2
Patch Set 12 : Nits, periods. #
Total comments: 2
Patch Set 13 : Nits, function rename. #Patch Set 14 : Rebase #Patch Set 15 : Die memory leak! From ProfileChooserView::CreateProfileChooserView() #Patch Set 16 : Further clean up condition controlling OptionsView. #
Messages
Total messages: 35 (0 generated)
Hi Hui, I've added the UMA stats for the promo card of the avatar menu. I believe you did a lot of work on that menu. Can you review that I've added them correctly? Thanks, Mike
https://codereview.chromium.org/227083007/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/227083007/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics.h:90: }; mirror promo is not dismiss-able at the moment ... https://codereview.chromium.org/227083007/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/227083007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/new_avatar_button.h:33: friend class NewAvatarMenuButtonTest; no longer needs NewAvatarMenuButtonTest? https://codereview.chromium.org/227083007/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/227083007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:532: ProfileMetrics::LogProfileMirrorEnrollment(ProfileMetrics::PROMO_DISMISS); this dismisses the confirmation tutorial, not the mirror promo tutorial. Currently mirror promo is not dismiss-able. https://codereview.chromium.org/227083007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:590: ProfileMetrics::LogProfileMirrorEnrollment(ProfileMetrics::PROMO_ACCEPT); the link opens the learn more url, it dose not mean user accepts the mirror promo. Instead you should put the code in EnableNewProfileManagementPreview. https://codereview.chromium.org/227083007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:672: ProfileMetrics::LogProfileMirrorEnrollment(ProfileMetrics::PROMO_DISPLAY); we have two different tutorials at the moment, one that prompts the user to try out mirror (aka mirror promo), and one that notifies the user that mirror has been enabled successfully, i don't think you want to mix both. You should only count the mirror promo tutorial created from CreateNewProfileManagementPreviewView.
Hi Hui, Can you have a second look? I've changed the intentions behind the metrics, and should better align with the flow now. 谢谢 https://codereview.chromium.org/227083007/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/227083007/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics.h:90: }; I've renamed these. I know Mirror isn't disable-able right now, but we will need to eventually. On 2014/04/16 15:47:45, guohui wrote: > mirror promo is not dismiss-able at the moment ... https://codereview.chromium.org/227083007/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/227083007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/new_avatar_button.h:33: friend class NewAvatarMenuButtonTest; That test still exists, I didn't change it. I copied the existing test to base my test on, but the old one is still there. On 2014/04/16 15:47:45, guohui wrote: > no longer needs NewAvatarMenuButtonTest? https://codereview.chromium.org/227083007/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/227083007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:532: ProfileMetrics::LogProfileMirrorEnrollment(ProfileMetrics::PROMO_DISMISS); On 2014/04/16 15:47:45, guohui wrote: > this dismisses the confirmation tutorial, not the mirror promo tutorial. > Currently mirror promo is not dismiss-able. Done. https://codereview.chromium.org/227083007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:590: ProfileMetrics::LogProfileMirrorEnrollment(ProfileMetrics::PROMO_ACCEPT); On 2014/04/16 15:47:45, guohui wrote: > the link opens the learn more url, it dose not mean user accepts the mirror > promo. Instead you should put the code in EnableNewProfileManagementPreview. Renamed. Done. https://codereview.chromium.org/227083007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:672: ProfileMetrics::LogProfileMirrorEnrollment(ProfileMetrics::PROMO_DISPLAY); On 2014/04/16 15:47:45, guohui wrote: > we have two different tutorials at the moment, one that prompts the user to try > out mirror (aka mirror promo), and one that notifies the user that mirror has > been enabled successfully, i don't think you want to mix both. You should only > count the mirror promo tutorial created from > CreateNewProfileManagementPreviewView. Done.
https://codereview.chromium.org/227083007/diff/70001/chrome/browser/profiles/... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/227083007/diff/70001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics.h:86: ENROLL_LAUNCH_TUTORIAL, // The Mirror Promo was displayed it is very confusing to have the same comment for two different enum values. Please update. https://codereview.chromium.org/227083007/diff/70001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics.h:86: ENROLL_LAUNCH_TUTORIAL, // The Mirror Promo was displayed launch_tutorial means user clicks the learn more link, right? Please change the name to somehting like MIRROR_LEARN_MORE. We already use 'tutorial' to mean the blue tutorial card, it is confusing to reuse the term for the learn more link. https://codereview.chromium.org/227083007/diff/70001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics.h:88: ENROLL_CLOSE_PROMO_CARD, // The Promo was dismissed there are two tutorial cards, it is no clear which one your refer to. The mirror promo card is not dismiss-able at the moment, please change the name to something like ENROLL_CLOSE_WELCOME_CARD., and update the comment. https://codereview.chromium.org/227083007/diff/70001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics.h:89: MIRROR_DISABLE, // Mirror was disabled after having been enabled nit: missing . at the end of the sentence. https://codereview.chromium.org/227083007/diff/70001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/227083007/diff/70001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:674: if (tutorial_shown == FALSE && !switches::IsNewProfileManagement()) { Currently tutorial_shown only tracks the welcome-enabled view, not the preview-promo view, thus tutorial_shown == false would be always false for the preview-promo view. You need to update tutorial_shown for preview-promo the same way as we are doing for welcome-enabled.
Hi Hui, Back over to you. Thanks! https://codereview.chromium.org/227083007/diff/70001/chrome/browser/profiles/... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/227083007/diff/70001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics.h:86: ENROLL_LAUNCH_TUTORIAL, // The Mirror Promo was displayed On 2014/04/25 14:46:50, guohui wrote: > it is very confusing to have the same comment for two different enum values. > Please update. Done. https://codereview.chromium.org/227083007/diff/70001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics.h:86: ENROLL_LAUNCH_TUTORIAL, // The Mirror Promo was displayed On 2014/04/25 14:46:50, guohui wrote: > launch_tutorial means user clicks the learn more link, right? Please change the > name to somehting like MIRROR_LEARN_MORE. We already use 'tutorial' to mean the > blue tutorial card, it is confusing to reuse the term for the learn more link. Done. https://codereview.chromium.org/227083007/diff/70001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics.h:88: ENROLL_CLOSE_PROMO_CARD, // The Promo was dismissed On 2014/04/25 14:46:50, guohui wrote: > there are two tutorial cards, it is no clear which one your refer to. The mirror > promo card is not dismiss-able at the moment, please change the name to > something like ENROLL_CLOSE_WELCOME_CARD., and update the comment. Done. https://codereview.chromium.org/227083007/diff/70001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics.h:89: MIRROR_DISABLE, // Mirror was disabled after having been enabled On 2014/04/25 14:46:50, guohui wrote: > nit: missing . at the end of the sentence. Done. https://codereview.chromium.org/227083007/diff/70001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/227083007/diff/70001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:674: if (tutorial_shown == FALSE && !switches::IsNewProfileManagement()) { On 2014/04/25 14:46:50, guohui wrote: > Currently tutorial_shown only tracks the welcome-enabled view, not the > preview-promo view, thus tutorial_shown == false would be always false for the > preview-promo view. > > You need to update tutorial_shown for preview-promo the same way as we are doing > for welcome-enabled. Ok. I had put in the condition on tutorial_shown because I was getting strange double-counting before, but the refactoring you did seems to have eliminated the need for me to put it. It's not necessary especially since I only track displays of the Preview card, not the Welcome card.
https://codereview.chromium.org/227083007/diff/110001/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/227083007/diff/110001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:89: MIRROR_DISABLE, // Mirror was disabled after having been enabled. nit: i think the convention is to prefix each enum constant with the enum name, e.g. PROFILE_ENROLLMENT_SHOW_PREVIEW_PROMO https://codereview.chromium.org/227083007/diff/110001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/227083007/diff/110001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:688: } hmm, but you do track the tutorial display on view refresh, please update tutorial_shown to track the preview-promo tutorial as well and then only fire UMA event if !tutorial_shown.
https://codereview.chromium.org/227083007/diff/110001/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/227083007/diff/110001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:89: MIRROR_DISABLE, // Mirror was disabled after having been enabled. On 2014/04/28 19:26:18, guohui wrote: > nit: i think the convention is to prefix each enum constant with the enum name, > e.g. PROFILE_ENROLLMENT_SHOW_PREVIEW_PROMO Done. https://codereview.chromium.org/227083007/diff/110001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/227083007/diff/110001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:688: } On 2014/04/28 19:26:18, guohui wrote: > hmm, but you do track the tutorial display on view refresh, please update > tutorial_shown to track the preview-promo tutorial as well and then only fire > UMA event if !tutorial_shown. Done.
lgtm
Review Requested! Could I please ask for reviews by OWNERs please? noms - c/b/profiles msw - c/b/ui/views asvitkine - histograms.xml Thanks!
Looking good! A couple of comments https://codereview.chromium.org/227083007/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/227083007/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:93: // Mirror was disabled after having been enabled nit: needs a . at the end https://codereview.chromium.org/227083007/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:95: NUM_PROFILE_ENROLLMENT_METRICS I think enums have to end in a comma https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:535: LogProfileMirrorEnrollment( nit: move the LogProfile... to the previous line, after the ::. Then you can still break the new line after the bracket. (here and in 3 more places) https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:541: LogProfileMirrorEnrollment( move this above the EnableNew...() call. That method opens the user manager, which will take focus away from the bubble and close it, so this looks a bit sketchy, I think. https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:608: ProfileMetrics:: nit: move this above the //TODO, so that the comment refers to the right line. https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (left): https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:113: BrowserList::GetInstance(chrome::GetActiveDesktop()); Hmm I don't understand this diff. :( Did you rename the file, and then delete the SignOut test? I mean, even if it's flaky, this is a test we should fix, not delete...Or, if it's a brand new file, I am confused about how git is tracking this. https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: 2014? https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:87: UMAHistogramHelper histograms; Is this used anywhere? https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:115: IN_PROC_BROWSER_TEST_F(ProfileChooserViewBrowserTest, MAYBE_ViewProfileUMA) { I don't think this is flaky. Sign out is flaky because of the asynchronous calls/user manager/guest profile creation, but just opening the bubble shouldn't be flaky (famous last words) :)
https://codereview.chromium.org/227083007/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/227083007/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:93: // Mirror was disabled after having been enabled On 2014/04/29 14:27:31, Monica Dinculescu wrote: > nit: needs a . at the end Done. https://codereview.chromium.org/227083007/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:95: NUM_PROFILE_ENROLLMENT_METRICS On 2014/04/29 14:27:31, Monica Dinculescu wrote: > I think enums have to end in a comma Done. https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:535: LogProfileMirrorEnrollment( On 2014/04/29 14:27:31, Monica Dinculescu wrote: > nit: move the LogProfile... to the previous line, after the ::. Then you can > still break the new line after the bracket. (here and in 3 more places) Done. https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:541: LogProfileMirrorEnrollment( On 2014/04/29 14:27:31, Monica Dinculescu wrote: > move this above the EnableNew...() call. That method opens the user manager, > which will take focus away from the bubble and close it, so this looks a bit > sketchy, I think. Done. https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:541: LogProfileMirrorEnrollment( On 2014/04/29 14:27:31, Monica Dinculescu wrote: > move this above the EnableNew...() call. That method opens the user manager, > which will take focus away from the bubble and close it, so this looks a bit > sketchy, I think. Done. https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:608: ProfileMetrics:: On 2014/04/29 14:27:31, Monica Dinculescu wrote: > nit: move this above the //TODO, so that the comment refers to the right line. Done. https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (left): https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:113: BrowserList::GetInstance(chrome::GetActiveDesktop()); On 2014/04/29 14:27:31, Monica Dinculescu wrote: > Hmm I don't understand this diff. :( > Did you rename the file, and then delete the SignOut test? I mean, even if it's > flaky, this is a test we should fix, not delete...Or, if it's a brand new file, > I am confused about how git is tracking this. Uploaded with a different similarity setting - now showing up as a new file in rietveld! https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/04/29 14:27:31, Monica Dinculescu wrote: > nit: 2014? Done. https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:87: UMAHistogramHelper histograms; On 2014/04/29 14:27:31, Monica Dinculescu wrote: > Is this used anywhere? No. https://codereview.chromium.org/227083007/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:115: IN_PROC_BROWSER_TEST_F(ProfileChooserViewBrowserTest, MAYBE_ViewProfileUMA) { On 2014/04/29 14:27:31, Monica Dinculescu wrote: > I don't think this is flaky. Sign out is flaky because of the asynchronous > calls/user manager/guest profile creation, but just opening the bubble shouldn't > be flaky (famous last words) :) Ok, I removed Linux and the flaky comment, added mobile OSes to the list (since they don't have an avatar menu).
profiles lgtm with nits https://codereview.chromium.org/227083007/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/227083007/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:110: IN_PROC_BROWSER_TEST_F(ProfileChooserViewBrowserTest, MAYBE_ViewProfileUMA) { This shouldn't be a MAYBE_, if it's not flaky :) https://codereview.chromium.org/227083007/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:124: if (!UserManagerView::IsShowing()) You're just showing the bubble, which has nothing to do with the user manager. So this isn't ever actually going to show up :)
https://codereview.chromium.org/227083007/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/227083007/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:110: IN_PROC_BROWSER_TEST_F(ProfileChooserViewBrowserTest, MAYBE_ViewProfileUMA) { On 2014/04/29 20:33:48, Monica Dinculescu wrote: > This shouldn't be a MAYBE_, if it's not flaky :) Discuss in person - we'll keep the current structure. https://codereview.chromium.org/227083007/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:124: if (!UserManagerView::IsShowing()) On 2014/04/29 20:33:48, Monica Dinculescu wrote: > You're just showing the bubble, which has nothing to do with the user manager. > So this isn't ever actually going to show up :) Done.
https://codereview.chromium.org/227083007/diff/180008/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/227083007/diff/180008/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:84: enum ProfileEnrollment { nit: should this match the histograms' "ProfileMirrorEnrollment"? https://codereview.chromium.org/227083007/diff/180008/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:85: // The Mirror Promo was displayed. nit: make these comments more similar with ProfileMirrorEnrollment. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:94: PROFILE_ENROLLMENT_DISABLE_MIRROR, nit: this seems fairly distinct from "Webdata DB error" as it's described in ProfileMirrorEnrollment. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:687: LogProfileMirrorEnrollment( nit: wrap this on the line above. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:64: void ProfileChooserViewBrowserTest::CreateTestingProfile() { Can this use TestingProfileManager::CreateTestingProfile? (bonus points if you fix up NewAvatarMenuButtonTest too) https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:68: // Sign in the default profile Is this comment correct? nit: period at the end https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:85: BrowserView* browser_view = reinterpret_cast<BrowserView*>( nit: use BrowserView::GetBrowserViewForBrowser https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:88: // Ensure that the avatar icon button is not also showing. This comment doesn't look correct, merely copied. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:95: button->NotifyClick(mouse_ev); Can you call MenuButton::Activate instead?
I thought "Project Mirror" was an internal codename? If so, we probably shouldn't use it in Chromium source code... Would it make more sense to use "NewProfileManagement" instead? https://codereview.chromium.org/227083007/diff/180008/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/227083007/diff/180008/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:83: // Enum for tracking whether Mirror is enabled and Promo views. Nit: Mention in comment that this is used in a histogram and entries shouldn't be removed or re-ordered. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:685: if (tutorial_shown == false && !switches::IsNewProfileManagement()) { Nit: !tutorial_shown https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:18: #include "chrome/browser/ui/views/profiles/profile_chooser_view.h" This should be the first include.
Alexei, Good point - the histogram title has been renamed to Profile.UpgradeEnrollment. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.h (right): https://codereview.chromium.org/227083007/diff/180008/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:83: // Enum for tracking whether Mirror is enabled and Promo views. On 2014/04/30 16:45:37, Alexei Svitkine wrote: > Nit: Mention in comment that this is used in a histogram and entries shouldn't > be removed or re-ordered. Done. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:84: enum ProfileEnrollment { On 2014/04/29 23:10:52, msw wrote: > nit: should this match the histograms' "ProfileMirrorEnrollment"? Done. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:85: // The Mirror Promo was displayed. On 2014/04/29 23:10:52, msw wrote: > nit: make these comments more similar with ProfileMirrorEnrollment. Done. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.h:94: PROFILE_ENROLLMENT_DISABLE_MIRROR, On 2014/04/29 23:10:52, msw wrote: > nit: this seems fairly distinct from "Webdata DB error" as it's described in > ProfileMirrorEnrollment. Sorry, I hadn't updated the histograms.xml. Done. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:685: if (tutorial_shown == false && !switches::IsNewProfileManagement()) { On 2014/04/30 16:45:37, Alexei Svitkine wrote: > Nit: !tutorial_shown Done. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:687: LogProfileMirrorEnrollment( On 2014/04/29 23:10:52, msw wrote: > nit: wrap this on the line above. Done. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:18: #include "chrome/browser/ui/views/profiles/profile_chooser_view.h" On 2014/04/30 16:45:37, Alexei Svitkine wrote: > This should be the first include. Done. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:64: void ProfileChooserViewBrowserTest::CreateTestingProfile() { On 2014/04/29 23:10:52, msw wrote: > Can this use TestingProfileManager::CreateTestingProfile? > (bonus points if you fix up NewAvatarMenuButtonTest too) This has been greatly simplified in this test - no need for any new profiles at all! https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:68: // Sign in the default profile On 2014/04/29 23:10:52, msw wrote: > Is this comment correct? nit: period at the end Done. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:85: BrowserView* browser_view = reinterpret_cast<BrowserView*>( On 2014/04/29 23:10:52, msw wrote: > nit: use BrowserView::GetBrowserViewForBrowser Done. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:88: // Ensure that the avatar icon button is not also showing. On 2014/04/29 23:10:52, msw wrote: > This comment doesn't look correct, merely copied. It was indeed. Removed, the code is self-explanatory. https://codereview.chromium.org/227083007/diff/180008/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:95: button->NotifyClick(mouse_ev); On 2014/04/29 23:10:52, msw wrote: > Can you call MenuButton::Activate instead? I tried this call - it doesn't cause the necessary effects to happen on the ProfileChooserView, so both the IsShowing() call doesn't return the correct value and the histogram value also doesn't get set.
lgtm with a nit https://codereview.chromium.org/227083007/diff/240001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/227083007/diff/240001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39534: + <int value="0" label="User viewed the Upgrade promo card in the user menu"/> nit: add trailing periods to the label here and those below.
LGTM % comment https://codereview.chromium.org/227083007/diff/240002/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.cc (right): https://codereview.chromium.org/227083007/diff/240002/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.cc:292: void ProfileMetrics::LogProfileMirrorEnrollment( Seems like you're still using the Mirror codename in the function name. Can you update it?
Thanks, reviewers! https://codereview.chromium.org/227083007/diff/240001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/227083007/diff/240001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39534: + <int value="0" label="User viewed the Upgrade promo card in the user menu"/> On 2014/04/30 18:46:50, msw wrote: > nit: add trailing periods to the label here and those below. Done. https://codereview.chromium.org/227083007/diff/240002/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.cc (right): https://codereview.chromium.org/227083007/diff/240002/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.cc:292: void ProfileMetrics::LogProfileMirrorEnrollment( On 2014/04/30 19:01:35, Alexei Svitkine wrote: > Seems like you're still using the Mirror codename in the function name. Can you > update it? Done.
Also, please update the CL description to not mention the Mirror code name as well.
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/227083007/290001
Message was sent while issue was closed.
Change committed as 267716
lgtm
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/227083007/310001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/227083007/310001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/227083007/310001
Message was sent while issue was closed.
Change committed as 268094 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
