Support for password manager suggestions on password fields.

Support for password manager suggestions on password fields.

This adds support for the password manager to, optionally, request
suggestions to be displayed on a password field rather than the
traditional choice of a username field. While not used right now, this
is a prerequisite for implementing "fill on account select" as an
experiment since there needs to be a way for users to fill on select if
the username field is uneditable.

This is implemented by adding an option to the autofill IPC message that
the field to receive suggestions is a password field, not a username
field. Thus, this the password manager (a) knows to only show one
suggestion for the specified username, and (b) knows that there should
be a title in th dropdown menu.

BUG=410963
Committed: https://crrev.com/1d7e800d82d104222425aba32e97e19a4edae324
Cr-Commit-Position: refs/heads/master@{#307549}

[jww, 2014-12-01 21:43:03]

jww@chromium.org changed reviewers:
+ gcasto@chromium.org

[jww, 2014-12-01 21:57:46]

I forgot to mention that this CL was originally split from
https://codereview.chromium.org/492043003/ and is a prerequisite for
https://codereview.chromium.org/773573004/.

[Garrett Casto, 2014-12-03 00:35:15]

gcasto@chromium.org changed reviewers:
+ estade@chromium.org

[Garrett Casto, 2014-12-03 00:35:16]

Adding Evan to review the Autofill portion of this.

https://codereview.chromium.org/767353002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/767353002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:432:
identifiers_[selected_line] == POPUP_ITEM_ID_TITLE)
I would think that if you wanted to add special casing here it would be to
return early for any line that !CanAccept(), though I'm not sure if their
omission here is on purpose or it's just not possible to select these elements.
Seems possible via SetSelectionAtPoint(). Probably should verify this with Evan.

https://codereview.chromium.org/767353002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right):

https://codereview.chromium.org/767353002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.h:173: int
GetFirstSelectableLine();
Function doesn't exist anymore.

https://codereview.chromium.org/767353002/diff/20001/components/autofill/cont...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/767353002/diff/20001/components/autofill/cont...
components/autofill/content/common/autofill_messages.h:300: int /* show all
suggestions and the field is a password field options */,
You need to update PasswordAutofillAgent to match this new IPC signature.

[jww, 2014-12-03 04:56:11]

https://codereview.chromium.org/767353002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/767353002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:432:
identifiers_[selected_line] == POPUP_ITEM_ID_TITLE)
On 2014/12/03 00:35:15, Garrett Casto wrote:
> I would think that if you wanted to add special casing here it would be to
> return early for any line that !CanAccept(), though I'm not sure if their
> omission here is on purpose or it's just not possible to select these
elements.
> Seems possible via SetSelectionAtPoint(). Probably should verify this with
Evan.

I'm pretty sure you're correct that this can and should be simplified to
!CanAccept().

The reason that this is necessary now is because otherwise in the call to
delegate_->DidSelectSuggestion(...) on line 445, it will hit an assert failure
if the value is not a username (in this case, it's the title line). Previously,
this assert was never hit simply because there was never a line in the password
autofill popup that could not be accepted, so the !CanAccept() check was never
needed.

All that having been said, I agree that confirmation from Evan would be good :-)

https://codereview.chromium.org/767353002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right):

https://codereview.chromium.org/767353002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.h:173: int
GetFirstSelectableLine();
On 2014/12/03 00:35:16, Garrett Casto wrote:
> Function doesn't exist anymore.

Great catch, thanks.

https://codereview.chromium.org/767353002/diff/20001/components/autofill/cont...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/767353002/diff/20001/components/autofill/cont...
components/autofill/content/common/autofill_messages.h:300: int /* show all
suggestions and the field is a password field options */,
On 2014/12/03 00:35:16, Garrett Casto wrote:
> You need to update PasswordAutofillAgent to match this new IPC signature.

Done.

[jww, 2014-12-03 04:56:46]

jww@chromium.org changed reviewers:
+ tsepez@chromium.org

[jww, 2014-12-03 04:56:47]

tsepez@chromium.org: Please review changes in
components/autofill/content/common/autofill_messages.h and related IPC changes.
Thanks!

[Evan Stade, 2014-12-03 17:54:50]

https://codereview.chromium.org/767353002/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/767353002/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:121:
title_font_list_ = name_font_list_.DeriveWithStyle(gfx::Font::BOLD);
nit: put this below subtext_font_list so there are fewer ifdef/else blocks to
wade through

https://codereview.chromium.org/767353002/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:432:
!CanAccept(identifiers_[selected_line]))
I don't think this is right. You should be calling ClearPreviewedForm in this
case and you still need to invalidate the old selected row.

Instead, on L438, if !CanAccept, set selected_line to kNoSelection. Also, no
bounds check is needed on |selected_line|. In contrast, |selected_line_| needs a
bounds check because it might refer to a stale model.

https://codereview.chromium.org/767353002/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:536: return id !=
POPUP_ITEM_ID_SEPARATOR && id != POPUP_ITEM_ID_WARNING_MESSAGE &&
nit: put each inequality on its own row (or just run git cl format)

https://codereview.chromium.org/767353002/diff/40001/components/autofill/cont...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/767353002/diff/40001/components/autofill/cont...
components/autofill/content/common/autofill_messages.h:300: int /* show all
suggestions and the field is a password field options */,
This comment isn't very clear.

"options bitmask" should be enough.

https://codereview.chromium.org/767353002/diff/40001/components/autofill/core...
File components/autofill/core/common/autofill_constants.h (right):

https://codereview.chromium.org/767353002/diff/40001/components/autofill/core...
components/autofill/core/common/autofill_constants.h:23: enum
ShowPasswordSuggestionsOptions {
docs for each enum value

https://codereview.chromium.org/767353002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_autofill_manager.cc
(right):

https://codereview.chromium.org/767353002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_autofill_manager.cc:115: bool
show_all =
nit: declare variables where they're used rather than at the top of the function

https://codereview.chromium.org/767353002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_autofill_manager.cc:119:
base::string16 title;
s/title/password_field_suggestions_title

https://codereview.chromium.org/767353002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_autofill_manager.cc:148: if
(!title.empty()) {
you can combine this with the is_password_field conditional

https://codereview.chromium.org/767353002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/767353002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_autofill_manager.h:49: void
ShowPasswordSuggestions(int key,
doesn't look like this is defined anywhere (?)

[Tom Sepez, 2014-12-03 18:17:09]

Messages themselves LGTM from a security point-of-view once the implementation
details are worked out.

https://codereview.chromium.org/767353002/diff/40001/components/autofill/cont...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/767353002/diff/40001/components/autofill/cont...
components/autofill/content/common/autofill_messages.h:300: int /* show all
suggestions and the field is a password field options */,
nit: Maybe its an enum (if its not a bitmask).

[jww, 2014-12-04 18:59:57]

https://codereview.chromium.org/767353002/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/767353002/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:121:
title_font_list_ = name_font_list_.DeriveWithStyle(gfx::Font::BOLD);
On 2014/12/03 17:54:49, Evan Stade wrote:
> nit: put this below subtext_font_list so there are fewer ifdef/else blocks to
> wade through

Done.

https://codereview.chromium.org/767353002/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:432:
!CanAccept(identifiers_[selected_line]))
On 2014/12/03 17:54:49, Evan Stade wrote:
> I don't think this is right. You should be calling ClearPreviewedForm in this
> case and you still need to invalidate the old selected row.
> 
> Instead, on L438, if !CanAccept, set selected_line to kNoSelection. Also, no
> bounds check is needed on |selected_line|. In contrast, |selected_line_| needs
a
> bounds check because it might refer to a stale model.

Done, with the slight caveat that we actually have to make sure that the line
isn't already kNoSelection already.

https://codereview.chromium.org/767353002/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:536: return id !=
POPUP_ITEM_ID_SEPARATOR && id != POPUP_ITEM_ID_WARNING_MESSAGE &&
On 2014/12/03 17:54:49, Evan Stade wrote:
> nit: put each inequality on its own row (or just run git cl format)

This is the result of git-cl format, so I'm going to leave it.

https://codereview.chromium.org/767353002/diff/40001/components/autofill/cont...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/767353002/diff/40001/components/autofill/cont...
components/autofill/content/common/autofill_messages.h:300: int /* show all
suggestions and the field is a password field options */,
On 2014/12/03 18:17:09, Tom Sepez wrote:
> nit: Maybe its an enum (if its not a bitmask).

Changed to "options bitmask of autofill::ShowPasswordSuggestionsOptions" so the
option choices  are explicit. Please let me know if you think this is too
pedantic though. (tsepez: It is a bitmask, but I use an enum to define the
options. See the change in autofill_constansts.)

https://codereview.chromium.org/767353002/diff/40001/components/autofill/core...
File components/autofill/core/common/autofill_constants.h (right):

https://codereview.chromium.org/767353002/diff/40001/components/autofill/core...
components/autofill/core/common/autofill_constants.h:23: enum
ShowPasswordSuggestionsOptions {
On 2014/12/03 17:54:50, Evan Stade wrote:
> docs for each enum value

Done.

https://codereview.chromium.org/767353002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_autofill_manager.cc
(right):

https://codereview.chromium.org/767353002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_autofill_manager.cc:115: bool
show_all =
On 2014/12/03 17:54:50, Evan Stade wrote:
> nit: declare variables where they're used rather than at the top of the
function

Done.

https://codereview.chromium.org/767353002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_autofill_manager.cc:119:
base::string16 title;
On 2014/12/03 17:54:50, Evan Stade wrote:
> s/title/password_field_suggestions_title

Done.

https://codereview.chromium.org/767353002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/767353002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_autofill_manager.h:49: void
ShowPasswordSuggestions(int key,
On 2014/12/03 17:54:50, Evan Stade wrote:
> doesn't look like this is defined anywhere (?)

Oops; leftover from old version. Good catch!

[Garrett Casto, 2014-12-05 19:30:54]

LGTM with nits.

https://codereview.chromium.org/767353002/diff/80001/components/autofill/cont...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/767353002/diff/80001/components/autofill/cont...
components/autofill/content/common/autofill_messages.h:13: #include
"components/autofill/core/common/autofill_constants.h"
Shouldn't need to include this here.

https://codereview.chromium.org/767353002/diff/80001/components/password_mana...
File components/password_manager/core/browser/password_autofill_manager.cc
(right):

https://codereview.chromium.org/767353002/diff/80001/components/password_mana...
components/password_manager/core/browser/password_autofill_manager.cc:136: if
(options & autofill::ShowPasswordSuggestionsOptions::IS_PASSWORD_FIELD) {
You should merge these two new if statements.

https://codereview.chromium.org/767353002/diff/80001/components/password_mana...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/767353002/diff/80001/components/password_mana...
components/password_manager/core/browser/password_autofill_manager.h:14:
#include "components/autofill/core/common/autofill_constants.h"
Should include this in the cc file instead.

[jww, 2014-12-05 20:23:10]

estade, can you take another look at the latest version? Thanks!

https://codereview.chromium.org/767353002/diff/80001/components/autofill/cont...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/767353002/diff/80001/components/autofill/cont...
components/autofill/content/common/autofill_messages.h:13: #include
"components/autofill/core/common/autofill_constants.h"
On 2014/12/05 19:30:54, Garrett Casto wrote:
> Shouldn't need to include this here.

Done.

https://codereview.chromium.org/767353002/diff/80001/components/password_mana...
File components/password_manager/core/browser/password_autofill_manager.cc
(right):

https://codereview.chromium.org/767353002/diff/80001/components/password_mana...
components/password_manager/core/browser/password_autofill_manager.cc:136: if
(options & autofill::ShowPasswordSuggestionsOptions::IS_PASSWORD_FIELD) {
On 2014/12/05 19:30:54, Garrett Casto wrote:
> You should merge these two new if statements.

Done.

https://codereview.chromium.org/767353002/diff/80001/components/password_mana...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/767353002/diff/80001/components/password_mana...
components/password_manager/core/browser/password_autofill_manager.h:14:
#include "components/autofill/core/common/autofill_constants.h"
On 2014/12/05 19:30:54, Garrett Casto wrote:
> Should include this in the cc file instead.

Done.

[Evan Stade, 2014-12-06 01:59:48]

sorry, lgtm

[jww, 2014-12-06 17:20:52]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-12-06 17:21:51]

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

[commit-bot: I haz the power, 2014-12-06 17:25:51]

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

[commit-bot: I haz the power, 2014-12-06 17:25:52]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[jww, 2014-12-06 17:51:29]

jww@chromium.org changed reviewers:
+ jochen@chromium.org

[jww, 2014-12-06 17:51:30]

Jochen, can you take a look at components/password_manager/DEPS? I added a DEPS
to components/strings/grit, and according to the presubmit, I guess I need
approval from an OWNER of components/strings/grit as well. Thanks!

[jochen (gone - plz use gerrit), 2014-12-08 12:13:18]

do you need to also update the gyp / gn configuration to have this dependency?

[jww, 2014-12-08 18:10:29]

jochen: I think it is unnecessary (it compiled fine), but I added it to the GN
deps since it can't hurt.

[jochen (gone - plz use gerrit), 2014-12-09 15:47:17]

lgtm

[jww, 2014-12-09 16:37:06]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-12-09 16:38:10]

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

[commit-bot: I haz the power, 2014-12-09 16:47:47]

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

[commit-bot: I haz the power, 2014-12-09 16:47:48]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[jww, 2014-12-09 17:13:39]

jww@chromium.org changed reviewers:
+ cjhopman@chromium.org

[jww, 2014-12-09 17:13:40]

cjhopman@chromium.org: Please review changes in  build/ios/grit_whitelist.txt.
Thanks!

[cjhopman, 2014-12-09 19:23:25]

lgtm

[jww, 2014-12-09 19:45:19]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-12-09 19:46:43]

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

[commit-bot: I haz the power, 2014-12-09 21:02:44]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2014-12-09 21:03:33]

Patchset 13 (id:??) landed as
https://crrev.com/1d7e800d82d104222425aba32e97e19a4edae324
Cr-Commit-Position: refs/heads/master@{#307549}