Fix sync-credential-related metrics

Fix sync-credential-related metrics

This CL makes PasswordManager_SyncCredentialUsed obsolete, beacuse it does not
meaure what it should (see the associated bug).

The CL also introduces a new user action,
PasswordManager_SyncCredentialFilledAndLoginSuccessfull, to actually measure
what the above action was supposed to measure.

The CL also introduces another user action, PasswordManager_ProcessingUpdate,
which keeps measuring what PasswordManager_SyncCredentialUsed measured, because
that data was also useful.

BUG=635428
Committed: https://crrev.com/3a69d6d6755a3f7c645ea682d777e55d7d4340b3
Cr-Commit-Position: refs/heads/master@{#410999}

[vabr (Chromium), 2016-08-08 13:01:55]

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

[commit-bot: I haz the power, 2016-08-08 13:02:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-08-08 13:03:19]

vabr@chromium.org changed reviewers:
+ vasilii@chromium.org

[vabr (Chromium), 2016-08-08 13:03:19]

Hi Vasilii,

Could you please review?

I will wait with histograms OWNERS review until you are OK with the change.

Thanks!
Vaclav

[commit-bot: I haz the power, 2016-08-08 13:15:33]

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

[commit-bot: I haz the power, 2016-08-08 13:15:33]

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

[vabr (Chromium), 2016-08-08 13:28:39]

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

[commit-bot: I haz the power, 2016-08-08 13:28:54]

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, 2016-08-08 13:48:28]

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

[commit-bot: I haz the power, 2016-08-08 13:48:28]

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

[vabr (Chromium), 2016-08-08 14:52:55]

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

[commit-bot: I haz the power, 2016-08-08 14:53:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-08-08 15:34:24]

lgtm

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:636:
client_.GetStoreResultFilter()),
I think returning const MockStoreResultFilter* from MockPasswordManagerClient
would allow us to get rid of the static cast. You can just change the
declaration of GetStoreResultFilter in the mock.

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
File
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc
(right):

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc:52:
PasswordStore* GetPasswordStore() const override {
It can return MockPasswordStore*. No need in GetMockPasswordStore. It's not
called anyway.

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc:89:
enum { NO_HISTOGRAM, HISTOGRAM_REPORTED } histogram_reported;
enums should precede the members.

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc:293:
"PasswordManager_SyncCredentialFilledAndLoginSuccessfull"));
I'd organize a constant for the string.

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc:296:
EXPECT_EQ(1, tester.GetActionCount(
How do you ensure that the results are cleared between the tests?

https://codereview.chromium.org/2220423002/diff/40001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2220423002/diff/40001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12398: Chrome to updat it with the current
password value.
update

[commit-bot: I haz the power, 2016-08-08 16:36:04]

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

[commit-bot: I haz the power, 2016-08-08 16:36:05]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-08-08 18:54:49]

vabr@chromium.org changed reviewers:
+ isherman@chromium.org

[vabr (Chromium), 2016-08-08 18:54:50]

Ilya, could you please review 	tools/metrics/actions/actions.xml ?

Vasilii, thanks for the comments. I will address them tomorrow when I'm at a
machine with Chrome checkout.

Thanks,
Vaclav

[Ilya Sherman, 2016-08-08 22:17:14]

Metrics LGTM % nits, though if you have nontrivial responses to these comments,
I'd like to take another look.  Thanks.

https://codereview.chromium.org/2220423002/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2220423002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12392: <action
name="PasswordManager_ProcessingUpdate">
nit: Perhaps change the suffix to something like "LoginFollowingAutofill"?

https://codereview.chromium.org/2220423002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12398: Chrome to updat it with the current
password value.
nit: s/updat/update

https://codereview.chromium.org/2220423002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12398: Chrome to updat it with the current
password value.
What does it mean to authorize Chrome to update the password value?  I thought
that we automatically update password values for credentials that have already
been authorized for the old password value.

[vabr (Chromium), 2016-08-09 11:21:41]

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

[commit-bot: I haz the power, 2016-08-09 11:21:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-08-09 12:19:53]

Thanks for both reviews. I answered the comments.

Ilya, please feel free to have another look, I'm not in a rush.

Vasilii, I pushed back on one particular comment of yours, which is not relevant
to the change done here and requires follow-up (details below).

Cheers,
Vaclav

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:636:
client_.GetStoreResultFilter()),
On 2016/08/08 15:34:24, vasilii wrote:
> I think returning const MockStoreResultFilter* from MockPasswordManagerClient
> would allow us to get rid of the static cast. You can just change the
> declaration of GetStoreResultFilter in the mock.

Thanks to you, Vasilii, for teaching me that the return types can be inheriting
types of the base class' method's return types. I made the effort to find this
in the standard, not sure how I managed to not notice this so far, it seems very
useful. :)
Done.

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
File
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc
(right):

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc:52:
PasswordStore* GetPasswordStore() const override {
On 2016/08/08 15:34:24, vasilii wrote:
> It can return MockPasswordStore*. No need in GetMockPasswordStore. It's not
> called anyway.

Done.

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc:89:
enum { NO_HISTOGRAM, HISTOGRAM_REPORTED } histogram_reported;
On 2016/08/08 15:34:24, vasilii wrote:
> enums should precede the members.

Well, these are both enum declarations and member declarations.
Not sure what the style guide says about that. One thing we can do is split them
into named enums above, and using those names below for the actual fields.
Personally I like the current version bettor for how succinct it is, but I am
happy to discuss.

However, because this is not code I introduced or touched in this CL, I will
keep it as is. Happy to do a follow-up after a discussion, but let's not block
this CL.

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc:293:
"PasswordManager_SyncCredentialFilledAndLoginSuccessfull"));
On 2016/08/08 15:34:24, vasilii wrote:
> I'd organize a constant for the string.

Done.

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc:296:
EXPECT_EQ(1, tester.GetActionCount(
On 2016/08/08 15:34:24, vasilii wrote:
> How do you ensure that the results are cleared between the tests?

UserActionTester owns the counters, so a freshly created one has them all
zeroed.

Based on that, the EXPECT_EQ(0, ...) on line 292 and similar are therefore not
necessary and add clutter, so I'll remove them.

https://codereview.chromium.org/2220423002/diff/40001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2220423002/diff/40001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12398: Chrome to updat it with the current
password value.
On 2016/08/08 15:34:24, vasilii wrote:
> update

Done.

https://codereview.chromium.org/2220423002/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2220423002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12392: <action
name="PasswordManager_ProcessingUpdate">
On 2016/08/08 22:17:14, Ilya Sherman wrote:
> nit: Perhaps change the suffix to something like "LoginFollowingAutofill"?

Done.

https://codereview.chromium.org/2220423002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12398: Chrome to updat it with the current
password value.
On 2016/08/08 22:17:14, Ilya Sherman wrote:
> nit: s/updat/update

Done.

https://codereview.chromium.org/2220423002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12398: Chrome to updat it with the current
password value.
On 2016/08/08 22:17:13, Ilya Sherman wrote:
> What does it mean to authorize Chrome to update the password value?  I thought
> that we automatically update password values for credentials that have already
> been authorized for the old password value.

In the past we indeed updated passwords silently. However, as part of support
for change-password forms, this behaviour was changed to an explicit prompt,
like by saving. The reason was the pre-existing, and now more likely, situation,
when Chrome misunderstood what the new password was, and silently overwrote the
correct password.

This behaviour has been live on desktop for a couple of versions, and is being
launched on mobile in M54.

[commit-bot: I haz the power, 2016-08-09 13:02:20]

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

[commit-bot: I haz the power, 2016-08-09 13:02:21]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2016-08-09 19:13:54]

metrics still lgtm, thanks

[vabr (Chromium), 2016-08-10 08:00:54]

On 2016/08/09 19:13:54, Ilya Sherman wrote:
> metrics still lgtm, thanks

Thanks, Ilya.

Vasilii is OOO at the moment, and the only push-back I had towards his comments
was in an unrelated code block, so I take the liberty of sending this to the CQ.
Happy to do a follow-up, Vasilii, once you are back.

Cheers,
Vaclav

[vabr (Chromium), 2016-08-10 08:01:01]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-08-10 08:01:02]

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/2220423002/#ps100001
(title: "Comments addressed")

[commit-bot: I haz the power, 2016-08-10 08:01:36]

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, 2016-08-10 08:06:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-10 08:08:09]

Description was changed from

==========
Fix sync-credential-related metrics

This CL makes PasswordManager_SyncCredentialUsed obsolete, beacuse it does not
meaure what it should (see the associated bug).

The CL also introduces a new user action,
PasswordManager_SyncCredentialFilledAndLoginSuccessfull, to actually measure
what the above action was supposed to measure.

The CL also introduces another user action, PasswordManager_ProcessingUpdate,
which keeps measuring what PasswordManager_SyncCredentialUsed measured, because
that data was also useful.

BUG=635428
==========

to

==========
Fix sync-credential-related metrics

This CL makes PasswordManager_SyncCredentialUsed obsolete, beacuse it does not
meaure what it should (see the associated bug).

The CL also introduces a new user action,
PasswordManager_SyncCredentialFilledAndLoginSuccessfull, to actually measure
what the above action was supposed to measure.

The CL also introduces another user action, PasswordManager_ProcessingUpdate,
which keeps measuring what PasswordManager_SyncCredentialUsed measured, because
that data was also useful.

BUG=635428

Committed: https://crrev.com/3a69d6d6755a3f7c645ea682d777e55d7d4340b3
Cr-Commit-Position: refs/heads/master@{#410999}
==========

[commit-bot: I haz the power, 2016-08-10 08:08:10]

Patchset 6 (id:??) landed as
https://crrev.com/3a69d6d6755a3f7c645ea682d777e55d7d4340b3
Cr-Commit-Position: refs/heads/master@{#410999}

[vasilii, 2016-08-11 10:26:10]

LGTM

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
File
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc
(right):

https://codereview.chromium.org/2220423002/diff/40001/components/password_man...
components/password_manager/sync/browser/sync_credentials_filter_unittest.cc:89:
enum { NO_HISTOGRAM, HISTOGRAM_REPORTED } histogram_reported;
On 2016/08/09 12:19:52, vabr (Chromium) wrote:
> On 2016/08/08 15:34:24, vasilii wrote:
> > enums should precede the members.
> 
> Well, these are both enum declarations and member declarations.
> Not sure what the style guide says about that. One thing we can do is split
them
> into named enums above, and using those names below for the actual fields.
> Personally I like the current version bettor for how succinct it is, but I am
> happy to discuss.
> 
> However, because this is not code I introduced or touched in this CL, I will
> keep it as is. Happy to do a follow-up after a discussion, but let's not block
> this CL.

Acknowledged. I didn't notice the variable here.