[Password Generation] Wait longer to dismiss suggestion UI

[Password Generation] Wait longer to dismiss suggestion UI

UX feedback is that users generally look at the keyboard when starting to
type passwords and thus miss the prompt. The UI will now show until the user
types 5 characters instead of being dismissed after they type anything.

BUG=318977
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288935

[Ilya Sherman, 2014-08-06 22:15:14]

This seems like a pretty odd compromise.  Why not just keep the UI up while the
user is typing?  (Or maybe I'm misunderstanding the thrust of this CL?)

[Ilya Sherman, 2014-08-06 22:16:51]

On 2014/08/06 22:15:14, Ilya Sherman wrote:
> This seems like a pretty odd compromise.  Why not just keep the UI up while
the
> user is typing?  (Or maybe I'm misunderstanding the thrust of this CL?)

Never mind, I think I was just thrown off by a typo in the CL description: You
have "not" instead of "now".  It seems simpler to me to just keep the UI up
indefinitely, which I'd expect to feel more predictable to most users, but I
guess dismissing after 5 typed characters is ok if that's what the UX team
recommended.

[Ilya Sherman, 2014-08-06 22:25:09]

https://codereview.chromium.org/447873004/diff/20001/chrome/renderer/autofill...
File chrome/renderer/autofill/password_generation_agent_browsertest.cc (right):

https://codereview.chromium.org/447873004/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/password_generation_agent_browsertest.cc:270: // Make a
password small just under maximum offer size.
nit: This wording feels a little odd.  Mind rephrasing?

https://codereview.chromium.org/447873004/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/password_generation_agent_browsertest.cc:289:
std::string(password_generation_->kMaximumOfferSize + 1, 'a')));
You have tests for |kMaximumOfferSize - 1| and |kMaximumOfferSize + 1|, but none
for equal to |kMaximumOfferSize| exactly.

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/password_generation_agent.cc (right):

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:291: // Assume
that if the password field has less than kMaximumOfferSize
Optional nit: Hmm, "kMaximumOfferSize" makes me think that the UI should still
be shown when the max is reached, i.e. the code below looks like an off-by-one
error.  WDYT of either renaming the variable, or changing the constant from 5 to
4, and the comparison from "<" to "<="?

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:293: // the
password suggestion.
This comment suggests that we think the user probably is finished typing their
password once they've typed 5 characters.  Do we really think that user
passwords are so short?

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:296:
element->value().length() < kMaximumOfferSize) {
Optional nit: I think .size() is more commonly used than .length() in Chromium
code (though, obviously, they're equivalent).

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:331: } else if
(element.value().length() > kMaximumOfferSize) {
If the comparison above is "<", then the inverse here should be ">=".

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:335: //
Password isn't generated and there is less than kMaximumOfferSize
nit: "is less" -> "are fewer"

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:337:
ShowGenerationPopup();
This looks like it'll show a new popup, rather than keeping the current one up. 
Is that intentional?

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/password_generation_agent.h (right):

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.h:43: static
const int kMaximumOfferSize = 5;
nit: Can this by a size_t?

[Garrett Casto, 2014-08-06 22:26:00]

On 2014/08/06 22:16:51, Ilya Sherman wrote:
> On 2014/08/06 22:15:14, Ilya Sherman wrote:
> > This seems like a pretty odd compromise.  Why not just keep the UI up while
> the
> > user is typing?  (Or maybe I'm misunderstanding the thrust of this CL?)
> 
> Never mind, I think I was just thrown off by a typo in the CL description: You
> have "not" instead of "now".  It seems simpler to me to just keep the UI up
> indefinitely, which I'd expect to feel more predictable to most users, but I
> guess dismissing after 5 typed characters is ok if that's what the UX team
> recommended.

Whoops, fixed the typo. Yeah, I'm not sure what I think about the 5 char limit
vs. just keeping it up forever. I think we are trying not to be overly pushy
with the UI, which is the reason. I generally agree with your sentiments though.
We can revisit this after additional user feedback.

[Garrett Casto, 2014-08-08 18:19:20]

https://codereview.chromium.org/447873004/diff/20001/chrome/renderer/autofill...
File chrome/renderer/autofill/password_generation_agent_browsertest.cc (right):

https://codereview.chromium.org/447873004/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/password_generation_agent_browsertest.cc:270: // Make a
password small just under maximum offer size.
On 2014/08/06 22:25:08, Ilya Sherman wrote:
> nit: This wording feels a little odd.  Mind rephrasing?

Typo, cleaned up.

https://codereview.chromium.org/447873004/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/password_generation_agent_browsertest.cc:289:
std::string(password_generation_->kMaximumOfferSize + 1, 'a')));
On 2014/08/06 22:25:08, Ilya Sherman wrote:
> You have tests for |kMaximumOfferSize - 1| and |kMaximumOfferSize + 1|, but
none
> for equal to |kMaximumOfferSize| exactly.

Done.

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/password_generation_agent.cc (right):

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:291: // Assume
that if the password field has less than kMaximumOfferSize
On 2014/08/06 22:25:08, Ilya Sherman wrote:
> Optional nit: Hmm, "kMaximumOfferSize" makes me think that the UI should still
> be shown when the max is reached, i.e. the code below looks like an off-by-one
> error.  WDYT of either renaming the variable, or changing the constant from 5
to
> 4, and the comparison from "<" to "<="?

Ah, there actually is an off by one error here. This should be <=. Updated and
added a test.

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:293: // the
password suggestion.
On 2014/08/06 22:25:08, Ilya Sherman wrote:
> This comment suggests that we think the user probably is finished typing their
> password once they've typed 5 characters.  Do we really think that user
> passwords are so short?

I think the idea is that they aren't finish, so it's still possible to influence
them to use a generated password. I'll try to clarify this.

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:296:
element->value().length() < kMaximumOfferSize) {
On 2014/08/06 22:25:08, Ilya Sherman wrote:
> Optional nit: I think .size() is more commonly used than .length() in Chromium
> code (though, obviously, they're equivalent).

blink::WebString doesn't have size()

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:331: } else if
(element.value().length() > kMaximumOfferSize) {
On 2014/08/06 22:25:08, Ilya Sherman wrote:
> If the comparison above is "<", then the inverse here should be ">=".

This one is right, the other is wrong.

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:335: //
Password isn't generated and there is less than kMaximumOfferSize
On 2014/08/06 22:25:08, Ilya Sherman wrote:
> nit: "is less" -> "are fewer"

Done.

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:337:
ShowGenerationPopup();
On 2014/08/06 22:25:08, Ilya Sherman wrote:
> This looks like it'll show a new popup, rather than keeping the current one
up. 
> Is that intentional?

It actually just updates the bounds of the current popup if one exists.  Updated
the comment to reflect this.

[Garrett Casto, 2014-08-08 18:24:29]

Missed one.

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/password_generation_agent.h (right):

https://codereview.chromium.org/447873004/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.h:43: static
const int kMaximumOfferSize = 5;
On 2014/08/06 22:25:08, Ilya Sherman wrote:
> nit: Can this by a size_t?

Done.

[Ilya Sherman, 2014-08-12 01:49:11]

LGTM, thanks.

[Garrett Casto, 2014-08-12 03:43:52]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-08-12 03:45:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/447873004/40001

[commit-bot: I haz the power, 2014-08-12 11:47:07]

Change committed as 288935