Allow deleting autofill password suggestions on Shift+Delete

Allow deleting autofill password suggestions on Shift+Delete.

NOTE: This CL was replaced by https://codereview.chromium.org/223133003/, due to username change of the author. It could be closed, but we keep it open for a short while yet, to allow investigation of an issue with adding collaborators.

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.

COLLABORATOR=rchtara@chromium.org
BUG=329038

[vabr (Chromium), 2014-01-10 18:40:00]

Hi Riadh,

Congrats for your first upload to codereview.chromium.org! :)

Before my in-line comments are listed, a couple of general ones. I'll be verbose
to give you more background, so don't be scared by the text length below. :)

Also, if you don't understand what I mean, don't hesitate to ask.

1) The CL description needs more care (use the "Edit Issue" link):
   A) The title (which should be kept the same as the first line
      of description) is ideally a quick sentence saying what and 
      in which part of Chromium this CL does. It should be
      understandable for people working on other parts of 
      Chromium. Your title says the general topic of deleting 
      passwords on Shift+Delete, but not what this CL does about
      it (could be fixing, disabling, …). What about
      "Allow deleting autofill password suggestions on Shift+Delete"?
   B) The rest of the description should describe what is the
      current state, what is wrong about it, and how does this CL
      fix it. You do not need to write an essay here, because
      detailed information is available on the linked bug, but it
      is important that people understand the CL without clicking
      through to the bug report. (Think of somebody going quickly
      through a commit history of a file, trying to find changes
      relevant to some breakage.)

2) It is advisable to do the following before you sent out a
   review request:
   A) Check the formatting with the clang-format tool, if
      available.
   B) Compile the code and relevant tests, to make sure it is
      syntactically complete.
   C) Run relevant tests, to make sure no existing ones were
      broken.
   D) After uploading to the codereview site, click on the links
      in the "Lint" column (even if they say "? errors"), and
      check for reported problems.
   Having said that, I understand that it is not always practical
   to do all of this, and I'm guilty of skipping those steps
   often. It's just a good way to speed up the review by
   eliminating rounds when the reviewer reports errors detectable
   by the above steps. Especially step B) is rather important.
   If the code won't compile, and you don't know how to fix that,
   it's good to highlight this to the reviewer.

3) About the trouble with including WebFormElement headers – I
   don't think you need to deal with WebFormElement anywhere in
   the code you add in components/. WebFormElement is the HTML
   <FORM> element, but you should be interested in getting the
   credentials information, stored in PasswordForm.
   So at the place when you would see a WebFormElement at the
   first time, you should use autofill::CreatePasswordForm to
   convert it into a PasswordForm.
   This reminds me, you don't seem to introduce the WebFormElement
   anywhere in your current patch. The nearest you get to it is
   adding the extra argument to OnQueryFormFieldAutofill, but you
   don't change the declaration of the
   AutofillHostMsg_QueryFormFieldAutofill message. So your next
   steps should lead there, and work up from the point of
   creation of that message to find the right spot to convert the
   WebFormElement into PasswordForm.

4) Finally, you should always remember to add tests for anything
   you introduced, not just adjust existing tests to work with
   changed code. There are two sources of need for tests:
   A) If you are fixing a bug, you should create a test which
      will reconstruct the failure, simulate what the user did
      and went wrong. That test should not only pass with your fix
      applied, but also fail without your fix applied (that's
      really important to test).
   B) If you introduce new public interface (by "public" I mean
      accessible to other pieces of code, so roughly the same
      meaning as "public:" in the C++ class definition), you
      should add tests for that. These are typically unit tests.
   It's common that the fix is a couple of lines written within
   one hour, while the corresponding tests are couple of hundred
   lines and take over two days to write and get right. That's
   still a good trade-off, because these tests will make sure
   that what was once fixed, gets not broken the same way later.

Thanks for your enthusiasm and hard work. I must say we all in the team are
impressed by your quick start and direct attack at coding, 4 months before your
start date! :) The more that you have other things to do. So keep up the good
work, it's a pleasure to support you.

Cheers,
Vaclav

https://codereview.chromium.org/133893004/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/password_manager.cc (left):

https://codereview.chromium.org/133893004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager.cc:13: #include
"base/threading/platform_thread.h"
Don't remove this header, it declares, e.g., PlatformThreadId, which is used
below.

https://codereview.chromium.org/133893004/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/133893004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager.cc:359: const PasswordForm&
password_form) {
"const PasswordForm&" does not match the argument type of
AutofillHostMes_RemovePasswordSuggestion, which is blink::WebFormElement.

These are very different types (PasswordForm is a structure holding login
credentials, WebFormElement represents a <FORM> HTML element).

You need to create PasswordForm from WebFormElement first, and preferably before
sending anything, so that you can avoid touching blink:: objects.

https://codereview.chromium.org/133893004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager.cc:360: PasswordStore*
password_store = PasswordStoreFactory::GetForProfile(
Please either make password_store a scoped_refptr<PasswordStore>, or call
RemoveLogin() directly on the result of GetForProfile().

https://codereview.chromium.org/133893004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_manager.cc:363: 
nit: Please remove this empty line.

https://codereview.chromium.org/133893004/diff/1/components/autofill/content/...
File components/autofill/content/browser/autofill_driver_impl.cc (left):

https://codereview.chromium.org/133893004/diff/1/components/autofill/content/...
components/autofill/content/browser/autofill_driver_impl.cc:208:
IPC_MESSAGE_FORWARD(AutofillHostMsg_ShowPasswordSuggestions,
If you only meant to change the order of this message declaration, then please
revert the change. This will make the CL smaller, and create less confusion when
looking at the code history.

https://codereview.chromium.org/133893004/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/133893004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_external_delegate.cc:118:
}OnShowPasswordSuggestions
This looks misplaced. Please delete "OnShowPasswordSuggestions" or repair
otherwise.

(BTW., did this compile?)

https://codereview.chromium.org/133893004/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/133893004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_manager.cc:359: const FormData& form,
Wrong indentation, should match the column position of "int query_id".

Have you been able to use clang-format? In particular, in the form of "git cl
format", it will autoformat all your changes.

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

https://codereview.chromium.org/133893004/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_manager.h:29: #include
"third_party/WebKit/public/web/WebFormControlElement.h"
Do not include WebFormControllElement.h, when you only ever use WebFormElement.

(I realise that this comment will become moot once we get rid of the added
blink:: stuff.)

[riadh.chtara, 2014-01-11 09:58:06]

Hi Vaclav,
I was very glad when reading your mail :) 
There is a lot of great hints in it that are going to help me in the future.
So thanks a lot for taking the time to write all the details, especially on
Friday after 18h.


Cheers,
Riadh

[riadh.chtara, 2014-01-12 17:44:56]

Hi Vaclav, 
I updated the patch but still got 2 problems:
* I have problem with git cl format
I installed clang-format-3.5 but it doesn't work for me.
I also tried to do like
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/AGGFoJ2TUtE
this means:
"Actually a simpler possibility is to add symlinks to /usr/bin/clang-format and
/usr/lib/clang-format/clang-format-diff.py instead."

but I got this error msg
Traceback (most recent call last):
  File "/usr/lib/clang-format/clang-format-diff.py", line 114, in <module>
    main()
  File "/usr/lib/clang-format/clang-format-diff.py", line 97, in main
    stderr=None, stdin=subprocess.PIPE)
  File "/usr/lib/python2.7/subprocess.py", line 679, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1259, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory
Command "/usr/bin/python /usr/lib/clang-format/clang-format-diff.py -p0 -style
Chromium -i" failed.



*For the patch, I solved many of the problems you have mention, and I still get
only some formatting warnings when I use git cl upload that I believe can be
solved when the the first issue with clang-format is solved.
But when I tried to build chromium, I got this error: 
In file included from
../../components/autofill/content/renderer/password_autofill_agent.cc:14:0:
../../components/autofill/core/common/autofill_messages.h:208:1: error: expected
nested-name-specifier before ‘TE’
../../components/autofill/core/common/autofill_messages.h:208:1: error: expected
‘>’ before ‘typename’
../../components/autofill/core/common/autofill_messages.h:208:1: error: ‘TE’ is
not a type
../../components/autofill/core/common/autofill_messages.h:208:1: error: ‘TF’ has
not been declared
[7/742] CXX
obj/content/browser/renderer_host/pepper/content.pepper_message_filter.o
ninja: build stopped: subcommand failed.

It's related to the use of IPC_MESSAGE_ROUTED6 as I'm the first one to use it
and had to implement it. I tired to do that but at the end I still got this
error.

I'm not sure at least until now how to solve these problems.
Could you please what do you think about them?

Thanks a lot
Cheers,
Riadh

[riadh.chtara, 2014-01-13 14:22:21]

Hi Vaclav, 
I want just to say that git cl upload is working now for me
Cheers,
Riadh

[vabr (do not use), 2014-01-13 14:23:49]

Thanks for the update, Riadh!

I'm currently reviewing your change, so you will hear from me later today.

Cheers,
Vaclav


On Mon, Jan 13, 2014 at 3:22 PM, <riadh.chtara@gmail.com> wrote:

> Hi Vaclav,
> I want just to say that git cl upload is working now for me
> Cheers,
> Riadh
>
> https://codereview.chromium.org/133893004/
>



-- 
Google Germany GmbH, Dienerstr. 12, 80331 München
Registergericht und -nummer: Hamburg, HRB 86891, Sitz der Gesellschaft:
Hamburg, Geschäftsführer: Graham Law, Christine Elizabeth Flores

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vabr (Chromium), 2014-01-13 14:46:28]

Hi Riadh,

Thanks for your further changes. And I also really like the CL description you
updated -- it's concise and clear.

I'm unfortunately unsure how to help you with the clang-format. If you cannot
make it work by the time we clear all other concerns, I might run it for you.
For now, I'm ignoring indentation mistakes in your changes, but we'll correct
that before sending out to the OWNERS review.

For the compilation problem: see my comment about a missing comma in one of the
templates. Maybe that helps.

Aside from the particular comments below, you are still missing tests for new
code you added.

Also one random piece of general code review advice: it's a good practice to
answer on all comments, so that just by looking at the number of added comments,
one can easily check if all previous were addressed. For comments where you
simply agree with the suggestion and change the code accordingly, it is
sufficient to just write "Done." (There is even a button for that.) Moreover, if
such a comment does not require a code change, "Ack" (for acknowledged) is also
an option. For comments where you don't agree, an answer is obviously useful. :)

Cheers,
Vaclav

https://codereview.chromium.org/133893004/diff/210001/chrome/browser/password...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/133893004/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager.cc:364:
password_store.get()->RemoveLogin(password_form);
You don't need .get() when using the operator -> .

https://codereview.chromium.org/133893004/diff/210001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:577: password_form.get(),
You have to dereference the pointer here:
*password_form
(also note that .get() is not necessary when using *)

https://codereview.chromium.org/133893004/diff/210001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:629:
*password_form.get(),
The ".get()" should not be needed.

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

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:38: const
PasswordForm& passwordform,
Why do you actually need the PasswordForm in OnQuery? Would it be enough just to
receive it in OnShowPasswordSuggestions?
If yes, it would save you the hassle with templates and lot of lines in the
tests.

(I'm not sure myself, so if it's needed here as well, just don't be shy to let
me know why.)

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.h:64: const
PasswordForm& passwordform,
nit: "password form" are two words, so it's better to name the variable
password_form (with the underscore).
(Also on other places in this CL where you use "passwordform".)

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.h:149: PasswordForm
autofill_query_password_form_;
Please put in a comment about what is this member variable for.

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

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:34: 
Please remove this extra line.

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
File components/autofill/core/browser/password_autofill_manager.cc (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/password_autofill_manager.cc:57:
password_form.get()));
*password_form
(dereference instead of getting a pointer)

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
File components/autofill/core/browser/password_autofill_manager.h (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/password_autofill_manager.h:39: // Clear the
user name and the password associated with the form |form|.
"form" no longer matches the name of any of the two arguments. Please update the
comment to explain both of those arguments.

https://codereview.chromium.org/133893004/diff/210001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/133893004/diff/210001/ipc/ipc_message_macros....
ipc/ipc_message_macros.h:243: #define IPC_MESSAGE_CONTROL6(msg_class, type1,
type2, type3, type4, type5, type6) \
What do you need this definition for?

https://codereview.chromium.org/133893004/diff/210001/ipc/ipc_message_macros....
ipc/ipc_message_macros.h:571: template<typename TA, typename TB, typename TC,
typename TD, typename TE    \
Are you missing a "," after TE?

[riadh.chtara, 2014-01-13 20:33:20]

Hi Vaclav,
Thanks a lot for your answers and advice.
I solved many of the problems within the code.
But it still has some small issues and of course the tests.
I starting becoming a bit busy. So I will may spent more time working on the
code.
Thanks a lot again
Cheers,
Riadh

[riadh.chtara, 2014-01-13 20:34:09]

https://codereview.chromium.org/133893004/diff/210001/chrome/browser/password...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/133893004/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager.cc:364:
password_store.get()->RemoveLogin(password_form);
On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> You don't need .get() when using the operator -> .

Done.

https://codereview.chromium.org/133893004/diff/210001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:577: password_form.get(),
On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> You have to dereference the pointer here:
> *password_form
> (also note that .get() is not necessary when using *)

Done.

https://codereview.chromium.org/133893004/diff/210001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:629:
*password_form.get(),
On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> The ".get()" should not be needed.

Done.

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.h:64: const
PasswordForm& passwordform,
On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> nit: "password form" are two words, so it's better to name the variable
> password_form (with the underscore).
> (Also on other places in this CL where you use "passwordform".)

Done.

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.h:149: PasswordForm
autofill_query_password_form_;
On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> Please put in a comment about what is this member variable for.

Done.

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

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:34: 
On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> Please remove this extra line.

Done.

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
File components/autofill/core/browser/password_autofill_manager.cc (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/password_autofill_manager.cc:57:
password_form.get()));
On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> *password_form
> (dereference instead of getting a pointer)

Done.

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
File components/autofill/core/browser/password_autofill_manager.h (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/password_autofill_manager.h:39: // Clear the
user name and the password associated with the form |form|.
On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> "form" no longer matches the name of any of the two arguments. Please update
the
> comment to explain both of those arguments.

Done.

https://codereview.chromium.org/133893004/diff/210001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/133893004/diff/210001/ipc/ipc_message_macros....
ipc/ipc_message_macros.h:243: #define IPC_MESSAGE_CONTROL6(msg_class, type1,
type2, type3, type4, type5, type6) \
On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> What do you need this definition for?

Done.

https://codereview.chromium.org/133893004/diff/210001/ipc/ipc_message_macros....
ipc/ipc_message_macros.h:571: template<typename TA, typename TB, typename TC,
typename TD, typename TE    \
On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> Are you missing a "," after TE?

Done.

[riadh.chtara, 2014-01-13 20:56:07]

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

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:38: const
PasswordForm& passwordform,
I m not really but this the logic I followed:
When I created the  autofill_query_password_form_, I wanted something that
behave like  autofill_query_field_. So whenever autofill_query_field_ changes my
autofill_query_password_form_ needs to change too.
In OnQuery, autofill_query_field_ changes so this is why I changed the
autofill_query_password_form_.

[vabr (Chromium), 2014-01-14 14:11:02]

Hi Riadh.

First of all, it looks like you got the formatting script working. If so --
congrats!

Also, don't worry about slowing down on this CL. You should obviously first do
anything you need to do in the school and other parts of your life. Google does
not expect you to start working before April, so there is really no rush. :)

I left one comment about OnQuery vs. OnShowPasswordSuggestions.

But I also spotted a couple of places, where you replied "Done", but without
actually changing the code as suggested. I started to reply to those, but then I
though maybe there is some misunderstanding.
Did you forgot to upload the most recent version of your checkout?
Or maybe I was confusing when explaining the role of "Done." -- you should only
reply "Done." when you accept the suggestion and change the code accordingly. In
particular, you should not reply "Done." if you have not changed the code, or if
the comment is not a suggestion, but a question.

And one more piece of unsolicited advice :) -- proper naming of the patch sets.
It's not that important, and some people don't name them at all. It's fine if
you want to name them all the same, or not at all.
On the other hand, it can be useful to name them, to make it easier for the
reviewer to understand, what happened.
If the new patchset introduces one significant change, you can say that, e.g.,
"Now formatted with a script", or "Reverted changes to SomeImportantFunction".
If the new patchset contains a lot of little changes based on review comments,
it's OK just to say "Comments addressed" or similar.

Also one piece of advice for future: if you happen to rebase your checkout on
the tip of the tree during your work on this CL, you should upload one patchset
just with the rebasing changes, to make it easy to distinguish your changes from
others' changes. In more detail:
1. Suppose your last patch set in the CL has number 3.
2. Then you decide to run "git pull --rebase && gclient sync" or whatever works
for your set-up, to pick up the newest source code changes.
(3. Possibly, you might also need to resolve manually some conflicts.)
4. After the rebasing and conflict resolution, you should immediately run "git
cl upload", and use the patch set title like "Just rebased on ToT" (ToT = tip of
the [source] tree). That will create patch set 4.
5. You do your other changes, and upload again, patch set 5.
After those 5 steps, the reviewer can see your new changes as the diff between
patch set 4 and 5. Without the extra upload in step 4, those new changes would
be mixed with whichever changes have been made to the source tree in the files
you work on.


I hope I don't overdo it with these long e-mails. Feel free to tell me to shut
up. :)

Have a great day,
Vaclav

https://codereview.chromium.org/133893004/diff/210001/chrome/browser/password...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/133893004/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager.cc:364:
password_store.get()->RemoveLogin(password_form);
On 2014/01/13 20:34:10, riadh.chtara wrote:
> On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> > You don't need .get() when using the operator -> .
> 
> Done.

Does not seem done -- there is still the ".get()" part on this line.

https://codereview.chromium.org/133893004/diff/210001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:629:
*password_form.get(),
On 2014/01/13 20:34:10, riadh.chtara wrote:
> On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> > The ".get()" should not be needed.
> 
> Done.

Again, not done -- ".get()" is still there.

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

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:38: const
PasswordForm& passwordform,
We should not just blindly copy the lifetime of another object. :)

I observed, when are OnQuery and OnShowPasswordSuggestions called:
OnQuery is triggered basically with any character typed into a form field.
OnShowPasswordSuggestions is triggered, when Chrome provides a credentials
suggestion, i.e., when you type something into the "username" field, and a
pop-up menu appears, containing some suggested credentials.

So to me it looks like you can leave OnQuery untouched, and only care about
OnShowPasswordSuggestions.

On 2014/01/13 20:56:08, riadh.chtara wrote:
> I m not really but this the logic I followed:
> When I created the  autofill_query_password_form_, I wanted something that
> behave like  autofill_query_field_. So whenever autofill_query_field_ changes
my
> autofill_query_password_form_ needs to change too.
> In OnQuery, autofill_query_field_ changes so this is why I changed the
> autofill_query_password_form_.

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.h:64: const
PasswordForm& passwordform,
On 2014/01/13 20:34:10, riadh.chtara wrote:
> On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> > nit: "password form" are two words, so it's better to name the variable
> > password_form (with the underscore).
> > (Also on other places in this CL where you use "passwordform".)
> 
> Done.

I'm still seeing many occurrences of "passwordform" in patch set 3.

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
File components/autofill/core/browser/password_autofill_manager.cc (right):

https://codereview.chromium.org/133893004/diff/210001/components/autofill/cor...
components/autofill/core/browser/password_autofill_manager.cc:57:
password_form.get()));
On 2014/01/13 20:34:10, riadh.chtara wrote:
> On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> > *password_form
> > (dereference instead of getting a pointer)
> 
> Done.

Does not look done -- there is still .get() instead of the dereference.

https://codereview.chromium.org/133893004/diff/210001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/133893004/diff/210001/ipc/ipc_message_macros....
ipc/ipc_message_macros.h:243: #define IPC_MESSAGE_CONTROL6(msg_class, type1,
type2, type3, type4, type5, type6) \
On 2014/01/13 20:34:10, riadh.chtara wrote:
> On 2014/01/13 14:46:29, vabr (Chromium) wrote:
> > What do you need this definition for?
> 
> Done.

I doubt "done" is really an answer for the question. :)

[riadh.chtara, 2014-01-14 14:48:12]

Hi valcav,
Yeah, git cl format is working now for me. It Seems not only me have this issue.
So I followed some suggestions from the dev group link I show you.
And now it's working.

Sorry for the misunderstanding. You're right.
I didn't update the patch yet.
But I just did it.

Thanks a lot for the tips :)
Have a great day too
Cheers
Riadh

[vabr (Chromium), 2014-01-14 15:01:20]

Thanks, Riadh!

I really like where this CL is going, it looks better with each patchset. :)

For your next steps:
1. If you manage to get it working without involving OnQuery (based on my
previous comment from today), that would be awesome, and also would spare much
of the code changes.
2. Once the main code is OK, and you can verify the Shift+Delete works in the
browser, try to think about the new tests.

Thanks for your work.
Cheers,
Vaclav

https://codereview.chromium.org/133893004/diff/600001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/600001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:557:
DCHECK(password_form.get());
Even here you should be able to spare the .get() part, converting to boolean
works directly.

https://codereview.chromium.org/133893004/diff/600001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/600001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:617:
DCHECK(password_form.get());
Also here, .get() is not necessary.

https://codereview.chromium.org/133893004/diff/600001/components/autofill/cor...
File components/autofill/core/browser/password_autofill_manager.cc (right):

https://codereview.chromium.org/133893004/diff/600001/components/autofill/cor...
components/autofill/core/browser/password_autofill_manager.cc:58: void
PasswordAutofillManager::Reset() { login_to_password_info_.clear(); }
nit: This might be because of the automatic formatting, but could you please
revert the definition of Reset() into it's original state (3 lines instead of
1)?

The idea is to minimise the changes to the source (for easy look-ups in history
of who introduced which change). Because both variants of writing Reset() are
acceptable, and you don't change the code of it, we should leave it.

[vabr (Chromium), 2014-01-15 14:30:58]

BTW., Looks like we lost the BUG=329038 line from the CL description, so I added
that back.

[riadh.chtara, 2014-01-15 21:27:47]

Hi Vaclav,
I'm really glad that you like the CL.
And it's thanks to your help. :)
I will try to to finish these steps as soon as possible.
I will be excited to see this bug fixed in the next chrome release.

Thanks also for adding BUG=329038 line

Cheers,
Riadh

[riadh.chtara, 2014-01-17 08:25:08]

Hi,
The current patch could be complied (it's the first one that can do that)
But the issue is not yet fixed.
When I debug PasswordManager::OnRemovePasswordSuggestion is never called.
I'm not sure, but the cause is probably I'm sending the msg to the wrong place
Cheers
Riadh

https://codereview.chromium.org/133893004/diff/600001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/600001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:557:
DCHECK(password_form.get());
On 2014/01/14 15:01:21, vabr (Chromium) wrote:
> Even here you should be able to spare the .get() part, converting to boolean
> works directly.

Done.

https://codereview.chromium.org/133893004/diff/600001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/600001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:617:
DCHECK(password_form.get());
On 2014/01/14 15:01:21, vabr (Chromium) wrote:
> Also here, .get() is not necessary.

Done.

https://codereview.chromium.org/133893004/diff/600001/components/autofill/cor...
File components/autofill/core/browser/password_autofill_manager.cc (right):

https://codereview.chromium.org/133893004/diff/600001/components/autofill/cor...
components/autofill/core/browser/password_autofill_manager.cc:58: void
PasswordAutofillManager::Reset() { login_to_password_info_.clear(); }
Yes you're right, it's git cl format who have done this

[vabr (Chromium), 2014-01-17 18:40:12]

Hi Riadh,

Thanks for resolving more comments.

Crossing fingers for you with getting this working. I'm sorry I can't be of more
help currently, as today was busy, and we expect network outage in the office as
of now.

Cheers,
Vaclav

[riadh.chtara, 2014-01-19 12:13:43]

Hi Vaclav,
No problem.
I found out why PasswordManager::OnRemovePasswordSuggestion was never called.
And now it's called.
But I still have some other issues that I need to solve before before every
thing starts working well.
Thanks a lot
Cheers
Riadh

[riadh.chtara, 2014-01-23 11:05:00]

Hi Vaclav,
I'm stuck :)
As I have said the PasswordManager::OnRemovePasswordSuggestion is now called but
there is other issues that haven't been solved yet

When I see the suggestions buble, select one login and click on shift + del: I
get this log msg
[15144:15167:0123/112135:WARNING:native_backend_gnome_x.cc(560)] Keyring delete
failed: No matching results
[15144:15144:0123/112148:FATAL:autofill_external_delegate.cc(210)] Check failed:
success. 

The login is only temporely removed (when the page is refreshed or when I
continue typing the saved username, the buble reapears)


When I select the same login press and press shift + del this log msg



[15144:15144:0123/112148:FATAL:autofill_external_delegate.cc(210)] Check failed:
success. 
 [0x7fe8c9f0216c] base::debug::StackTrace::StackTrace()
 [0x7fe8c9f52d5b] logging::LogMessage::~LogMessage()
 [0x7fe8ce0fdf71] autofill::AutofillExternalDelegate::RemoveSuggestion()
 [0x7fe8cdb67b49] autofill::AutofillPopupControllerImpl::RemoveSelectedLine()
 [0x7fe8cdb66f80] autofill::AutofillPopupControllerImpl::HandleKeyPressEvent()
 [0x7fe8cdb69aef] base::internal::RunnableAdapter<>::Run()
 [0x7fe8cdb699c5] base::internal::InvokeHelper<>::MakeItSo()
 [0x7fe8cdb695aa] base::internal::Invoker<>::Run()
 [0x7fe8c4a8d5ed] base::Callback<>::Run()
 [0x7fe8c4a87f4d] content::RenderWidgetHostImpl::KeyPressListenersHandleEvent()
 [0x7fe8c4a83a18] content::RenderWidgetHostImpl::ForwardKeyboardEvent()
 [0x7fe8c4a69d1a] content::RenderViewHostImpl::ForwardKeyboardEvent()
 [0x7fe8c4a9b928] content::RenderWidgetHostViewGtk::ForwardKeyboardEvent()
 [0x7fe8c4970564] content::GtkIMContextWrapper::ProcessUnfilteredKeyPressEvent()
 [0x7fe8c496fe22] content::GtkIMContextWrapper::ProcessKeyEvent()
 [0x7fe8c4a9d047]
content::RenderWidgetHostViewGtkWidget::OnKeyPressReleaseEvent()
 [0x7fe8be436599] <unknown>
 [0x7fe8bd4a2140] g_closure_invoke
 [0x7fe8bd4b3550] <unknown>
 [0x7fe8bd4bb0cb] g_signal_emit_valist
 [0x7fe8bd4bb642] g_signal_emit
 [0x7fe8be54f95e] <unknown>
 [0x7fe8be56439b] gtk_window_propagate_key_event
 [0x7fe8cd86aee0] BrowserWindowGtk::OnKeyPress()
 [0x7fe8cd86c702] BrowserWindowGtk::OnKeyPressThunk()
 [0x7fe8be436599] <unknown>
 [0x7fe8bd4a2140] g_closure_invoke
 [0x7fe8bd4b3550] <unknown>
 [0x7fe8bd4bb0cb] g_signal_emit_valist
 [0x7fe8bd4bb642] g_signal_emit
 [0x7fe8be54f95e] <unknown>
 [0x7fe8be434a07] gtk_propagate_event
 [0x7fe8be434c8b] gtk_main_do_event
 [0x7fe8c9ed34a0] base::MessagePumpGtk::DispatchEvents()
 [0x7fe8c9ed36cd] base::MessagePumpGtk::EventDispatcher()
 [0x7fe8be0a9d7c] <unknown>
 [0x7fe8bd1e2ab5] g_main_context_dispatch
 [0x7fe8bd1e2de8] <unknown>
 [0x7fe8bd1e2ea4] g_main_context_iteration
 [0x7fe8c9ed2770] base::MessagePumpGlib::RunWithDispatcher()
 [0x7fe8c9ed2c58] base::MessagePumpGlib::Run()
 [0x7fe8c9f66916] base::MessageLoop::RunInternal()
 [0x7fe8c9f66834] base::MessageLoop::RunHandler()
 [0x7fe8c9fa87f0] base::RunLoop::Run()
 [0x7fe8ccc24a0e] ChromeBrowserMainParts::MainMessageLoopRun()
 [0x7fe8c4639954] content::BrowserMainLoop::RunMainMessageLoopParts()
 [0x7fe8c4641876] content::BrowserMainRunnerImpl::Run()
 [0x7fe8c463575f] content::BrowserMain()
 [0x7fe8c460177c] content::RunNamedProcessTypeMain()
 [0x7fe8c4602abd] content::ContentMainRunnerImpl::Run()
 [0x7fe8c4600b0b] content::ContentMain()
 [0x7fe8cc501b65] ChromeMain
 [0x7fe8cc501b25] main
 [0x7fe8bbb0376d] __libc_start_main
 [0x7fe8cc5019f9] <unknown>


Could you please help me?
Thanks a lot

[vabr (Chromium), 2014-01-24 19:09:42]

Hi Riadh,

I'm sorry for a late response. It's been busy in the office lately.

On 2014/01/23 11:05:00, riadh.chtara wrote:
> Hi Vaclav,
> I'm stuck :)
> As I have said the PasswordManager::OnRemovePasswordSuggestion is now called
but
> there is other issues that haven't been solved yet
> 
> When I see the suggestions buble, select one login and click on shift + del: I
> get this log msg
> [15144:15167:0123/112135:WARNING:native_backend_gnome_x.cc(560)] Keyring
delete
> failed: No matching results
I think the error log above is the key one: Chrome attempts to find the password
to delete, but does not see it in the database.

I suggest you inspect (in debugger or via logging) the value of the |form|
argument of NativeBackendGnome::RemoveLogin, and compare it both with what it
should be deleting and what you have in the keychain (run seahorse to see that).
I suspect the right information about the form might have gone lost during all
that passing along from the renderer.

Cheers,
Vaclav


> [15144:15144:0123/112148:FATAL:autofill_external_delegate.cc(210)] Check
failed:
> success. 
> 
> The login is only temporely removed (when the page is refreshed or when I
> continue typing the saved username, the buble reapears)
> 
> 
> When I select the same login press and press shift + del this log msg
> 
> 
> 
> [15144:15144:0123/112148:FATAL:autofill_external_delegate.cc(210)] Check
failed:
> success. 
>  [0x7fe8c9f0216c] base::debug::StackTrace::StackTrace()
>  [0x7fe8c9f52d5b] logging::LogMessage::~LogMessage()
>  [0x7fe8ce0fdf71] autofill::AutofillExternalDelegate::RemoveSuggestion()
>  [0x7fe8cdb67b49] autofill::AutofillPopupControllerImpl::RemoveSelectedLine()
>  [0x7fe8cdb66f80] autofill::AutofillPopupControllerImpl::HandleKeyPressEvent()
>  [0x7fe8cdb69aef] base::internal::RunnableAdapter<>::Run()
>  [0x7fe8cdb699c5] base::internal::InvokeHelper<>::MakeItSo()
>  [0x7fe8cdb695aa] base::internal::Invoker<>::Run()
>  [0x7fe8c4a8d5ed] base::Callback<>::Run()
>  [0x7fe8c4a87f4d]
content::RenderWidgetHostImpl::KeyPressListenersHandleEvent()
>  [0x7fe8c4a83a18] content::RenderWidgetHostImpl::ForwardKeyboardEvent()
>  [0x7fe8c4a69d1a] content::RenderViewHostImpl::ForwardKeyboardEvent()
>  [0x7fe8c4a9b928] content::RenderWidgetHostViewGtk::ForwardKeyboardEvent()
>  [0x7fe8c4970564]
content::GtkIMContextWrapper::ProcessUnfilteredKeyPressEvent()
>  [0x7fe8c496fe22] content::GtkIMContextWrapper::ProcessKeyEvent()
>  [0x7fe8c4a9d047]
> content::RenderWidgetHostViewGtkWidget::OnKeyPressReleaseEvent()
>  [0x7fe8be436599] <unknown>
>  [0x7fe8bd4a2140] g_closure_invoke
>  [0x7fe8bd4b3550] <unknown>
>  [0x7fe8bd4bb0cb] g_signal_emit_valist
>  [0x7fe8bd4bb642] g_signal_emit
>  [0x7fe8be54f95e] <unknown>
>  [0x7fe8be56439b] gtk_window_propagate_key_event
>  [0x7fe8cd86aee0] BrowserWindowGtk::OnKeyPress()
>  [0x7fe8cd86c702] BrowserWindowGtk::OnKeyPressThunk()
>  [0x7fe8be436599] <unknown>
>  [0x7fe8bd4a2140] g_closure_invoke
>  [0x7fe8bd4b3550] <unknown>
>  [0x7fe8bd4bb0cb] g_signal_emit_valist
>  [0x7fe8bd4bb642] g_signal_emit
>  [0x7fe8be54f95e] <unknown>
>  [0x7fe8be434a07] gtk_propagate_event
>  [0x7fe8be434c8b] gtk_main_do_event
>  [0x7fe8c9ed34a0] base::MessagePumpGtk::DispatchEvents()
>  [0x7fe8c9ed36cd] base::MessagePumpGtk::EventDispatcher()
>  [0x7fe8be0a9d7c] <unknown>
>  [0x7fe8bd1e2ab5] g_main_context_dispatch
>  [0x7fe8bd1e2de8] <unknown>
>  [0x7fe8bd1e2ea4] g_main_context_iteration
>  [0x7fe8c9ed2770] base::MessagePumpGlib::RunWithDispatcher()
>  [0x7fe8c9ed2c58] base::MessagePumpGlib::Run()
>  [0x7fe8c9f66916] base::MessageLoop::RunInternal()
>  [0x7fe8c9f66834] base::MessageLoop::RunHandler()
>  [0x7fe8c9fa87f0] base::RunLoop::Run()
>  [0x7fe8ccc24a0e] ChromeBrowserMainParts::MainMessageLoopRun()
>  [0x7fe8c4639954] content::BrowserMainLoop::RunMainMessageLoopParts()
>  [0x7fe8c4641876] content::BrowserMainRunnerImpl::Run()
>  [0x7fe8c463575f] content::BrowserMain()
>  [0x7fe8c460177c] content::RunNamedProcessTypeMain()
>  [0x7fe8c4602abd] content::ContentMainRunnerImpl::Run()
>  [0x7fe8c4600b0b] content::ContentMain()
>  [0x7fe8cc501b65] ChromeMain
>  [0x7fe8cc501b25] main
>  [0x7fe8bbb0376d] __libc_start_main
>  [0x7fe8cc5019f9] <unknown>
> 
> 
> Could you please help me?
> Thanks a lot

[riadh.chtara, 2014-01-26 08:20:52]

Hi Vaclav,
> I'm sorry for a late response. It's been busy in the office lately.
> 
I'm busy too,  So I understand what you've been through.


> I think the error log above is the key one: Chrome attempts to find the
password
> to delete, but does not see it in the database.
> 
> I suggest you inspect (in debugger or via logging) the value of the |form|
> argument of NativeBackendGnome::RemoveLogin, and compare it both with what it
> should be deleting and what you have in the keychain (run seahorse to see
that).
> I suspect the right information about the form might have gone lost during all
> that passing along from the renderer.
> 
Great, I will try to check what happened to the form.
Thanks a lot Vaclav
Have a nice weekend
Cheers
Riadh

[riadh.chtara, 2014-01-27 22:55:00]

Hi Vaclav,
The bug was solved and the logins can now be removed.
But there still another issue: after refreshing the page the popup is working
well. However the popup needs to be updated to remove the deleted username even
before the page refresh.
The best solution I was able to find to solve this is to implement a new method
in the AutofillPopupControllerImpl class that allows removing on item from the
popup and call it in AutofillExternalDelegate::RemoveSuggestion through
AutofillManagerDelegate
But I thought there is maybe other ways to do it.
Could you please tell me if there are other ways for doing this?
Thanks a lot

Cheers
Riadh Chtara

[vabr (Chromium), 2014-01-29 16:09:35]

Hi Riadh!

Great to hear it works already. Also kudos to you for not forgetting to also
update the password bubble (ManagePasswordsBubbleUIController)!

To answer your question about how to update the suggestion popup: I thought that
already happens on hitting Shift+Delete. Does that code path get replaced by the
way you handle the case of password suggestions? In any case,
AutofillPopupControllerImpl::RemoveSelectedLine() does exactly this, so at least
you can hopefully reuse it.

I left a couple of comments in the code again. In particular, I'm still not sure
why you need to modify the general OnQuery handling (sorry for being vague here,
just see the comments, please).

Thanks,
Vaclav

https://codereview.chromium.org/133893004/diff/860001/chrome/browser/password...
File chrome/browser/password_manager/password_store.h (right):

https://codereview.chromium.org/133893004/diff/860001/chrome/browser/password...
chrome/browser/password_manager/password_store.h:24: namespace autofill { struct
PasswordForm; }
Please revert this formatting change, to decrease the number of affected files.

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
File components/autofill/content/browser/autofill_driver_impl.cc (left):

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl.cc:4: 
Please return those two blank lines.

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
File components/autofill/content/browser/autofill_driver_impl.cc (right):

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl.cc:148: 
Please remove this blank line.

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:557:
DCHECK(password_form);
Is QueryAutofillSuggestions also called for non-password web forms? In that
case, the code should handle password_form==NULL.

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:573: Send(new
AutofillHostMsg_QueryFormFieldAutofill(routing_id(),
Do you actually need to include the password form here, when you also send add
it to the AutofillHostMsg_ShowPasswordSuggestions message it in the
PasswordAutofillAgent::ShowSuggestionPopup ?

https://codereview.chromium.org/133893004/diff/860001/components/autofill/cor...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/133893004/diff/860001/components/autofill/cor...
components/autofill/core/browser/autofill_driver.h:11: #include
"components/autofill/core/common/password_form.h"
You should not need to #include password_form.h for just declaring a "const
PasswordForm&" argument. A forward declaration should be enough.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Forward_Declar...

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

https://codereview.chromium.org/133893004/diff/860001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:43:
//autofill_query_password_form_ = password_form;
If you don't need this line, then please remove it. Also remove the
password_form argument, if it's not used.

https://codereview.chromium.org/133893004/diff/860001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:208:
autofill_query_password_form_.username_value = value;
It looks like you mix the |value| with the rest of
|autofill_query_password_form_|. Are you sure that's right?

Can it happen, that |autofill_query_password_form_| will contain the credentials
pair ("Joe User", "secret password"), and then this line replaces "Joe User"
with "Ann User", but keeps the "secret password"?

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

https://codereview.chromium.org/133893004/diff/860001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:29: #include
"components/autofill/core/common/password_form.h"
If possible, use forward reference instead of #include. Since you only use a
reference for PasswordForm below, that should be possible.

[riadh.chtara, 2014-01-29 23:01:38]

Hi Vacalv,

> Great to hear it works already. Also kudos to you for not forgetting to also
> update the password bubble (ManagePasswordsBubbleUIController)!
Thanks a lot :)

> To answer your question about how to update the suggestion popup: I thought
that
> already happens on hitting Shift+Delete. Does that code path get replaced by
the
> way you handle the case of password suggestions? In any case,
> AutofillPopupControllerImpl::RemoveSelectedLine() does exactly this, so at
least
> you can hopefully reuse it.
Great, I will try it.

> I left a couple of comments in the code again. In particular, I'm still not
sure
> why you need to modify the general OnQuery handling (sorry for being vague
here,
> just see the comments, please).
OK, I will see what I can do.
For the OnQuery changes, You're right. I will removed them after I fix the popup
bug.
I'm unfortunately busy right now, I will to answer the comments and work on the
bug as soon as I can.
Thanks a lot
Cheers
Riadh

[riadh.chtara, 2014-02-04 16:22:02]

Hey Vaclav,
Thanks a lot for you comments
By the way :Deletion is now working :)

https://codereview.chromium.org/133893004/diff/860001/chrome/browser/password...
File chrome/browser/password_manager/password_store.h (right):

https://codereview.chromium.org/133893004/diff/860001/chrome/browser/password...
chrome/browser/password_manager/password_store.h:24: namespace autofill { struct
PasswordForm; }
On 2014/01/29 16:09:36, vabr (Chromium) wrote:
> Please revert this formatting change, to decrease the number of affected
files.

Done.

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
File components/autofill/content/browser/autofill_driver_impl.cc (right):

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl.cc:148: 
On 2014/01/29 16:09:36, vabr (Chromium) wrote:
> Please remove this blank line.

Done.

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:557:
DCHECK(password_form);
I'm going to remove it.

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:573: Send(new
AutofillHostMsg_QueryFormFieldAutofill(routing_id(),
OK, I will remove it.

https://codereview.chromium.org/133893004/diff/860001/components/autofill/cor...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/133893004/diff/860001/components/autofill/cor...
components/autofill/core/browser/autofill_driver.h:11: #include
"components/autofill/core/common/password_form.h"
On 2014/01/29 16:09:36, vabr (Chromium) wrote:
> You should not need to #include password_form.h for just declaring a "const
> PasswordForm&" argument. A forward declaration should be enough.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Forward_Declar...

Done.

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

https://codereview.chromium.org/133893004/diff/860001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:43:
//autofill_query_password_form_ = password_form;
On 2014/01/29 16:09:36, vabr (Chromium) wrote:
> If you don't need this line, then please remove it. Also remove the
> password_form argument, if it's not used.

Done.

https://codereview.chromium.org/133893004/diff/860001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:208:
autofill_query_password_form_.username_value = value;
This is the line that solved the issue which was:
the |username_value|  of  |autofill_query_password_form_| is the actual value in
the form. So if you have two users : Joe and JoeUser. And you put in the user
input field Jo, |username_value| of |autofill_query_password_form_| is going to
be Jo.
The popup is going to be shown with two values. And if you select JoeUser form
the popup without validating your choice (by pressing enter), |value| is going
to be Joe.
Before, this line was added, we try to remove the saved login that have Jo as
its username and this why I got login not found issue.
Now if we add this line the problem is solved.
For the value of the password I don't have a problem with it.
Even if we have many credentials with different passwords, and we have also a
different password in the password field, every thing works fine.
I'm not sure how removing credential happens but I guess when deleting a
credential:
* it checks whether or not a completely match exists: with the same password and
user name, otherwise it checks if there is a partial match (only username is
matching).
or
* it just checks for the username and not the password, because it's not
possible to have two credentials with same user name and different passwords. 
But it works fine.

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

https://codereview.chromium.org/133893004/diff/860001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:29: #include
"components/autofill/core/common/password_form.h"
On 2014/01/29 16:09:36, vabr (Chromium) wrote:
> If possible, use forward reference instead of #include. Since you only use a
> reference for PasswordForm below, that should be possible.

Done.

[vabr (Chromium), 2014-02-05 20:34:19]

Hi Riadh,

Thanks for your update, I'm really glad to hear about your success!

I left a couple of comments below. You already slimmed the Cl down, and we are
going to lose a couple of more files, that's a great sign that the CL is getting
ready.

I drew a diagram to see what happens in your new code, and I have a bit trouble
to understand.
There is a communication between the browser process and the renderer process:
1. In the browser process, AutofillExternalDelegate gets notified that it should
RemoveSuggestion.
2. The delegate uses the PasswordAutofillManager and the AutofillDriver (still
in the browser process) to inform the PasswordAutofillAgent, in the renderer
process.
3. The PasswordAutofillAgent does something which I don't understand, and then
sends a message back to the browser process, to PasswordManager to perform the
actual deletion.

My question is: What is PasswordAutofillAgent doing before forwarding to
PasswordManager? Couldn't we just skip PasswordAutofillAgent completely, and
call the PasswordManager directly from the AutofillExternalDelegate? That would
spare the IPC round trip.


Finally, two notes:
A. This week, blundell@, dubroy@, gcasto@ and me are refactoring some password
manager code. Files are moving and classes are being split. Be aware of that, if
you rebase. You could get rebasing conflicts, although it's unlikely they would
be puzzling.
B. I'm sure you are still aware we will also need some tests for your fix, but
pointing that out just to be sure.


Thanks for the great work so far, Riadh. I'm really impressed by your early
start on Chromium, and I'm really looking forward to having you as a member of
our team of Munich soon!

Vaclav

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
File components/autofill/content/browser/autofill_driver_impl.cc (left):

https://codereview.chromium.org/133893004/diff/860001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl.cc:4: 
On 2014/01/29 16:09:36, vabr (Chromium) wrote:
> Please return those two blank lines.

I believe you forgot to address this.

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

https://codereview.chromium.org/133893004/diff/860001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:208:
autofill_query_password_form_.username_value = value;
Thanks for explaining the problem (with Jo insetad of Joe being in the form
submitted for deletion)!

But instead of modifying autofill_query_password_form_, could you please create
a local copy of autofill_query_password_form_ in this code block, change the
value for that (please add a comment explaining you are replacing the user-typed
prefix with the full name from the suggestion), and pass the local copy to
RemovePasswordSuggestion?

My concern is that if you modify autofill_query_password_form_, and the user
hits Submit without selecting anything, they could possibly store the password
for Joe under the username Jo.

On 2014/02/04 16:22:03, riadh.chtara wrote:
> This is the line that solved the issue which was:
> the |username_value|  of  |autofill_query_password_form_| is the actual value
in
> the form. So if you have two users : Joe and JoeUser. And you put in the user
> input field Jo, |username_value| of |autofill_query_password_form_| is going
to
> be Jo.
> The popup is going to be shown with two values. And if you select JoeUser form
> the popup without validating your choice (by pressing enter), |value| is going
> to be Joe.
> Before, this line was added, we try to remove the saved login that have Jo as
> its username and this why I got login not found issue.
> Now if we add this line the problem is solved.
> For the value of the password I don't have a problem with it.
> Even if we have many credentials with different passwords, and we have also a
> different password in the password field, every thing works fine.
> I'm not sure how removing credential happens but I guess when deleting a
> credential:
> * it checks whether or not a completely match exists: with the same password
and
> user name, otherwise it checks if there is a partial match (only username is
> matching).
> or
> * it just checks for the username and not the password, because it's not
> possible to have two credentials with same user name and different passwords. 
> But it works fine.

https://codereview.chromium.org/133893004/diff/880001/chrome/browser/password...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/133893004/diff/880001/chrome/browser/password...
chrome/browser/password_manager/password_manager.cc:363:
Profile::EXPLICIT_ACCESS);
nit: Add a comment that we use EXPLICIT_ACCESS because removing passwords is
allowed in Incognito profiles.

https://codereview.chromium.org/133893004/diff/880001/chrome/browser/password...
File chrome/browser/password_manager/password_store.h (right):

https://codereview.chromium.org/133893004/diff/880001/chrome/browser/password...
chrome/browser/password_manager/password_store.h:25: struct PasswordForm;
Don't add indentation inside namespace:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
File components/autofill/content/browser/autofill_driver_impl.cc (left):

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/browser/autofill_driver_impl.cc:4: 
Do not remove these two blank lines.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:17: #include
"components/autofill/content/renderer/password_form_conversion_utils.h"
Please remove the #include, because there are no other changes in this file.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:565: // We own
the FormElements* in forms.
nit: Add the vertical bars around forms: |forms|, to avoid confusion with the
general meaning of the word.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:565: // We own
the FormElements* in forms.
Actually, could you please remove the comment? It only seemed needed because of
the non-obvious use of scoped_ptr |form_elements| below, which I would like to
eliminated (see below).

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:567:
FindFormElements(render_view()->GetWebView(), password_form.form_data, &forms);
This line, and a couple of lines below, seem to be longer than 80 characters,
please reformat.

(Did the presubmit check not warn you?)

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:570:
scoped_ptr<FormElements> form_elements(*iter);
Please don't do this. If you leave the for cycle earlier (and you will, through
the break statement), the rest of the FormElements will never get freed.

Instead leave |forms| as is in this cycle, and add
STLDeleteElements(forms);
after the for cycle.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:575: if
(login_to_password_info_.find(username_element) !=
Please prefer ContainsKey from base/stl_util.h to the find != end pattern, for
readability of the code.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:575: if
(login_to_password_info_.find(username_element) !=
nit: Instead of doing

for (...) {
  if (condition) {
    // many lines
  }
}

do

for (...) {
  if (!condition)
    continue;
  // many lines
}

This will indent the "//many lines" part to the left, and decrease the level of
nesting, which is better for readability.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:579:
password_form));
Please indent correctly.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:581:
scoped_ptr<PasswordForm>
password_form(CreatePasswordForm(username_element.form()));
Also, note that now you have a name collision: the function argument
|password_form| and the local scoped_ptr |password_form|. Please avoid that.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:584: Send(new
AutofillHostMsg_PasswordFormsParsed(routing_id(), password_forms));
I'm not sure I follow you here: Why are you sending the PasswordFormsParsed
message?

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

https://codereview.chromium.org/133893004/diff/880001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:615: const PasswordForm&
password_form,
Please revert all changes in this file and it's *.h file (also noted in the *.h
file).

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

https://codereview.chromium.org/133893004/diff/880001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:29: #include
"components/autofill/core/common/password_form.h"
I believe you acn also revert all changes in this file, and it's *.cc file (also
noted there), because that code is not involved with the rest.

[riadh.chtara, 2014-02-06 08:45:51]

Hi Vaclav,
> Thanks for your update, I'm really glad to hear about your success!
> 
Thanks  a lot for your support :)
> I left a couple of comments below. You already slimmed the Cl down, and we are
> going to lose a couple of more files, that's a great sign that the CL is
getting
> ready.
> 
> I drew a diagram to see what happens in your new code, and I have a bit
trouble
> to understand.
> There is a communication between the browser process and the renderer process:
> 1. In the browser process, AutofillExternalDelegate gets notified that it
should
> RemoveSuggestion.
> 2. The delegate uses the PasswordAutofillManager and the AutofillDriver (still
> in the browser process) to inform the PasswordAutofillAgent, in the renderer
> process.
> 3. The PasswordAutofillAgent does something which I don't understand, and then
> sends a message back to the browser process, to PasswordManager to perform the
> actual deletion.
> 
You're diagram is right.
> My question is: What is PasswordAutofillAgent doing before forwarding to
> PasswordManager? Couldn't we just skip PasswordAutofillAgent completely, and
> call the PasswordManager directly from the AutofillExternalDelegate? That
would
> spare the IPC round trip.
> 

In my patch 7, I did almost like that:the only difference is  I just send a msg 
to the render containing the PasswordForm and then forward that msg to the 
password manger. And I did nothing in the renderer.
And like I said, it worked almost  fine, the only problem with it is that if 
you delete a login, the username disappears from the popup but if you type 
cleaned the username field you are going to see the deleted user name
again in the popup.
It's because the renderer has a copy of the autofill data in
login_to_password_info_.
And it uses the copy to show the popup.
This copy is not updated automatically, this is why the renderer needs to be
informed.
about the change.
So the code I written was to update login_to_password_info_.
I  found that the easiest way to do that is to send a 
Send(new AutofillHostMsg_PasswordFormsParsed(routing_id(), password_forms));
But before that I need to deleted the login_to_password_info_ of the
username_element,
because, otherwise login_to_password_info_ 's not going to be updated.
So the code' purpose is to find the username_element from the password_form
> Finally, two notes:
> A. This week, blundell@, dubroy@, gcasto@ and me are refactoring some password
> manager code. Files are moving and classes are being split. Be aware of that,
if
> you rebase. You could get rebasing conflicts, although it's unlikely they
would
> be puzzling.
OK, thanks for t
> B. I'm sure you are still aware we will also need some tests for your fix, but
> pointing that out just to be sure.
Yeah, I didn't forget that 
I need to check the article in the chromium website about that.
(It will take me same time before I finish answering about your comments, and
write
the tests so sorry about that.
> 
> 
> Thanks for the great work so far, Riadh. I'm really impressed by your early
> start on Chromium, and I'm really looking forward to having you as a member of
> our team of Munich soon!
> 

I'm also so excited for that :)
Thanks a lot  Vaclav
Riadh

[vabr (Chromium), 2014-02-11 17:58:35]

Hi Riadh,

Thanks for your explanation of the current code in password_autofill_agent.cc.

It still looks like you don't need the second IPC, from the renderer back to
browser:
could you instead, at the time when AutofillExternalDelegate uses the
PasswordAutofillManager and the AutofillDriver to call the renderer, also make
it so that AutofillExternalDelegate calls (by a method, not IPC) PasswordManager
to delete the form?

One more thing is: looking at PasswordAutofillAgent::OnRemovePasswordSuggestion,
it seems like PasswordForms get created quite often. Maybe there are some
redundancies, but it's hard to tell now, as the code already has a lot of
comments to be addressed. I'll take a look in the next round, but you might want
to focus on simplifying that as well.

Thanks, and sorry for the delayed response (I figured you are not blocked by me
at the moment, and was too busy travelling). If you find yourself blocked by me,
feel free to "ping" (=urge by e-mail) me. :)

Cheers,
Vaclav

On 2014/02/06 08:45:51, riadh.chtara wrote:
> Hi Vaclav,
> > Thanks for your update, I'm really glad to hear about your success!
> > 
> Thanks  a lot for your support :)
> > I left a couple of comments below. You already slimmed the Cl down, and we
are
> > going to lose a couple of more files, that's a great sign that the CL is
> getting
> > ready.
> > 
> > I drew a diagram to see what happens in your new code, and I have a bit
> trouble
> > to understand.
> > There is a communication between the browser process and the renderer
process:
> > 1. In the browser process, AutofillExternalDelegate gets notified that it
> should
> > RemoveSuggestion.
> > 2. The delegate uses the PasswordAutofillManager and the AutofillDriver
(still
> > in the browser process) to inform the PasswordAutofillAgent, in the renderer
> > process.
> > 3. The PasswordAutofillAgent does something which I don't understand, and
then
> > sends a message back to the browser process, to PasswordManager to perform
the
> > actual deletion.
> > 
> You're diagram is right.
> > My question is: What is PasswordAutofillAgent doing before forwarding to
> > PasswordManager? Couldn't we just skip PasswordAutofillAgent completely, and
> > call the PasswordManager directly from the AutofillExternalDelegate? That
> would
> > spare the IPC round trip.
> > 
> 
> In my patch 7, I did almost like that:the only difference is  I just send a
msg 
> to the render containing the PasswordForm and then forward that msg to the 
> password manger. And I did nothing in the renderer.
> And like I said, it worked almost  fine, the only problem with it is that if 
> you delete a login, the username disappears from the popup but if you type 
> cleaned the username field you are going to see the deleted user name
> again in the popup.
> It's because the renderer has a copy of the autofill data in
> login_to_password_info_.
> And it uses the copy to show the popup.
> This copy is not updated automatically, this is why the renderer needs to be
> informed.
> about the change.
> So the code I written was to update login_to_password_info_.
> I  found that the easiest way to do that is to send a 
> Send(new AutofillHostMsg_PasswordFormsParsed(routing_id(), password_forms));
> But before that I need to deleted the login_to_password_info_ of the
> username_element,
> because, otherwise login_to_password_info_ 's not going to be updated.
> So the code' purpose is to find the username_element from the password_form
> > Finally, two notes:
> > A. This week, blundell@, dubroy@, gcasto@ and me are refactoring some
password
> > manager code. Files are moving and classes are being split. Be aware of
that,
> if
> > you rebase. You could get rebasing conflicts, although it's unlikely they
> would
> > be puzzling.
> OK, thanks for t
> > B. I'm sure you are still aware we will also need some tests for your fix,
but
> > pointing that out just to be sure.
> Yeah, I didn't forget that 
> I need to check the article in the chromium website about that.
> (It will take me same time before I finish answering about your comments, and
> write
> the tests so sorry about that.
> > 
> > 
> > Thanks for the great work so far, Riadh. I'm really impressed by your early
> > start on Chromium, and I'm really looking forward to having you as a member
of
> > our team of Munich soon!
> > 
> 
> I'm also so excited for that :)
> Thanks a lot  Vaclav
> Riadh

[riadh.chtara, 2014-02-11 21:03:45]

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:565: // We own
the FormElements* in forms.
On 2014/02/05 20:34:20, vabr (Chromium) wrote:
> nit: Add the vertical bars around forms: |forms|, to avoid confusion with the
> general meaning of the word.

Done.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:565: // We own
the FormElements* in forms.
On 2014/02/05 20:34:20, vabr (Chromium) wrote:
> nit: Add the vertical bars around forms: |forms|, to avoid confusion with the
> general meaning of the word.

Done.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:565: // We own
the FormElements* in forms.
On 2014/02/05 20:34:20, vabr (Chromium) wrote:
> Actually, could you please remove the comment? It only seemed needed because
of
> the non-obvious use of scoped_ptr |form_elements| below, which I would like to
> eliminated (see below).

Done.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:567:
FindFormElements(render_view()->GetWebView(), password_form.form_data, &forms);
On 2014/02/05 20:34:20, vabr (Chromium) wrote:
> This line, and a couple of lines below, seem to be longer than 80 characters,
> please reformat.
> 
> (Did the presubmit check not warn you?)

Done.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:575: if
(login_to_password_info_.find(username_element) !=
On 2014/02/05 20:34:20, vabr (Chromium) wrote:
> Please prefer ContainsKey from base/stl_util.h to the find != end pattern, for
> readability of the code.

Done.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:575: if
(login_to_password_info_.find(username_element) !=
On 2014/02/05 20:34:20, vabr (Chromium) wrote:
> nit: Instead of doing
> 
> for (...) {
>   if (condition) {
>     // many lines
>   }
> }
> 
> do
> 
> for (...) {
>   if (!condition)
>     continue;
>   // many lines
> }
> 
> This will indent the "//many lines" part to the left, and decrease the level
of
> nesting, which is better for readability.

Done.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:579:
password_form));
On 2014/02/05 20:34:20, vabr (Chromium) wrote:
> Please indent correctly.

Done.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:581:
scoped_ptr<PasswordForm>
password_form(CreatePasswordForm(username_element.form()));
On 2014/02/05 20:34:20, vabr (Chromium) wrote:
> Also, note that now you have a name collision: the function argument
> |password_form| and the local scoped_ptr |password_form|. Please avoid that.

Done.

https://codereview.chromium.org/133893004/diff/880001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:584: Send(new
AutofillHostMsg_PasswordFormsParsed(routing_id(), password_forms));
|AutofillHostMsg_PasswordFormsParsed| is sent so that every thing related to the
|password_forms| is reinitialized

[riadh.chtara, 2014-02-11 21:12:21]

Hi Vaclav,
Thanks a lot for your comments.
> Thanks for your explanation of the current code in password_autofill_agent.cc.
> 
> It still looks like you don't need the second IPC, from the renderer back to
> browser:
> could you instead, at the time when AutofillExternalDelegate uses the
> PasswordAutofillManager and the AutofillDriver to call the renderer, also make
> it so that AutofillExternalDelegate calls (by a method, not IPC)
PasswordManager
> to delete the form?
Yes you're right. I have just finished doing that.

> One more thing is: looking at
PasswordAutofillAgent::OnRemovePasswordSuggestion,
> it seems like PasswordForms get created quite often. Maybe there are some
> redundancies, but it's hard to tell now, as the code already has a lot of
> comments to be addressed. I'll take a look in the next round, but you might
want
> to focus on simplifying that as well.
> 
I will try to figure out that.
I'm working on the tests right now.
And I will update the comments also soon.

> Thanks, and sorry for the delayed response (I figured you are not blocked by
me
> at the moment, and was too busy travelling). If you find yourself blocked by
me,
> feel free to "ping" (=urge by e-mail) me. :)
No problem. Take you time. 
It's not urgent :)
Cheers
Riadh

[vabr (Chromium), 2014-02-14 11:57:37]

Thanks, Riadh.

I'll wait with a new review until you have a chance to address all of the
outstanding comments.

Thanks, and best regards,
Vaclav

[riadh.chtara, 2014-02-16 15:54:35]

wHi Vaclav,
Great.
I have discovered two issues with my code.
*If there is only one form in the page everything works fine. 
*If we open a page with a form that is similar but not exactly identical to a
form for which we already store a login. There is an issue:
When trying to remove the login, the login is only removed from the ui, and this
msg is logged:

[21544:21591:0216/155334:WARNING:native_backend_gnome_x.cc(560)] Keyring delete
failed: No matching results

When refreshing the page, the login reappears:=> it's not deleted because in
fact the login we are trying to delete doesn't exist.
But there is a similar login that keeps showing. 
So we need to find the similar login and deleted (this what happens in firefox
where I did the same test with the same pages, and the lgoin was deleted)
To delete the login, I think we need to keep a mulimap in the PasswordStore
(mapping our current page PasswordForm to similar  PassowordForms forms we
found).
The multimap is populated in PostConsumerCallback.
When we want to delete the password, we need to use the multimap, to get the the
similar form that needs to be deleted.
* If the page have many forms which are the same, deleting from any of them
works, but the deleted value only disappears form the one it was deleted from.
After refreshing every thing is fine again.
So we need to refresh all the passworform in the page, instead of refreshing
only the current passwordform.
I think it's probably possible to solve this problem by calling of
PasswordFormsParsed in the passwordmanager but until I'm failing to do that.
If you have ideas about how to solve these problems please let me know
Thanks a lot
Best Regards
Riadh Chata

[vabr (Chromium), 2014-02-17 16:10:41]

Hi Riadh,

Thanks for the thorough testing!
Please find my answers in-line.

On 2014/02/16 15:54:35, riadh.chtara wrote:
> wHi Vaclav,
> Great.
> I have discovered two issues with my code.
> *If there is only one form in the page everything works fine. 
> *If we open a page with a form that is similar but not exactly identical to a
> form for which we already store a login. There is an issue:
> When trying to remove the login, the login is only removed from the ui, and
this
> msg is logged:
> 
> [21544:21591:0216/155334:WARNING:native_backend_gnome_x.cc(560)] Keyring
delete
> failed: No matching results
> 
> When refreshing the page, the login reappears:=> it's not deleted because in
> fact the login we are trying to delete doesn't exist.

As you observed, the problem might be in the way you create the PasswordForm to
pass to the password manager for deletion.
It's hard for me to judge what happens, because all I see is your 2 weeks old
patch.
But you can try to figure out, what part of the PasswordForm fails to match when
the keyring is checked, and work back from there to fix that.

> But there is a similar login that keeps showing. 
> So we need to find the similar login and deleted (this what happens in firefox
> where I did the same test with the same pages, and the lgoin was deleted)
> To delete the login, I think we need to keep a mulimap in the PasswordStore
> (mapping our current page PasswordForm to similar  PassowordForms forms we
> found).
> The multimap is populated in PostConsumerCallback.
> When we want to delete the password, we need to use the multimap, to get the
the
> similar form that needs to be deleted.

I am not sure we need to have the multimap. The difference must be something
predictable (possibly a field empty when it should not be or so), so the right
path to me seems to be to create a PasswordForm identifying the one in the
keyring correctly.

> * If the page have many forms which are the same, deleting from any of them
> works, but the deleted value only disappears form the one it was deleted from.
> After refreshing every thing is fine again.
> So we need to refresh all the passworform in the page, instead of refreshing
> only the current passwordform.
> I think it's probably possible to solve this problem by calling of
> PasswordFormsParsed in the passwordmanager but until I'm failing to do that.

This looks like a minor usability problem: first, not that often do we autofill
passwords in multiple forms. Second, if page reload fixes it, and we handle
gracefully when the user deletes the same password from multiple forms (the
problem being the lack of corresponding entries in the password database after
the first deletion), then it might be OK. I'm not currently sure what would be
the best way to refresh all the password forms on a page. Calling
PasswordFormsParsed sounds a bit confusing, because they in fact have not been
parsed. But I believe you are right in the sense that what you need here is to
iterate over all active PasswordFormManager instances (in
pending_login_managers_), and call FetchMatchingLoginsFromPasswordStore on them,
as happens in OnPasswordFormsParsed. What you should not need to do is actually
to recreate the PasswordFormManagers.

Thanks and best regards,
Vaclav

> If you have ideas about how to solve these problems please let me know
> Thanks a lot
> Best Regards
> Riadh Chata

[riadh.chtara, 2014-02-19 10:48:19]

Hi Vaclav,
Sorry for my late response, I was really busy this days
Thanks a lot for your suggestions.
The first problem was solved, I need a bit more time to clean my code.
I think figuring out the changes that need to be made to the PasswordForm to get
a match is going to be complex if we do it by ourself.
So the easiest way for doing that is using FetchMatchingLoginsFromPasswordStore.
In my previous version, after deleting we are going to do a
FetchMatchingLoginsFromPasswordStore. So, I thought it's probably better to do
FetchMatchingLoginsFromPasswordStore before removing the password, so we get a
PasswordForms map of all the matches.
And then instead of sending this map directly to do PasswordManager::Autofill,
we get the password we want to remove, we remove it form the store, we remove it
from the map and then we send the map to the PasswordManager::Autofill.
I will sent you the code as soon as possible.

* for the second issue: I totally agree with you: for the usability problem.
I will a bit try to solved.
But if I figure out this may need a lot of time. It probably doesn't worth it.

Thanks a lot again for all the suggestions
Best Regards
Riadh Chtara

[vabr (Chromium), 2014-02-20 13:02:52]

Thanks, Riadh.

I don't think I understand completely what you meant in your last e-mail, but I
think it's best if we both wait until you have the updated code, and speak about
the changes over the up-to-date code.

And really don't worry, there is no rush. Your internship has not even started
yet. :)

Cheers,
Vaclav

[riadh.chtara, 2014-02-21 17:42:36]

Hi,
So this the new code, handling similar forms  works fine.
(I didn't have time to clean all the mess I made,  sorry about, I will try
do that in the near future)

Even  handling many forms works fine, but to do that I was obliged to send
a msg that contains the list of every form that needs to be updated from
the password_autofill_agent to the password_manger.

The password_form_manger are no longer deleted and recreated:· they are
just updated.
OnPasswordFormsParsed is not longer called.
When there  multiple forms in the page with many saved logins, this what
happens:
*if you delete a login that is not selected: it's deleted from all the
others
*if you delete a login that is selected: it's deleted from the current form
and all other forms where the deleted login is not selected.
however, for all the forms where the deleted form is selected, the username
of the deleted login remains in the username field and the password of the
deleted login remains in password field. But even for this forms, the
username of the deleted login never reappears in the popup.

I took some screen shot to better explain

1) adding new login (a)


2) adding new login (b)


3) adding new login (c)



after adding the logins



removing c



removing a



after removing a



when changing the content of any username field only b is there



PS : my internship is going to start after 40 very long days.

Thanks a lot
Best Regards
Riadh

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vabr (Chromium), 2014-03-07 23:43:35]

Hi Riadh,

I'm really sorry it took me so long to respond at all. Unfortunately I had not
time to review the whole CL yet. I'm hopeful that this goes in the right
direction, but I'll have to think through properly the way it works.

I left a couple of comments, nevertheless. Also -- do you know why the empty
tools/gdb/gdb_chrome.py appears in your CL?

I'm on holiday next week (that's part of the reason, why I had been busy with
other work until now), so I'm sorry to report further delays on my side :(.

But in the meantime, you can take a look at merging your CL to the current tip
of the tree. Beware, that a lot of password manager-related classes moved into
the components/ directory. That means not only a change of path, but also rather
intensive refactoring to keep chrome/* includes out of those files. That will
probably heavily affect your CL. Things like sending IPC messages are no longer
available in components/password_manager. The way to do these things is to
implement them in the ChromePasswordManagerClient, or
ContentPasswordManagerDriver. The latter is probably preferred, as that one
abstracts operations dependent on content/ (like using RenderViewHost).

Once you merge your CL to the current tree, you might go over it and
sanity-check it. Among my comments there is some amount of issues like
accidentally removed punctuation etc., which you can filter as well.

After I'm back, I will try to locally compile your CL, and check the remaining
problems you described last time, if there are any left at that point.

Thanks!
Vaclav

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
File chrome/browser/password_manager/password_form_manager.cc (right):

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_form_manager.cc:113: void
PasswordFormManager::RemoveAndUpdate(const string16& username_to_remove) {
This is duplicated code: RemoveAndUpdate and Update should be one function, with
an argument specifying the future value of |only_update_|.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_form_manager.cc:285: PasswordForm f =
observed_form_;
Could you please explain to me why do we need to delete the username for
GetLogins?

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
File chrome/browser/password_manager/password_form_manager.h (right):

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_form_manager.h:100: void
OnRequestDone(const std::vector<autofill::PasswordForm*>& result);
nit: please return the blank line below

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_form_manager.h:273: bool remover_;
What about getting rid of |remover_|, and using username_to_remove_ emptiness to
mean remover_ == false?
Also, I suggest renaming |only_update_| to |remove_candidate_from_store_| (this
would negate the meaning, so code needs to be changed after the rename).
Plus, there needs to be a comment explaining when these two variables are used.

In any case, the names need to change a bit: A variable of type bool is a yes/no
piece of information. A variable called |remover_| sounds like a whole object
doing some removing.

The name |only_update_| does not specify what is to be updated and when.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.cc:324: //
AutofillHostMsg_PasswordFormsParsed is going to be recived in the near
typos: recived, genrate

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.cc:326: // PasswordFormManager
Please end sentences with a full stop.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.cc:333: iter++) {
nit: prefer prefix increment ++iter to postfix iter++
Prefix is consistently used in this file. Also, the value of the expression
iter++ (which is iter before increment) is not used, so it does not make sense
to produce it.

Also below.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.cc:349: for
(std::vector<PasswordFormManager*>::iterator pfm =
Please do not use abbreviations like 'pfm' or 'pf'. That looks cryptic (also,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...),
and once it's decrypted, brings no interesting information. Use |iter| for
general iterators, as that looks consistent with the file. Ideally, use more
descriptive name, like |form_manager_update_candidate|.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.cc:360: pf =
updated_password_forms.erase(pf);
Single element deleting in vector is most of the time wasteful: every deletion
requires moving the element in the tail of the vector. Unless we know the vector
being very small (like <4 elements) for the very majority of cases, I would
suggest a different approach.

In this case, I don't really understand, why you delete some of the elements in
the for loop, while the rest will get deleted just a couple of lines below, at
the function end.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.cc:361: 
nit: remove blank line

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager.h (left):

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.h:152: // and stored in this
collection until user navigates away from page.
Please return the removed full-stop at the end of line.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
File chrome/browser/password_manager/password_store.cc (right):

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_store.cc:42: const PasswordForm& form,
Please indicate, what is the form needed for. Ideally using the variable name,
or at least a comment.

https://codereview.chromium.org/133893004/diff/1290001/components/autofill/co...
File components/autofill/content/browser/autofill_driver_impl.cc (right):

https://codereview.chromium.org/133893004/diff/1290001/components/autofill/co...
components/autofill/content/browser/autofill_driver_impl.cc:134: if
(!RendererIsAvailable())
You can leave this test, and check host below for being NULL instead. It's the
very same test.

https://codereview.chromium.org/133893004/diff/1290001/components/autofill/co...
File components/autofill/core/common/autofill_messages.h (right):

https://codereview.chromium.org/133893004/diff/1290001/components/autofill/co...
components/autofill/core/common/autofill_messages.h:164: // Instruct the
renderer that a password mapping has to be removed.
nit: Not clear what mapping is meant. Could you elaborate?
Also below.

https://codereview.chromium.org/133893004/diff/1290001/components/autofill/co...
components/autofill/core/common/autofill_messages.h:166: autofill::PasswordForm
/* the password form */)
nit: Maybe rather "password to be removed"?
It's clear that it's a password form, but it's not entirely clear what is the
meaning of it.

https://codereview.chromium.org/133893004/diff/1290001/components/autofill/co...
components/autofill/core/common/autofill_messages.h:266: autofill::PasswordForm
/* the password form */,
Please explain the meaning of the arguments. Especially the vector of forms.

[riadh.chtara, 2014-03-08 16:43:02]

Hi Vaclav,
Thanks a lot for you answer.
Have a nice holidays :)
Mean while I will try to do what ask me to.
Cheers
Riadh Chtara

[riadh.chtara, 2014-03-14 17:16:07]

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
File chrome/browser/password_manager/password_form_manager.cc (right):

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_form_manager.cc:113: void
PasswordFormManager::RemoveAndUpdate(const string16& username_to_remove) {
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> This is duplicated code: RemoveAndUpdate and Update should be one function,
with
> an argument specifying the future value of |only_update_|.

Done.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
File chrome/browser/password_manager/password_form_manager.h (right):

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_form_manager.h:100: void
OnRequestDone(const std::vector<autofill::PasswordForm*>& result);
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> nit: please return the blank line below

Done.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_form_manager.h:273: bool remover_;
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> What about getting rid of |remover_|, and using username_to_remove_ emptiness
to
> mean remover_ == false?
> Also, I suggest renaming |only_update_| to |remove_candidate_from_store_|
(this
> would negate the meaning, so code needs to be changed after the rename).
> Plus, there needs to be a comment explaining when these two variables are
used.
> 
> In any case, the names need to change a bit: A variable of type bool is a
yes/no
> piece of information. A variable called |remover_| sounds like a whole object
> doing some removing.
> 
> The name |only_update_| does not specify what is to be updated and when.

Done.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager.cc (right):

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.cc:324: //
AutofillHostMsg_PasswordFormsParsed is going to be recived in the near
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> typos: recived, genrate

Done.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.cc:326: // PasswordFormManager
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> Please end sentences with a full stop.

Done.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.cc:333: iter++) {
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> nit: prefer prefix increment ++iter to postfix iter++
> Prefix is consistently used in this file. Also, the value of the expression
> iter++ (which is iter before increment) is not used, so it does not make sense
> to produce it.
> 
> Also below.

Done.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.cc:349: for
(std::vector<PasswordFormManager*>::iterator pfm =
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> Please do not use abbreviations like 'pfm' or 'pf'. That looks cryptic (also,
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...),
> and once it's decrypted, brings no interesting information. Use |iter| for
> general iterators, as that looks consistent with the file. Ideally, use more
> descriptive name, like |form_manager_update_candidate|.

Done.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.cc:360: pf =
updated_password_forms.erase(pf);
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> Single element deleting in vector is most of the time wasteful: every deletion
> requires moving the element in the tail of the vector. Unless we know the
vector
> being very small (like <4 elements) for the very majority of cases, I would
> suggest a different approach.
> 
> In this case, I don't really understand, why you delete some of the elements
in
> the for loop, while the rest will get deleted just a couple of lines below, at
> the function end.

Done.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.cc:361: 
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager.h (left):

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager.h:152: // and stored in this
collection until user navigates away from page.
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> Please return the removed full-stop at the end of line.

Done.

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
File chrome/browser/password_manager/password_store.cc (right):

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_store.cc:42: const PasswordForm& form,
it's not needed.
I forgot to remove it.
Sorry for that

https://codereview.chromium.org/133893004/diff/1290001/components/autofill/co...
File components/autofill/content/browser/autofill_driver_impl.cc (right):

https://codereview.chromium.org/133893004/diff/1290001/components/autofill/co...
components/autofill/content/browser/autofill_driver_impl.cc:134: if
(!RendererIsAvailable())
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> You can leave this test, and check host below for being NULL instead. It's the
> very same test.

Done.

https://codereview.chromium.org/133893004/diff/1290001/components/autofill/co...
File components/autofill/core/common/autofill_messages.h (right):

https://codereview.chromium.org/133893004/diff/1290001/components/autofill/co...
components/autofill/core/common/autofill_messages.h:164: // Instruct the
renderer that a password mapping has to be removed.
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> nit: Not clear what mapping is meant. Could you elaborate?
> Also below.

Done.

https://codereview.chromium.org/133893004/diff/1290001/components/autofill/co...
components/autofill/core/common/autofill_messages.h:166: autofill::PasswordForm
/* the password form */)
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> nit: Maybe rather "password to be removed"?
> It's clear that it's a password form, but it's not entirely clear what is the
> meaning of it.

Done.

https://codereview.chromium.org/133893004/diff/1290001/components/autofill/co...
components/autofill/core/common/autofill_messages.h:266: autofill::PasswordForm
/* the password form */,
On 2014/03/07 23:43:36, vabr (Chromium)OOO till 16Mar wrote:
> Please explain the meaning of the arguments. Especially the vector of forms.

Done.

[riadh.chtara, 2014-03-14 18:06:17]

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
File chrome/browser/password_manager/password_form_manager.cc (right):

https://codereview.chromium.org/133893004/diff/1290001/chrome/browser/passwor...
chrome/browser/password_manager/password_form_manager.cc:285: PasswordForm f =
observed_form_;
I thought that using GetLogins with a PasswordForm where the username_value is
not null will return only the login with the same username.
But I tried to remove the additional lines I wrote, and every thing was fine.
So I was wrong and in my latest patch, this problem was fixed.

[vabr (Chromium), 2014-04-01 16:56:07]

Hi Riadh,

A couple of more minor comments. I'm going to ask you some more things directly.

Cheers,
Vaclav

https://codereview.chromium.org/133893004/diff/1440025/AUTHORS
File AUTHORS (left):

https://codereview.chromium.org/133893004/diff/1440025/AUTHORS#oldcode39
AUTHORS:39: Arunprasad Rajkumar <ararunprasad@gmail.com>
Make sure you don't remove people from here.
Also, with the @chromium.org account, you should no longer need to be in the
AUTHORS list.

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/content/browser/autofill_driver_impl.cc (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/content/browser/autofill_driver_impl.cc:128:
content::RenderViewHost* host = GetWebContents()->GetRenderViewHost();
nit: for consistency with the rest of the file, consider using web_contents()
instead of GetWebContents()

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/content/common/autofill_messages.h:260: // Send also a list
of forms that needs to be updated after the deletion
needs -> need
(not the list, but the forms need to be updated)

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/content/common/autofill_messages.h:263:
std::vector<autofill::PasswordForm> /* password
password -> password forms

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:660: if
(new_password_form.get()) {
Should we also test if new_password_form is similar to password_form?

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/core/browser/autofill_driver.h:68: // Remove the
|password_form| sugesstion
missing a full-stop + there's a typo (sugesstion).

I propose this comment:
"Remove the credentials of |password_form| from suggestions."

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/core/browser/autofill_external_delegate.cc:210: value);
wrong indent

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/core/browser/autofill_external_delegate.h:39:
AutofillExternalDelegate(AutofillManager* autofill_manager,
Please undo this change (adding the "autofill_" prefixes).

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/core/browser/autofill_external_delegate.h:76: // Show
password suggestions in the popup.
It should be mentioned, what is the purpose of |password_form|.

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/core/browser/autofill_manager.h:49: class AutofillDriver;
This forward declaration looks unused, please remove it.

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/core/browser/autofill_manager.h:97: void
OnShowAutofillDialog();
Is this used?

[vabr (Chromium), 2014-04-01 17:14:34]

Hi Riadh,

Just added a comment recording what we spoke about today.

Cheers,
Vaclav

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:729:
CreatePasswordForm(user_input.form());
To record, what we just spoke about: instead of creating the PasswordForm and
sending it to the browser, only to receive it back later, maybe you could store
the WebFormElement form (which is just a small handle to a refcounted object) as
a member variable here, and create the PasswordForm from it in
OnRemovePasswordSuggestion on demand.

[rchtara, 2014-04-03 08:30:56]

Hi,
I just did what you asked me to do.
Then I wanted to check whether what i did doesnt break within a page with
multiple frames (I didnt done anything spetial to support that so I had a
negative feeling about it).
So I created this page
https://c9.io/riadh/w1/workspace/home.html
and i have checked it but apprently the password manager in chromium and even
the latest chrome version i got cannot autofill a page within mu‬ltiple frames.
So could please tell me whether or not the password manager support pages with
multiple frames ?

Thanks a lot
Riadh

[rchtara, 2014-04-03 08:44:49]

https://codereview.chromium.org/133893004/diff/1440025/AUTHORS
File AUTHORS (left):

https://codereview.chromium.org/133893004/diff/1440025/AUTHORS#oldcode39
AUTHORS:39: Arunprasad Rajkumar <ararunprasad@gmail.com>
On 2014/04/01 16:56:08, vabr (Chromium) wrote:
> Make sure you don't remove people from here.
> Also, with the @chromium.org account, you should no longer need to be in the
> AUTHORS list.

Done.

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/content/browser/autofill_driver_impl.cc (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/content/browser/autofill_driver_impl.cc:128:
content::RenderViewHost* host = GetWebContents()->GetRenderViewHost();
On 2014/04/01 16:56:08, vabr (Chromium) wrote:
> nit: for consistency with the rest of the file, consider using web_contents()
> instead of GetWebContents()

Done.

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/content/common/autofill_messages.h:260: // Send also a list
of forms that needs to be updated after the deletion
On 2014/04/01 16:56:08, vabr (Chromium) wrote:
> needs -> need
> (not the list, but the forms need to be updated)

Done.

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/content/common/autofill_messages.h:263:
std::vector<autofill::PasswordForm> /* password
On 2014/04/01 16:56:08, vabr (Chromium) wrote:
> password -> password forms

Done.

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:660: if
(new_password_form.get()) {
I'm sure but i think it better to leave that to the password manager.
Testing whether two forms are similar is not easy . And if we do that we are
going to duplicate the code that already exist in the browser.
But we going to send less message.
So I"m not sure what to do?

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:729:
CreatePasswordForm(user_input.form());
On 2014/04/01 17:14:35, vabr (Chromium) wrote:
> To record, what we just spoke about: instead of creating the PasswordForm and
> sending it to the browser, only to receive it back later, maybe you could
store
> the WebFormElement form (which is just a small handle to a refcounted object)
as
> a member variable here, and create the PasswordForm from it in
> OnRemovePasswordSuggestion on demand.

Done.

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/core/browser/autofill_driver.h:68: // Remove the
|password_form| sugesstion
On 2014/04/01 16:56:08, vabr (Chromium) wrote:
> missing a full-stop + there's a typo (sugesstion).
> 
> I propose this comment:
> "Remove the credentials of |password_form| from suggestions."

Done.

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/core/browser/autofill_external_delegate.cc:210: value);
On 2014/04/01 16:56:08, vabr (Chromium) wrote:
> wrong indent

Done.

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/core/browser/autofill_external_delegate.h:39:
AutofillExternalDelegate(AutofillManager* autofill_manager,
On 2014/04/01 16:56:08, vabr (Chromium) wrote:
> Please undo this change (adding the "autofill_" prefixes).

Done.

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/core/browser/autofill_manager.h:49: class AutofillDriver;
On 2014/04/01 16:56:08, vabr (Chromium) wrote:
> This forward declaration looks unused, please remove it.

Done.

https://codereview.chromium.org/133893004/diff/1440025/components/autofill/co...
components/autofill/core/browser/autofill_manager.h:97: void
OnShowAutofillDialog();
On 2014/04/01 16:56:08, vabr (Chromium) wrote:
> Is this used?

Done.

[vabr (Chromium), 2014-04-03 11:16:37]

On 2014/04/03 08:30:56, rchtara wrote:
> Hi,
> I just did what you asked me to do.
> Then I wanted to check whether what i did doesnt break within a page with
> multiple frames (I didnt done anything spetial to support that so I had a
> negative feeling about it).
> So I created this page
> https://c9.io/riadh/w1/workspace/home.html
> and i have checked it but apprently the password manager in chromium and even
> the latest chrome version i got cannot autofill a page within mu‬ltiple
frames.
> So could please tell me whether or not the password manager support pages with
> multiple frames ?
> 
> Thanks a lot
> Riadh

Thanks, Riadh, I'll take a look at your changes.

As for the frames: I know autofilling passwords in iframes is currently disabled
(http://crbug.com/257156), with plans to lift this restriction for same-domain
iframes (http://crbug.com/345371). I'm not completely certain whether frames and
iframes are handled the same, but I would strongly suspect so.

Cheers,
Vaclav

[vabr (Chromium), 2014-04-03 11:32:07]

On 2014/04/03 11:16:37, vabr (Chromium) wrote:
> On 2014/04/03 08:30:56, rchtara wrote:
> > Hi,
> > I just did what you asked me to do.
> > Then I wanted to check whether what i did doesnt break within a page with
> > multiple frames (I didnt done anything spetial to support that so I had a
> > negative feeling about it).
> > So I created this page
> > https://c9.io/riadh/w1/workspace/home.html
> > and i have checked it but apprently the password manager in chromium and
even
> > the latest chrome version i got cannot autofill a page within mu‬ltiple
> frames.
> > So could please tell me whether or not the password manager support pages
with
> > multiple frames ?
> > 
> > Thanks a lot
> > Riadh
> 
> Thanks, Riadh, I'll take a look at your changes.
> 
> As for the frames: I know autofilling passwords in iframes is currently
disabled
> (http://crbug.com/257156), with plans to lift this restriction for same-domain
> iframes (http://crbug.com/345371). I'm not completely certain whether frames
and
> iframes are handled the same, but I would strongly suspect so.
> 
> Cheers,
> Vaclav

Also, did you forget to upload the newest patch?

[rchtara1, 2014-04-03 12:18:46]

No, I didn't forget that.
I just wanted to update comments and tests before


On Thu, Apr 3, 2014 at 1:32 PM, <vabr@chromium.org> wrote:

> On 2014/04/03 11:16:37, vabr (Chromium) wrote:
>
>> On 2014/04/03 08:30:56, rchtara wrote:
>> > Hi,
>> > I just did what you asked me to do.
>> > Then I wanted to check whether what i did doesnt break within a page
>> with
>> > multiple frames (I didnt done anything spetial to support that so I had
>> a
>> > negative feeling about it).
>> > So I created this page
>> > https://c9.io/riadh/w1/workspace/home.html
>> > and i have checked it but apprently the password manager in chromium and
>>
> even
>
>> > the latest chrome version i got cannot autofill a page within multiple
>> frames.
>> > So could please tell me whether or not the password manager support
>> pages
>>
> with
>
>> > multiple frames ?
>> >
>> > Thanks a lot
>> > Riadh
>>
>
>  Thanks, Riadh, I'll take a look at your changes.
>>
>
>  As for the frames: I know autofilling passwords in iframes is currently
>>
> disabled
>
>> (http://crbug.com/257156), with plans to lift this restriction for
>> same-domain
>> iframes (http://crbug.com/345371). I'm not completely certain whether
>> frames
>>
> and
>
>> iframes are handled the same, but I would strongly suspect so.
>>
>
>  Cheers,
>> Vaclav
>>
>
> Also, did you forget to upload the newest patch?
>
> https://codereview.chromium.org/133893004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.