New smart passsword bubble algorithm.

New smart passsword bubble algorithm.

The behavior is controlled by Finch. If user clicks "Not save" n times in row then the default value for the button becomes "Never for this site".
The previous experiment didn't pop up the bubble occasionally. We collected enough statistics and stopped it few months ago.

BUG=431739
Committed: https://crrev.com/8a132a4fdde202a0295072e7ef42554eac53c6be
Cr-Commit-Position: refs/heads/master@{#322340}

[vasilii, 2015-03-23 19:44:49]

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

[vasilii, 2015-03-23 19:46:39]

Hi guys,

please review

groby@: chrome/browser/ui/cocoa/
vabr@: other files

[groby-ooo-7-16, 2015-03-23 20:19:41]

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm
(right):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:124:
[nopeButton_ insertItemWithTitle:@"" atIndex:0];
Why is this not simply using -setTitle? Or -selectItem...? (You'll still need a
dummy item - popup entries always start at index 1. Don't blame me :)

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller_unittest.mm
(right):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller_unittest.mm:78:
SavePasswordRefusalComboboxModel comboboxModel(false);
Why not have a ComboBoxModel member?

[vabr (Chromium), 2015-03-24 09:09:04]

Code LGTM with some nits.
I still have a design question below (look for NO_DIRECT_INTERACTION), though.

Also, please extend the CL description by saying which experiments this CL got
rid of, and why.

Thanks!
Vaclav

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (left):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:276:
RecordExperimentStatistics(web_contents(), dismissal_reason_);
Was this removed, because metrics_util::CLICKED_CREDENTIAL should not cause the
reset of the nopes_count?

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_bubble_experiment.cc (right):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_bubble_experiment.cc:53: nopes_count = 0;
I'm not sure why clicking "Never" or just abandoning the bubble
(metrics_util::NO_DIRECT_INTERACTION) should reset the nopes_count. What were
the reasons behind that?

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_bubble_experiment.h (right):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_bubble_experiment.h:37: // The consecutive
nopes threshold after which the user sees "Never for this
nit: The comment sounds like the constant below is the (numerical) threshold,
but it is only the name of that parameter. Maybe specify, that this can be used
by GetVariationParamValue to get the actual value, or say something similar?

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc (right):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc:75: // Don't
save password enough times.
This sentence is ambiguous (don't save password, or save it less times than
needed), and describing not only the for-loop, but also the one more saving
after it. What about separating the comments, writing
// Repeatedly refuse to save password, for |kNopeThreshold|-1 times.
above the for-loop, and
// Refuse to save once more to make Never the default button.
just above line 84?

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc:93: // Don't
save password enough times.
This sentence is ambiguous (don't save password, or save it less times than
needed). What about:
// Repeatedly refuse to save password, for |kNopeThreshold| times.

[vasilii, 2015-03-24 10:30:53]

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm
(right):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller.mm:124:
[nopeButton_ insertItemWithTitle:@"" atIndex:0];
On 2015/03/23 20:19:41, groby wrote:
> Why is this not simply using -setTitle? Or -selectItem...? (You'll still need
a
> dummy item - popup entries always start at index 1. Don't blame me :)

Done.

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller_unittest.mm
(right):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_pending_view_controller_unittest.mm:78:
SavePasswordRefusalComboboxModel comboboxModel(false);
On 2015/03/23 20:19:41, groby wrote:
> Why not have a ComboBoxModel member?

Done.

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (left):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:276:
RecordExperimentStatistics(web_contents(), dismissal_reason_);
On 2015/03/24 09:09:04, vabr (Chromium) wrote:
> Was this removed, because metrics_util::CLICKED_CREDENTIAL should not cause
the
> reset of the nopes_count?

The credential chooser bubble has nothing to do with the "Save password" bubble.
Thus, you are right. CLICKED_CREDENTIAL shouldn't break the CLICKED_NOPE row.

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_bubble_experiment.cc (right):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_bubble_experiment.cc:53: nopes_count = 0;
On 2015/03/24 09:09:04, vabr (Chromium) wrote:
> I'm not sure why clicking "Never" or just abandoning the bubble
> (metrics_util::NO_DIRECT_INTERACTION) should reset the nopes_count. What were
> the reasons behind that?

The case of NO_DIRECT_INTERACTION was discussed with Sabine. This experiment is
just temporary thing. Later we should implement this logic on a per domain
basis. So we don't want to opt out our users aggressively. Another point is that
NO_DIRECT_INTERACTION is not a strong signal. For example a page may contain a
redirect which took > 1 second. In this case user maybe wanted to save the
password but we closed it too fast.
"Never" wasn't discussed but it's my interpretation. Users which user "Never"
are not a target of this experiment. They may meaningfully decide when they want
to blacklist the site. Therefore, they don't need our hints.

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_bubble_experiment.h (right):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_bubble_experiment.h:37: // The consecutive
nopes threshold after which the user sees "Never for this
On 2015/03/24 09:09:04, vabr (Chromium) wrote:
> nit: The comment sounds like the constant below is the (numerical) threshold,
> but it is only the name of that parameter. Maybe specify, that this can be
used
> by GetVariationParamValue to get the actual value, or say something similar?

Done.

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc (right):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc:75: // Don't
save password enough times.
On 2015/03/24 09:09:04, vabr (Chromium) wrote:
> This sentence is ambiguous (don't save password, or save it less times than
> needed), and describing not only the for-loop, but also the one more saving
> after it. What about separating the comments, writing
> // Repeatedly refuse to save password, for |kNopeThreshold|-1 times.
> above the for-loop, and
> // Refuse to save once more to make Never the default button.
> just above line 84?

Done.

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc:93: // Don't
save password enough times.
On 2015/03/24 09:09:04, vabr (Chromium) wrote:
> This sentence is ambiguous (don't save password, or save it less times than
> needed). What about:
> // Repeatedly refuse to save password, for |kNopeThreshold| times.

Done.

[vabr (Chromium), 2015-03-24 10:40:59]

Still LGTM, with one comment below.

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_bubble_experiment.cc (right):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_bubble_experiment.cc:53: nopes_count = 0;
On 2015/03/24 10:30:53, vasilii wrote:
> On 2015/03/24 09:09:04, vabr (Chromium) wrote:
> > I'm not sure why clicking "Never" or just abandoning the bubble
> > (metrics_util::NO_DIRECT_INTERACTION) should reset the nopes_count. What
were
> > the reasons behind that?
> 
> The case of NO_DIRECT_INTERACTION was discussed with Sabine. This experiment
is
> just temporary thing. Later we should implement this logic on a per domain
> basis. So we don't want to opt out our users aggressively. Another point is
that
> NO_DIRECT_INTERACTION is not a strong signal. For example a page may contain a
> redirect which took > 1 second. In this case user maybe wanted to save the
> password but we closed it too fast.
> "Never" wasn't discussed but it's my interpretation. Users which user "Never"
> are not a target of this experiment. They may meaningfully decide when they
want
> to blacklist the site. Therefore, they don't need our hints.

Fair enough for NO_DIRECT_INTERACTION. To be clear, I was not suggesting to
change nopes_count for NO_DIRECT_INTERACTION at all (in particular not to
increase it), so the redirect use-case would not be an option. But if you
discussed it, and it will be reconsidered once the experimental phase is over,
that's fine.

For clicking Never, I'm concerned about the users who only came across never
through Never becoming default after clicking Nope x-times. The current code
would switch back to Nope after the first time such Never is clicked, which
might seem like a glitch to the user, who did not discover the non-default parts
of the negative button (did it offer Never just while ago?).

[vasilii, 2015-03-24 11:00:04]

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_bubble_experiment.cc (right):

https://codereview.chromium.org/1022843003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_bubble_experiment.cc:53: nopes_count = 0;
On 2015/03/24 10:40:59, vabr (Chromium) wrote:
> On 2015/03/24 10:30:53, vasilii wrote:
> > On 2015/03/24 09:09:04, vabr (Chromium) wrote:
> > > I'm not sure why clicking "Never" or just abandoning the bubble
> > > (metrics_util::NO_DIRECT_INTERACTION) should reset the nopes_count. What
> were
> > > the reasons behind that?
> > 
> > The case of NO_DIRECT_INTERACTION was discussed with Sabine. This experiment
> is
> > just temporary thing. Later we should implement this logic on a per domain
> > basis. So we don't want to opt out our users aggressively. Another point is
> that
> > NO_DIRECT_INTERACTION is not a strong signal. For example a page may contain
a
> > redirect which took > 1 second. In this case user maybe wanted to save the
> > password but we closed it too fast.
> > "Never" wasn't discussed but it's my interpretation. Users which user
"Never"
> > are not a target of this experiment. They may meaningfully decide when they
> want
> > to blacklist the site. Therefore, they don't need our hints.
> 
> Fair enough for NO_DIRECT_INTERACTION. To be clear, I was not suggesting to
> change nopes_count for NO_DIRECT_INTERACTION at all (in particular not to
> increase it), so the redirect use-case would not be an option. But if you
> discussed it, and it will be reconsidered once the experimental phase is over,
> that's fine.
> 
> For clicking Never, I'm concerned about the users who only came across never
> through Never becoming default after clicking Nope x-times. The current code
> would switch back to Nope after the first time such Never is clicked, which
> might seem like a glitch to the user, who did not discover the non-default
parts
> of the negative button (did it offer Never just while ago?).

I changed the algorithm and the test.

[vabr (Chromium), 2015-03-24 11:37:02]

LGTM!

Because you already noted that this is an experiment, and that the full version
will be site-specific, I have no further comments.

If this should stick as a profile-level setting (across different websites),
then I would urge to consider whether, e.g., the user choosing a non-default
negative button should not make that button default next time (unless it is a
long row of Nopes).

I'll be glad to join the design discussions on this, if you want me to.
Especially because of cases like abandoning the bubble being a positive signal
instead of neutral/negative one. There is no need to block this CL.

Cheers,
Vaclav

[vasilii, 2015-03-25 09:25:50]

Rachel?

[groby-ooo-7-16, 2015-03-26 00:00:19]

LGTM - sorry, you got email filtered :(

[vasilii, 2015-03-26 08:29:31]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2015-03-26 08:29:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1022843003/60001

[commit-bot: I haz the power, 2015-03-26 09:17:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-26 09:17:59]

Patchset 4 (id:??) landed as
https://crrev.com/8a132a4fdde202a0295072e7ef42554eac53c6be
Cr-Commit-Position: refs/heads/master@{#322340}