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

Issue 794133002: [Password Manager] Add UMA signals to understand how often the password form submit navigate… (Closed)

Created:
6 years ago by Pritam Nikam
Modified:
6 years ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

[Password Manager] Add UMA signals to understand how often the password form submit navigates to different domain or host. BUG=435152 Committed: https://crrev.com/8b52071f8eafc83cb827334f8a281399fda51679 Cr-Commit-Position: refs/heads/master@{#308978}

Patch Set 1 : #

Total comments: 6

Patch Set 2 : Incorporated review comments. #

Total comments: 6

Patch Set 3 : Incorporated review inputs. #

Total comments: 8

Patch Set 4 : Addressed nits. #

Total comments: 6

Patch Set 5 : Incorporated review comments from Ilya. #

Total comments: 4

Patch Set 6 : Restored the c++ changes. #

Patch Set 7 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -0 lines) Patch
M chrome/browser/password_manager/chrome_password_manager_client.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/password_manager/chrome_password_manager_client.cc View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/password_manager.h View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/password_manager.cc View 1 2 3 4 5 6 4 chunks +18 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/password_manager_client.h View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/password_manager_client.cc View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 2 chunks +14 lines, -0 lines 0 comments Download

Messages

Total messages: 28 (8 generated)
Pritam Nikam
Hi Vaclav, I've added UMA matrics for password form navigating to different site on submission, ...
6 years ago (2014-12-11 12:14:26 UTC) #4
vabr (Chromium)
Hi Pritam, First -- did you remove the Cc line? If yes, please do not ...
6 years ago (2014-12-11 12:31:33 UTC) #5
Pritam Nikam
Thanks Vaclav for review. I've attempted addressing your review inputs along patch set #2, PTAL, ...
6 years ago (2014-12-12 14:30:09 UTC) #7
vabr (Chromium)
Hi Pritam Nikam, Thanks, this looks better than the the previous patch. I still think ...
6 years ago (2014-12-12 16:24:00 UTC) #8
Pritam Nikam
Thanks Vaclav for your inputs. I've addressed these along newest patch (#3). PTAL. Thanks! https://codereview.chromium.org/794133002/diff/80001/chrome/browser/password_manager/chrome_password_manager_client.cc ...
6 years ago (2014-12-15 07:41:49 UTC) #9
vabr (Chromium)
Thanks, Pritam. This LGTM with a couple of nits below. On important thing, though -- ...
6 years ago (2014-12-15 09:53:51 UTC) #10
Pritam Nikam
+ Ilya, Thanks Vaclav for review! Hi Ilya, PTAL @ tools/metrics/histograms/histograms.xml Feel free to glance ...
6 years ago (2014-12-15 10:30:53 UTC) #12
vabr (Chromium)
Hi Pritam, Please confirm that you have tested your patch on both a page where ...
6 years ago (2014-12-15 16:15:51 UTC) #13
Pritam Nikam
On 2014/12/15 16:15:51, vabr (OOO soon) wrote: > Hi Pritam, > > Please confirm that ...
6 years ago (2014-12-15 17:38:37 UTC) #14
Ilya Sherman
Pritam, just to double-check, are you aware that UMA metrics are only visible to Googlers? ...
6 years ago (2014-12-15 19:31:14 UTC) #15
Pritam Nikam
Thanks Ilya for review. I've addressed your inputs along newest patch set. PTAL! Thanks! https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histograms/histograms.xml ...
6 years ago (2014-12-16 07:16:52 UTC) #16
vabr (Chromium)
Thanks, Pritam! LGTM. The testing on your test domains is OK, thanks for confirming. I ...
6 years ago (2014-12-16 10:53:37 UTC) #17
Ilya Sherman
https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/794133002/diff/120001/tools/metrics/histograms/histograms.xml#newcode22999 tools/metrics/histograms/histograms.xml:22999: +<histogram name="PasswordManager.SubmitNavigatesToDifferentDomain" On 2014/12/16 10:53:37, vabr (back in January) ...
6 years ago (2014-12-16 19:56:26 UTC) #18
Pritam Nikam
Thanks Ilya & Vaclav. PTAL at new patch set. Thanks! https://codereview.chromium.org/794133002/diff/140001/components/password_manager/core/browser/password_manager.cc File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/794133002/diff/140001/components/password_manager/core/browser/password_manager.cc#newcode101 ...
6 years ago (2014-12-17 05:22:44 UTC) #19
Ilya Sherman
LGTM, thanks.
6 years ago (2014-12-17 21:18:53 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794133002/160001
6 years ago (2014-12-17 21:19:37 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/105757) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/100205) win_chromium_compile_dbg ...
6 years ago (2014-12-17 21:25:36 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794133002/180001
6 years ago (2014-12-18 07:35:55 UTC) #26
commit-bot: I haz the power
Committed patchset #7 (id:180001)
6 years ago (2014-12-18 09:42:11 UTC) #27
commit-bot: I haz the power
6 years ago (2014-12-18 09:42:52 UTC) #28
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/8b52071f8eafc83cb827334f8a281399fda51679
Cr-Commit-Position: refs/heads/master@{#308978}

Powered by Google App Engine
This is Rietveld 408576698