[Password Manager] Use successful XHR submission as a signal for password saving

[Password Manager] Use successful XHR submission as a signal for password saving

If an XHR has succeeded and the form that the user was typing in has now
disappeared either by no longer being visible or being removed from the DOM,
prompt to save.

BUG=464891
Committed: https://crrev.com/c21f1c4caebb3f26706083c52b8f240edef494d2
Cr-Commit-Position: refs/heads/master@{#321443}

[Garrett Casto, 2015-03-17 01:04:24]

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

[Garrett Casto, 2015-03-17 01:16:29]

You should ignore the failing trybots as I both need to rebase and this CL won't
succeed until the blink cl (https://codereview.chromium.org/1006793002/) has
landed. That CL should land tomorrow and tests pass locally.

[Evan Stade, 2015-03-17 01:38:53]

estade@chromium.org changed reviewers:
+ estade@chromium.org, mathp@chromium.org

[Evan Stade, 2015-03-17 01:38:54]

+cc mathp

Might be worth doing something like this for Autofill as well.

[vabr (Chromium), 2015-03-17 10:14:39]

LGTM with nits.
Cheers,
Vaclav

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

https://codereview.chromium.org/1012853002/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:863: // If the
user hasn't fully filled out the form, don't try to save.
optional nit: To me, the comment duplicates what the method name already says.
But feel free to leave it if you feel otherwise.

https://codereview.chromium.org/1012853002/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:877: if
(!is_form_visible) {
Why not replace the bool directly with the call to IsWebNodeVisible(form) here?

https://codereview.chromium.org/1012853002/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1404:
(!provisionally_saved_form_->password_value.empty() ||
Did you mean to apply the negation to the whole disjunction instead of just to
the first operand?

[Garrett Casto, 2015-03-17 21:36:15]

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

https://codereview.chromium.org/1012853002/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:863: // If the
user hasn't fully filled out the form, don't try to save.
On 2015/03/17 10:14:38, vabr (Chromium) wrote:
> optional nit: To me, the comment duplicates what the method name already says.
> But feel free to leave it if you feel otherwise.

I'm pretty indifferent. If you don't feel that it adds value, I'll remove it.

https://codereview.chromium.org/1012853002/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:877: if
(!is_form_visible) {
On 2015/03/17 10:14:38, vabr (Chromium) wrote:
> Why not replace the bool directly with the call to IsWebNodeVisible(form)
here?

Copy and paste issue. Fixed.

https://codereview.chromium.org/1012853002/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1404:
(!provisionally_saved_form_->password_value.empty() ||
On 2015/03/17 10:14:38, vabr (Chromium) wrote:
> Did you mean to apply the negation to the whole disjunction instead of just to
> the first operand?

Yikes. Nice catch. Added additional tests to verify that this works as expected
for signup forms as well.

[Garrett Casto, 2015-03-18 07:40:44]

The CQ bit was checked by gcasto@chromium.org

[Garrett Casto, 2015-03-18 07:40:45]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1012853002/#ps80001
(title: "Comments")

[commit-bot: I haz the power, 2015-03-18 07:40:50]

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

[vabr (Chromium), 2015-03-18 08:37:47]

Thanks, LGTM!

[commit-bot: I haz the power, 2015-03-18 12:04:42]

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

[commit-bot: I haz the power, 2015-03-18 12:04:43]

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

[Garrett Casto, 2015-03-19 20:53:46]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2015-03-19 20:54:03]

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

[commit-bot: I haz the power, 2015-03-19 21:57:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-19 21:57:43]

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