Allow deleting autofill password suggestions on Shift+Delete

Allow deleting autofill password suggestions on Shift+Delete.

As described in the bug, when someone clicks on shift+del on a saved login, the saved login is removed only from the display and is going to reappear when the page is refreshed.  

This CL allows to make the remove of a saved login persistent by also deleting the corresponding entry in the PasswordStore.

Note: this is a continuation of https://codereview.chromium.org/133893004/.

BUG=329038

[vabr (Chromium), 2014-04-04 10:51:40]

Hi Riadh,

After you address my comments below, I believe it's ready for Ilya's review.
Make sure to run "git cl format" and check the lint messages before sending it
out to him.

Thanks!
Vaclav

https://codereview.chromium.org/223133003/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/223133003/diff/1/AUTHORS#newcode271
AUTHORS:271: Riadh Chtara <rchtara@chromium.org>
You are already covered with
"The Chromium Authors <*@chromium.org>" above.
Please revert this change.

https://codereview.chromium.org/223133003/diff/1/components/autofill/content/...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/223133003/diff/1/components/autofill/content/...
components/autofill/content/common/autofill_messages.h:8: 
nit: #include <vector> seems appropriate here.
This header file already uses std::vector on many places, so it get's included
through some of the other headers, but it won't hurt include it here explicitly.

https://codereview.chromium.org/223133003/diff/1/components/autofill/content/...
components/autofill/content/common/autofill_messages.h:266: // Instruct the
browser that a password suggestion has to be removed.
Actually, it's not a password suggestion, we like to remove. It's the saved
password, right?
If so, we should say "stored password" instead of "password suggestion" here,
and also change the name of the message.

https://codereview.chromium.org/223133003/diff/1/components/autofill/content/...
components/autofill/content/common/autofill_messages.h:267: // Send also a list
of forms that need to be updated after the deletion
Missing full-stop (period).

https://codereview.chromium.org/223133003/diff/1/components/autofill/content/...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/223133003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:647:
DCHECK(password_form);
Please check with a debug build of Chrome, that using Shift+Delete on a
*non-password* form does not crash the renderer.
I looks like PasswordAutofillAgent should not get involved on such forms, but
better safe than sorry. :)

https://codereview.chromium.org/223133003/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/password_autofill_manager.h (right):

https://codereview.chromium.org/223133003/diff/1/components/autofill/core/bro...
components/autofill/core/browser/password_autofill_manager.h:39: // Clear the
login associated with the field |field|
nit: this breaks too early (use the whole line)

https://codereview.chromium.org/223133003/diff/1/components/autofill/core/bro...
components/autofill/core/browser/password_autofill_manager.h:39: // Clear the
login associated with the field |field|
"Clear the login" sounds ambiguous to me. What is it doing exactly? It looks
like staying close to the function name is less confusing: "Removes the password
suggestion associated..."

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.cc:142: bool
remove_candidate_from_store) {
I suggest not taking care of removing the candidate from store here at all.
Let's PasswordManager do it, the code will be much simpler and easier to
understand.

Also, instead of removing the username manually, let's just fetch the  current
password candidates from the password store.

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.cc:443: if
(username_to_remove_.size() == 0) {
Let's revert changes to this method completely. If we make sure the password for
username_to_remove_ is already removed from the password store, we should not be
able to find it here any more anyway.

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.h:99: void
Update(
This method definitely needs a comment explaining what it does, and what are the
arguments.

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.h:281: bool
remove_candidate_from_store_;
Given my suggestions in the *.cc file, these variables can be removed. (If we
end up not removing them, then we should at least comment on the purpose of
them.)

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager.cc:270: void
PasswordManager::OnRemovePasswordSuggestion(
I suggest the following simplification of this method:

1) Erase the stored password, identified by |password_form| from the password
store before updating any PasswordFormManagers.
2) Then go through all the PasswordFormManagers, and call Update on them,
regardless of what they manage.

Step 2) should not make a lot of difference, as far as I understand the code,
the PasswordFormManagers should actually correspond 1:1 to
|updated_password_forms|.

As a bonus, you can get rid of the vector of PasswordForms in the argument and
the IPC message.

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager.cc:270: void
PasswordManager::OnRemovePasswordSuggestion(
Also, I think we are missing a test for this method yet. Could you add
something?

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager.cc:273: 
nit: unnecessary blank line

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager.cc:274: // Deleting
the PasswordFormManager for the current |password_form| to avoid
I did not understand what this comment warned about, and I suggest dropping it
completely. Feel free to push back on this, if you find it useful.

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager.cc:280: // Avoid
prompting the user for access to a password if they don't have
nit: instead of duplicating this piece of code from
PasswordManager::OnPasswordFormsParsed, consider putting it in a helper method
and use on those two places.

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager.h:99: void
OnRemovePasswordSuggestion(
As pointed out higher in the CL for the name of the message processed by this
function -- it's not actually removing a password suggestion, is it?
I understand it removes the saved password, and updates the PasswordFormManagers
to reflect that. We should name it accordingly (and be consistent with the name
of the message).

[rchtara, 2014-04-04 16:07:18]

https://codereview.chromium.org/223133003/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/223133003/diff/1/AUTHORS#newcode271
AUTHORS:271: Riadh Chtara <rchtara@chromium.org>
On 2014/04/04 10:51:40, vabr (Chromium) wrote:
> You are already covered with
> "The Chromium Authors <mailto:*@chromium.org>" above.
> Please revert this change.

Done.

https://codereview.chromium.org/223133003/diff/1/components/autofill/content/...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/223133003/diff/1/components/autofill/content/...
components/autofill/content/common/autofill_messages.h:266: // Instruct the
browser that a password suggestion has to be removed.
On 2014/04/04 10:51:40, vabr (Chromium) wrote:
> Actually, it's not a password suggestion, we like to remove. It's the saved
> password, right?
> If so, we should say "stored password" instead of "password suggestion" here,
> and also change the name of the message.

Done.

https://codereview.chromium.org/223133003/diff/1/components/autofill/content/...
components/autofill/content/common/autofill_messages.h:267: // Send also a list
of forms that need to be updated after the deletion
On 2014/04/04 10:51:40, vabr (Chromium) wrote:
> Missing full-stop (period).

Done.

https://codereview.chromium.org/223133003/diff/1/components/autofill/content/...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/223133003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:647:
DCHECK(password_form);
I just checked it and it's working
:)

https://codereview.chromium.org/223133003/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/password_autofill_manager.h (right):

https://codereview.chromium.org/223133003/diff/1/components/autofill/core/bro...
components/autofill/core/browser/password_autofill_manager.h:39: // Clear the
login associated with the field |field|
On 2014/04/04 10:51:40, vabr (Chromium) wrote:
> nit: this breaks too early (use the whole line)

Done.

https://codereview.chromium.org/223133003/diff/1/components/autofill/core/bro...
components/autofill/core/browser/password_autofill_manager.h:39: // Clear the
login associated with the field |field|
On 2014/04/04 10:51:40, vabr (Chromium) wrote:
> nit: this breaks too early (use the whole line)

Done.

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/223133003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager.h:99: void
OnRemovePasswordSuggestion(
On 2014/04/04 10:51:40, vabr (Chromium) wrote:
> As pointed out higher in the CL for the name of the message processed by this
> function -- it's not actually removing a password suggestion, is it?
> I understand it removes the saved password, and updates the
PasswordFormManagers
> to reflect that. We should name it accordingly (and be consistent with the
name
> of the message).

Done.

[rchtara, 2014-04-04 16:57:31]

Hi Ilya,
Could you please review this cl and tell me your opinion about it?
By he way there is a bug when manipulating many forms.
(*go to https://c9.io/riadh/w1/workspace/index.html
 *fill the first form, then submit.
 *go back to the first page
 *in the second form, select the user name you have a just typed and press
shift+del to delete
 *retype a user name and and a password in the second form
 *submit
=> you expect to see the save password bubble, but it doesn't appear)

It was fixed in the previous version of the patch, but it came back now, but I
think it"s going be easy to fix it.
I'm going also to work a test for the
PasswordManager::OnRemoveSavedPasswordAndUpdateManagers
 
Thanks a lot
Cheers
Riadh

[vabr (Chromium), 2014-04-07 09:35:39]

Hi Riadh,
A nit I randomly spotted below.

Vaclav

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

https://codereview.chromium.org/223133003/diff/80001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:296: // Avoid
prompting the user for access to a password if they don't have
This comment should be removed (it was replaced by the comment at the
declaration of PasswordManager::GetPromptPolicy).

[rchtara, 2014-04-07 15:02:04]

Hi Ilya,
I wanted just to tell you that the bug I told you about on Friday is fixed.
Thanks a lot
Cheers
Riadh

[Ilya Sherman, 2014-04-09 06:33:05]

Apologies for my delay in getting to this review!

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:8: #include <vector>
Why did you add this?

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:167: base::string16
/*user name to be removed */)
How/why does the renderer deal with UI?  Patrick Dubroy is working on removing
knowledge of usernames and passwords from the renderer until they are filled.  I
think it's probably best to not send any IPC at all for removing passwords from
the data store.

https://codereview.chromium.org/223133003/diff/120001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/223133003/diff/120001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:209: } else {
nit: Please DCHECK that the identifier is for an autocomplete entry.

[vabr (Chromium), 2014-04-09 11:17:12]

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:8: #include <vector>
On 2014/04/09 06:33:05, Ilya Sherman wrote:
> Why did you add this?

I asked Riadh to include that at a point when one of the messages had a
std::vector as an argument. That addition was dropped, but there are such
messages in the ToT version already. The style guide seems to require #including
all we use and cannot forward-declare, in particular templates
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...).
It's also what is done for std::string.

I'm not insisting on that change, though.

If we keep it, we should also
#include "base/strings/string16.h"

[rchtara, 2014-04-09 13:52:43]

Hi Ilya,
No problem and thanks  a lot for your review.
Cheers
Riadh

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:167: base::string16
/*user name to be removed */)
I'm not sure how it could be possible to not send any ipc.
The best thing I was able to do, with Vaclav, was to send just an empty msg by
removing the string16 parameter from the
AutofillMsg_RemoveSavedPassword. (see new patch)
The PasswordManager have to know which is the current selected PasswordForm, and
this is a why the second msg was send from the rendered. But there is probably
another way for doing so. But I'm not able to find it.
It would be great if you give us some suggestions.

https://codereview.chromium.org/223133003/diff/120001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/223133003/diff/120001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:209: } else {
On 2014/04/09 06:33:05, Ilya Sherman wrote:
> nit: Please DCHECK that the identifier is for an autocomplete entry.

Done.

[vabr (Chromium), 2014-04-09 14:06:29]

LGTM with comments, but please wait until Ilya approves as well.

Thanks!
Vaclav

https://codereview.chromium.org/223133003/diff/140001/components/autofill/con...
File components/autofill/content/browser/content_autofill_driver_unittest.cc
(right):

https://codereview.chromium.org/223133003/diff/140001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver_unittest.cc:264:
EXPECT_TRUE(GetRemoveSavedPasswordMessage());
Could you please also check, that the right username was stored in the
AutofillManager?

https://codereview.chromium.org/223133003/diff/140001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/223133003/diff/140001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:164: // The renderer is
going to clean the suggestions it saves and
Maybe:
"The renderer is going to clear the username-to-form map it keeps and"

https://codereview.chromium.org/223133003/diff/140001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/223133003/diff/140001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:187: username_to_remove_()
{
This should not be needed, please remove.

https://codereview.chromium.org/223133003/diff/140001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1177: base::string16
AutofillManager::GetUserNameToRemove() {
nit: you can name these two accessors
username_to_remove()
and
set_username_to_remove(), respectively, and put the definitions in the class
definition in the header file. It's allowed by the style guide, and makes the
code more readable (it's immediately clear from the function name, that it's
just a simple accessor).

https://codereview.chromium.org/223133003/diff/140001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/223133003/diff/140001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:349: base::string16
username_to_remove_;
Please comment, e.g.:
"When the user deletes a password suggestion, the associated username is kept
here, so that the password can be removed from the PasswordStore later."

[rchtara, 2014-04-09 15:28:20]

https://codereview.chromium.org/223133003/diff/140001/components/autofill/con...
File components/autofill/content/browser/content_autofill_driver_unittest.cc
(right):

https://codereview.chromium.org/223133003/diff/140001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver_unittest.cc:264:
EXPECT_TRUE(GetRemoveSavedPasswordMessage());
On 2014/04/09 14:06:30, vabr (Chromium) wrote:
> Could you please also check, that the right username was stored in the
> AutofillManager?

Done.

https://codereview.chromium.org/223133003/diff/140001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/223133003/diff/140001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:164: // The renderer is
going to clean the suggestions it saves and
On 2014/04/09 14:06:30, vabr (Chromium) wrote:
> Maybe:
> "The renderer is going to clear the username-to-form map it keeps and"

Done.

https://codereview.chromium.org/223133003/diff/140001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/223133003/diff/140001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:187: username_to_remove_()
{
On 2014/04/09 14:06:30, vabr (Chromium) wrote:
> This should not be needed, please remove.

Done.

https://codereview.chromium.org/223133003/diff/140001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1177: base::string16
AutofillManager::GetUserNameToRemove() {
On 2014/04/09 14:06:30, vabr (Chromium) wrote:
> nit: you can name these two accessors
> username_to_remove()
> and
> set_username_to_remove(), respectively, and put the definitions in the class
> definition in the header file. It's allowed by the style guide, and makes the
> code more readable (it's immediately clear from the function name, that it's
> just a simple accessor).

Done.

[Ilya Sherman, 2014-04-09 21:42:20]

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:8: #include <vector>
On 2014/04/09 11:17:13, vabr (Chromium) wrote:
> On 2014/04/09 06:33:05, Ilya Sherman wrote:
> > Why did you add this?
> 
> I asked Riadh to include that at a point when one of the messages had a
> std::vector as an argument. That addition was dropped, but there are such
> messages in the ToT version already. The style guide seems to require
#including
> all we use and cannot forward-declare, in particular templates
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...).
> It's also what is done for std::string.
> 
> I'm not insisting on that change, though.
> 
> If we keep it, we should also
> #include "base/strings/string16.h"

If this is fixing up IWYU violations in existing code, then let's push those
fixes out to a separate CL.  I'm always happy to LGTM+CQ small cleanup CLs :) 
Thanks!

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:167: base::string16
/*user name to be removed */)
On 2014/04/09 13:52:43, rchtara wrote:
> I'm not sure how it could be possible to not send any ipc.
> The best thing I was able to do, with Vaclav, was to send just an empty msg by
> removing the string16 parameter from the
> AutofillMsg_RemoveSavedPassword. (see new patch)
> The PasswordManager have to know which is the current selected PasswordForm,
and
> this is a why the second msg was send from the rendered. But there is probably
> another way for doing so. But I'm not able to find it.
> It would be great if you give us some suggestions.

Please discuss this with Patrick Dubroy.  My suggestion might not make sense
w.r.t. the current state of the code, but ought to make sense after Patrick's
changes finish landing.  So as to not add code that we'll need to remove in a
week or two, I'd recommend either: (a) removing all IPC from this CL, and being
ok with an imperfect (but still pretty good) UX until Patrick's changes make it
in; or (b) delaying this CL until Patrick's changes have landed.

[Ilya Sherman, 2014-04-09 21:43:49]

Argh, I typed the mystical quattrogram without intending to activate its
mystical powers!  Just to be clear, this CL "not lgtm" in its current state.

[vabr (Chromium), 2014-04-10 13:45:16]

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:8: #include <vector>
On 2014/04/09 21:42:20, Ilya Sherman wrote:
> On 2014/04/09 11:17:13, vabr (Chromium) wrote:
> > On 2014/04/09 06:33:05, Ilya Sherman wrote:
> > > Why did you add this?
> > 
> > I asked Riadh to include that at a point when one of the messages had a
> > std::vector as an argument. That addition was dropped, but there are such
> > messages in the ToT version already. The style guide seems to require
> #including
> > all we use and cannot forward-declare, in particular templates
> >
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...).
> > It's also what is done for std::string.
> > 
> > I'm not insisting on that change, though.
> > 
> > If we keep it, we should also
> > #include "base/strings/string16.h"
> 
> If this is fixing up IWYU violations in existing code, then let's push those
> fixes out to a separate CL.  I'm always happy to LGTM+CQ small cleanup CLs :) 
> Thanks!

I agree. Riadh, please disregard my previous request and remove the #include. If
you find spare time, do file a new clean-up CL for Include-What-You-Use. :)

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:167: base::string16
/*user name to be removed */)
On 2014/04/09 21:42:20, Ilya Sherman wrote:
> On 2014/04/09 13:52:43, rchtara wrote:
> > I'm not sure how it could be possible to not send any ipc.
> > The best thing I was able to do, with Vaclav, was to send just an empty msg
by
> > removing the string16 parameter from the
> > AutofillMsg_RemoveSavedPassword. (see new patch)
> > The PasswordManager have to know which is the current selected PasswordForm,
> and
> > this is a why the second msg was send from the rendered. But there is
probably
> > another way for doing so. But I'm not able to find it.
> > It would be great if you give us some suggestions.
> 
> Please discuss this with Patrick Dubroy.  My suggestion might not make sense
> w.r.t. the current state of the code, but ought to make sense after Patrick's
> changes finish landing.  So as to not add code that we'll need to remove in a
> week or two, I'd recommend either: (a) removing all IPC from this CL, and
being
> ok with an imperfect (but still pretty good) UX until Patrick's changes make
it
> in; or (b) delaying this CL until Patrick's changes have landed.

Thanks, Ilya, for making us aware of the coming changes. I believe we need to
wait for Patrick then, he should be back in May. I advise against pushing out
only the IPC-less part -- this might generate confused bugreports, and also
after the planned code changes, some things might become easier to implement.

[Patrick Dubroy, 2014-04-29 12:47:05]

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:167: base::string16
/*user name to be removed */)
On 2014/04/09 21:42:20, Ilya Sherman wrote:
> On 2014/04/09 13:52:43, rchtara wrote:
> > I'm not sure how it could be possible to not send any ipc.
> > The best thing I was able to do, with Vaclav, was to send just an empty msg
by
> > removing the string16 parameter from the
> > AutofillMsg_RemoveSavedPassword. (see new patch)
> > The PasswordManager have to know which is the current selected PasswordForm,
> and
> > this is a why the second msg was send from the rendered. But there is
probably
> > another way for doing so. But I'm not able to find it.
> > It would be great if you give us some suggestions.
> 
> Please discuss this with Patrick Dubroy.  My suggestion might not make sense
> w.r.t. the current state of the code, but ought to make sense after Patrick's
> changes finish landing.  So as to not add code that we'll need to remove in a
> week or two, I'd recommend either: (a) removing all IPC from this CL, and
being
> ok with an imperfect (but still pretty good) UX until Patrick's changes make
it
> in; or (b) delaying this CL until Patrick's changes have landed.

The browser renders the pop-up, but the suggestions themselves are still
provided by the renderer. Removing all login info from the renderer is out of
scope for my changes (it's not necessary to solve the Keychain prompting
problem), but I think we all agree that it would be nice to do.

[Ilya Sherman, 2014-04-29 20:40:16]

On 2014/04/29 12:47:05, Patrick Dubroy wrote:
>
https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
> File components/autofill/content/common/autofill_messages.h (right):
> 
>
https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
> components/autofill/content/common/autofill_messages.h:167: base::string16
> /*user name to be removed */)
> On 2014/04/09 21:42:20, Ilya Sherman wrote:
> > On 2014/04/09 13:52:43, rchtara wrote:
> > > I'm not sure how it could be possible to not send any ipc.
> > > The best thing I was able to do, with Vaclav, was to send just an empty
msg
> by
> > > removing the string16 parameter from the
> > > AutofillMsg_RemoveSavedPassword. (see new patch)
> > > The PasswordManager have to know which is the current selected
PasswordForm,
> > and
> > > this is a why the second msg was send from the rendered. But there is
> probably
> > > another way for doing so. But I'm not able to find it.
> > > It would be great if you give us some suggestions.
> > 
> > Please discuss this with Patrick Dubroy.  My suggestion might not make sense
> > w.r.t. the current state of the code, but ought to make sense after
Patrick's
> > changes finish landing.  So as to not add code that we'll need to remove in
a
> > week or two, I'd recommend either: (a) removing all IPC from this CL, and
> being
> > ok with an imperfect (but still pretty good) UX until Patrick's changes make
> it
> > in; or (b) delaying this CL until Patrick's changes have landed.
> 
> The browser renders the pop-up, but the suggestions themselves are still
> provided by the renderer. Removing all login info from the renderer is out of
> scope for my changes (it's not necessary to solve the Keychain prompting
> problem), but I think we all agree that it would be nice to do.

Hmm, PasswordAutofillManager::AcceptSuggestion() passes the username and
password from the browser into the renderer.  Are you saying that the flow is
that
  (1) The full list starts in the browser, then
  (2) is passed to the renderer, then
  (3) is passed back to the browser when showing the popup, and then finally
  (4) a single username/password combination is passed to the renderer when
chosen from the popup?

If so, it sounds like we can safely get rid of steps 1 and 2.  If that's
accurate, then I'd prefer for this CL to be written assuming that we'll end up
in a world where steps (1) and (2) don't exist.  Rather than adding complexity
to handle steps (1) and (2), we should just eliminate those steps.  I bet it's
fine to do that either before or after this CL lands.

[vabr (Chromium), 2014-04-30 07:31:23]

On 2014/04/29 20:40:16, Ilya Sherman wrote:
> On 2014/04/29 12:47:05, Patrick Dubroy wrote:
> >
>
https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
> > File components/autofill/content/common/autofill_messages.h (right):
> > 
> >
>
https://codereview.chromium.org/223133003/diff/120001/components/autofill/con...
> > components/autofill/content/common/autofill_messages.h:167: base::string16
> > /*user name to be removed */)
> > On 2014/04/09 21:42:20, Ilya Sherman wrote:
> > > On 2014/04/09 13:52:43, rchtara wrote:
> > > > I'm not sure how it could be possible to not send any ipc.
> > > > The best thing I was able to do, with Vaclav, was to send just an empty
> msg
> > by
> > > > removing the string16 parameter from the
> > > > AutofillMsg_RemoveSavedPassword. (see new patch)
> > > > The PasswordManager have to know which is the current selected
> PasswordForm,
> > > and
> > > > this is a why the second msg was send from the rendered. But there is
> > probably
> > > > another way for doing so. But I'm not able to find it.
> > > > It would be great if you give us some suggestions.
> > > 
> > > Please discuss this with Patrick Dubroy.  My suggestion might not make
sense
> > > w.r.t. the current state of the code, but ought to make sense after
> Patrick's
> > > changes finish landing.  So as to not add code that we'll need to remove
in
> a
> > > week or two, I'd recommend either: (a) removing all IPC from this CL, and
> > being
> > > ok with an imperfect (but still pretty good) UX until Patrick's changes
make
> > it
> > > in; or (b) delaying this CL until Patrick's changes have landed.
> > 
> > The browser renders the pop-up, but the suggestions themselves are still
> > provided by the renderer. Removing all login info from the renderer is out
of
> > scope for my changes (it's not necessary to solve the Keychain prompting
> > problem), but I think we all agree that it would be nice to do.
> 
> Hmm, PasswordAutofillManager::AcceptSuggestion() passes the username and
> password from the browser into the renderer.  Are you saying that the flow is
> that
>   (1) The full list starts in the browser, then
>   (2) is passed to the renderer, then
>   (3) is passed back to the browser when showing the popup, and then finally
>   (4) a single username/password combination is passed to the renderer when
> chosen from the popup?
> 
> If so, it sounds like we can safely get rid of steps 1 and 2.  If that's
> accurate, then I'd prefer for this CL to be written assuming that we'll end up
> in a world where steps (1) and (2) don't exist.  Rather than adding complexity
> to handle steps (1) and (2), we should just eliminate those steps.  I bet it's
> fine to do that either before or after this CL lands.

Yesterday, during an off-line chat with Riadh and Patrick, we agreed that before
returning to this CL, the code should be refactored to eliminate steps (1) and
(2). This is because the feature being added here is not time-wise critical, so
we should not increase the technical debt just for that. Once I get to it, I'll
file a bug with a more detailed description of the refactoring (if anybody does
that before me, even better :)).

Cheers,
Vaclav

[vabr (Chromium), 2014-05-26 10:23:24]

> Yesterday, during an off-line chat with Riadh and Patrick, we agreed that
before
> returning to this CL, the code should be refactored to eliminate steps (1) and
> (2). This is because the feature being added here is not time-wise critical,
so
> we should not increase the technical debt just for that. Once I get to it,
I'll
> file a bug with a more detailed description of the refactoring (if anybody
does
> that before me, even better :)).

Done as http://crbug.com/377422.

> 
> Cheers,
> Vaclav

[rchtara, 2014-05-26 16:40:10]

Thanks a lot vaclav, for not forgetting it
Cheers
Riadh