Add a basic functionality to the force-saving menu item.

Add a basic functionality to the force-saving menu item.

Currently, clicking on this item in the context menu will invoke the password save bubble. The credentials can be saved, but password manager will not work correctly until the page is reloaded; this is WIP.

BUG=484891
Committed: https://crrev.com/6bb816c5c0d8caed240931e87a0be2a9316f0112
Cr-Commit-Position: refs/heads/master@{#332028}

[msramek, 2015-05-28 19:10:58]

msramek@chromium.org changed reviewers:
+ avi@chromium.org, mkwst@chromium.org, vabr@chromium.org

[msramek, 2015-05-28 19:10:59]

Hi Mike, Václav and Avi,

please take a look.

Mike: autofill_messages
Avi: render_view_context_menu
Václav: password_manager (i.e. the rest)

Thanks,
Martin

[Avi (use Gerrit), 2015-05-28 19:15:08]

On 2015/05/28 19:10:59, msramek wrote:
> Avi: render_view_context_menu

LGTM

[vabr (Chromium), 2015-05-28 19:41:24]

LGTM with a couple of nits and a question.

Cheers,
Vaclav

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

https://codereview.chromium.org/1159143002/diff/1/components/autofill/content...
components/autofill/content/common/autofill_messages.h:191: // can be saved.
Perhaps rather than "so that the password can be saved" mention that the
renderer is expected to answer with
AutofillHostMsg_PasswordFormForceSaveRequested.

The current formulation makes it unclear what else needs to be done to save the
password, and saving the password is not a part of the expected renderer
response.

https://codereview.chromium.org/1159143002/diff/1/components/autofill/content...
components/autofill/content/common/autofill_messages.h:192:
IPC_MESSAGE_ROUTED0(AutofillMsg_ForceSavePassword)
Also, the name of the message, and the associated PasswordAutofillAgent handler,
is misleading. There is no (non-provisional) saving involved, just locating the
focussed form. Please reflect that in the name.

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

https://codereview.chromium.org/1159143002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:1249:
DCHECK(element.hasHTMLTagName("input"));
optional: Consider adding
<< element.tagName()
to the DCHECK for more information in case of failure.

https://codereview.chromium.org/1159143002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:1254:
ProvisionallySavePassword(input.form(), RESTRICTION_NONE);
Why do we need to provisionally save the password, when we send the credentials
to the browser on the next line?

[msramek, 2015-05-28 20:28:28]

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

https://codereview.chromium.org/1159143002/diff/1/components/autofill/content...
components/autofill/content/common/autofill_messages.h:191: // can be saved.
On 2015/05/28 19:41:23, vabr (Chromium) wrote:
> Perhaps rather than "so that the password can be saved" mention that the
> renderer is expected to answer with
> AutofillHostMsg_PasswordFormForceSaveRequested.
> 
> The current formulation makes it unclear what else needs to be done to save
the
> password, and saving the password is not a part of the expected renderer
> response.

Done.

https://codereview.chromium.org/1159143002/diff/1/components/autofill/content...
components/autofill/content/common/autofill_messages.h:192:
IPC_MESSAGE_ROUTED0(AutofillMsg_ForceSavePassword)
On 2015/05/28 19:41:23, vabr (Chromium) wrote:
> Also, the name of the message, and the associated PasswordAutofillAgent
handler,
> is misleading. There is no (non-provisional) saving involved, just locating
the
> focussed form. Please reflect that in the name.

Done.

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

https://codereview.chromium.org/1159143002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:1249:
DCHECK(element.hasHTMLTagName("input"));
On 2015/05/28 19:41:23, vabr (Chromium) wrote:
> optional: Consider adding
> << element.tagName()
> to the DCHECK for more information in case of failure.

Done.

https://codereview.chromium.org/1159143002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:1254:
ProvisionallySavePassword(input.form(), RESTRICTION_NONE);
On 2015/05/28 19:41:23, vabr (Chromium) wrote:
> Why do we need to provisionally save the password, when we send the
credentials
> to the browser on the next line?

Here, ProvisionallySavePassword() is used to transform WebFormElement into
PasswordForm, also applying autofill hints that this class got in
OnAutofillUsernameDataReceived.

The fact that the result is stored in provisionally_saved_form_ doesn't seem to
have any side-effects, especially because PasswordManager is indeed going to
offer to save that form after the callback from this method.

However, now that we changed the name of the method so that it doesn't refer to
force-saving, it will be of course cleaner if we just call CreatePasswordForm
instead.

[vabr (Chromium), 2015-05-29 06:48:50]

LGTM, thanks!

Vaclav

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

https://codereview.chromium.org/1159143002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:1254:
ProvisionallySavePassword(input.form(), RESTRICTION_NONE);
On 2015/05/28 20:28:28, msramek wrote:
> On 2015/05/28 19:41:23, vabr (Chromium) wrote:
> > Why do we need to provisionally save the password, when we send the
> credentials
> > to the browser on the next line?
> 
> Here, ProvisionallySavePassword() is used to transform WebFormElement into
> PasswordForm, also applying autofill hints that this class got in
> OnAutofillUsernameDataReceived.
> 
> The fact that the result is stored in provisionally_saved_form_ doesn't seem
to
> have any side-effects, especially because PasswordManager is indeed going to
> offer to save that form after the callback from this method.
> 
> However, now that we changed the name of the method so that it doesn't refer
to
> force-saving, it will be of course cleaner if we just call CreatePasswordForm
> instead.

Acknowledged.

[Mike West, 2015-05-29 07:24:49]

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

https://codereview.chromium.org/1159143002/diff/20001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:347: // Inform the
browser which password form is currently focused.
What does it return if there is not a password form currently focused?

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

https://codereview.chromium.org/1159143002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1249:
DCHECK(!element.isNull());
Hrm. Execution doesn't stop while a context menu is open. It's entirely possible
for a site to pop up an ad, for instance, and move focus out from under you. It
seems like we should gracefully handle this case without crashing.

[msramek, 2015-05-29 11:57:21]

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

https://codereview.chromium.org/1159143002/diff/20001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:347: // Inform the
browser which password form is currently focused.
On 2015/05/29 07:24:49, Mike West wrote:
> What does it return if there is not a password form currently focused?

My original intention was that if there is no form, this message would never be
sent (thus, ContentPasswordManagerDriver would never get the response, and so
nothing would happen).

However, it might be helpful to return an empty |autofill::PasswordForm|. The
password manager will be able to handle this as an error and show an error
message in the bubble. How exactly this would work is still up to discussion
with UI, but since the user made an explicit action, we shouldn't just fail
silently.

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

https://codereview.chromium.org/1159143002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1249:
DCHECK(!element.isNull());
On 2015/05/29 07:24:49, Mike West wrote:
> Hrm. Execution doesn't stop while a context menu is open. It's entirely
possible
> for a site to pop up an ad, for instance, and move focus out from under you.
It
> seems like we should gracefully handle this case without crashing.

Ehh, thanks for the sanity check.

I also added a check whether input.form() exists, which I forgot about. Password
manager cannot handle form-less password fields, so it would make sense to also
change the logic in Blink not to show the button in that case.

An interesting side effect is that we might accidentally save a different
password if the focus changes to a different password field, but commands like
"Paste" seem to behave the same way.

The other comment explains why we return an empty form instead of just bailing.

[Mike West, 2015-05-29 12:26:53]

Thanks for taking another pass. LGTM % optional nit.

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

https://codereview.chromium.org/1159143002/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1259:
password_form.reset(new PasswordForm());
Optional nit: I'd fold this into line 1247; that is, set it to an empty password
form, and replace it iff you find one focused on the page.

[msramek, 2015-05-29 12:38:55]

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

https://codereview.chromium.org/1159143002/diff/40001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1259:
password_form.reset(new PasswordForm());
On 2015/05/29 12:26:53, Mike West wrote:
> Optional nit: I'd fold this into line 1247; that is, set it to an empty
password
> form, and replace it iff you find one focused on the page.

If the WebFormElement is ununderstandable to CreatePasswordForm(), it will
return a null scoped_ptr. So we would still need to check it here.

[msramek, 2015-05-29 18:45:38]

I added empty methods to the password manager client and driver. This is just a
syntactical necessity from the fact that ForceSavePassword() methods are
declared as pure virtual in both interfaces. With this inconsequential change,
I'll proceed to the CQ.

[msramek, 2015-05-29 18:45:43]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-05-29 18:45:43]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
vabr@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1159143002/#ps60001
(title: "Added stub methods.")

[commit-bot: I haz the power, 2015-05-29 18:47:14]

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

[commit-bot: I haz the power, 2015-05-29 19:37:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-29 19:37:51]

Patchset 4 (id:??) landed as
https://crrev.com/6bb816c5c0d8caed240931e87a0be2a9316f0112
Cr-Commit-Position: refs/heads/master@{#332028}

[vabr (Chromium), 2015-06-01 07:17:21]

Thanks for the explanation in #11.

However, to minimize churn for iOS, non-essential methods in the client and
driver interface should be actually stubbed directly in the interface. I should
have checked this during my review, sorry for that.

Could you please file a follow-up CL to move the stubs from the *Stub classes to
the interfaces? It is likely too late for the iOS merge of the already landed
part, but at least to keep this consistent.

Thanks!
Vaclav

https://codereview.chromium.org/1159143002/diff/60001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/1159143002/diff/60001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:99: virtual
void ForceSavePassword() = 0;
Actually, since this is experimental only, it is not required on all platforms
yet, and should be implemented here instead of in the stub.

This means that for the iOS build there will be no need to add a similar stub
again.

https://codereview.chromium.org/1159143002/diff/60001/components/password_man...
File components/password_manager/core/browser/password_manager_driver.h (right):

https://codereview.chromium.org/1159143002/diff/60001/components/password_man...
components/password_manager/core/browser/password_manager_driver.h:73: virtual
void ForceSavePassword() = 0;
Ditto here, please stub it out directly in the interface.

[msramek, 2015-06-01 08:05:11]

I see. My understanding is that while the used UI is experimental, the
functionality itself is not considered non-essential, and thus I declared it as
pure virtual.

However, you're right that we don't want to worry about al OS right now. Sorry
for that; I'll send a follow-up CL shortly.

[vabr (Chromium), 2015-06-01 08:47:27]

On 2015/06/01 08:05:11, msramek wrote:
> I see. My understanding is that while the used UI is experimental, the
> functionality itself is not considered non-essential, and thus I declared it
as
> pure virtual.
You are right in that once it is no longer experimental, it will be essential.
At that moment we will have an iOS implementation, and will be able to make the
interface method pure virtual again.

> 
> However, you're right that we don't want to worry about al OS right now. Sorry
> for that; I'll send a follow-up CL shortly.

Thanks!

Vaclav