Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1973293002:
[Smart Lock] Adds UMA Metrics for actions on Account Chooser. (Closed)
|
Created:
4 years, 7 months ago by melandory Modified:
4 years, 6 months ago Reviewers:
vasilii CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Smart Lock] Adds UMA Metrics for actions on Account Chooser.
Account chooser: How many clicks on an account and on “Cancel”?
BUG=611006
Committed: https://crrev.com/b3da8cc062b762d4fb9b9208405a7237dc601e89
Cr-Commit-Position: refs/heads/master@{#395844}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 1
Messages
Total messages: 27 (11 generated)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973293002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973293002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Smart Lock] Adds UMA Metrics for actions on Account Chooser. Account chooser: How many clicks on an account and on “Cancel”? BUG=611006 ========== to ========== [Smart Lock] Adds UMA Metrics for actions on Account Chooser. Account chooser: How many clicks on an account and on “Cancel”? BUG=611006 ==========
melandory@chromium.org changed reviewers: + vabr@chromium.org
melandory@chromium.org changed reviewers: + vasilii@chromium.org - vabr@chromium.org
PTAL. P.S. Absence of tests is known issue, it was decided that they will be implemented by intern.
On 2016/05/17 07:20:47, melandory wrote: > PTAL. > > P.S. Absence of tests is known issue, it was decided that they will be > implemented by intern. Decided by whom?
On 2016/05/17 09:30:25, vasilii wrote: > On 2016/05/17 07:20:47, melandory wrote: > > PTAL. > > > > P.S. Absence of tests is known issue, it was decided that they will be > > implemented by intern. > > Decided by whom? Talked to Vaclav on Friday: https://bugs.chromium.org/p/chromium/issues/detail?id=611718 The problem here, that Sabine, want this metrics in M51 and I have no capacity for adding tests right now.
LGTM with the pending tests. You missed M51, last beta was yesterday. https://codereview.chromium.org/1973293002/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/account_chooser_dialog_android.cc (right): https://codereview.chromium.org/1973293002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/account_chooser_dialog_android.cc:211: } no space https://codereview.chromium.org/1973293002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/account_chooser_dialog_android.cc:229: password_manager::metrics_util::LogAccountChooserUserAction( indent properly
On 2016/05/17 09:45:16, vasilii wrote: > LGTM with the pending tests. You missed M51, last beta was yesterday. I think stable is good enough. > > https://codereview.chromium.org/1973293002/diff/1/chrome/browser/password_man... > File chrome/browser/password_manager/account_chooser_dialog_android.cc (right): > > https://codereview.chromium.org/1973293002/diff/1/chrome/browser/password_man... > chrome/browser/password_manager/account_chooser_dialog_android.cc:211: } > no space > > https://codereview.chromium.org/1973293002/diff/1/chrome/browser/password_man... > chrome/browser/password_manager/account_chooser_dialog_android.cc:229: > password_manager::metrics_util::LogAccountChooserUserAction( > indent properly
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973293002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973293002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/1973293002/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973293002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973293002/60001
Message was sent while issue was closed.
Description was changed from ========== [Smart Lock] Adds UMA Metrics for actions on Account Chooser. Account chooser: How many clicks on an account and on “Cancel”? BUG=611006 ========== to ========== [Smart Lock] Adds UMA Metrics for actions on Account Chooser. Account chooser: How many clicks on an account and on “Cancel”? BUG=611006 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Smart Lock] Adds UMA Metrics for actions on Account Chooser. Account chooser: How many clicks on an account and on “Cancel”? BUG=611006 ========== to ========== [Smart Lock] Adds UMA Metrics for actions on Account Chooser. Account chooser: How many clicks on an account and on “Cancel”? BUG=611006 Committed: https://crrev.com/b3da8cc062b762d4fb9b9208405a7237dc601e89 Cr-Commit-Position: refs/heads/master@{#395844} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b3da8cc062b762d4fb9b9208405a7237dc601e89 Cr-Commit-Position: refs/heads/master@{#395844}
Message was sent while issue was closed.
https://codereview.chromium.org/1973293002/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager_metrics_util.cc (right): https://codereview.chromium.org/1973293002/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_metrics_util.cc:94: ACCOUNT_CHOOSER_ACTION_COUNT); What was the reason to add this method if you have to drop it in the follow-up patch as unused?? Note that it doesn't exist in M52 if you hoped to merge back just this CL and not https://codereview.chromium.org/2009773003/
Message was sent while issue was closed.
Oh, ok I see where I messed up stuff. On 25 May 2016 5:18 PM, <vasilii@chromium.org> wrote: > > > https://codereview.chromium.org/1973293002/diff/60001/components/password_man... > File > components/password_manager/core/browser/password_manager_metrics_util.cc > (right): > > > https://codereview.chromium.org/1973293002/diff/60001/components/password_man... > > components/password_manager/core/browser/password_manager_metrics_util.cc:94: > ACCOUNT_CHOOSER_ACTION_COUNT); > What was the reason to add this method if you have to drop it in the > follow-up patch as unused?? > > Note that it doesn't exist in M52 if you hoped to merge back just this > CL and not https://codereview.chromium.org/2009773003/ > > https://codereview.chromium.org/1973293002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/05/25 15:40:36, chromium-reviews wrote: > Oh, ok I see where I messed up stuff. > On 25 May 2016 5:18 PM, <mailto:vasilii@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/1973293002/diff/60001/components/password_man... > > File > > components/password_manager/core/browser/password_manager_metrics_util.cc > > (right): > > > > > > > https://codereview.chromium.org/1973293002/diff/60001/components/password_man... > > > > components/password_manager/core/browser/password_manager_metrics_util.cc:94: > > ACCOUNT_CHOOSER_ACTION_COUNT); > > What was the reason to add this method if you have to drop it in the > > follow-up patch as unused?? > > > > Note that it doesn't exist in M52 if you hoped to merge back just this > > CL and not https://codereview.chromium.org/2009773003/ > > > > https://codereview.chromium.org/1973293002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I'll just revent this one.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2009273002/ by melandory@chromium.org. The reason for reverting is: Obsolete change. This CL https://codereview.chromium.org/2009773003/ does right thing.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
