Finch experiment for limiting the password bubble annoyance.

Finch experiment for limiting the password bubble annoyance.

BUG=431739
TBR=battre@chromium.org

Committed: https://crrev.com/4d7cdf425d0b57f4c2a766433f5ffdc3cedd909f
Cr-Commit-Position: refs/heads/master@{#303630}

[vasilii, 2014-11-10 14:58:36]

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

[vasilii, 2014-11-10 14:58:57]

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

[vasilii, 2014-11-10 14:59:15]

Hi guys,

please review.

[vabr (Chromium), 2014-11-10 15:30:31]

LGTM with comments.

Cheers,
Vaclav

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/password_bubble_experiment.cc (right):

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.cc:73: // If user clicks
dismisses the bubble > n times during d days then the bubble
grammar nit: "clicks dismisses" -- sounds broken, please check

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.cc:73: // If user clicks
dismisses the bubble > n times during d days then the bubble
nit: The comment does not make it clear where to find "n" and "d".

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.cc:90: void
UpdateTimeSpanPrefs(
nit: Please add a comment on what does this do, and when it is used. For
example, it is not clear, why UpdateTimeSpanPrefs should also update the "nope
counts".

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.cc:133: // We keep the
history of last n interactions with the bubble. We implicitly
nit: It would be easier if you used the same variables in the comment and in the
code. For example, is "m" the same as "fake_saves"?

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.cc:165: auto
history_beginning = interactions.size() > history_length ?
Please specify the type here. According to the style guide, you should "Use auto
to avoid type names that are just clutter". Writing size_t or int is not adding
more clutter, while with auto you are hardly sure if the variable is unsigned,
for example.

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/password_bubble_experiment.h (right):

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.h:17: // The class
handles the algorithms according to which the "Save password?"
Why is this a class? It is just a bundle of 3 methods with no associated data.

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.h:22: static bool
ShouldShowBubble(PrefService* prefs);
nit: Please add comment stating how the decision is made, and who should ask for
it and when. Mention the default value.

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.h:23: static void
RecordBubbleClosed(
nit: Please add a comment saying when this should be called and what does it do.

[vasilii, 2014-11-10 17:06:39]

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/password_bubble_experiment.cc (right):

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.cc:73: // If user clicks
dismisses the bubble > n times during d days then the bubble
On 2014/11/10 15:30:31, vabr (Chromium) wrote:
> grammar nit: "clicks dismisses" -- sounds broken, please check

Done.

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.cc:73: // If user clicks
dismisses the bubble > n times during d days then the bubble
On 2014/11/10 15:30:31, vabr (Chromium) wrote:
> nit: The comment does not make it clear where to find "n" and "d".

Done.

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.cc:90: void
UpdateTimeSpanPrefs(
On 2014/11/10 15:30:31, vabr (Chromium) wrote:
> nit: Please add a comment on what does this do, and when it is used. For
> example, it is not clear, why UpdateTimeSpanPrefs should also update the "nope
> counts".

Done.

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.cc:133: // We keep the
history of last n interactions with the bubble. We implicitly
On 2014/11/10 15:30:31, vabr (Chromium) wrote:
> nit: It would be easier if you used the same variables in the comment and in
the
> code. For example, is "m" the same as "fake_saves"?

Done.

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.cc:165: auto
history_beginning = interactions.size() > history_length ?
On 2014/11/10 15:30:31, vabr (Chromium) wrote:
> Please specify the type here. According to the style guide, you should "Use
auto
> to avoid type names that are just clutter". Writing size_t or int is not
adding
> more clutter, while with auto you are hardly sure if the variable is unsigned,
> for example.
> 
> https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto

Done.

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/password_bubble_experiment.h (right):

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.h:17: // The class
handles the algorithms according to which the "Save password?"
On 2014/11/10 15:30:31, vabr (Chromium) wrote:
> Why is this a class? It is just a bundle of 3 methods with no associated data.

Done.

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.h:22: static bool
ShouldShowBubble(PrefService* prefs);
On 2014/11/10 15:30:31, vabr (Chromium) wrote:
> nit: Please add comment stating how the decision is made, and who should ask
for
> it and when. Mention the default value.

Done.

https://codereview.chromium.org/711043002/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/password_bubble_experiment.h:23: static void
RecordBubbleClosed(
On 2014/11/10 15:30:31, vabr (Chromium) wrote:
> nit: Please add a comment saying when this should be called and what does it
do.

Done.

[vabr (Chromium), 2014-11-11 09:07:55]

Still LGTM, with one comment.
Thanks for adding the test.
Cheers,
Vaclav

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

https://codereview.chromium.org/711043002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc:91:
EXPECT_FALSE(password_bubble_experiment::ShouldShowBubble(prefs()));
nit: This EXPECT_FALSE is surprising, especially compared to the other test.
Please consider extending the comment on line 85 with explanation of the
expectation (or adding the explanation elsewhere, as you see fit).

[vasilii, 2014-11-11 09:47:48]

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

https://codereview.chromium.org/711043002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc:91:
EXPECT_FALSE(password_bubble_experiment::ShouldShowBubble(prefs()));
On 2014/11/11 09:07:55, vabr (Chromium) wrote:
> nit: This EXPECT_FALSE is surprising, especially compared to the other test.
> Please consider extending the comment on line 85 with explanation of the
> expectation (or adding the explanation elsewhere, as you see fit).

Done.

[vabr (Chromium), 2014-11-11 10:10:58]

LGTM with 2 very easy comments.
Thanks!
Vaclav

https://codereview.chromium.org/711043002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc (right):

https://codereview.chromium.org/711043002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc:89: // apn
isn't over.
typo? apn->span

https://codereview.chromium.org/711043002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc:110: // Set
the old time span again and record "Nope". The counter restart from 0.
grammar nit: "counter restarts"

[vasilii, 2014-11-11 10:39:40]

https://codereview.chromium.org/711043002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc (right):

https://codereview.chromium.org/711043002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc:89: // apn
isn't over.
On 2014/11/11 10:10:58, vabr (Chromium) wrote:
> typo? apn->span

Done.

https://codereview.chromium.org/711043002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/password_bubble_experiment_unittest.cc:110: // Set
the old time span again and record "Nope". The counter restart from 0.
On 2014/11/11 10:10:58, vabr (Chromium) wrote:
> grammar nit: "counter restarts"

Done.

[vabr (Chromium), 2014-11-11 10:43:53]

lgtm

LGTM

[vasilii, 2014-11-11 10:59:10]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-11-11 10:59:37]

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

[commit-bot: I haz the power, 2014-11-11 12:30:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-11 12:30:34]

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