Fix saving password in Password Manager on nytimes.com.

Fix saving password in Password Manager on nytimes.com.

In refactoring on https://codereview.chromium.org/1381003004 it was implemented that PasswordAutofiilAgent considers that target frame is loaded only if all its subframes are loaded. But this could never happen, e.g. on nytimes.com there are  a lot of frames and we never see that all of them are loaded.

This patch is returning to our previous logic, namely to check that only target frame is loaded, but not to check its subframes.

BUG=569577
Committed: https://crrev.com/3d0810ccce9f38b9f2acb0441743f44b67af28a9
Cr-Commit-Position: refs/heads/master@{#365827}

[dvadym, 2015-12-17 13:23:10]

Description was changed from

==========
Fixed a condition for checking finish of page loading in PasswordManager.

BUG=569874
==========

to

==========
Fix saving password in Password Manager on nytimes.com.

In refactoring on https://codereview.chromium.org/1381003004 it was implemented
that PasswordAutofiilAgent considers that target frame is loaded only if all
subframes is loaded. But this could never happen, e.g. on nytimes.com there are 
 a lot of frames and we never see that all of them are loaded.

This patch is returning to our previous logic, namely to check that only target
frame is loaded, but not to check its subframes.

BUG=569874
==========

[dvadym, 2015-12-17 13:24:04]

Description was changed from

==========
Fix saving password in Password Manager on nytimes.com.

In refactoring on https://codereview.chromium.org/1381003004 it was implemented
that PasswordAutofiilAgent considers that target frame is loaded only if all
subframes is loaded. But this could never happen, e.g. on nytimes.com there are 
 a lot of frames and we never see that all of them are loaded.

This patch is returning to our previous logic, namely to check that only target
frame is loaded, but not to check its subframes.

BUG=569874
==========

to

==========
Fix saving password in Password Manager on nytimes.com.

In refactoring on https://codereview.chromium.org/1381003004 it was implemented
that PasswordAutofiilAgent considers that target frame is loaded only if all its
subframes are loaded. But this could never happen, e.g. on nytimes.com there are
 a lot of frames and we never see that all of them are loaded.

This patch is returning to our previous logic, namely to check that only target
frame is loaded, but not to check its subframes.

BUG=569874
==========

[dvadym, 2015-12-17 13:28:20]

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

[dvadym, 2015-12-17 13:28:20]

Hi Vaclav,

 Could you please have a look at this CL?

Regards,
Vadym

[vabr (Chromium), 2015-12-17 13:51:48]

This LGTM, but we should definitely add tests both to document this behaviour,
and to make sure someone does not "fix" it back later.

Cheers,
Vaclav

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

https://codereview.chromium.org/1536653002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:1056:
blink::WebFrame* frame = render_frame()->GetWebFrame()->top();
nit: frame -> main_frame
(to make clearer below, which frame this is about)

[dvadym, 2015-12-17 14:00:27]

Thanks for comments Vaclav!

We definitely should make sure that nobody in future will not fix it. But I have
no idea how we can test it.

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

https://codereview.chromium.org/1536653002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:1056:
blink::WebFrame* frame = render_frame()->GetWebFrame()->top();
On 2015/12/17 13:51:48, vabr (Chromium) wrote:
> nit: frame -> main_frame
> (to make clearer below, which frame this is about)

It's not necessary the main frame, it might be a target frame of this form for
example.

[dvadym, 2015-12-17 14:17:40]

Thanks Vaclav for review!

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

https://codereview.chromium.org/1536653002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_autofill_agent.cc:1056:
blink::WebFrame* frame = render_frame()->GetWebFrame()->top();
On 2015/12/17 14:00:27, dvadym wrote:
> On 2015/12/17 13:51:48, vabr (Chromium) wrote:
> > nit: frame -> main_frame
> > (to make clearer below, which frame this is about)
> 
> It's not necessary the main frame, it might be a target frame of this form for
> example.

Thanks, it's really top frame and it makes sense to check loading of the main
frame.

[dvadym, 2015-12-17 14:18:00]

After an offline discussion we realized that it's almost impossible to test this
change.

[dvadym, 2015-12-17 14:21:57]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2015-12-17 14:21:58]

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/1536653002/#ps20001
(title: "Small renaming")

[commit-bot: I haz the power, 2015-12-17 14:22:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1536653002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1536653002/20001

[dvadym, 2015-12-17 14:22:57]

Description was changed from

==========
Fix saving password in Password Manager on nytimes.com.

In refactoring on https://codereview.chromium.org/1381003004 it was implemented
that PasswordAutofiilAgent considers that target frame is loaded only if all its
subframes are loaded. But this could never happen, e.g. on nytimes.com there are
 a lot of frames and we never see that all of them are loaded.

This patch is returning to our previous logic, namely to check that only target
frame is loaded, but not to check its subframes.

BUG=569874
==========

to

==========
Fix saving password in Password Manager on nytimes.com.

In refactoring on https://codereview.chromium.org/1381003004 it was implemented
that PasswordAutofiilAgent considers that target frame is loaded only if all its
subframes are loaded. But this could never happen, e.g. on nytimes.com there are
 a lot of frames and we never see that all of them are loaded.

This patch is returning to our previous logic, namely to check that only target
frame is loaded, but not to check its subframes.

BUG=569577
==========

[commit-bot: I haz the power, 2015-12-17 15:33:22]

Description was changed from

==========
Fix saving password in Password Manager on nytimes.com.

In refactoring on https://codereview.chromium.org/1381003004 it was implemented
that PasswordAutofiilAgent considers that target frame is loaded only if all its
subframes are loaded. But this could never happen, e.g. on nytimes.com there are
 a lot of frames and we never see that all of them are loaded.

This patch is returning to our previous logic, namely to check that only target
frame is loaded, but not to check its subframes.

BUG=569577
==========

to

==========
Fix saving password in Password Manager on nytimes.com.

In refactoring on https://codereview.chromium.org/1381003004 it was implemented
that PasswordAutofiilAgent considers that target frame is loaded only if all its
subframes are loaded. But this could never happen, e.g. on nytimes.com there are
 a lot of frames and we never see that all of them are loaded.

This patch is returning to our previous logic, namely to check that only target
frame is loaded, but not to check its subframes.

BUG=569577
==========

[commit-bot: I haz the power, 2015-12-17 15:33:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-17 15:34:17]

Description was changed from

==========
Fix saving password in Password Manager on nytimes.com.

In refactoring on https://codereview.chromium.org/1381003004 it was implemented
that PasswordAutofiilAgent considers that target frame is loaded only if all its
subframes are loaded. But this could never happen, e.g. on nytimes.com there are
 a lot of frames and we never see that all of them are loaded.

This patch is returning to our previous logic, namely to check that only target
frame is loaded, but not to check its subframes.

BUG=569577
==========

to

==========
Fix saving password in Password Manager on nytimes.com.

In refactoring on https://codereview.chromium.org/1381003004 it was implemented
that PasswordAutofiilAgent considers that target frame is loaded only if all its
subframes are loaded. But this could never happen, e.g. on nytimes.com there are
 a lot of frames and we never see that all of them are loaded.

This patch is returning to our previous logic, namely to check that only target
frame is loaded, but not to check its subframes.

BUG=569577

Committed: https://crrev.com/3d0810ccce9f38b9f2acb0441743f44b67af28a9
Cr-Commit-Position: refs/heads/master@{#365827}
==========

[commit-bot: I haz the power, 2015-12-17 15:34:18]

Patchset 2 (id:??) landed as
https://crrev.com/3d0810ccce9f38b9f2acb0441743f44b67af28a9
Cr-Commit-Position: refs/heads/master@{#365827}