[Password Manager] Show omnibox icon and anchored prompt once user start typing password

[Password Manager] Show omnibox icon and anchored prompt once users start typing password

If the password to be save is a generated by Chrome password, the confirmation bubble will be shown (the bubble that a user sees when she submits a form with generated password). 

If the password is not a generated one, a save or update bubble will be shown.

BUG=725883
Review-Url: https://codereview.chromium.org/2915763003
Cr-Commit-Position: refs/heads/master@{#493337}
Committed: https://chromium.googlesource.com/chromium/src/+/7cf7408af869f5b605a6f35cdd458380c56fb73d

[kolos1, 2017-05-31 14:06:31]

Description was changed from

==========
o


Merge branch 'master' of https://chromium.googlesource.com/chromium/src into
mf_show_icon

o

o


Merge branch 'mf_scoped_refptr' into mf_show_icon

o

Merge branch 'master' of https://chromium.googlesource.com/chromium/src into
mf_scoped_refptr

o

o


o


Merge branch 'master' of https://chromium.googlesource.com/chromium/src into
mf_scoped_refptr

o

o


o


o


o


o


o


o


Merge branch 'master' of https://chromium.googlesource.com/chromium/src into
mf_scoped_refptr oo


o


o


Merge branch 'master' of https://chromium.googlesource.com/chromium/src into
mf_scoped_refptr

o

o


o


o


o


o


o


Merge branch 'master' of https://chromium.googlesource.com/chromium/src into
mf_scoped_refptr

o

o


Merge branc
BUG=
==========

to

==========
[Password Manager] Show omnibox icon and anchored prompt once user start typing
password

BUG=725883
==========

[kolos1, 2017-07-11 09:52:39]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 09:52:56]

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-07-11 10:37:09]

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

[commit-bot: I haz the power, 2017-07-11 10:37:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kolos1, 2017-07-12 14:10:53]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-12 14:11:03]

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-07-12 15:45:01]

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

[commit-bot: I haz the power, 2017-07-12 15:45:02]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-07-14 11:12:54]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-14 11:12:57]

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-07-14 11:48:07]

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

[commit-bot: I haz the power, 2017-07-14 11:48:09]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[kolos1, 2017-07-14 13:24:52]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-14 13:24:58]

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-07-14 15:32:31]

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

[commit-bot: I haz the power, 2017-07-14 15:32:32]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-07-19 09:56:19]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-19 09:56:30]

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-07-19 10:42:40]

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

[commit-bot: I haz the power, 2017-07-19 10:42:42]

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

[kolos1, 2017-07-19 12:48:25]

Patchset #14 (id:260001) has been deleted

[kolos1, 2017-07-20 15:23:34]

kolos@chromium.org changed reviewers:
+ vasilii@chromium.org

[kolos1, 2017-07-20 15:23:35]

Hi Vasilii,

Please review this CL.

Regards,
Maxim

[kolos1, 2017-07-21 11:04:55]

Patchset #18 (id:360001) has been deleted

[vasilii, 2017-07-21 12:48:21]

Please next time upload smaller patches. That one could easily be split into
three well-defined pieces.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:102:
passwords_data_.OnAutomaticPasswordSave(std::move(form_manager));
That I don't understand. What scenario do you keep in mind and why is UI
confused with OnShowManualFallback in that case?

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:818:
controller()->OnShowManualFallback(std::move(test_form_manager), is_update);
You can also check opened_bubble()

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:819:
ExpectIconStateIs(is_update
Why not ExpectIconAndControllerStateIs?

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:824:
EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility());
Why do we expect it only here?

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:872:
controller()->OnHideManualFallback();
I need more info what's being tested here.
OnBubbleHidden() is a bug because you don't pop up the bubble.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/passwords_client_ui_delegate.h (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/passwords_client_ui_delegate.h:42: // save/update
prompt anchored to the omnibox icon but keeps it hidden.
It doesn't create any prompt. Only the icon.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/passwords_client_ui_delegate.h:43: virtual void
OnShowManualFallback(
ForSaving

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/passwords_client_ui_delegate.h:48: // save/update
prompt and switch UI to |MANAGE_STATE|.
There is no a hidden prompt at any point in time. It just switches the state.
Also it's interesting to know what happens if the bubble is currently open.

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
File chrome/renderer/autofill/fake_content_password_manager_driver.h (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:133: int
called_show_manual_fallback_cnt() {
const

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:222: int
called_show_manual_fallback_cnt_ = 0;
What is "cnt"? Probably should be renamed to "count"

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:3086: //
available to save the entered value. It's a subject to re-consider.
Why do we need to reconsider it?

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:3107: // Clear
all password fields. The fallback should be disabled.
What happens if you have two password fields. The user
- edits the first password
- edits the second password
- removes the second password

Is the manual fallback shown?

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
File components/autofill/content/common/autofill_driver.mojom (right):

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
components/autofill/content/common/autofill_driver.mojom:82: // be available.
Can we get a bit lower level here. Describe what triggers it. Same below.

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
components/autofill/content/common/autofill_driver.mojom:83:
ShowManualFallback(PasswordForm password_form);
Emphasize that it's for saving. Same below.

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:1782: if
(password_manager::ManualFallbackForSavingEnabled()) {
Are you sure we need this check here? It introduces a dependency. We are going
to launch this feature anyway. Why not filtering those calls somewhere in the
browser?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:140: bool
IsPasswordUpdate(const PasswordFormManager& provisional_save_manager) {
Comment?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:489: if
(provisional_save_manager_->form_fetcher()->GetState() !=
== WAITING?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.h:158: void
ShowManualFallback(password_manager::PasswordManagerDriver* driver,
ForSaving

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager_driver.h (right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_driver.h:81: // Tells
the driver to show the manual fallback for password saving, i.e. to
Who tells whom? I think PasswordManagerDriver doesn't need this events.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:87:
ShowManualFallbackPtr(manager.release(), is_update);
Memory leak here?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1828:
.WillRepeatedly(WithArg<1>(InvokeConsumer(stored_form)));
I'm concerned if it happens more than once. Is it true?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1834:
EXPECT_CALL(client_, ShowManualFallbackPtr(_, true));
Why don't we check anything about the form being passed

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1868:
EXPECT_CALL(client_, ShowManualFallbackPtr(_, false));
Move down to ShowManualFallback. Do I understand right that the fallback isn't
shown automatically upon receiving the forms?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/common/password_manager_features.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/common/password_manager_features.cc:23:
"enable-manual-fallback-for-password-saving",
The documentation asks to use CamelCase for the name

https://codereview.chromium.org/2915763003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right):

https://codereview.chromium.org/2915763003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:203:
(!is_browser_shortcut || initial_key_event.text[0] == '\b')) {
Can you clarify this?

[kolos1, 2017-07-24 14:02:05]

Patchset #19 (id:400001) has been deleted

[kolos1, 2017-07-24 14:50:35]

Patchset #18 (id:380001) has been deleted

[kolos1, 2017-07-24 15:32:35]

Description was changed from

==========
[Password Manager] Show omnibox icon and anchored prompt once user start typing
password

BUG=725883
==========

to

==========
[Password Manager] Show omnibox icon and anchored prompt once users start typing
password

If the password to be save is a generated by Chrome password, the confirmation
bubble will be shown (the bubble that a user sees when she submits a form with
generated password). 

If the password is not a generated one, a save or update bubble will be shown.

BUG=725883
==========

[kolos1, 2017-07-24 15:32:59]

Patchset #18 (id:420001) has been deleted

[kolos1, 2017-07-24 15:33:31]

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:102:
passwords_data_.OnAutomaticPasswordSave(std::move(form_manager));
On 2017/07/21 12:48:19, vasilii wrote:
> That I don't understand. What scenario do you keep in mind and why is UI
> confused with OnShowManualFallback in that case?
If a user accepts a generated password, the icon appears in the omnibox and the
confirmation bubble is anchored to that icon.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:818:
controller()->OnShowManualFallback(std::move(test_form_manager), is_update);
On 2017/07/21 12:48:19, vasilii wrote:
> You can also check opened_bubble()

Done.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:819:
ExpectIconStateIs(is_update
On 2017/07/21 12:48:20, vasilii wrote:
> Why not ExpectIconAndControllerStateIs?

Done.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:824:
EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility());
On 2017/07/21 12:48:19, vasilii wrote:
> Why do we expect it only here?
It is strange to me too. UpdatePassword calls UpdateBubbleAndIconVisibility, but
SavePassword doesn't.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:872:
controller()->OnHideManualFallback();
On 2017/07/21 12:48:20, vasilii wrote:
> I need more info what's being tested here.
> OnBubbleHidden() is a bug because you don't pop up the bubble.
Added to comments to all tests.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/passwords_client_ui_delegate.h (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/passwords_client_ui_delegate.h:42: // save/update
prompt anchored to the omnibox icon but keeps it hidden.
On 2017/07/21 12:48:20, vasilii wrote:
> It doesn't create any prompt. Only the icon.

Done.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/passwords_client_ui_delegate.h:43: virtual void
OnShowManualFallback(
On 2017/07/21 12:48:20, vasilii wrote:
> ForSaving

Done.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/passwords_client_ui_delegate.h:48: // save/update
prompt and switch UI to |MANAGE_STATE|.
On 2017/07/21 12:48:20, vasilii wrote:
> There is no a hidden prompt at any point in time. It just switches the state.
> Also it's interesting to know what happens if the bubble is currently open.

Done.

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
File chrome/renderer/autofill/fake_content_password_manager_driver.h (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:133: int
called_show_manual_fallback_cnt() {
On 2017/07/21 12:48:20, vasilii wrote:
> const

Done.

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:222: int
called_show_manual_fallback_cnt_ = 0;
On 2017/07/21 12:48:20, vasilii wrote:
> What is "cnt"? Probably should be renamed to "count"

Done.

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:3086: //
available to save the entered value. It's a subject to re-consider.
On 2017/07/21 12:48:20, vasilii wrote:
> Why do we need to reconsider it?
Below the user types into the new password field. So, if there is both current
and new passwords fields, perhaps it doesn't make sense to enable the fallback
with current password... On the other hand, our heuristic may fail to classify
password fields.

I will remove that comment. It is confusing.

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:3107: // Clear
all password fields. The fallback should be disabled.
On 2017/07/21 12:48:20, vasilii wrote:
> What happens if you have two password fields. The user
> - edits the first password
> - edits the second password
> - removes the second password
> 
> Is the manual fallback shown?
The manual fallback relies on what GetPasswordForm returns. If there will be
non-empty |password_value| or |new_password_value|, there is the password to
save. If both, the fallback will save new password value.

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
File components/autofill/content/common/autofill_driver.mojom (right):

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
components/autofill/content/common/autofill_driver.mojom:82: // be available.
On 2017/07/21 12:48:20, vasilii wrote:
> Can we get a bit lower level here. Describe what triggers it. Same below.

Done.

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
components/autofill/content/common/autofill_driver.mojom:83:
ShowManualFallback(PasswordForm password_form);
On 2017/07/21 12:48:20, vasilii wrote:
> Emphasize that it's for saving. Same below.

Done.

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:1782: if
(password_manager::ManualFallbackForSavingEnabled()) {
On 2017/07/21 12:48:20, vasilii wrote:
> Are you sure we need this check here? It introduces a dependency. We are going
> to launch this feature anyway. Why not filtering those calls somewhere in the
> browser?

The flag is removed.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:140: bool
IsPasswordUpdate(const PasswordFormManager& provisional_save_manager) {
On 2017/07/21 12:48:20, vasilii wrote:
> Comment?

Done.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:489: if
(provisional_save_manager_->form_fetcher()->GetState() !=
On 2017/07/21 12:48:20, vasilii wrote:
> == WAITING?

Done.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.h:158: void
ShowManualFallback(password_manager::PasswordManagerDriver* driver,
On 2017/07/21 12:48:20, vasilii wrote:
> ForSaving

Done.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager_driver.h (right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_driver.h:81: // Tells
the driver to show the manual fallback for password saving, i.e. to
On 2017/07/21 12:48:21, vasilii wrote:
> Who tells whom? I think PasswordManagerDriver doesn't need this events.
"Tells the driver..." is pattern for comments for this class. See below and
above. 

PasswordAutofillAgent calls these functions.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:87:
ShowManualFallbackPtr(manager.release(), is_update);
On 2017/07/21 12:48:21, vasilii wrote:
> Memory leak here?
Fixed.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1828:
.WillRepeatedly(WithArg<1>(InvokeConsumer(stored_form)));
On 2017/07/21 12:48:21, vasilii wrote:
> I'm concerned if it happens more than once. Is it true?
Yes, it is called twice.

In tests above, WillRepeatedly is used.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1834:
EXPECT_CALL(client_, ShowManualFallbackPtr(_, true));
On 2017/07/21 12:48:21, vasilii wrote:
> Why don't we check anything about the form being passed
Added some checks.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1868:
EXPECT_CALL(client_, ShowManualFallbackPtr(_, false));
On 2017/07/21 12:48:21, vasilii wrote:
> Move down to ShowManualFallback. Do I understand right that the fallback isn't
> shown automatically upon receiving the forms?
Good point. Done.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/common/password_manager_features.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/common/password_manager_features.cc:23:
"enable-manual-fallback-for-password-saving",
On 2017/07/21 12:48:21, vasilii wrote:
> The documentation asks to use CamelCase for the name
Not sure I understand you. 
A name of constant should start with "k"
(https://google.github.io/styleguide/cppguide.html#Constant_Names)

https://codereview.chromium.org/2915763003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right):

https://codereview.chromium.org/2915763003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:203:
(!is_browser_shortcut || initial_key_event.text[0] == '\b')) {
On 2017/07/21 12:48:21, vasilii wrote:
> Can you clarify this?
Please ignore changes in this file. It is a quick patch to fix "the backspace
bug".

[vasilii, 2017-07-25 11:17:47]

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:102:
passwords_data_.OnAutomaticPasswordSave(std::move(form_manager));
On 2017/07/24 15:33:29, kolos1 wrote:
> On 2017/07/21 12:48:19, vasilii wrote:
> > That I don't understand. What scenario do you keep in mind and why is UI
> > confused with OnShowManualFallback in that case?
> If a user accepts a generated password, the icon appears in the omnibox and
the
> confirmation bubble is anchored to that icon.

That password is saved automatically and the confirmation bubble has a very
little value because there is no option to view/edit anything.
We discussed on the meeting briefly that for now it's fine if we don't provide
manual fallbacks for the generation case because the user will have the correct
password saved anyway. Thus, I think we shouldn't call this function at all. Of
course the icon may appear for manual fallback on another form on the page.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:824:
EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility());
On 2017/07/24 15:33:30, kolos1 wrote:
> On 2017/07/21 12:48:19, vasilii wrote:
> > Why do we expect it only here?
> It is strange to me too. UpdatePassword calls UpdateBubbleAndIconVisibility,
but
> SavePassword doesn't.

Ahh, that's my own trick. The function still should be called. But after
OnBubbleHidden(). In the real life that method always follows
UpdatePassword/SavePassword

No need to address.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1828:
.WillRepeatedly(WithArg<1>(InvokeConsumer(stored_form)));
On 2017/07/24 15:33:30, kolos1 wrote:
> On 2017/07/21 12:48:21, vasilii wrote:
> > I'm concerned if it happens more than once. Is it true?
> Yes, it is called twice.
> 
> In tests above, WillRepeatedly is used.

Why is it called twice if the form is only one?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/common/password_manager_features.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/common/password_manager_features.cc:23:
"enable-manual-fallback-for-password-saving",
On 2017/07/24 15:33:31, kolos1 wrote:
> On 2017/07/21 12:48:21, vasilii wrote:
> > The documentation asks to use CamelCase for the name
> Not sure I understand you. 
> A name of constant should start with "k"
> (https://google.github.io/styleguide/cppguide.html#Constant_Names)

I commented on "enable-manual-fallback-for-password-saving", not on the variable

https://codereview.chromium.org/2915763003/diff/440001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/440001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:833: }
How about clearing the expectations on controller() after every iteration?

https://codereview.chromium.org/2915763003/diff/440001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/440001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1853:
EXPECT_THAT(new_form, FormMatches(*form_manager_to_save->submitted_form()));
Check pending_credentials(). The UI is entirely based on it.

Also the expectation should be on |form_manager_to_save| and not on |new_form|

[kolos1, 2017-07-26 13:40:58]

Hi Vasilii,

PTAL.

Please skip the generation related issues. We will discuss them tomorrow. 

Regards,
Maxim

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:102:
passwords_data_.OnAutomaticPasswordSave(std::move(form_manager));
On 2017/07/25 11:17:46, vasilii wrote:
> On 2017/07/24 15:33:29, kolos1 wrote:
> > On 2017/07/21 12:48:19, vasilii wrote:
> > > That I don't understand. What scenario do you keep in mind and why is UI
> > > confused with OnShowManualFallback in that case?
> > If a user accepts a generated password, the icon appears in the omnibox and
> the
> > confirmation bubble is anchored to that icon.
> 
> That password is saved automatically and the confirmation bubble has a very
> little value because there is no option to view/edit anything.
> We discussed on the meeting briefly that for now it's fine if we don't provide
> manual fallbacks for the generation case because the user will have the
correct
> password saved anyway. Thus, I think we shouldn't call this function at all.
Of
> course the icon may appear for manual fallback on another form on the page.

As discussed in person, let's put the question to the team meeting agenda.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:824:
EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility());
On 2017/07/25 11:17:46, vasilii wrote:
> On 2017/07/24 15:33:30, kolos1 wrote:
> > On 2017/07/21 12:48:19, vasilii wrote:
> > > Why do we expect it only here?
> > It is strange to me too. UpdatePassword calls UpdateBubbleAndIconVisibility,
> but
> > SavePassword doesn't.
> 
> Ahh, that's my own trick. The function still should be called. But after
> OnBubbleHidden(). In the real life that method always follows
> UpdatePassword/SavePassword
> 
> No need to address.

Acknowledged.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1828:
.WillRepeatedly(WithArg<1>(InvokeConsumer(stored_form)));
On 2017/07/25 11:17:46, vasilii wrote:
> On 2017/07/24 15:33:30, kolos1 wrote:
> > On 2017/07/21 12:48:21, vasilii wrote:
> > > I'm concerned if it happens more than once. Is it true?
> > Yes, it is called twice.
> > 
> > In tests above, WillRepeatedly is used.
> 
> Why is it called twice if the form is only one?

Investigated the case. Here it is called once actually. Fixed.

In the test below, it is called twice, because of cloning PasswordFormManager
for showing the fallback. It clones FormFetcher too. Since the original 
FormFetcher is still waiting for credentials, the clone will fetch credentials
too.
Fixed the second test and added a comment.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/common/password_manager_features.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/common/password_manager_features.cc:23:
"enable-manual-fallback-for-password-saving",
On 2017/07/25 11:17:46, vasilii wrote:
> On 2017/07/24 15:33:31, kolos1 wrote:
> > On 2017/07/21 12:48:21, vasilii wrote:
> > > The documentation asks to use CamelCase for the name
> > Not sure I understand you. 
> > A name of constant should start with "k"
> > (https://google.github.io/styleguide/cppguide.html#Constant_Names)
> 
> I commented on "enable-manual-fallback-for-password-saving", not on the
variable
Changes in this file are reverted.

https://codereview.chromium.org/2915763003/diff/440001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/440001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:833: }
On 2017/07/25 11:17:47, vasilii wrote:
> How about clearing the expectations on controller() after every iteration?

Done.

https://codereview.chromium.org/2915763003/diff/440001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/440001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1853:
EXPECT_THAT(new_form, FormMatches(*form_manager_to_save->submitted_form()));
On 2017/07/25 11:17:47, vasilii wrote:
> Check pending_credentials(). The UI is entirely based on it.
> 
> Also the expectation should be on |form_manager_to_save| and not on |new_form|

Done.

[vasilii, 2017-07-26 14:59:33]

https://codereview.chromium.org/2915763003/diff/460001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/460001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1875: //
credentials too.
I'm confused about the comment. The test and the implementation are clear that
the fallback isn't shown until we get the results from the store. Thus, cloning
shouldn't happen.

[kolos1, 2017-07-27 09:54:31]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-27 09:54:37]

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-07-27 09:58:27]

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

[commit-bot: I haz the power, 2017-07-27 09:58:28]

Dry run: 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...)

[kolos1, 2017-07-28 08:24:14]

Patchset #23 (id:540001) has been deleted

[kolos1, 2017-07-28 08:24:43]

Patchset #20 (id:480001) has been deleted

[kolos1, 2017-07-28 08:24:54]

Patchset #20 (id:500001) has been deleted

[kolos1, 2017-07-28 08:25:06]

Patchset #20 (id:520001) has been deleted

[kolos1, 2017-08-01 09:23:09]

Patchset #21 (id:580001) has been deleted

[kolos1, 2017-08-01 10:18:56]

Patchset #21 (id:600001) has been deleted

[kolos1, 2017-08-01 10:19:03]

Patchset #20 (id:560001) has been deleted

[kolos1, 2017-08-01 10:19:26]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-01 10:19:39]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-08-01 11:04:35]

Hi Vasilii,

PTAL. I added a test to password_manager/password_manager_interactive_uitest.cc
and changes some functions for browser/interactive tests.

Regards,
Maxim

https://codereview.chromium.org/2915763003/diff/460001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/460001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1875: //
credentials too.
On 2017/07/26 14:59:33, vasilii wrote:
> I'm confused about the comment. The test and the implementation are clear that
> the fallback isn't shown until we get the results from the store. Thus,
cloning
> shouldn't happen.

The issue fixed. The comment is not needed anymore.

[commit-bot: I haz the power, 2017-08-01 11:11:30]

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

[commit-bot: I haz the power, 2017-08-01 11:11:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kolos1, 2017-08-01 11:37:28]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-01 11:37:40]

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-08-01 12:16:08]

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

[commit-bot: I haz the power, 2017-08-01 12:16:09]

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

[kolos1, 2017-08-02 13:05:01]

Patchset #20 (id:620001) has been deleted

[kolos1, 2017-08-02 13:15:52]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-02 13:16:10]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-08-02 13:20:05]

Patchset #20 (id:640001) has been deleted

[commit-bot: I haz the power, 2017-08-02 15:36:40]

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

[commit-bot: I haz the power, 2017-08-02 15:36:41]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[vasilii, 2017-08-02 17:41:11]

I'll keep reviewing it tomorrow.

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:179: // password
store contains only 1 credential.
I don't get from the comment if the method check that there is just one
credential or not.

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:181:
password_manager::TestPasswordStore* password_store,
What's the point to pass the store if it's essentially a singleton that it easy
to obtain?

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:101: if
(form_manager->has_generated_password())
Didn't we agree to drop it for now?

[kolos1, 2017-08-03 07:58:02]

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:179: // password
store contains only 1 credential.
On 2017/08/02 17:41:11, vasilii wrote:
> I don't get from the comment if the method check that there is just one
> credential or not.

Changed the comment.

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:181:
password_manager::TestPasswordStore* password_store,
On 2017/08/02 17:41:11, vasilii wrote:
> What's the point to pass the store if it's essentially a singleton that it
easy
> to obtain?

I just copied the function from
https://cs.chromium.org/chromium/src/chrome/browser/password_manager/password....


Yes, it can be obtained within the function, but the tests that use
CheckThatCredentialsStored already obtained the password store and use it for
additional checks. So, let them pass the store to the function.

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:101: if
(form_manager->has_generated_password())
On 2017/08/02 17:41:11, vasilii wrote:
> Didn't we agree to drop it for now?

IIUC, the team agreed that we should show a confirmation bubble for generation
cases. 

Do you mean to pass |has_generated_password| as an argument?

[vasilii, 2017-08-03 08:59:15]

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:181:
password_manager::TestPasswordStore* password_store,
On 2017/08/03 07:58:01, kolos1 wrote:
> On 2017/08/02 17:41:11, vasilii wrote:
> > What's the point to pass the store if it's essentially a singleton that it
> easy
> > to obtain?
> 
> I just copied the function from
>
https://cs.chromium.org/chromium/src/chrome/browser/password_manager/password....
> 
> 
> Yes, it can be obtained within the function, but the tests that use
> CheckThatCredentialsStored already obtained the password store and use it for
> additional checks. So, let them pass the store to the function. 

It wasn't a method of a class.

[kolos1, 2017-08-03 09:08:03]

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:181:
password_manager::TestPasswordStore* password_store,
On 2017/08/03 08:59:15, vasilii wrote:
> On 2017/08/03 07:58:01, kolos1 wrote:
> > On 2017/08/02 17:41:11, vasilii wrote:
> > > What's the point to pass the store if it's essentially a singleton that it
> > easy
> > > to obtain?
> > 
> > I just copied the function from
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/password_manager/password....
> > 
> > 
> > Yes, it can be obtained within the function, but the tests that use
> > CheckThatCredentialsStored already obtained the password store and use it
for
> > additional checks. So, let them pass the store to the function. 
> 
> It wasn't a method of a class.

No, it wasn't. But in PasswordManagerBrowserTestBase we still have to call 
PasswordStoreFactory::GetForProfile
(https://cs.chromium.org/chromium/src/chrome/browser/password_manager/password...).
There is no direct pointer to the store or so, is there?

Is it not easier to pass the pointer to the store as an argument?

[vasilii, 2017-08-03 09:55:39]

More comments are coming.

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:181:
password_manager::TestPasswordStore* password_store,
On 2017/08/03 09:08:03, kolos1 wrote:
> On 2017/08/03 08:59:15, vasilii wrote:
> > On 2017/08/03 07:58:01, kolos1 wrote:
> > > On 2017/08/02 17:41:11, vasilii wrote:
> > > > What's the point to pass the store if it's essentially a singleton that
it
> > > easy
> > > > to obtain?
> > > 
> > > I just copied the function from
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/password_manager/password....
> > > 
> > > 
> > > Yes, it can be obtained within the function, but the tests that use
> > > CheckThatCredentialsStored already obtained the password store and use it
> for
> > > additional checks. So, let them pass the store to the function. 
> > 
> > It wasn't a method of a class.
> 
> No, it wasn't. But in PasswordManagerBrowserTestBase we still have to call 
> PasswordStoreFactory::GetForProfile
>
(https://cs.chromium.org/chromium/src/chrome/browser/password_manager/password...).
> There is no direct pointer to the store or so, is there?
> 
> Is it not easier to pass the pointer to the store as an argument?

From the point of view of the caller, obviously 2 arguments are better than 3.
Also I can easily imagine a test (e.g.
PasswordManagerBrowserTestBase.PromptForNormalSubmit) that doesn't need this
|store| pointer except for passing to the method. It hampers readabilty.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:100: bool
was_prompt_shown_;
was_prompt_automatically_shown_?

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:150: if
(target_state_ == password_manager::ui::PENDING_PASSWORD_STATE) {
I think this all the existing callers of WaitForSavePrompt() are looking forward
to the automatic prompt. We shouldn't downgrade their expectations to the manual
fallback.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:250: bool
BubbleObserver::WasSavePromptShown() const {
Not sure why those methods are called "Was..". Shouldn't it be Is.. ?

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:279: void
BubbleObserver::OpenAndAcceptSavePrompt() const {
Not sure why we need this method. It's almost a duplicate of AcceptSavePrompt.
For AcceptSavePrompt we checked only the state just to make sure that
passwords_ui_controller_->SavePassword won't crash. It seems OK to continue
doing so. If a test cares about automatic save prompt then it should call
WaitForSavePrompt() or ASSERT_TRUE(WasSavePromptShown()) explicitly.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:71: // Checks if
the save prompt is being currently available.
Clarify that it can be both due to both manual fallback or successful login.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:77: // Checks if
the save prompt was shown.
was shown automatically?

[vasilii, 2017-08-03 10:46:37]

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_interactive_uitest.cc
(right):

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_interactive_uitest.cc:112:
prompt_observer.WaitForSavePrompt();
I think that is the only place which waits for the manual fallback and it's
confusing. There is no prompt in this test. We are waiting for the state. I'd
fix the name.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_interactive_uitest.cc:123:
CheckThatCredentialsStored(password_store.get(), base::string16(),
One more example where we don't need |password_store| in the test.

https://codereview.chromium.org/2915763003/diff/660002/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/660002/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1886: 
How about a test for the generation scenario?

[kolos1, 2017-08-04 08:53:33]

Patchset #23 (id:710001) has been deleted

[kolos1, 2017-08-04 09:14:19]

Patchset #23 (id:730001) has been deleted

[kolos1, 2017-08-04 09:16:14]

Patchset #23 (id:750001) has been deleted

[kolos1, 2017-08-04 12:12:20]

Patchset #23 (id:770001) has been deleted

[kolos1, 2017-08-04 16:18:28]

Patchset #24 (id:810001) has been deleted

[kolos1, 2017-08-04 16:24:17]

Patchset #24 (id:830001) has been deleted

[kolos1, 2017-08-04 16:24:54]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-04 16:25:05]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-08-04 16:36:18]

Hi Vasilii,

PTAL. 

Thanks,
Maxim

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:181:
password_manager::TestPasswordStore* password_store,
On 2017/08/03 09:55:38, vasilii wrote:
> On 2017/08/03 09:08:03, kolos1 wrote:
> > On 2017/08/03 08:59:15, vasilii wrote:
> > > On 2017/08/03 07:58:01, kolos1 wrote:
> > > > On 2017/08/02 17:41:11, vasilii wrote:
> > > > > What's the point to pass the store if it's essentially a singleton
that
> it
> > > > easy
> > > > > to obtain?
> > > > 
> > > > I just copied the function from
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/password_manager/password....
> > > > 
> > > > 
> > > > Yes, it can be obtained within the function, but the tests that use
> > > > CheckThatCredentialsStored already obtained the password store and use
it
> > for
> > > > additional checks. So, let them pass the store to the function. 
> > > 
> > > It wasn't a method of a class.
> > 
> > No, it wasn't. But in PasswordManagerBrowserTestBase we still have to call 
> > PasswordStoreFactory::GetForProfile
> >
>
(https://cs.chromium.org/chromium/src/chrome/browser/password_manager/password...).
> > There is no direct pointer to the store or so, is there?
> > 
> > Is it not easier to pass the pointer to the store as an argument?
> 
> From the point of view of the caller, obviously 2 arguments are better than 3.
> Also I can easily imagine a test (e.g.
> PasswordManagerBrowserTestBase.PromptForNormalSubmit) that doesn't need this
> |store| pointer except for passing to the method. It hampers readabilty.

Done.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_interactive_uitest.cc
(right):

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_interactive_uitest.cc:112:
prompt_observer.WaitForSavePrompt();
On 2017/08/03 10:46:37, vasilii wrote:
> I think that is the only place which waits for the manual fallback and it's
> confusing. There is no prompt in this test. We are waiting for the state. I'd
> fix the name.

Name is fixed. Special method for waiting for fallback is added.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_interactive_uitest.cc:123:
CheckThatCredentialsStored(password_store.get(), base::string16(),
On 2017/08/03 10:46:37, vasilii wrote:
> One more example where we don't need |password_store| in the test.

Acknowledged.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:100: bool
was_prompt_shown_;
On 2017/08/03 09:55:38, vasilii wrote:
> was_prompt_automatically_shown_?

Done.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:150: if
(target_state_ == password_manager::ui::PENDING_PASSWORD_STATE) {
On 2017/08/03 09:55:39, vasilii wrote:
> I think this all the existing callers of WaitForSavePrompt() are looking
forward
> to the automatic prompt. We shouldn't downgrade their expectations to the
manual
> fallback.

Added special processing for fallbacks.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:250: bool
BubbleObserver::WasSavePromptShown() const {
On 2017/08/03 09:55:38, vasilii wrote:
> Not sure why those methods are called "Was..". Shouldn't it be Is.. ?

Done.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:279: void
BubbleObserver::OpenAndAcceptSavePrompt() const {
On 2017/08/03 09:55:38, vasilii wrote:
> Not sure why we need this method. It's almost a duplicate of AcceptSavePrompt.
> For AcceptSavePrompt we checked only the state just to make sure that
> passwords_ui_controller_->SavePassword won't crash. It seems OK to continue
> doing so. If a test cares about automatic save prompt then it should call
> WaitForSavePrompt() or ASSERT_TRUE(WasSavePromptShown()) explicitly.

Let me not inline the method. I added a parameter to
BubbleObserver::AcceptSavePrompt. I guess it will be useful for more tests of
manual fallback for saving.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:71: // Checks if
the save prompt is being currently available.
On 2017/08/03 09:55:39, vasilii wrote:
> Clarify that it can be both due to both manual fallback or successful login.

Done.

https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:77: // Checks if
the save prompt was shown.
On 2017/08/03 09:55:39, vasilii wrote:
> was shown automatically?

Done.

https://codereview.chromium.org/2915763003/diff/660002/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/660002/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1886: 
On 2017/08/03 10:46:37, vasilii wrote:
> How about a test for the generation scenario?

Done.

[commit-bot: I haz the power, 2017-08-04 17:48:19]

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

[commit-bot: I haz the power, 2017-08-04 17:48:20]

Dry run: This issue passed the CQ dry run.

[vasilii, 2017-08-07 17:13:24]

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:211:
scoped_refptr<password_manager::TestPasswordStore> password_store =
unused

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1021:
EXPECT_FALSE(password_store->IsEmpty());
excessive, also |password_store|

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1527:
EXPECT_FALSE(password_store->IsEmpty());
same as above

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2290:
EXPECT_FALSE(password_store->IsEmpty());
and here

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:101: void
CheckThatThereIsNoAnotherWaitLoop();
Why should it be called? What are you trying to prevent?

I don't see how it's possible to call WaitFor* recursively.

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:136: if
(GetState() == target_state)
I'm concerned that if there is a manual fallback currently and I call
WaitForSavePrompt then the function returns immediately. I think it doesn't
match the expectations of the caller.

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:180: if
(wait_for_fallback_) {
Should we assign false?

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:311:
ASSERT_TRUE(expected_automatic_prompt ? IsSavePromptShownAutomatically()
This assertion is only needed so that the test crashes nicely if the current
state is wrong. If a test really cares which prompt is shown then it should
check it.

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (left):

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:88: //
PasswordManagerBrowserTestBase.
Any reason why you don't like this comment? It's very important. Also it's
applicable to your methods

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:89: void
AcceptSavePrompt(bool expected_automatic_prompt) const;
No, I don't like this interface. This method isn't about expecting a bubble or
not. It just emulates "Save" click.
If a test has some expectations regarding the bubble like the state is
"pending", the bubble was automatically shown etc. then it should be part of the
test. Actually most of the tests do it anyway already.

[kolos1, 2017-08-08 12:35:08]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[kolos1, 2017-08-08 12:35:10]

Patchset #25 (id:870001) has been deleted

[commit-bot: I haz the power, 2017-08-08 12:35:20]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-08-08 12:37:17]

Hi Vasilii,

Thanks for comments. PTAL.

Regards,
Maxim

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:211:
scoped_refptr<password_manager::TestPasswordStore> password_store =
On 2017/08/07 17:13:23, vasilii wrote:
> unused
Sorry for missing that. Double checked also uses of PasswordStore, found one
more variable to remove.
Done.

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1021:
EXPECT_FALSE(password_store->IsEmpty());
On 2017/08/07 17:13:23, vasilii wrote:
> excessive, also |password_store|

Done.

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1527:
EXPECT_FALSE(password_store->IsEmpty());
On 2017/08/07 17:13:23, vasilii wrote:
> same as above

Done.

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2290:
EXPECT_FALSE(password_store->IsEmpty());
On 2017/08/07 17:13:23, vasilii wrote:
> and here

Done.

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:101: void
CheckThatThereIsNoAnotherWaitLoop();
On 2017/08/07 17:13:24, vasilii wrote:
> Why should it be called? What are you trying to prevent?
> 
> I don't see how it's possible to call WaitFor* recursively.

Yes, it is not possible to call a sequence of two Wait... functions.
It was a sort of additional check. Removed.

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:136: if
(GetState() == target_state)
On 2017/08/07 17:13:24, vasilii wrote:
> I'm concerned that if there is a manual fallback currently and I call
> WaitForSavePrompt then the function returns immediately. I think it doesn't
> match the expectations of the caller.

Alright. Fixed.

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:180: if
(wait_for_fallback_) {
On 2017/08/07 17:13:23, vasilii wrote:
> Should we assign false?

Yep, created a method where all expectations are reset.

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:311:
ASSERT_TRUE(expected_automatic_prompt ? IsSavePromptShownAutomatically()
On 2017/08/07 17:13:24, vasilii wrote:
> This assertion is only needed so that the test crashes nicely if the current
> state is wrong. If a test really cares which prompt is shown then it should
> check it.
Fixed.

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (left):

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:88: //
PasswordManagerBrowserTestBase.
On 2017/08/07 17:13:24, vasilii wrote:
> Any reason why you don't like this comment? It's very important. Also it's
> applicable to your methods

Can we mention here something except the parameters of the method? It looks like
an outdated comment.

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:89: void
AcceptSavePrompt(bool expected_automatic_prompt) const;
On 2017/08/07 17:13:24, vasilii wrote:
> No, I don't like this interface. This method isn't about expecting a bubble or
> not. It just emulates "Save" click.
> If a test has some expectations regarding the bubble like the state is
> "pending", the bubble was automatically shown etc. then it should be part of
the
> test. Actually most of the tests do it anyway already.

Ok, reverted to the old version, but edited the comment. Prompt must be
available, but hasn't to be shown.

[commit-bot: I haz the power, 2017-08-08 13:11:30]

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

[commit-bot: I haz the power, 2017-08-08 13:11:31]

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

[vasilii, 2017-08-08 13:26:31]

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (left):

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:88: //
PasswordManagerBrowserTestBase.
On 2017/08/08 12:37:17, kolos1 wrote:
> On 2017/08/07 17:13:24, vasilii wrote:
> > Any reason why you don't like this comment? It's very important. Also it's
> > applicable to your methods
> 
> Can we mention here something except the parameters of the method? It looks
like
> an outdated comment.

Why is it outdated? Everything will crash if you call any Wait* method on a new
tab.

[kolos1, 2017-08-08 13:30:51]

Patchset #25 (id:890001) has been deleted

[kolos1, 2017-08-08 13:31:07]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-08 13:31:19]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-08-08 13:41:58]

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (left):

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:88: //
PasswordManagerBrowserTestBase.
On 2017/08/08 13:26:31, vasilii wrote:
> On 2017/08/08 12:37:17, kolos1 wrote:
> > On 2017/08/07 17:13:24, vasilii wrote:
> > > Any reason why you don't like this comment? It's very important. Also it's
> > > applicable to your methods
> > 
> > Can we mention here something except the parameters of the method? It looks
> like
> > an outdated comment.
> 
> Why is it outdated? Everything will crash if you call any Wait* method on a
new
> tab.

I mean that it is unclear what |web_contents| is. It is not a parameter, there
is even no occurrence of web_contents or so within the method. Well, for someone
who knows the browser tests code well, the comment may make sense. I'll keep it
if you want. 

What if we say something about CustomManagePasswordsUIController or new tab
(preferably)?

[kolos1, 2017-08-08 13:54:13]

MojoConnectionRecreatedAfterNavigation fixed.

[vasilii, 2017-08-08 13:57:42]

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (left):

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:88: //
PasswordManagerBrowserTestBase.
On 2017/08/08 13:41:57, kolos1 wrote:
> On 2017/08/08 13:26:31, vasilii wrote:
> > On 2017/08/08 12:37:17, kolos1 wrote:
> > > On 2017/08/07 17:13:24, vasilii wrote:
> > > > Any reason why you don't like this comment? It's very important. Also
it's
> > > > applicable to your methods
> > > 
> > > Can we mention here something except the parameters of the method? It
looks
> > like
> > > an outdated comment.
> > 
> > Why is it outdated? Everything will crash if you call any Wait* method on a
> new
> > tab.
> 
> I mean that it is unclear what |web_contents| is. It is not a parameter, there
> is even no occurrence of web_contents or so within the method. Well, for
someone
> who knows the browser tests code well, the comment may make sense. I'll keep
it
> if you want. 
> 
> What if we say something about CustomManagePasswordsUIController or new tab
> (preferably)?

|web_contents| is a parameter to the constructor. We can clarify it.
CustomManagePasswordsUIController isn't in the header. "new tab" makes sense
when the reader understands the "old tab".
Anyway, a comment on every method using  CustomManagePasswordsUIController is
required.

[commit-bot: I haz the power, 2017-08-08 14:29:31]

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

[commit-bot: I haz the power, 2017-08-08 14:29:32]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-08-08 16:20:07]

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (left):

https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:88: //
PasswordManagerBrowserTestBase.
On 2017/08/08 13:57:42, vasilii wrote:
> On 2017/08/08 13:41:57, kolos1 wrote:
> > On 2017/08/08 13:26:31, vasilii wrote:
> > > On 2017/08/08 12:37:17, kolos1 wrote:
> > > > On 2017/08/07 17:13:24, vasilii wrote:
> > > > > Any reason why you don't like this comment? It's very important. Also
> it's
> > > > > applicable to your methods
> > > > 
> > > > Can we mention here something except the parameters of the method? It
> looks
> > > like
> > > > an outdated comment.
> > > 
> > > Why is it outdated? Everything will crash if you call any Wait* method on
a
> > new
> > > tab.
> > 
> > I mean that it is unclear what |web_contents| is. It is not a parameter,
there
> > is even no occurrence of web_contents or so within the method. Well, for
> someone
> > who knows the browser tests code well, the comment may make sense. I'll keep
> it
> > if you want. 
> > 
> > What if we say something about CustomManagePasswordsUIController or new tab
> > (preferably)?
> 
> |web_contents| is a parameter to the constructor. We can clarify it.
> CustomManagePasswordsUIController isn't in the header. "new tab" makes sense
> when the reader understands the "old tab".
> Anyway, a comment on every method using  CustomManagePasswordsUIController is
> required.

The comment is back :)

[vasilii, 2017-08-09 09:32:15]

https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:99: bool
CheckIfTargetStateIsObserved(
IsTargetStateObserved

https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:104: const
password_manager::ui::State target_state,
I don't see any single place where |target_state| is needed (different from
target_state_.

https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:334:
ASSERT_TRUE(IsUpdatePromptShownAutomatically());
Automatic or not we don't care. Also this check limits the function to be called
on the custom tab only.

https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:76: // Checks if
the save prompt was shown automatically.
This can only be called on the custom tab.

[kolos1, 2017-08-09 09:53:07]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-09 09:53:14]

Dry run: CQ is trying da patch.

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

[kolos1, 2017-08-09 09:55:06]

Hi Vasilii

Your comments have been addressed. PTAL.

Regards,
Maxim

https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:99: bool
CheckIfTargetStateIsObserved(
On 2017/08/09 09:32:15, vasilii wrote:
> IsTargetStateObserved

Done.

https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:104: const
password_manager::ui::State target_state,
On 2017/08/09 09:32:14, vasilii wrote:
> I don't see any single place where |target_state| is needed (different from
> target_state_.

True. For this function we can always use target_state_.

https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:334:
ASSERT_TRUE(IsUpdatePromptShownAutomatically());
On 2017/08/09 09:32:14, vasilii wrote:
> Automatic or not we don't care. Also this check limits the function to be
called
> on the custom tab only.

oh, of course. Fixed.

https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:76: // Checks if
the save prompt was shown automatically.
On 2017/08/09 09:32:15, vasilii wrote:
> This can only be called on the custom tab.

Done.

[vasilii, 2017-08-09 10:31:32]

lgtm

https://codereview.chromium.org/2915763003/diff/950001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/950001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:76: // Checks if
the save prompt was shown automatically. This can only be called
Just copy the sentence about PasswordManagerBrowserTestBase from below

[kolos1, 2017-08-09 11:37:22]

Thanks Vasilii for this durable review.

https://codereview.chromium.org/2915763003/diff/950001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_test_base.h (right):

https://codereview.chromium.org/2915763003/diff/950001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:76: // Checks if
the save prompt was shown automatically. This can only be called
On 2017/08/09 10:31:32, vasilii wrote:
> Just copy the sentence about PasswordManagerBrowserTestBase from below

Done.

[kolos1, 2017-08-09 11:39:08]

kolos@chromium.org changed reviewers:
+ mkwst@chromium.org

[kolos1, 2017-08-09 11:39:10]

Hi Mike! 

Please review changes in
components/autofill/content/common/autofill_driver.mojom

Thanks,
Maxim

[kolos1, 2017-08-09 11:39:36]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-09 11:39:45]

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-08-09 12:51:15]

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

[commit-bot: I haz the power, 2017-08-09 12:51:17]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-08-10 06:31:39]

mkwst: Hi Mike! Friendly ping. Please mojo changes in autofill_driver.mojom.
Thx.

[Mike West, 2017-08-10 06:52:14]

On 2017/08/10 at 06:31:39, kolos wrote:
> mkwst: Hi Mike! Friendly ping. Please mojo changes in autofill_driver.mojom.
Thx.

mojo LGTM.

[kolos1, 2017-08-10 07:06:38]

The CQ bit was checked by kolos@chromium.org

[kolos1, 2017-08-10 07:06:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
vasilii@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2915763003/#ps1010001
(title: "Rebase")

[commit-bot: I haz the power, 2017-08-10 07:06: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-08-10 08:00:14]

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

[commit-bot: I haz the power, 2017-08-10 08:00:16]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[kolos1, 2017-08-10 08:31:00]

The CQ bit was checked by kolos@chromium.org

[commit-bot: I haz the power, 2017-08-10 08:31:15]

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-08-10 09:00:06]

CQ is committing da patch.

Bot data: {"patchset_id": 1010001, "attempt_start_ts": 1502353860596100,
"parent_rev": "4f9d2469ba713829aaa2e62fdd766a64cb5965d3", "commit_rev":
"7cf7408af869f5b605a6f35cdd458380c56fb73d"}

[commit-bot: I haz the power, 2017-08-10 09:00:29]

Description was changed from

==========
[Password Manager] Show omnibox icon and anchored prompt once users start typing
password

If the password to be save is a generated by Chrome password, the confirmation
bubble will be shown (the bubble that a user sees when she submits a form with
generated password). 

If the password is not a generated one, a save or update bubble will be shown.

BUG=725883
==========

to

==========
[Password Manager] Show omnibox icon and anchored prompt once users start typing
password

If the password to be save is a generated by Chrome password, the confirmation
bubble will be shown (the bubble that a user sees when she submits a form with
generated password). 

If the password is not a generated one, a save or update bubble will be shown.

BUG=725883

Review-Url: https://codereview.chromium.org/2915763003
Cr-Commit-Position: refs/heads/master@{#493337}
Committed:
https://chromium.googlesource.com/chromium/src/+/7cf7408af869f5b605a6f35cdd45...
==========

[commit-bot: I haz the power, 2017-08-10 09:00:32]

Committed patchset #30 (id:1010001) as
https://chromium.googlesource.com/chromium/src/+/7cf7408af869f5b605a6f35cdd45...