UMA histogram of user action on multi-account password update bubble.

UMA histogram of user action on multi-account password update bubble.

BUG=359315
Committed: https://crrev.com/8071b1d4e8b970acfb167042a54a7782e5f0428e
Cr-Commit-Position: refs/heads/master@{#369395}

[dvadym, 2016-01-13 13:27:14]

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

[dvadym, 2016-01-13 13:27:15]

Hi Vasilii,

 Could you please review this CL?

Regards,
Vadym

[vasilii, 2016-01-13 13:54:26]

I'm not sure how you are using the new histogram. In 99% cases
best_match_username and the preferred account are the same. However, you report
only one value.

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:35: bool&
is_default_preferred) {
Move the function to the class.

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:74: :
password_forms_(password_forms), action_reported(false) {
line break.

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:126: action =
password_manager::metrics_util::
if the block is 2-lines then you need {}

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:127:
DEFAULT_ACCOUNT_MATCHED_BY_PASSWORD_USER_REJECTED_UPDATE;
the account is matched by username not by password

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:134: } else if
(did_user_change_selection) {
I'd inline did_user_change_selection here.

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/credentials_selection_view.h (right):

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.h:31: void
ReportUserAction(bool was_update_rejected, int selected_index);
Better name ReportUserActionOnce

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.h:38: bool
action_reported;
Add '_' to the end.

[dvadym, 2016-01-13 14:55:38]

Thanks Vasilii for comments! I've addressed them PTAL.

I wouldn't say that preferred and best matched are in 99% the same, since for
any change password forms without password, it's impossible to match with
password, but I agree that it's very often. But it's not important. I'm only
interested how good we are in predicting correct credentials in different cases
in order to understand:
 1.Can we remove dropdown in case when we match by password?
 2.Whether predicted by preferred good enough?
 3.Is it too often when we simply show first, should we do something more about
it?

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:35: bool&
is_default_preferred) {
On 2016/01/13 13:54:25, vasilii wrote:
> Move the function to the class.

Thanks, it makes sense

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:74: :
password_forms_(password_forms), action_reported(false) {
On 2016/01/13 13:54:25, vasilii wrote:
> line break.

Done.

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:126: action =
password_manager::metrics_util::
On 2016/01/13 13:54:25, vasilii wrote:
> if the block is 2-lines then you need {}

Done.

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:127:
DEFAULT_ACCOUNT_MATCHED_BY_PASSWORD_USER_REJECTED_UPDATE;
On 2016/01/13 13:54:25, vasilii wrote:
> the account is matched by username not by password

Maybe name |best_matched_username| is a little bit misleading, it means username
of best matched credentials, but in case of multi-accounts these matching is
done by password. If username is matched, then shown update bubble without
combobox.

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:134: } else if
(did_user_change_selection) {
On 2016/01/13 13:54:25, vasilii wrote:
> I'd inline did_user_change_selection here.

Done.

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/credentials_selection_view.h (right):

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.h:31: void
ReportUserAction(bool was_update_rejected, int selected_index);
On 2016/01/13 13:54:25, vasilii wrote:
> Better name ReportUserActionOnce

Done.

https://codereview.chromium.org/1586613003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.h:38: bool
action_reported;
On 2016/01/13 13:54:25, vasilii wrote:
> Add '_' to the end.

Done.

[vasilii, 2016-01-13 15:23:46]

You can't answer the question #2 because preferred is pre-empted by the password
match.

https://codereview.chromium.org/1586613003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1586613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:32:
views::Combobox* CredentialsSelectionView::GenerateUsernameCombobox(
The definition order should match the declaration order. Move the method down.

https://codereview.chromium.org/1586613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:116: int
selected_index) {
indent

https://codereview.chromium.org/1586613003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/credentials_selection_view.h (right):

https://codereview.chromium.org/1586613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.h:28: const
autofill::PasswordForm* GetSelectedCredentials();
Comment that the method reports statistics like if the user clicked Update.

[dvadym, 2016-01-13 15:39:57]

Thanks Vasilii! PTAL.

In offline discussion it was clarified what metrics are calculated in this CL.

https://codereview.chromium.org/1586613003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1586613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:32:
views::Combobox* CredentialsSelectionView::GenerateUsernameCombobox(
On 2016/01/13 15:23:46, vasilii wrote:
> The definition order should match the declaration order. Move the method down.

Done.

https://codereview.chromium.org/1586613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:116: int
selected_index) {
On 2016/01/13 15:23:46, vasilii wrote:
> indent

Done.

https://codereview.chromium.org/1586613003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/credentials_selection_view.h (right):

https://codereview.chromium.org/1586613003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_selection_view.h:28: const
autofill::PasswordForm* GetSelectedCredentials();
On 2016/01/13 15:23:46, vasilii wrote:
> Comment that the method reports statistics like if the user clicked Update.

Done.

[vasilii, 2016-01-13 15:43:23]

lgtm

[dvadym, 2016-01-13 16:09:12]

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

[dvadym, 2016-01-13 16:09:13]

Hi Ilya,

 Could you please review changes in histograms.xml?

Regards,
Vadym

[Ilya Sherman, 2016-01-14 01:07:24]

metrics lgtm

https://codereview.chromium.org/1586613003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1586613003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32540: +    A user action when the
password update bubble with multiple accounts is
nit: s/the/a

https://codereview.chromium.org/1586613003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:70951: +  <int value="8"
label="DEFAULT_ACCOUNT_FIRST_USER_REJECTED_UPDATE"/>
Optional nit: I'd recommend more human-friendly labels, though these are ok if
you think it's important to maintain consistency with the C++ enum names.

[dvadym, 2016-01-14 10:44:32]

Thanks Ilya!

https://codereview.chromium.org/1586613003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1586613003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32540: +    A user action when the
password update bubble with multiple accounts is
On 2016/01/14 01:07:24, Ilya Sherman wrote:
> nit: s/the/a

Done.

https://codereview.chromium.org/1586613003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:70951: +  <int value="8"
label="DEFAULT_ACCOUNT_FIRST_USER_REJECTED_UPDATE"/>
On 2016/01/14 01:07:24, Ilya Sherman wrote:
> Optional nit: I'd recommend more human-friendly labels, though these are ok if
> you think it's important to maintain consistency with the C++ enum names.

Thanks, I'd rather leave C++ enum names in order to easy match with code.

[dvadym, 2016-01-14 10:44:46]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2016-01-14 10:44:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
vasilii@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1586613003/#ps80001
(title: "Reviewers comments addressed")

[commit-bot: I haz the power, 2016-01-14 10:44:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586613003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586613003/80001

[commit-bot: I haz the power, 2016-01-14 12:10:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-14 12:11:06]

Description was changed from

==========
UMA histogram of user action on multi-account password update bubble.

BUG=359315
==========

to

==========
UMA histogram of user action on multi-account password update bubble.

BUG=359315

Committed: https://crrev.com/8071b1d4e8b970acfb167042a54a7782e5f0428e
Cr-Commit-Position: refs/heads/master@{#369395}
==========

[commit-bot: I haz the power, 2016-01-14 12:11:07]

Patchset 5 (id:??) landed as
https://crrev.com/8071b1d4e8b970acfb167042a54a7782e5f0428e
Cr-Commit-Position: refs/heads/master@{#369395}