PasswordManager: Add rendered forms to pending_login_managers_

PasswordManager: Add rendered forms to pending_login_managers_

|pending_login_managers_| is the list of all password form managers for forms on the current page. It is important when we detect a password to save -- it can only be saved if it correspondes to a pending login manager, which means to a form on the page the user is navigating from.

The list is currently updated on 2 occasions:
1) when the forms on the current page are parsed, and
2) when JavaScript adds a new form.

The exception to 2) is when the main frame loads -- we don't add forms added during that time, because they might be just confusing fragments.

It has been observed though, that there are still dynamically created forms which are not just fragments, but which fall prey to the above exception.

Those forms have been observed during the phase forms are actually rendered. So this CL adds a new place to update |pending_login_managers_|:
3) when the forms on the current page are rendered.

BUG=367768
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278704

[vabr (Chromium), 2014-06-18 16:45:58]

Hi Garrett,

Since you have been helping me on the bug, would you like to also review this?

Thanks!
Vaclav

[Garrett Casto, 2014-06-19 00:05:40]

lgtm

Few nits, fix itself looks good.

https://codereview.chromium.org/343663002/diff/40001/chrome/test/data/passwor...
File chrome/test/data/password/between_parsing_and_rendering.html (right):

https://codereview.chromium.org/343663002/diff/40001/chrome/test/data/passwor...
chrome/test/data/password/between_parsing_and_rendering.html:16: To make sure
the frame loads after the form creation, this page
I think that this comment isn't quite accurate. I believe that the ordering of
the frame loading vs. form creation should always be the same, it's just a
matter of having enough time elapsed between form creation and frame loading so
that we won't get callback from OnDynamicFormsSeen after frame loading is
finished. Is that your understanding as well?

https://codereview.chromium.org/343663002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/343663002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:338:
->DoesManage(*iter, PasswordFormManager::ACTION_MATCH_REQUIRED))
Nit: Is it possible to line break like

if ((*old_manager)->DoesManage(*iter,
       ...

I personally don't like breaking at "->" but I realize other people have
different opinions.

[vabr (Chromium), 2014-06-20 07:55:07]

Thanks, Garrett!

Comments addressed, sending into CQ.

Cheers,
Vaclav

https://codereview.chromium.org/343663002/diff/40001/chrome/test/data/passwor...
File chrome/test/data/password/between_parsing_and_rendering.html (right):

https://codereview.chromium.org/343663002/diff/40001/chrome/test/data/passwor...
chrome/test/data/password/between_parsing_and_rendering.html:16: To make sure
the frame loads after the form creation, this page
On 2014/06/19 00:05:40, Garrett Casto wrote:
> I think that this comment isn't quite accurate. I believe that the ordering of
> the frame loading vs. form creation should always be the same, it's just a
> matter of having enough time elapsed between form creation and frame loading
so
> that we won't get callback from OnDynamicFormsSeen after frame loading is
> finished. Is that your understanding as well?

Agreed. I tried to amend the comment.

https://codereview.chromium.org/343663002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/343663002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:338:
->DoesManage(*iter, PasswordFormManager::ACTION_MATCH_REQUIRED))
On 2014/06/19 00:05:40, Garrett Casto wrote:
> Nit: Is it possible to line break like
> 
> if ((*old_manager)->DoesManage(*iter,
>        ...
> 
> I personally don't like breaking at "->" but I realize other people have
> different opinions.

Unfortunately, the second argument is 1 character too long for any indentation
started as in your suggestion to be possible. :)
Also, I prefer to keep what clang-format indented, to make future refactorings
easier.

But, I managed to address your comment by slightly changing the code, where I
don't keep the old_manager iterator outside the loop, and instead keep just a
bool signifying whether an old manager was found or not.

That makes the code more readable IMO, and as a side-effect leads to a different
indentation of the DoesManager line. :)

Please let me know if you have further objections, I'm happy to address them in
a follow-up CL.

[vabr (Chromium), 2014-06-20 07:55:11]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-06-20 07:57:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vabr@chromium.org/343663002/60001

[commit-bot: I haz the power, 2014-06-20 13:13:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-20 13:58:38]

Change committed as 278704