Implemented showing update bubble pop-up on password overriding

Implemented showing update bubble pop-up when the user override password on sign-in form. It prevents from accidentally changing password in the PasswordManager (crbug.com/400772). It's currently behind a flag, until launching change password support

BUG=359315
Committed: https://crrev.com/bd86f12bcf6d760893818c2cbaf22ee17d80bb7d
Cr-Commit-Position: refs/heads/master@{#343915}

[dvadym, 2015-08-17 13:07:37]

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

[dvadym, 2015-08-17 13:07:37]

Hi Vaclav,

 Could you please review this CL? Tests will be added in an hour or two.

Best regards,
Vadym

[vabr (Chromium), 2015-08-17 13:21:46]

Thanks, Vadym!

This LGTM with some nits.

Feel free to TBR me for the tests, or perhaps ask Vasilii for a review, because
I will only be able to respond again once I'm back next week.

Cheers,
Vaclav

https://codereview.chromium.org/1297963002/diff/1/chrome/browser/ui/passwords...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/1297963002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:356:
password_manager::PasswordFormManager* form_manager =
nit: A const pointer should suffice.

https://codereview.chromium.org/1297963002/diff/1/chrome/browser/ui/passwords...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/1297963002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:159: bool
PasswordOverriden() const;
nit: Please add a comment explaining who overrides the password. This might get
confused with the user overriding the generated passwords, so we need to mention
that this means that updating the password will override the old password of
this credential.

https://codereview.chromium.org/1297963002/diff/1/components/password_manager...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1297963002/diff/1/components/password_manager...
components/password_manager/core/browser/password_form_manager.h:183: // This
function returns if the user typed new password for stored account.
nit: I would not necessarily blame the user here. The new password might be a
result of Chrome misunderstanding what password field to use, or other error
(that's partly why we need this protection in the first place).

https://codereview.chromium.org/1297963002/diff/1/components/password_manager...
components/password_manager/core/browser/password_form_manager.h:183: // This
function returns if the user typed new password for stored account.
Also, I would move the comment from here, and only keep one next to the
declaration of |password_overriden_| (but apply the above comment to it).

https://codereview.chromium.org/1297963002/diff/1/components/password_manager...
components/password_manager/core/browser/password_form_manager.h:399: bool
password_overriden_;
typo: overridden (double d)

[dvadym, 2015-08-17 14:25:34]

Thanks Vaclav!
I've addressed your comment (PatchSet 3) and added tests (PatchSet 2). PTAL

https://codereview.chromium.org/1297963002/diff/1/chrome/browser/ui/passwords...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/1297963002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:356:
password_manager::PasswordFormManager* form_manager =
On 2015/08/17 13:21:46, vabr (on holiday) wrote:
> nit: A const pointer should suffice.

Done.

https://codereview.chromium.org/1297963002/diff/1/chrome/browser/ui/passwords...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/1297963002/diff/1/chrome/browser/ui/passwords...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:159: bool
PasswordOverriden() const;
On 2015/08/17 13:21:46, vabr (on holiday) wrote:
> nit: Please add a comment explaining who overrides the password. This might
get
> confused with the user overriding the generated passwords, so we need to
mention
> that this means that updating the password will override the old password of
> this credential.

I've added comment // True if the password for previously stored account was
overridden, i.e. on newly submitted form it's different from stored one.

https://codereview.chromium.org/1297963002/diff/1/components/password_manager...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1297963002/diff/1/components/password_manager...
components/password_manager/core/browser/password_form_manager.h:183: // This
function returns if the user typed new password for stored account.
On 2015/08/17 13:21:46, vabr (on holiday) wrote:
> nit: I would not necessarily blame the user here. The new password might be a
> result of Chrome misunderstanding what password field to use, or other error
> (that's partly why we need this protection in the first place).

Done.

https://codereview.chromium.org/1297963002/diff/1/components/password_manager...
components/password_manager/core/browser/password_form_manager.h:399: bool
password_overriden_;
On 2015/08/17 13:21:46, vabr (on holiday) wrote:
> typo: overridden (double d)

Done.

[dvadym, 2015-08-18 09:31:47]

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

[dvadym, 2015-08-18 09:34:59]

Hi Vasilii,

 Could you please review this CL? Vaclav reviewed PatchSet1, after that PatchSet
browser tests were added and some small changes made.

Best regards,
Vadym

[vasilii, 2015-08-18 10:01:44]

https://codereview.chromium.org/1297963002/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1297963002/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2131: #ifndef
OS_MACOSX
#if !defined(OS_MACOSX)

https://codereview.chromium.org/1297963002/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2164: const
autofill::PasswordForm& stored_form =
Create a copy of the form.

https://codereview.chromium.org/1297963002/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2178: #ifndef
OS_MACOSX
#if !defined(OS_MACOSX)

[dvadym, 2015-08-18 11:55:33]

Thanks Vasilii! I've addressed your comments. PTAL.

https://codereview.chromium.org/1297963002/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1297963002/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2131: #ifndef
OS_MACOSX
On 2015/08/18 10:01:44, vasilii wrote:
> #if !defined(OS_MACOSX)

Done.

https://codereview.chromium.org/1297963002/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2164: const
autofill::PasswordForm& stored_form =
On 2015/08/18 10:01:44, vasilii wrote:
> Create a copy of the form.

Done.

https://codereview.chromium.org/1297963002/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2178: #ifndef
OS_MACOSX
On 2015/08/18 10:01:44, vasilii wrote:
> #if !defined(OS_MACOSX)

Done.

[vasilii, 2015-08-18 11:58:53]

lgtm

[dvadym, 2015-08-18 12:34:46]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2015-08-18 12:34:46]

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/1297963002/#ps80001
(title: "Comments were addressed")

[commit-bot: I haz the power, 2015-08-18 12:34:50]

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

[commit-bot: I haz the power, 2015-08-18 14:16:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-18 14:16:37]

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