Add password form search using blink::WebNode reference comparison.

Add password form search using blink::WebNode reference comparison.

After an XHR is completed, PasswordAutofillAgent tries to check if the
form is still visible (i.e. if the user has successfuly logged in or
still sees the form) by searching the form with the same action URL.
This works until the form has action URL. If it doesn't, the search is
performed using field matching. This will fail if the form is modified
by the website right before the XHR has completed.
This CL adds support for the form search using blink::WebFormElement
and blink::WebInputElement (for unowned forms) reference comparison.

BUG=700862
R=dvadym@chromium.org

Review-Url: https://codereview.chromium.org/2746033004
Cr-Commit-Position: refs/heads/master@{#458032}
Committed: https://chromium.googlesource.com/chromium/src/+/531d81feacb5306300387558824ee4ce11e98b89

[sense (YandexTeam), 2017-03-14 08:44:29]

Patchset #1 (id:1) has been deleted

[sense (YandexTeam), 2017-03-14 08:49:28]

The CQ bit was checked by sense@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 08:49:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 09:46:19]

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

[commit-bot: I haz the power, 2017-03-14 09:46:20]

Dry run: This issue passed the CQ dry run.

[dvadym, 2017-03-14 17:11:11]

Thanks for fixing this! Overall looks good, I like approach with a separate
class ProvisionallySavedForm. I have some comments.

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

https://codereview.chromium.org/2746033004/diff/20001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2218:
LoadHTMLWithUrlOverride(kHTMLWithHiddenField, "https://www.example.com");
Why not LoadHTML?

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

https://codereview.chromium.org/2746033004/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1236: // Try to match
the WebFormElement reference first.
I think it's better to move up these lines before checking whether action is
empty or not. In such way we can be robust against JavaScript add/remove action

https://codereview.chromium.org/2746033004/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1237: if
(!form_element.isNull() && form.equals(form_element)) {
Nit: you can use == operator (form == form_element). That's used more frequent
then equal.

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

https://codereview.chromium.org/2746033004/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:143: if
(!input_element.isNull() && form_util::IsWebNodeVisible(input_element))
Could you please move this statement as the first statement iof this method? It
would be better from performance reasons, since this check is very cheap in
comparison to others.

https://codereview.chromium.org/2746033004/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1010:
(provisionally_saved_form_.form_element().isNull() &&
Great, thanks for adding check form_element().isNull(). Now in case when
form_element is present, we can skip call of IsUnownedPasswordFormVisible.

https://codereview.chromium.org/2746033004/diff/20001/components/autofill/con...
File components/autofill/content/renderer/provisionally_saved_form.h (right):

https://codereview.chromium.org/2746033004/diff/20001/components/autofill/con...
components/autofill/content/renderer/provisionally_saved_form.h:29: void
Reset();
Nit: comment is trivial, just remove it.

https://codereview.chromium.org/2746033004/diff/20001/components/autofill/con...
components/autofill/content/renderer/provisionally_saved_form.h:39: const
PasswordForm& password_form() const {
Nit: comment is trivial, just remove it.

[sense (YandexTeam), 2017-03-15 04:51:24]

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

https://codereview.chromium.org/2746033004/diff/20001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2218:
LoadHTMLWithUrlOverride(kHTMLWithHiddenField, "https://www.example.com");
On 2017/03/14 17:11:10, dvadym wrote:
> Why not LoadHTML?

In this test we explicitly test a form without action URL. If we use LoadHTML
then form_util::GetCanonicalActionForForm() will return invalid URL and
CreatePasswordFormFromWebForm() will fail here:
password_form->action = form_util::GetCanonicalActionForForm(web_form);
if (!password_form->action.is_valid())
  return std::unique_ptr<PasswordForm>();

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

https://codereview.chromium.org/2746033004/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1236: // Try to match
the WebFormElement reference first.
On 2017/03/14 17:11:10, dvadym wrote:
> I think it's better to move up these lines before checking whether action is
> empty or not. In such way we can be robust against JavaScript add/remove
action

Yes, I agree.

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

https://codereview.chromium.org/2746033004/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:143: if
(!input_element.isNull() && form_util::IsWebNodeVisible(input_element))
On 2017/03/14 17:11:11, dvadym wrote:
> Could you please move this statement as the first statement iof this method?
It
> would be better from performance reasons, since this check is very cheap in
> comparison to others.

Done.

https://codereview.chromium.org/2746033004/diff/20001/components/autofill/con...
File components/autofill/content/renderer/provisionally_saved_form.h (right):

https://codereview.chromium.org/2746033004/diff/20001/components/autofill/con...
components/autofill/content/renderer/provisionally_saved_form.h:29: void
Reset();
On 2017/03/14 17:11:11, dvadym wrote:
> Nit: comment is trivial, just remove it.

Done.

[sense (YandexTeam), 2017-03-15 04:51:32]

The CQ bit was checked by sense@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 04:51:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 05:53:49]

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

[commit-bot: I haz the power, 2017-03-15 05:53:51]

Dry run: This issue passed the CQ dry run.

[dvadym, 2017-03-15 14:27:12]

LGTM

I just now realized, could you please rename
ProvisionallySavedForm -> ProvisionallySavedPasswordForm (there are files for
both Password Manager and Autofill in folder
components/autofill/content/renderer and string "password" in name is important
to distinguish)?

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

https://codereview.chromium.org/2746033004/diff/20001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2218:
LoadHTMLWithUrlOverride(kHTMLWithHiddenField, "https://www.example.com");
On 2017/03/15 04:51:24, sense (YandexTeam) wrote:
> On 2017/03/14 17:11:10, dvadym wrote:
> > Why not LoadHTML?
> 
> In this test we explicitly test a form without action URL. If we use LoadHTML
> then form_util::GetCanonicalActionForForm() will return invalid URL and
> CreatePasswordFormFromWebForm() will fail here:
> password_form->action = form_util::GetCanonicalActionForForm(web_form);
> if (!password_form->action.is_valid())
>   return std::unique_ptr<PasswordForm>();

Acknowledged

[sense (YandexTeam), 2017-03-16 01:12:24]

The CQ bit was checked by sense@yandex-team.ru

[sense (YandexTeam), 2017-03-16 01:12:25]

The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org
Link to the patchset: https://codereview.chromium.org/2746033004/#ps60001
(title: "Rename ProvisionallySavedForm to ProvisionallySavedPasswordForm.")

[commit-bot: I haz the power, 2017-03-16 01:12:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 01:21:33]

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

[commit-bot: I haz the power, 2017-03-16 01:21:34]

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

[sense (YandexTeam), 2017-03-16 01:39:08]

sense@yandex-team.ru changed reviewers:
+ sebsg@chromium.org

[sense (YandexTeam), 2017-03-16 01:39:08]

sebsg@ please take a look.

[sense (YandexTeam), 2017-03-20 04:30:00]

sense@yandex-team.ru changed reviewers:
+ gcasto@chromium.org

[sense (YandexTeam), 2017-03-20 04:30:00]

sebsg@ kindly ping.
components/autofill/content/renderer/BUILD.gn
components/autofill/content/renderer/form_autofill_util.cc
components/autofill/content/renderer/form_autofill_util.h

gcasto@ please take a look at:
chrome/renderer/autofill/password_autofill_agent_browsertest.cc

[vabr (Chromium), 2017-03-20 09:02:54]

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

[vabr (Chromium), 2017-03-20 09:02:55]

chrome/renderer/autofill/password_autofill_agent_browsertest.cc LGTM
Thanks!
Vaclav

[vabr (Chromium), 2017-03-20 09:06:46]

The whole CL LGTM, feel free to land.
Thanks!
Vaclav

[sense (YandexTeam), 2017-03-20 09:13:58]

The CQ bit was checked by sense@yandex-team.ru

[commit-bot: I haz the power, 2017-03-20 09:14:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 10:12:11]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490001238434160,
"parent_rev": "25937f56120530c2662a3a0b2e3ed7a2a3103ed4", "commit_rev":
"531d81feacb5306300387558824ee4ce11e98b89"}

[commit-bot: I haz the power, 2017-03-20 10:12:56]

Description was changed from

==========
Add password form search using blink::WebNode reference comparison.

After an XHR is completed, PasswordAutofillAgent tries to check if the
form is still visible (i.e. if the user has successfuly logged in or
still sees the form) by searching the form with the same action URL.
This works until the form has action URL. If it doesn't, the search is
performed using field matching. This will fail if the form is modified
by the website right before the XHR has completed.
This CL adds support for the form search using blink::WebFormElement
and blink::WebInputElement (for unowned forms) reference comparison.

BUG=700862
R=dvadym@chromium.org
==========

to

==========
Add password form search using blink::WebNode reference comparison.

After an XHR is completed, PasswordAutofillAgent tries to check if the
form is still visible (i.e. if the user has successfuly logged in or
still sees the form) by searching the form with the same action URL.
This works until the form has action URL. If it doesn't, the search is
performed using field matching. This will fail if the form is modified
by the website right before the XHR has completed.
This CL adds support for the form search using blink::WebFormElement
and blink::WebInputElement (for unowned forms) reference comparison.

BUG=700862
R=dvadym@chromium.org

Review-Url: https://codereview.chromium.org/2746033004
Cr-Commit-Position: refs/heads/master@{#458032}
Committed:
https://chromium.googlesource.com/chromium/src/+/531d81feacb5306300387558824e...
==========

[commit-bot: I haz the power, 2017-03-20 10:12:57]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/531d81feacb5306300387558824e...