Refactor Autofill for out of process iframes (OOPIF).

Refactor Autofill for out of process iframes (OOPIF).

- AutofillAgent is a RenderFrameObserver instead of a RenderViewObserver
- legacy RenderViewObserver code is temporarily split off into LegacyAutofillAgent

- ContentAutofillDriver is one per render frame host, instead of one per WebContents
- ContentAutofillDriverFactory is one per WebContents and spawns ContentAutofillDrivers for each frame

- ContentAutofillClient remains one per WebContents. Thus there are many drivers to each client. The PasswordManager is now owned by the client so it remains one per WebContents.

Similar changes are made to the parallel password manager classes.

Blink side is here: https://codereview.chromium.org/707723002/
- WebAutofillClient is a property of WebLocalFrame instead of WebView

BUG=425756, 400186
Committed: https://crrev.com/b1bc9bd012abbbb06eac639cda41dffd07b20ef4
Cr-Commit-Position: refs/heads/master@{#306479}

[Evan Stade, 2014-11-08 01:03:58]

estade@chromium.org changed reviewers:
+ vabr@google.com

[Evan Stade, 2014-11-08 01:03:59]

Vaclav, could you take a first pass for the high level changes? I'll add more
specific reviewers for subdirectories later.

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

vabr@chromium.org changed reviewers:
+ vabr@chromium.org
- vabr@google.com

[vabr (Chromium), 2014-11-10 14:34:36]

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

[vabr (Chromium), 2014-11-10 14:34:40]

Hi Evan,

Thank you for looping me in!

I left a couple of comments below, mostly nits, but also some more serious, and
one blocker (dependency cycle), please have a look.

+ Garret specifically for
components/autofill/content/renderer/password_generation_agent.*, just to
double-check the stuff I'm claiming about only working with main frames (as
opposed to sub-frames).

Cheers,
Vaclav

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

https://codereview.chromium.org/707173004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.cc:78:
render_frame_host_source_(NULL) {
NULL -> nullptr
("recommended for new code", http://chromium-cpp.appspot.com/)

https://codereview.chromium.org/707173004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.cc:321:
render_frame_host_source_ = NULL;
NULL -> nullptr

https://codereview.chromium.org/707173004/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/707173004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.h:68: //
DEPRECATED
nit: Further helpful information to put here:
* A link to a bug tracking removing this
* A hint to grab the driver for a frame instead.
* This driver is for the main frame.

https://codereview.chromium.org/707173004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.h:160: // When
non-NULL, the source of the current IPC message.
nit: Maybe explicitly add that this is only non-NULL during OnMessageReceived?

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
File components/autofill/content/browser/DEPS (right):

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/DEPS:2:
"+components/password_manager/content/browser",
I don't think we can do this. That would be a cycle (p.m. already depends on
autofill).

That means that we cannot use #include
"components/password_manager/content/browser/content_password_manager_driver.h"
below in content_autofill_driver.cc. This is effectively a componentising of the
DetectAccountCreationForms method. There are couple of (rather horrible)
possibilities I can see:

1) Keep DetectAccountCreationForms in the client, but add an argument to specify
the frame, to route to the driver.
2) Introduce an interface just for DetectAccountCreationForms in
components/autofill, and implement that by the password manager driver. There is
still the problem of getting the driver, though, so probably this is both too
intrusive, and not helpful.
3) Act before calling DetectAccountCreationForms: currently this is triggered in
the autofill code, and reaches out to password manager. Instead, maybe this
could be triggered in PasswordManager, and call back to the AutofillManager
(that dependency is OK).

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
File components/autofill/content/browser/content_autofill_driver.h (right):

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver.h:87:
content::RenderFrameHost* render_frame_host_;
Could this be
content::RenderFrameHost* const
to document that this is not only always non-NULL, but also not changing for the
life-time of |this|?

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
File components/autofill/content/browser/content_autofill_driver_factory.cc
(right):

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:57:
RenderFrameCreated(web_contents->GetMainFrame());
It's usually a bad idea to call virtual methods from constructors, because it is
not clear which class' implementation this uses.
You can move the body of RenderFrameCreated in a non-virtual method, and just
make both the constructor and RenderFrameCreated call that non-virtual method.
Or you can maybe call ContentAutofillDriverFactory::RenderFrameCreated, although
I'm not sure if that has the same problem as calling just RenderFrameCreated or
not.

Whatever you do, please also do this for the other classes where you used this
pattern, notably in ContentPasswordManagerDriverFactory but also elsewhere.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:65:
return frame_driver_map_[render_frame_host];
Note that this creates a new entry in the map, if there is none, and initializes
it with probably some garbage. You should probably check if the key is present,
and if it is not, return nullptr or assert.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:72:
DCHECK(driver);
Again, in addition to checking driver, the retrieval from the map should
actually be checked for the existence of the key.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:85:
DCHECK(driver);
DCHECKing driver does not do much here -- if driver is NULL, delete is a no-op.
Instead it should be checked that the key in the map exists:

auto driver_it = frame_driver_map_.find(render_frame_host);
DCHECK(driver_it != frame_driver_map_.end());
delete driver_it->second;
frame_driver_map_.erase(driver_it);

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:94:
ContentAutofillDriver* driver = frame_driver_map_[render_frame_host];
Please check the existence of the key, to avoid inserting trash into the map.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/autofill_agent.h:59: // to
RenderFrameObserver. Should eventually be removed.
nit: Bug reference to track the progress of moving the missing API from
RenderViewObserver to RenderFrameObserver would be helpful, or even formulating
this as a TODO() note.

Similarly for LegacyPasswordAutofillAgent. Feel free to include me (vabr) in the
TODO() association.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/autofill_agent.h:209: FormCache
form_cache_;
nit: This could use a comment about its purpose, and about the business with
resetting frames for it (see your TODO's in the .cc file).

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/autofill_agent.h:256: bool
main_frame_processed_;
Should the comment for this member variable be updated? What is the meaning of
the variable in agents observing a non-main frame?

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

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:899:
blink::WebFrame* frame = render_frame()->GetWebFrame();
So here the behaviour seems changed -- we try to save passwords even on subframe
navigation. Originally this code path was meant as a last resort, if a password
form is submitted in a way not triggering WillSubmitForm, with the idea, that if
the whole page reloads, that might be a result of a non-standard submit "button"
clicked.

We can discuss whether this should only happen on main frame navigation, but if
we are about to change it, that should happen in a different CL anyway. The goal
of this one is not to change the behaviour, but to make the agent observe frames
instead of views.

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

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.h:106:
linked_ptr<PasswordForm> > FrameToPasswordFormMap;
You can drop this typedef.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.h:239:
linked_ptr<PasswordForm> provisionally_saved_form_;
I believe you can use scoped_ptr instead of linked_ptr here. The linked_ptr was
necessary for becoming a value type of a std::map (which involves copying the
values), but here there will be no copying.
As a consequence, the #include of linked_ptr can be swapped for a scoped_ptr
#include.

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

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_generation_agent.cc:125:
not_blacklisted_password_form_origins_.clear();
Again, the old code only acted upon main frames, while the new one seems to act
on all frames. That should not be changed.

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:69:
// TODO(estade): should pass the proper PasswordManagerDriver.
I checked with mkwst@ (author of the credential manager code), and he says the
intent here was for the credential manager only to work with the main frame. So
in the current state the code should work fine, and maybe you want to amend the
TODO(estade) to say that "proper" driver actually means the main frame driver.

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/content/browser/content_password_manager_driver.cc:157:
password_manager_.DidNavigateMainFrame(details.is_in_page);
I don't think we should call DidNavigateMainFrame unless the implicit frame
indeed is the main one.

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/content/browser/content_password_manager_driver.cc:166:
: NULL;
nit: nullptr

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_password_manager_driver_factory.cc
(right):

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/content/browser/content_password_manager_driver_factory.cc:61:
return frame_driver_map_[render_frame_host];
As in the case of ContentAutofillDriverFactory, please be careful not to create
new entries in frame_driver_map_ through the use of the operator[]. Except for
line 66 where you indeed want to create a new entry, please prefer find() to
operator[] in this file.

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

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_client.h:94: //
TODO(estade): get rid of this.
nit: Please also specify what to use instead, and that it returns the driver for
the main frame.

[Garrett Casto, 2014-11-12 08:09:55]

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
File components/autofill/content/browser/DEPS (right):

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/DEPS:2:
"+components/password_manager/content/browser",
On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> I don't think we can do this. That would be a cycle (p.m. already depends on
> autofill).
> 

To be clear, the problem isn't that some code in Autofill depends on some code
in the Password Manager, it's that autofill_content_browser depends on
password_manager_content_browser and vice versa. Philosophically it seems better
if all the dependencies go one way between the components anyway, but the
problem is more specific than that.

> That means that we cannot use #include
>
"components/password_manager/content/browser/content_password_manager_driver.h"
> below in content_autofill_driver.cc. This is effectively a componentising of
the
> DetectAccountCreationForms method. There are couple of (rather horrible)
> possibilities I can see:
> 
> 1) Keep DetectAccountCreationForms in the client, but add an argument to
specify
> the frame, to route to the driver.
> 2) Introduce an interface just for DetectAccountCreationForms in
> components/autofill, and implement that by the password manager driver. There
is
> still the problem of getting the driver, though, so probably this is both too
> intrusive, and not helpful.

This seems like the right path to me, with ContentPasswordManagerDriver
implementing some generic observer class like AutofillResponseObserver. The
biggest issue is just that the obvious way of registering
ContentPasswordManagerDriver with ContentAutofillDriver doesn't work since they
are both being created on RenderFrameCreated() so there is no guarantee that
ContentAutofillDriver exists first. As long as we use a hook such that we can
guarantee that ContentePasswordManagerDriver is registered by the time that the
autofill response comes back, we should be fine.

> 3) Act before calling DetectAccountCreationForms: currently this is triggered
in
> the autofill code, and reaches out to password manager. Instead, maybe this
> could be triggered in PasswordManager, and call back to the AutofillManager
> (that dependency is OK).

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

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_generation_agent.cc:125:
not_blacklisted_password_form_origins_.clear();
On 2014/11/10 14:34:40, vabr (Chromium) wrote:
> Again, the old code only acted upon main frames, while the new one seems to
act
> on all frames. That should not be changed.

Confirmed.

[vabr (Chromium), 2014-11-13 11:19:18]

Evan,

One more piece of context: I'm writing up my PasswordAutofillAgent refactoring
design doc, and I added your plans into it:
https://docs.google.com/document/d/1epgLvufx3OqgRiMy9z0jT0HCLfFx29-jGh4TNg0ZF...

Unless you already have concrete plans for getting rid of
LegacyPasswordAutofillAgent I gladly volunteer to do that piece of work. You are
also welcome to delegate any other task from the area of refactoring autofill
for OOPIF to me.

Cheers,
Vaclav

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

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

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager.h:52:
PasswordManager(PasswordManagerClient* client, PasswordManagerDriver* driver);
Oh, and actually, having one PasswordManager per frame rather than page is a big
change. Currently it aggregates password form information from all frames, so
isolating that would certainly change its behaviour.

At a first shot it looks to me like there should be exactly one PasswordManager
for one PasswordManagerClient, and it should get access to the driver factory
instead of to particular drivers.

But I'll need to think about this more. Will update this CL if I find reason
against having managers to clients 1:1.

[vabr (Chromium), 2014-11-13 14:20:28]

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_password_manager_driver.h
(right):

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/content/browser/content_password_manager_driver.h:49:
bool DidLastPageLoadEncounterSSLErrors() override;
This information relates to the whole WebContents, so the method should be
probably moved to the PasswordManagerClient interface.

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/content/browser/content_password_manager_driver.h:50:
bool IsOffTheRecord() override;
ditto here: should be moved to the client

[vabr (Chromium), 2014-11-13 17:07:49]

On 2014/11/13 14:01:34, vabr (Chromium) wrote:
>
https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
> File components/password_manager/core/browser/password_manager.h (right):
> 
>
https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
> components/password_manager/core/browser/password_manager.h:52:
> PasswordManager(PasswordManagerClient* client, PasswordManagerDriver* driver);
> Oh, and actually, having one PasswordManager per frame rather than page is a
big
> change. Currently it aggregates password form information from all frames, so
> isolating that would certainly change its behaviour.
In particular, it looks to me that password managers in their current form in
this CL, if instantiated for sub-frames, would display "save password" prompts
when they should not. The password managers for main frames, on the other hand,
might fail to recognise a login error (if the re-displayed login form ends up in
an iframe), and thus invite the user to save a wrong password.

> 
> At a first shot it looks to me like there should be exactly one
PasswordManager
> for one PasswordManagerClient, and it should get access to the driver factory
> instead of to particular drivers.
> 
> But I'll need to think about this more. Will update this CL if I find reason
> against having managers to clients 1:1.

So far I have not found a good reason against managers to clients being 1:1. The
current PasswordManager code seems to purposefully work with the whole frame
tree.

[Evan Stade, 2014-11-13 19:53:45]

On 2014/11/13 17:07:49, vabr (Chromium) wrote:
> On 2014/11/13 14:01:34, vabr (Chromium) wrote:
> >
>
https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
> > File components/password_manager/core/browser/password_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
> > components/password_manager/core/browser/password_manager.h:52:
> > PasswordManager(PasswordManagerClient* client, PasswordManagerDriver*
driver);
> > Oh, and actually, having one PasswordManager per frame rather than page is a
> big
> > change. Currently it aggregates password form information from all frames,
so
> > isolating that would certainly change its behaviour.
> In particular, it looks to me that password managers in their current form in
> this CL, if instantiated for sub-frames, would display "save password" prompts
> when they should not. The password managers for main frames, on the other
hand,
> might fail to recognise a login error (if the re-displayed login form ends up
in
> an iframe), and thus invite the user to save a wrong password.
> 
> > 
> > At a first shot it looks to me like there should be exactly one
> PasswordManager
> > for one PasswordManagerClient, and it should get access to the driver
factory
> > instead of to particular drivers.
> > 
> > But I'll need to think about this more. Will update this CL if I find reason
> > against having managers to clients 1:1.
> 
> So far I have not found a good reason against managers to clients being 1:1.
The
> current PasswordManager code seems to purposefully work with the whole frame
> tree.

Should this apply to all the managers? PasswordManager,
PasswordGenerationManager, PasswordAutofillManager. In which case they should
hang off the client rather than the driver.

[Evan Stade, 2014-11-13 21:53:23]

On 2014/11/13 19:53:45, Evan Stade (ooo till Nov 13) wrote:
> On 2014/11/13 17:07:49, vabr (Chromium) wrote:
> > On 2014/11/13 14:01:34, vabr (Chromium) wrote:
> > >
> >
>
https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
> > > File components/password_manager/core/browser/password_manager.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
> > > components/password_manager/core/browser/password_manager.h:52:
> > > PasswordManager(PasswordManagerClient* client, PasswordManagerDriver*
> driver);
> > > Oh, and actually, having one PasswordManager per frame rather than page is
a
> > big
> > > change. Currently it aggregates password form information from all frames,
> so
> > > isolating that would certainly change its behaviour.
> > In particular, it looks to me that password managers in their current form
in
> > this CL, if instantiated for sub-frames, would display "save password"
prompts
> > when they should not. The password managers for main frames, on the other
> hand,
> > might fail to recognise a login error (if the re-displayed login form ends
up
> in
> > an iframe), and thus invite the user to save a wrong password.
> > 
> > > 
> > > At a first shot it looks to me like there should be exactly one
> > PasswordManager
> > > for one PasswordManagerClient, and it should get access to the driver
> factory
> > > instead of to particular drivers.
> > > 
> > > But I'll need to think about this more. Will update this CL if I find
reason
> > > against having managers to clients 1:1.
> > 
> > So far I have not found a good reason against managers to clients being 1:1.
> The
> > current PasswordManager code seems to purposefully work with the whole frame
> > tree.
> 
> Should this apply to all the managers? PasswordManager,
> PasswordGenerationManager, PasswordAutofillManager. In which case they should
> hang off the client rather than the driver.

OK, I updated PasswordManager to be owned by client instead of driver. Other
managers are owned by driver. So now there's one PasswordManager per tab, again.

I did not address the following, which seem unlikely to cause major disruptions
to the rest of the patch:
- cyclical dependencies
- style nits
- restoring main-frame-only behavior
- adding/updating comments

Aside from these bullet points, how does this patch look? Sorry I had to rebase
to ToT.

[Evan Stade, 2014-11-13 22:24:55]

alright, fixed the cyclical dependency, but still need to work on:

- updates to tests
- style nits
- restoring main-frame-only behavior
- adding/updating comments

If you don't see any other major problems with this patch, I'll do the above and
clean up the CL for more thorough review.

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_password_manager_driver.h
(right):

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/content/browser/content_password_manager_driver.h:49:
bool DidLastPageLoadEncounterSSLErrors() override;
On 2014/11/13 14:20:28, vabr (Chromium) wrote:
> This information relates to the whole WebContents, so the method should be
> probably moved to the PasswordManagerClient interface.

Done.

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/content/browser/content_password_manager_driver.h:50:
bool IsOffTheRecord() override;
On 2014/11/13 14:20:28, vabr (Chromium) wrote:
> ditto here: should be moved to the client

Done.

[vabr (Chromium), 2014-11-14 14:46:55]

Thanks, Evan.

> Should this apply to all the managers? PasswordManager,
PasswordGenerationManager, PasswordAutofillManager. In which case they should
hang off the client rather than the driver.

PasswordAutofillManager looks like it does not need to know the frame structure.
Unless you discover that having one PasswordManager per WebContents, and one
PasswordAutofillManager per frame brings up major problems, I would prefer one
PasswordAutofillManager per frame.

For PasswordGenerationManager, for the same reasons, I would prefer one
PasswordGenerationManager per frame.

Thanks for having switched PasswordManager back to one per WebContents.

> Aside from these bullet points, how does this patch look?
To me the patch looks good, especially after you removed the cyclic dependency
(thanks!).

Cheers,
Vaclav

https://codereview.chromium.org/707173004/diff/40001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/707173004/diff/40001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper.cc:1257:
driver->GetPasswordManager()->AddSubmissionCallback(base::Bind(
I don't think you have to do this if PasswordManager is back to being one per
WebContents.

https://codereview.chromium.org/707173004/diff/40001/components/password_mana...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/707173004/diff/40001/components/password_mana...
components/password_manager/content/browser/content_password_manager_driver.cc:32:
client_(client),
Why don't you just keep pointer to the PasswordManager? It is aggregated in the
client, so GetPasswordManager will always return the same instance for the same
client.

https://codereview.chromium.org/707173004/diff/40001/components/password_mana...
components/password_manager/content/browser/content_password_manager_driver.cc:130:
void ContentPasswordManagerDriver::OnPasswordFormsParsed(
Out of curiosity -- why is this indirection needed? Could we just have kept
IPC_MESSAGE_FORWARD?

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

https://codereview.chromium.org/707173004/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager_client.h:133: // FIXME
I don't understand -- what should be fixed here? Also, this should be probably a
TODO
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments).

[Evan Stade, 2014-11-14 19:19:00]

(code not updated)

https://codereview.chromium.org/707173004/diff/40001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/707173004/diff/40001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper.cc:1257:
driver->GetPasswordManager()->AddSubmissionCallback(base::Bind(
On 2014/11/14 14:46:54, vabr (Chromium) wrote:
> I don't think you have to do this if PasswordManager is back to being one per
> WebContents.

good call

https://codereview.chromium.org/707173004/diff/40001/components/password_mana...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/707173004/diff/40001/components/password_mana...
components/password_manager/content/browser/content_password_manager_driver.cc:32:
client_(client),
On 2014/11/14 14:46:54, vabr (Chromium) wrote:
> Why don't you just keep pointer to the PasswordManager? It is aggregated in
the
> client, so GetPasswordManager will always return the same instance for the
same
> client.

I suppose that's equally valid, but I don't really see much advantage to it.
What if the fact you just stated changes? What if client_ is needed for some
other reason in the future? Keeping a pointer directly to the client implies
something about the relationship between the classes. But I think my argument is
kind of splitting hairs at this point. I'll change it if you feel strongly.

https://codereview.chromium.org/707173004/diff/40001/components/password_mana...
components/password_manager/content/browser/content_password_manager_driver.cc:130:
void ContentPasswordManagerDriver::OnPasswordFormsParsed(
On 2014/11/14 14:46:54, vabr (Chromium) wrote:
> Out of curiosity -- why is this indirection needed? Could we just have kept
> IPC_MESSAGE_FORWARD?

extra parameter (need to pass |this|)

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

https://codereview.chromium.org/707173004/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager_client.h:133: // FIXME
On 2014/11/14 14:46:55, vabr (Chromium) wrote:
> I don't understand -- what should be fixed here? Also, this should be probably
a
> TODO
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments).

this is a placeholder for a better comment

[Evan Stade, 2014-11-14 23:25:50]

alright, should be ready for full review.

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

https://codereview.chromium.org/707173004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.cc:78:
render_frame_host_source_(NULL) {
On 2014/11/10 14:34:38, vabr (Chromium) wrote:
> NULL -> nullptr
> ("recommended for new code", http://chromium-cpp.appspot.com/)

Done.

https://codereview.chromium.org/707173004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.cc:321:
render_frame_host_source_ = NULL;
On 2014/11/10 14:34:38, vabr (Chromium) wrote:
> NULL -> nullptr

Done.

https://codereview.chromium.org/707173004/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/707173004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.h:68: //
DEPRECATED
On 2014/11/10 14:34:38, vabr (Chromium) wrote:
> nit: Further helpful information to put here:
> * A link to a bug tracking removing this
> * A hint to grab the driver for a frame instead.
> * This driver is for the main frame.

Done.

https://codereview.chromium.org/707173004/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/chrome_password_manager_client.h:160: // When
non-NULL, the source of the current IPC message.
On 2014/11/10 14:34:38, vabr (Chromium) wrote:
> nit: Maybe explicitly add that this is only non-NULL during OnMessageReceived?

I think that's implied by "current IPC message"

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
File components/autofill/content/browser/content_autofill_driver.h (right):

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver.h:87:
content::RenderFrameHost* render_frame_host_;
On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> Could this be
> content::RenderFrameHost* const
> to document that this is not only always non-NULL, but also not changing for
the
> life-time of |this|?

Done.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
File components/autofill/content/browser/content_autofill_driver_factory.cc
(right):

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:57:
RenderFrameCreated(web_contents->GetMainFrame());
On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> It's usually a bad idea to call virtual methods from constructors, because it
is
> not clear which class' implementation this uses.
> You can move the body of RenderFrameCreated in a non-virtual method, and just
> make both the constructor and RenderFrameCreated call that non-virtual method.
> Or you can maybe call ContentAutofillDriverFactory::RenderFrameCreated,
although
> I'm not sure if that has the same problem as calling just RenderFrameCreated
or
> not.
> 
> Whatever you do, please also do this for the other classes where you used this
> pattern, notably in ContentPasswordManagerDriverFactory but also elsewhere.

Done.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:65:
return frame_driver_map_[render_frame_host];
On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> Note that this creates a new entry in the map, if there is none, and
initializes
> it with probably some garbage. You should probably check if the key is
present,
> and if it is not, return nullptr or assert.

actually it initializes with null

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:72:
DCHECK(driver);
On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> Again, in addition to checking driver, the retrieval from the map should
> actually be checked for the existence of the key.

see other comments

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:85:
DCHECK(driver);
On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> DCHECKing driver does not do much here -- if driver is NULL, delete is a
no-op.
> Instead it should be checked that the key in the map exists:
> 
> auto driver_it = frame_driver_map_.find(render_frame_host);
> DCHECK(driver_it != frame_driver_map_.end());
> delete driver_it->second;
> frame_driver_map_.erase(driver_it);

see other comments

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:94:
ContentAutofillDriver* driver = frame_driver_map_[render_frame_host];
On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> Please check the existence of the key, to avoid inserting trash into the map.

a) it's not inserting trash, it's inserting null
b) it shouldn't happen, so an early return would just paper over a bug
c) DCHECK is not appropriate (I've removed it) because the deref on the next
line implies that it shouldn't be null

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/autofill_agent.h:59: // to
RenderFrameObserver. Should eventually be removed.
On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> nit: Bug reference to track the progress of moving the missing API from
> RenderViewObserver to RenderFrameObserver would be helpful, or even
formulating
> this as a TODO() note.
> 
> Similarly for LegacyPasswordAutofillAgent. Feel free to include me (vabr) in
the
> TODO() association.

Done.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/autofill_agent.h:209: FormCache
form_cache_;
On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> nit: This could use a comment about its purpose, and about the business with
> resetting frames for it (see your TODO's in the .cc file).

Done.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/autofill_agent.h:256: bool
main_frame_processed_;
On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> Should the comment for this member variable be updated? What is the meaning of
> the variable in agents observing a non-main frame?

yea I think we can just remove it

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

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:899:
blink::WebFrame* frame = render_frame()->GetWebFrame();
On 2014/11/10 14:34:40, vabr (Chromium) wrote:
> So here the behaviour seems changed -- we try to save passwords even on
subframe
> navigation. Originally this code path was meant as a last resort, if a
password
> form is submitted in a way not triggering WillSubmitForm, with the idea, that
if
> the whole page reloads, that might be a result of a non-standard submit
"button"
> clicked.
> 
> We can discuss whether this should only happen on main frame navigation, but
if
> we are about to change it, that should happen in a different CL anyway. The
goal
> of this one is not to change the behaviour, but to make the agent observe
frames
> instead of views.

reverted.

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

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.h:106:
linked_ptr<PasswordForm> > FrameToPasswordFormMap;
On 2014/11/10 14:34:40, vabr (Chromium) wrote:
> You can drop this typedef.

Done.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.h:239:
linked_ptr<PasswordForm> provisionally_saved_form_;
On 2014/11/10 14:34:40, vabr (Chromium) wrote:
> I believe you can use scoped_ptr instead of linked_ptr here. The linked_ptr
was
> necessary for becoming a value type of a std::map (which involves copying the
> values), but here there will be no copying.
> As a consequence, the #include of linked_ptr can be swapped for a scoped_ptr
> #include.

Done.

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

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_generation_agent.cc:125:
not_blacklisted_password_form_origins_.clear();
On 2014/11/12 08:09:55, Garrett Casto wrote:
> On 2014/11/10 14:34:40, vabr (Chromium) wrote:
> > Again, the old code only acted upon main frames, while the new one seems to
> act
> > on all frames. That should not be changed.
> 
> Confirmed.

Reverted. The reason I changed this was that the comment suggested subframe
loads should not clear main frame state. But since state is now per-frame, that
wouldn't happen even without the early return for non-main frames.

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:69:
// TODO(estade): should pass the proper PasswordManagerDriver.
On 2014/11/10 14:34:40, vabr (Chromium) wrote:
> I checked with mkwst@ (author of the credential manager code), and he says the
> intent here was for the credential manager only to work with the main frame.
So
> in the current state the code should work fine, and maybe you want to amend
the
> TODO(estade) to say that "proper" driver actually means the main frame driver.

thanks for figuring that out. Updated the code.

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/content/browser/content_password_manager_driver.cc:157:
password_manager_.DidNavigateMainFrame(details.is_in_page);
On 2014/11/10 14:34:40, vabr (Chromium) wrote:
> I don't think we should call DidNavigateMainFrame unless the implicit frame
> indeed is the main one.

Done.

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
File
components/password_manager/content/browser/content_password_manager_driver_factory.cc
(right):

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/content/browser/content_password_manager_driver_factory.cc:61:
return frame_driver_map_[render_frame_host];
On 2014/11/10 14:34:40, vabr (Chromium) wrote:
> As in the case of ContentAutofillDriverFactory, please be careful not to
create
> new entries in frame_driver_map_ through the use of the operator[]. Except for
> line 66 where you indeed want to create a new entry, please prefer find() to
> operator[] in this file.

like with ContentAutofillDriverFactory, lookup should never fail; if it does,
there's a bug, and we should crash. This is equivalent to DCHECKing the return
value of find().

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

https://codereview.chromium.org/707173004/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_client.h:94: //
TODO(estade): get rid of this.
On 2014/11/10 14:34:40, vabr (Chromium) wrote:
> nit: Please also specify what to use instead, and that it returns the driver
for
> the main frame.

got rid of it

[vabr (Chromium), 2014-11-17 15:08:50]

LGTM.
Out of the comments below, only the last two might require some action from you.

Thanks for this CL!
Vaclav

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
File components/autofill/content/browser/content_autofill_driver_factory.cc
(right):

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:65:
return frame_driver_map_[render_frame_host];
On 2014/11/14 23:25:49, Evan Stade wrote:
> On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> > Note that this creates a new entry in the map, if there is none, and
> initializes
> > it with probably some garbage. You should probably check if the key is
> present,
> > and if it is not, return nullptr or assert.
> 
> actually it initializes with null

You are right, sorry fir spreading misinformation. A colleague of mine made me
aware of §8.5, sentence 10 of the C++ standard, implies that T() is the zeroed
value for any basic type T (and T() is what std::map ends up calling when
inserting values through calling operator[]).

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:72:
DCHECK(driver);
On 2014/11/14 23:25:49, Evan Stade wrote:
> On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> > Again, in addition to checking driver, the retrieval from the map should
> > actually be checked for the existence of the key.
> 
> see other comments

Acknowledged. Using the NULL-dereference to signal issues (if the returned
driver pointer is null) sounds OK.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:85:
DCHECK(driver);
On 2014/11/14 23:25:49, Evan Stade wrote:
> On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> > DCHECKing driver does not do much here -- if driver is NULL, delete is a
> no-op.
> > Instead it should be checked that the key in the map exists:
> > 
> > auto driver_it = frame_driver_map_.find(render_frame_host);
> > DCHECK(driver_it != frame_driver_map_.end());
> > delete driver_it->second;
> > frame_driver_map_.erase(driver_it);
> 
> see other comments

Acknowledged.

https://codereview.chromium.org/707173004/diff/1/components/autofill/content/...
components/autofill/content/browser/content_autofill_driver_factory.cc:94:
ContentAutofillDriver* driver = frame_driver_map_[render_frame_host];
On 2014/11/14 23:25:49, Evan Stade wrote:
> On 2014/11/10 14:34:39, vabr (Chromium) wrote:
> > Please check the existence of the key, to avoid inserting trash into the
map.
> 
> a) it's not inserting trash, it's inserting null
> b) it shouldn't happen, so an early return would just paper over a bug
> c) DCHECK is not appropriate (I've removed it) because the deref on the next
> line implies that it shouldn't be null

Acknowledged.

https://codereview.chromium.org/707173004/diff/40001/components/password_mana...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/707173004/diff/40001/components/password_mana...
components/password_manager/content/browser/content_password_manager_driver.cc:32:
client_(client),
On 2014/11/14 19:19:00, Evan Stade wrote:
> On 2014/11/14 14:46:54, vabr (Chromium) wrote:
> > Why don't you just keep pointer to the PasswordManager? It is aggregated in
> the
> > client, so GetPasswordManager will always return the same instance for the
> same
> > client.
> 
> I suppose that's equally valid, but I don't really see much advantage to it.
> What if the fact you just stated changes? What if client_ is needed for some
> other reason in the future? Keeping a pointer directly to the client implies
> something about the relationship between the classes. But I think my argument
is
> kind of splitting hairs at this point. I'll change it if you feel strongly.

Thanks for explaining. Keeping unnecessary expectations ("client does not
change") out of the code sounds good to me, so GetPasswordManager as opposed to
a cached pointer is OK.

https://codereview.chromium.org/707173004/diff/40001/components/password_mana...
components/password_manager/content/browser/content_password_manager_driver.cc:130:
void ContentPasswordManagerDriver::OnPasswordFormsParsed(
On 2014/11/14 19:19:00, Evan Stade wrote:
> On 2014/11/14 14:46:54, vabr (Chromium) wrote:
> > Out of curiosity -- why is this indirection needed? Could we just have kept
> > IPC_MESSAGE_FORWARD?
> 
> extra parameter (need to pass |this|)

Acknowledged.

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

https://codereview.chromium.org/707173004/diff/60001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:78:
previous->delegate_.get() == delegate.get() &&
NB: Here you seem to change the behaviour for the situation where there is a
previous controller, but with a different delegate.
As long as that's your intention, I have no objections, as I'm not familiar with
this part of the code and you are.

https://codereview.chromium.org/707173004/diff/60001/components/password_mana...
File
components/password_manager/content/browser/content_credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/707173004/diff/60001/components/password_mana...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:71:
// TODO(estade): should pass the proper PasswordManagerDriver.
Do you still need this TODO(estade)? Looks like the code now says exactly what
it should -- get the driver for the main frame.

[Evan Stade, 2014-11-17 20:34:38]

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

[Evan Stade, 2014-11-17 20:34:40]

Vaclav, had to make an additional change (see comment).

jam, could you review:
  chrome/browser/password_manager/chrome_password_manager_client.cc
  chrome/browser/password_manager/chrome_password_manager_client.h
  chrome/browser/prerender/prerender_tab_helper.cc
  chrome/browser/prerender/prerender_tab_helper.h
  chrome/browser/ui/login/login_prompt.cc
  chrome/browser/ui/login/login_prompt.h
  chrome/browser/ui/search/instant_search_prerenderer_unittest.cc
  chrome/browser/ui/sync/one_click_signin_helper.cc
  chrome/browser/ui/sync/one_click_signin_helper.h
  chrome/browser/ui/sync/one_click_signin_helper_unittest.cc
  chrome/browser/ui/tab_helpers.cc
  chrome/renderer/chrome_content_renderer_client.cc

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

https://codereview.chromium.org/707173004/diff/60001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:78:
previous->delegate_.get() == delegate.get() &&
On 2014/11/17 15:08:49, vabr (Chromium) wrote:
> NB: Here you seem to change the behaviour for the situation where there is a
> previous controller, but with a different delegate.
> As long as that's your intention, I have no objections, as I'm not familiar
with
> this part of the code and you are.

Yea, I think it's ok, and I actually can't think of a situation where it would
come up in practice.

https://codereview.chromium.org/707173004/diff/60001/components/password_mana...
File
components/password_manager/content/browser/content_credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/707173004/diff/60001/components/password_mana...
components/password_manager/content/browser/content_credential_manager_dispatcher.cc:71:
// TODO(estade): should pass the proper PasswordManagerDriver.
On 2014/11/17 15:08:49, vabr (Chromium) wrote:
> Do you still need this TODO(estade)? Looks like the code now says exactly what
> it should -- get the driver for the main frame.

correct, removed

https://codereview.chromium.org/707173004/diff/80001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/707173004/diff/80001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:426: Send(new
AutofillHostMsg_FirstUserGestureObserved(routing_id()));
Had to make some changes when updating blink code. This sends an IPC to the
browser process, which sends an IPC back to each frame's AutofillAgent, so that
a user gesture in any frame will be counted in every frame. This appears to be
the recommended approach according to the OOPIF faq.

[jam, 2014-11-17 23:13:21]

lgtm with comment

https://codereview.chromium.org/707173004/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/707173004/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:319:
IPC_BEGIN_MESSAGE_MAP(ChromePasswordManagerClient, message)
use IPC_BEGIN_MESSAGE_MAP_WITH_PARAM instead, and pass render_frame_host as the
param (this is what it was added for).

then you can remove render_frame_host_source_

[Evan Stade, 2014-11-18 00:39:06]

https://codereview.chromium.org/707173004/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/707173004/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:319:
IPC_BEGIN_MESSAGE_MAP(ChromePasswordManagerClient, message)
On 2014/11/17 23:13:21, jam wrote:
> use IPC_BEGIN_MESSAGE_MAP_WITH_PARAM instead, and pass render_frame_host as
the
> param (this is what it was added for).
> 
> then you can remove render_frame_host_source_

Done. Any problem with using two message maps in a row?

[vabr (Chromium), 2014-11-18 09:49:17]

Still LGTM, thanks for pointing out the additional change.
Cheers,
Vaclav

https://codereview.chromium.org/707173004/diff/80001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/707173004/diff/80001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:426: Send(new
AutofillHostMsg_FirstUserGestureObserved(routing_id()));
On 2014/11/17 20:34:39, Evan Stade wrote:
> Had to make some changes when updating blink code. This sends an IPC to the
> browser process, which sends an IPC back to each frame's AutofillAgent, so
that
> a user gesture in any frame will be counted in every frame. This appears to be
> the recommended approach according to the OOPIF faq.

Thanks for spotting this! I completely missed it, it would be yet another
nightmare with passwords not being filled.

It is sad that for transferring the single bit of information to the right place
all of the plumbing needs to be added, but it still looks like the best way to
do it to me.

[jam, 2014-11-18 17:33:51]

https://codereview.chromium.org/707173004/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/707173004/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:319:
IPC_BEGIN_MESSAGE_MAP(ChromePasswordManagerClient, message)
On 2014/11/18 00:39:05, Evan Stade wrote:
> On 2014/11/17 23:13:21, jam wrote:
> > use IPC_BEGIN_MESSAGE_MAP_WITH_PARAM instead, and pass render_frame_host as
> the
> > param (this is what it was added for).
> > 
> > then you can remove render_frame_host_source_
> 
> Done. Any problem with using two message maps in a row?

why are you using two instead of just one? i realize you may not be using the
RFH pointer now for the other two IPCs, but it's harmless having them in the
handler signature.


note as currently written, there are a few bugs (need to set handled = false in
first IPC_MESSAGE_UNHANDLED, need to check it before doing next message map, and
then need to reset it to true). much easier to just have one.

[Evan Stade, 2014-11-18 19:39:29]

https://codereview.chromium.org/707173004/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/707173004/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:319:
IPC_BEGIN_MESSAGE_MAP(ChromePasswordManagerClient, message)
On 2014/11/18 17:33:51, jam wrote:
> On 2014/11/18 00:39:05, Evan Stade wrote:
> > On 2014/11/17 23:13:21, jam wrote:
> > > use IPC_BEGIN_MESSAGE_MAP_WITH_PARAM instead, and pass render_frame_host
as
> > the
> > > param (this is what it was added for).
> > > 
> > > then you can remove render_frame_host_source_
> > 
> > Done. Any problem with using two message maps in a row?
> 
> why are you using two instead of just one? i realize you may not be using the
> RFH pointer now for the other two IPCs, but it's harmless having them in the
> handler signature.
> 
> 
> note as currently written, there are a few bugs (need to set handled = false
in
> first IPC_MESSAGE_UNHANDLED, need to check it before doing next message map,
and
> then need to reset it to true). much easier to just have one.

A couple of the message handling functions are called from elsewhere. Adding an
unused param to them and making callsites pass NULL seems hacky. Creating extra
thunk functions kinda defeats the purpose (i.e. less verbose code).

How would a bug occur with the the code as it is currently written? Could a
message somehow match in both maps? Would a message that matched neither not
trigger handled=false in the second UNHANDLED case?

[jam, 2014-11-19 17:43:28]

lgtm

https://codereview.chromium.org/707173004/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/707173004/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:319:
IPC_BEGIN_MESSAGE_MAP(ChromePasswordManagerClient, message)
On 2014/11/18 19:39:29, Evan Stade wrote:
> On 2014/11/18 17:33:51, jam wrote:
> > On 2014/11/18 00:39:05, Evan Stade wrote:
> > > On 2014/11/17 23:13:21, jam wrote:
> > > > use IPC_BEGIN_MESSAGE_MAP_WITH_PARAM instead, and pass render_frame_host
> as
> > > the
> > > > param (this is what it was added for).
> > > > 
> > > > then you can remove render_frame_host_source_
> > > 
> > > Done. Any problem with using two message maps in a row?
> > 
> > why are you using two instead of just one? i realize you may not be using
the
> > RFH pointer now for the other two IPCs, but it's harmless having them in the
> > handler signature.
> > 
> > 
> > note as currently written, there are a few bugs (need to set handled = false
> in
> > first IPC_MESSAGE_UNHANDLED, need to check it before doing next message map,
> and
> > then need to reset it to true). much easier to just have one.
> 
> A couple of the message handling functions are called from elsewhere. Adding
an
> unused param to them and making callsites pass NULL seems hacky. Creating
extra
> thunk functions kinda defeats the purpose (i.e. less verbose code).

I see, I didn't realize they're called from elsewhere.

> 
> How would a bug occur with the the code as it is currently written? Could a
> message somehow match in both maps? Would a message that matched neither not
> trigger handled=false in the second UNHANDLED case?

you're right, it'll work as it. just remove the "IPC_MESSAGE_UNHANDLED()" which
doesn't do anything.

[Evan Stade, 2014-11-19 22:20:05]

thanks for reviews, Vaclav and John. Now just need to wait for
https://codereview.chromium.org/737853002/ to land.

https://codereview.chromium.org/707173004/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/707173004/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:319:
IPC_BEGIN_MESSAGE_MAP(ChromePasswordManagerClient, message)
On 2014/11/19 17:43:27, jam wrote:
> On 2014/11/18 19:39:29, Evan Stade wrote:
> > On 2014/11/18 17:33:51, jam wrote:
> > > On 2014/11/18 00:39:05, Evan Stade wrote:
> > > > On 2014/11/17 23:13:21, jam wrote:
> > > > > use IPC_BEGIN_MESSAGE_MAP_WITH_PARAM instead, and pass
render_frame_host
> > as
> > > > the
> > > > > param (this is what it was added for).
> > > > > 
> > > > > then you can remove render_frame_host_source_
> > > > 
> > > > Done. Any problem with using two message maps in a row?
> > > 
> > > why are you using two instead of just one? i realize you may not be using
> the
> > > RFH pointer now for the other two IPCs, but it's harmless having them in
the
> > > handler signature.
> > > 
> > > 
> > > note as currently written, there are a few bugs (need to set handled =
false
> > in
> > > first IPC_MESSAGE_UNHANDLED, need to check it before doing next message
map,
> > and
> > > then need to reset it to true). much easier to just have one.
> > 
> > A couple of the message handling functions are called from elsewhere. Adding
> an
> > unused param to them and making callsites pass NULL seems hacky. Creating
> extra
> > thunk functions kinda defeats the purpose (i.e. less verbose code).
> 
> I see, I didn't realize they're called from elsewhere.
> 
> > 
> > How would a bug occur with the the code as it is currently written? Could a
> > message somehow match in both maps? Would a message that matched neither not
> > trigger handled=false in the second UNHANDLED case?
> 
> you're right, it'll work as it. just remove the "IPC_MESSAGE_UNHANDLED()"
which
> doesn't do anything.

Done.

[Evan Stade, 2014-11-20 21:30:41]

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

[Evan Stade, 2014-11-20 21:30:42]

palmer@, could you review components/autofill/content/common/autofill_messages.h

[palmer, 2014-11-20 21:42:34]

components/autofill/content/common/autofill_messages.h LGTM

[Evan Stade, 2014-11-20 23:01:47]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-11-20 23:04:14]

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

[commit-bot: I haz the power, 2014-11-20 23:22:06]

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

[commit-bot: I haz the power, 2014-11-20 23:22:09]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vabr (Chromium), 2014-11-21 09:31:52]

Hi Evan,

This still LGTM, but please address one more comment (found after the merge with
ToT).

Cheers,
Vaclav

https://codereview.chromium.org/707173004/diff/180001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (left):

https://codereview.chromium.org/707173004/diff/180001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:937:
logger->LogBoolean(Logger::STRING_FORM_FRAME_EQ_FRAME,
Please remove STRING_FORM_FRAME_EQ_FRAME also from
components/autofill/core/common/save_password_progress_logger.*

[Evan Stade, 2014-11-22 05:16:54]

I've spent some time making fixes, but a lot of things are still broken. I'll
ping again when trybots are green.

[Evan Stade, 2014-11-25 00:06:41]

Vaclav, please take another look. Compare patchset 17 to patchset 10 for new
changes.

There are a couple functional changes:
- create a driver for every frame, not just the main frame, when the autofill/pw
driver factory is created
- use some more RenderView ("legacy") observer methods in places where it's
necessary to know what's going on in other frames.

Most of the other changes are just for tests. Also note the test changes rely on
https://codereview.chromium.org/756743003/

[vabr (Chromium), 2014-11-25 15:56:57]

Hi Evan,

The main two changes you mention (more legacy methods, and creating drivers for
each frame) look good to me.

I have some minor concerns about password_autofill_agent_browsertest.cc below.

Cheers,
Vaclav

https://codereview.chromium.org/707173004/diff/320001/chrome/renderer/autofil...
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://codereview.chromium.org/707173004/diff/320001/chrome/renderer/autofil...
chrome/renderer/autofill/autofill_renderer_browsertest.cc:312: protected:
According to the style guide, the members need to be private, with the option to
make protected accessors for them.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control

https://codereview.chromium.org/707173004/diff/320001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/707173004/diff/320001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:915:
password_autofill_agent_->FirstUserGestureObserved();
This is decreasing the scope of the test: before it tested that if the page gets
clicked, password values appear. Not it tests that if the agent is told that
there has been a click, the passwords appear. Do you have a good suggestion
where to check the part between the click and the FirstUserGestureObserved()
call?

https://codereview.chromium.org/707173004/diff/320001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1521:
password_autofill_agent_->FirstUserGestureObserved();
Why is this needed? User-entered passwords should not be blocked by the
gesture-based gatekeeper. (And the very first line of
SimulateInputChangeForElement seems to directly set the value.)

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

https://codereview.chromium.org/707173004/diff/320001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:930: // Loop
through the forms on the page looking for one that has been
the page -> this frame

[Evan Stade, 2014-11-25 21:07:40]

https://codereview.chromium.org/707173004/diff/320001/chrome/renderer/autofil...
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://codereview.chromium.org/707173004/diff/320001/chrome/renderer/autofil...
chrome/renderer/autofill/autofill_renderer_browsertest.cc:312: protected:
On 2014/11/25 15:56:57, vabr (Chromium) wrote:
> According to the style guide, the members need to be private, with the option
to
> make protected accessors for them.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control

that link says "(for technical reasons, we allow data members of a test fixture
class to be protected when using Google Test)"

https://codereview.chromium.org/707173004/diff/320001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/707173004/diff/320001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:915:
password_autofill_agent_->FirstUserGestureObserved();
On 2014/11/25 15:56:57, vabr (Chromium) wrote:
> This is decreasing the scope of the test: before it tested that if the page
gets
> clicked, password values appear. Not it tests that if the agent is told that
> there has been a click, the passwords appear. Do you have a good suggestion
> where to check the part between the click and the FirstUserGestureObserved()
> call?

turns out I can revert this.

https://codereview.chromium.org/707173004/diff/320001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1521:
password_autofill_agent_->FirstUserGestureObserved();
On 2014/11/25 15:56:57, vabr (Chromium) wrote:
> Why is this needed? User-entered passwords should not be blocked by the
> gesture-based gatekeeper. (And the very first line of
> SimulateInputChangeForElement seems to directly set the value.)

Setting the value doesn't call FirstUserGestureObserved.

This line was already a part of SimulateInputChangeForElement. I've moved this
line back into that function.

[vabr (Chromium), 2014-11-26 09:26:27]

Thanks, LGTM, no further comments.

https://codereview.chromium.org/707173004/diff/320001/chrome/renderer/autofil...
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://codereview.chromium.org/707173004/diff/320001/chrome/renderer/autofil...
chrome/renderer/autofill/autofill_renderer_browsertest.cc:312: protected:
On 2014/11/25 21:07:39, Evan Stade wrote:
> On 2014/11/25 15:56:57, vabr (Chromium) wrote:
> > According to the style guide, the members need to be private, with the
option
> to
> > make protected accessors for them.
> >
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control
> 
> that link says "(for technical reasons, we allow data members of a test
fixture
> class to be protected when using Google Test)"

Ah, you are right, I missed that. Sorry!

[Evan Stade, 2014-11-26 22:04:32]

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

[Evan Stade, 2014-11-26 22:04:34]

+sgurun for android_webview

[sgurun-gerrit only, 2014-12-02 02:03:50]

aw lgtm with two small comments

https://codereview.chromium.org/707173004/diff/380001/android_webview/native/...
File android_webview/native/aw_autofill_client.cc (right):

https://codereview.chromium.org/707173004/diff/380001/android_webview/native/...
android_webview/native/aw_autofill_client.cc:162: const
std::vector<autofill::FormStructure*>& forms) {
himm, it seems we need a NOTIMPLEMENTED() here. We must have missed it.

https://codereview.chromium.org/707173004/diff/380001/android_webview/native/...
android_webview/native/aw_autofill_client.cc:170: void
AwAutofillClient::OnFirstUserGestureObserved() {
Please add a NOTIMPLEMENTED() here.

[Evan Stade, 2014-12-02 02:28:00]

https://codereview.chromium.org/707173004/diff/380001/android_webview/native/...
File android_webview/native/aw_autofill_client.cc (right):

https://codereview.chromium.org/707173004/diff/380001/android_webview/native/...
android_webview/native/aw_autofill_client.cc:162: const
std::vector<autofill::FormStructure*>& forms) {
On 2014/12/02 02:03:50, sgurun wrote:
> himm, it seems we need a NOTIMPLEMENTED() here. We must have missed it.

Done.

https://codereview.chromium.org/707173004/diff/380001/android_webview/native/...
android_webview/native/aw_autofill_client.cc:170: void
AwAutofillClient::OnFirstUserGestureObserved() {
On 2014/12/02 02:03:50, sgurun wrote:
> Please add a NOTIMPLEMENTED() here.

Done.

[Evan Stade, 2014-12-02 02:28:10]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-12-02 02:29:37]

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

[commit-bot: I haz the power, 2014-12-02 07:14:29]

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

[commit-bot: I haz the power, 2014-12-02 07:14:32]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2014-12-02 17:55:34]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-12-02 17:56:28]

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

[commit-bot: I haz the power, 2014-12-02 19:57:19]

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

[commit-bot: I haz the power, 2014-12-02 19:57:22]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Evan Stade, 2014-12-02 20:49:11]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-12-02 20:51:06]

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

[commit-bot: I haz the power, 2014-12-02 22:44:40]

Committed patchset #25 (id:480001)

[commit-bot: I haz the power, 2014-12-02 22:45:27]

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