Autofill & passwords: Replace NavigationEntryCommitted with DidFinishNavigation

Autofill & passwords: Replace NavigationEntryCommitted with DidFinishNavigation

The former is a possible candidate for deletion, the latter is a suitable replacement in the new navigation API.

BUG=576287
Committed: https://crrev.com/393a31d6c2ca9001b1985bf1cb426ca144205163
Cr-Commit-Position: refs/heads/master@{#381725}

[vabr (Chromium), 2016-03-15 15:12:16]

Description was changed from

==========
Autofill & passwords: Replace NavigationEntryCommitted with DidFinishNavigation

BUG=576287
==========

to

==========
Autofill & passwords: Replace NavigationEntryCommitted with DidFinishNavigation

The former is a possible candidate for deletion, the latter is a suitable
replacement in the new navigation API.

BUG=576287
==========

[vabr (Chromium), 2016-03-15 16:34:32]

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

[vabr (Chromium), 2016-03-15 16:34:32]

Hi Nasko,

Thanks for your advice in http://crbug.com/576287#c11 !

Could you PTAL?

Cheers,
Vaclav

[nasko, 2016-03-16 18:20:04]

https://codereview.chromium.org/1806593002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/1806593002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_test_base.h:45:
content::NavigationHandle* navigation_handle) override;
very minor nit: Timewise, navigation is finished, then document load starts,
then document load finishes. So moving this above the other method matches
logical progression.

https://codereview.chromium.org/1806593002/diff/1/components/autofill/content...
File components/autofill/content/browser/content_autofill_driver_factory.cc
(right):

https://codereview.chromium.org/1806593002/diff/1/components/autofill/content...
components/autofill/content/browser/content_autofill_driver_factory.cc:101: if
(navigation_handle->HasCommitted())
Do you care if this was successful commit or an error page? If you do, you
probably want to call IsErrorPage.

[vabr (Chromium), 2016-03-17 15:36:29]

Thank you, Nasko!

One comment answered, the other addressed. PTAL.

Cheers,
Vaclav

https://codereview.chromium.org/1806593002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/1806593002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_test_base.h:45:
content::NavigationHandle* navigation_handle) override;
On 2016/03/16 18:20:04, nasko (slow) wrote:
> very minor nit: Timewise, navigation is finished, then document load starts,
> then document load finishes. So moving this above the other method matches
> logical progression.

Done.

https://codereview.chromium.org/1806593002/diff/1/components/autofill/content...
File components/autofill/content/browser/content_autofill_driver_factory.cc
(right):

https://codereview.chromium.org/1806593002/diff/1/components/autofill/content...
components/autofill/content/browser/content_autofill_driver_factory.cc:101: if
(navigation_handle->HasCommitted())
On 2016/03/16 18:20:04, nasko (slow) wrote:
> Do you care if this was successful commit or an error page? If you do, you
> probably want to call IsErrorPage.

I thought about it, and whether successful or an error page, the autofill pop-up
likely no longer has the form to be shown with, so it should be hidden.

[nasko, 2016-03-17 15:48:53]

LGTM

[vabr (Chromium), 2016-03-17 16:03:12]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-03-17 16:03:30]

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

[commit-bot: I haz the power, 2016-03-17 16:16:57]

Description was changed from

==========
Autofill & passwords: Replace NavigationEntryCommitted with DidFinishNavigation

The former is a possible candidate for deletion, the latter is a suitable
replacement in the new navigation API.

BUG=576287
==========

to

==========
Autofill & passwords: Replace NavigationEntryCommitted with DidFinishNavigation

The former is a possible candidate for deletion, the latter is a suitable
replacement in the new navigation API.

BUG=576287
==========

[commit-bot: I haz the power, 2016-03-17 16:16:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-17 16:18:04]

Description was changed from

==========
Autofill & passwords: Replace NavigationEntryCommitted with DidFinishNavigation

The former is a possible candidate for deletion, the latter is a suitable
replacement in the new navigation API.

BUG=576287
==========

to

==========
Autofill & passwords: Replace NavigationEntryCommitted with DidFinishNavigation

The former is a possible candidate for deletion, the latter is a suitable
replacement in the new navigation API.

BUG=576287

Committed: https://crrev.com/393a31d6c2ca9001b1985bf1cb426ca144205163
Cr-Commit-Position: refs/heads/master@{#381725}
==========

[commit-bot: I haz the power, 2016-03-17 16:18:06]

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