Add UMA metrics to phishguard pings

Add UMA metrics to phishguard pings based on
go/phishguard-uma

BUG=720172
Review-Url: https://codereview.chromium.org/2869253002
Cr-Commit-Position: refs/heads/master@{#472158}
Committed: https://chromium.googlesource.com/chromium/src/+/f7efcd141c49fca16e706ade7172d16826ee7edc

[Jialiu Lin, 2017-05-10 00:24:42]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 00:25:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2017-05-10 20:19:22]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 20:21:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2017-05-10 21:13:18]

jialiul@chromium.org changed reviewers:
+ nparker@chromium.org

[commit-bot: I haz the power, 2017-05-10 21:27:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 21:27:08]

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_...)

[Jialiu Lin, 2017-05-10 22:10:58]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 22:12:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 04:15:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 04:15:38]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Jialiu Lin, 2017-05-11 23:11:27]

friendly ping :-)

[Nathan Parker, 2017-05-11 23:36:31]

lgtm

https://codereview.chromium.org/2869253002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right):

https://codereview.chromium.org/2869253002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service.cc:84:
feature.name == kProtectedPasswordEntryPinging.name;
Add a DCHECK() that feature is one of the two expected feature types, to ensure
we modify this code if we add another.

https://codereview.chromium.org/2869253002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service.cc:100:
UMA_HISTOGRAM_ENUMERATION(kPasswordEntryRequestOutcomeHistogramName,
An idea (you decide if this is any better): You could simplify this code to
return the DISABLED_DUE_TO_* value and have the caller (a wrapper function) put
that in the appropriate histogram.  If the value is SUCCESS (or whatever enum is
appropriate), then the caller would return "true".

https://codereview.chromium.org/2869253002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc
(right):

https://codereview.chromium.org/2869253002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc:107:
histograms_.GetAllSamples(kPasswordOnFocusRequestOutcomeHistogramName),
nit: Add up top
 
using testing:ElementsAre
using base::Bucket

https://codereview.chromium.org/2869253002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc:107:
histograms_.GetAllSamples(kPasswordOnFocusRequestOutcomeHistogramName),
How about a comment that says where the bucket values come from?  Like, then
enum.  That'd make it easier for readers to understand the numbers, and extend
this when more are added.

[Jialiu Lin, 2017-05-12 00:45:26]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 00:46:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 01:00:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 01:00:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jialiu Lin, 2017-05-12 01:04:44]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[Jialiu Lin, 2017-05-12 01:04:52]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-05-12 01:05:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2017-05-12 02:12:31]

jialiul@chromium.org changed reviewers:
+ engedy@chromium.org, isherman@chromium.org

[Jialiu Lin, 2017-05-12 02:12:32]

Thanks nparker!
+engedy@ for owner review of password_manager code
+isherman@ for histograms.xml and enums.xml

Thanks!

https://codereview.chromium.org/2869253002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service.cc (right):

https://codereview.chromium.org/2869253002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service.cc:84:
feature.name == kProtectedPasswordEntryPinging.name;
On 2017/05/11 at 23:36:31, Nathan Parker wrote:
> Add a DCHECK() that feature is one of the two expected feature types, to
ensure we modify this code if we add another.

Good point.

https://codereview.chromium.org/2869253002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service.cc:100:
UMA_HISTOGRAM_ENUMERATION(kPasswordEntryRequestOutcomeHistogramName,
On 2017/05/11 at 23:36:31, Nathan Parker wrote:
> An idea (you decide if this is any better): You could simplify this code to
return the DISABLED_DUE_TO_* value and have the caller (a wrapper function) put
that in the appropriate histogram.  If the value is SUCCESS (or whatever enum is
appropriate), then the caller would return "true".

SG. code refactored.

https://codereview.chromium.org/2869253002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc
(right):

https://codereview.chromium.org/2869253002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc:107:
histograms_.GetAllSamples(kPasswordOnFocusRequestOutcomeHistogramName),
On 2017/05/11 at 23:36:31, Nathan Parker wrote:
> nit: Add up top
>  
> using testing:ElementsAre
> using base::Bucket

Due to the code refactoring, no need to check histogram anymore.

[commit-bot: I haz the power, 2017-05-12 03:23:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 03:23:06]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-05-12 21:07:44]

https://codereview.chromium.org/2869253002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2869253002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:49933: +   
"PasswordProtection.Verdict.ProtectedPasswordEntry". Verdict types
Please mark this histogram as <obsolete>.

https://codereview.chromium.org/2869253002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:49964: +   
&quot;PasswordProtection.Verdict.PasswordFieldOnFocus&quot;. Verdict types
Please mark this histogram as <obsolete>.

https://codereview.chromium.org/2869253002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:89853: +  <suffix
name="ProtectedPasswordEntry"/>
nit: Please add a brief label for each suffix.

https://codereview.chromium.org/2869253002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:89854: +  <affected-histogram
name="PasswordProtection.RequestOutcome"/>
Please mark the underlying histogram as <obsolete>, since only the suffixes are
emitted to.  (I'm actually not 100% sure whether marking it as <obsolete> will
also mark the suffixes as obsolete.  If you don't want to test this and
potentially need to land a small fix-up CL, you can instead add the attribute
base="true" to the underlying histogram.)

[Jialiu Lin, 2017-05-12 21:49:54]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[Jialiu Lin, 2017-05-12 21:50:55]

Thanks isherman! :-)

https://codereview.chromium.org/2869253002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2869253002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:49933: +   
"PasswordProtection.Verdict.ProtectedPasswordEntry". Verdict types
On 2017/05/12 at 21:07:44, Ilya Sherman wrote:
> Please mark this histogram as <obsolete>.

Done.

https://codereview.chromium.org/2869253002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:49964: +   
&quot;PasswordProtection.Verdict.PasswordFieldOnFocus&quot;. Verdict types
On 2017/05/12 at 21:07:44, Ilya Sherman wrote:
> Please mark this histogram as <obsolete>.

Done.

https://codereview.chromium.org/2869253002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:89853: +  <suffix
name="ProtectedPasswordEntry"/>
On 2017/05/12 at 21:07:44, Ilya Sherman wrote:
> nit: Please add a brief label for each suffix.

Done.

https://codereview.chromium.org/2869253002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:89854: +  <affected-histogram
name="PasswordProtection.RequestOutcome"/>
On 2017/05/12 at 21:07:44, Ilya Sherman wrote:
> Please mark the underlying histogram as <obsolete>, since only the suffixes
are emitted to.  (I'm actually not 100% sure whether marking it as <obsolete>
will also mark the suffixes as obsolete.  If you don't want to test this and
potentially need to land a small fix-up CL, you can instead add the attribute
base="true" to the underlying histogram.)

base="true" sounds better. Thanks for pointing this out!

[commit-bot: I haz the power, 2017-05-12 21:50:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-05-12 22:18:51]

Metrics LGTM

[commit-bot: I haz the power, 2017-05-12 22:44:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 22:44:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jialiu Lin, 2017-05-15 02:16:35]

ping engedy@ for owner review c/b/password_manager   (function name change
only).

[engedy, 2017-05-15 12:30:45]

chrome/browser/password_manager LGTM.

[Jialiu Lin, 2017-05-15 21:33:12]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-05-15 21:33:13]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2869253002/#ps120001
(title: "resolve comments on histograms.xml")

[commit-bot: I haz the power, 2017-05-15 21:34:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 23:30:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 23:30:04]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Jialiu Lin, 2017-05-16 16:24:56]

The CQ bit was checked by jialiul@chromium.org

[commit-bot: I haz the power, 2017-05-16 16:25:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 18:06:00]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494951896185190,
"parent_rev": "5e8650321a3058567af36d42faf2618d93743e59", "commit_rev":
"429ff60bfd1dd17321afe60892280ca765a791fd"}

[commit-bot: I haz the power, 2017-05-16 18:06:31]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494951896185190,
"parent_rev": "5ff87ceb5f871cfd111b0d057eaf04d25ccb17ec", "commit_rev":
"f7efcd141c49fca16e706ade7172d16826ee7edc"}

[commit-bot: I haz the power, 2017-05-16 18:06:43]

Description was changed from

==========
Add UMA metrics to phishguard pings based on
go/phishguard-uma

BUG=720172
==========

to

==========
Add UMA metrics to phishguard pings based on
go/phishguard-uma

BUG=720172

Review-Url: https://codereview.chromium.org/2869253002
Cr-Commit-Position: refs/heads/master@{#472158}
Committed:
https://chromium.googlesource.com/chromium/src/+/f7efcd141c49fca16e706ade7172...
==========

[commit-bot: I haz the power, 2017-05-16 18:06:45]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/f7efcd141c49fca16e706ade7172...