Adding an edit button for username correction while saving new credentials for password management.

Adding an edit button for username correction while saving new credentials for password management.

This cl implements an edit button inside the password management bubble when saving a new credential for a site.

The feature is added as an experiment, and the edit button which is newly added currently does nothing. Screenshots for both rtl and ltr languages are in the bug and the document.

The document for the feature can be found here:

https: //docs.google.com/a/google.com/document/d/1zTXXiiyJ5NdYSNEUNUjPUT6jyzCqv52rqjsY8GXZ6II/edit?usp=sharing
Review-Url: https://codereview.chromium.org/2954093002
Cr-Commit-Position: refs/heads/master@{#482219}
Committed: https://chromium.googlesource.com/chromium/src/+/c0e8613b70e3d3899c09a4e40a1ac066fe3763d2

[irmakk, 2017-06-23 09:20:44]

Description was changed from

==========
Added edit button.

Added the feature as experiment and added an edit button which currently does
nothing. Screenshots are in the bug and the document

Created feature tags.

BUG=734965
==========

to

==========
Added edit button.

Added the feature as experiment and added an edit button which currently does
nothing. Screenshots are in the bug and the document.

BUG=734965
==========

[irmakk, 2017-06-23 09:20:45]

irmakk@google.com changed reviewers:
+ melandory@chromium.org, vasilii@chromium.org

[irmakk, 2017-06-23 09:22:04]

On 2017/06/23 09:20:45, irmakk wrote:
> mailto:irmakk@google.com changed reviewers:
> + mailto:melandory@chromium.org, mailto:vasilii@chromium.org

Will we have the edit button in the "update password for this username" bubble
as well?

[vasilii, 2017-06-23 12:02:07]

Change the description to something more meaningful.

Re: update bubble
This is tricky. We decide if we show the update or save bubble solely on the
username. If we showed the update bubble and the user wants to edit then we had
saved a wrong credential in the first place. The right approach would be save a
new one and optionally drop the old one. Anyway, it requires discussions with
the team. Don't do it for now.
Much more interesting and common scenario is when the edited username is already
stored and we should update the existing credentials from the save bubble. Once
you proceed with implementing the logic, you should keep this scenario in mind
so that everything works as expected (disclaimer: in case of update currently we
touch potentially more than 1 credential).

https://codereview.chromium.org/2954093002/diff/1/chrome/browser/ui/views/pas...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/2954093002/diff/1/chrome/browser/ui/views/pas...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:355:
BuildColumnSet(layout, TRIPLE_BUTTON_COLUMN_SET);
Funny that the style exists even though it was unused.

https://codereview.chromium.org/2954093002/diff/1/components/password_manager...
File components/password_manager/core/common/password_manager_features.h
(right):

https://codereview.chromium.org/2954093002/diff/1/components/password_manager...
components/password_manager/core/common/password_manager_features.h:24: extern
const base::Feature kEnableUsernameCorrection;
Where is it used? ;-)

[irmakk, 2017-06-23 12:16:12]

Description was changed from

==========
Added edit button.

Added the feature as experiment and added an edit button which currently does
nothing. Screenshots are in the bug and the document.

BUG=734965
==========

to

==========
Adding an edit button for username correction while saving new credentials for
password management.

This cl implements an edit button inside the password management bubble when
saving a new credential for a site.

The feature is added as an experiment, and the edit button which is newly added
currently does nothing. Screenshots for both rtl and ltr languages are in the
bug and the document.

The document for the feature can be found here:

https://docs.google.com/a/google.com/document/d/1zTXXiiyJ5NdYSNEUNUjPUT6jyzCq...
BUG=734965
==========

[irmakk, 2017-06-23 12:19:57]

Screenshot for right-to-left languages added to the bug. Currently not doing
anything for the update credentials part. I will keep in mind the changing
username to an already existing username case while implementing the logic,
thanks for the  warning.

https://codereview.chromium.org/2954093002/diff/1/chrome/browser/ui/views/pas...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/2954093002/diff/1/chrome/browser/ui/views/pas...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:355:
BuildColumnSet(layout, TRIPLE_BUTTON_COLUMN_SET);
On 2017/06/23 12:02:07, vasilii wrote:
> Funny that the style exists even though it was unused.

I was surprised too, maybe they used it before and decided not to remove the
style after changing the ui. Useful decision. :D

https://codereview.chromium.org/2954093002/diff/1/components/password_manager...
File components/password_manager/core/common/password_manager_features.h
(right):

https://codereview.chromium.org/2954093002/diff/1/components/password_manager...
components/password_manager/core/common/password_manager_features.h:24: extern
const base::Feature kEnableUsernameCorrection;
On 2017/06/23 12:02:07, vasilii wrote:
> Where is it used? ;-)

Oh, I thought we needed this to declare it in the cc file but now i see it is
not really needed. :) Removed.

[vasilii, 2017-06-23 12:35:19]

https://codereview.chromium.org/2954093002/diff/1/components/password_manager...
File components/password_manager/core/common/password_manager_features.h
(right):

https://codereview.chromium.org/2954093002/diff/1/components/password_manager...
components/password_manager/core/common/password_manager_features.h:24: extern
const base::Feature kEnableUsernameCorrection;
On 2017/06/23 12:19:56, irmakk wrote:
> On 2017/06/23 12:02:07, vasilii wrote:
> > Where is it used? ;-)
> 
> Oh, I thought we needed this to declare it in the cc file but now i see it is
> not really needed. :) Removed.

Hmm, strange decision. Your changes should be behind the flag and off by default

[irmakk, 2017-06-23 13:04:24]

https://codereview.chromium.org/2954093002/diff/1/components/password_manager...
File components/password_manager/core/common/password_manager_features.h
(right):

https://codereview.chromium.org/2954093002/diff/1/components/password_manager...
components/password_manager/core/common/password_manager_features.h:24: extern
const base::Feature kEnableUsernameCorrection;
On 2017/06/23 12:35:19, vasilii wrote:
> On 2017/06/23 12:19:56, irmakk wrote:
> > On 2017/06/23 12:02:07, vasilii wrote:
> > > Where is it used? ;-)
> > 
> > Oh, I thought we needed this to declare it in the cc file but now i see it
is
> > not really needed. :) Removed.
> 
> Hmm, strange decision. Your changes should be behind the flag and off by
default

OH, sorry I wasn't aware I didnt add the experiment checks, added now.

[irmakk, 2017-06-23 13:24:23]

The CQ bit was checked by irmakk@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 13:24:36]

Dry run: CQ is trying da patch.

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

[vasilii, 2017-06-23 13:27:19]

https://codereview.chromium.org/2954093002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/2954093002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:313:
views::Button* edit_button_;
potentially uninitialized in the constructor

https://codereview.chromium.org/2954093002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:361:
password_manager::features::kEnableUsernameCorrection)
I'd rather check |edit_button_| here.
There are cases when the button shouldn't be there even when the flag is on
(when a site saves via the Credential Manager API).

[irmakk, 2017-06-23 14:10:02]

PTAL

https://codereview.chromium.org/2954093002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/2954093002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:313:
views::Button* edit_button_;
On 2017/06/23 13:27:19, vasilii wrote:
> potentially uninitialized in the constructor

Done.

https://codereview.chromium.org/2954093002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:361:
password_manager::features::kEnableUsernameCorrection)
On 2017/06/23 13:27:19, vasilii wrote:
> I'd rather check |edit_button_| here.
> There are cases when the button shouldn't be there even when the flag is on
> (when a site saves via the Credential Manager API).

Done.

[vasilii, 2017-06-23 14:13:12]

lgtm

[irmakk, 2017-06-23 14:14:30]

The CQ bit was checked by irmakk@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 14:14:42]

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-06-23 15:20:35]

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

[commit-bot: I haz the power, 2017-06-23 15:20:36]

Dry run: This issue passed the CQ dry run.

[irmakk, 2017-06-26 08:40:43]

The CQ bit was checked by irmakk@google.com

[irmakk, 2017-06-26 08:40:45]

The CQ bit was unchecked by irmakk@google.com

[irmakk, 2017-06-26 08:41:39]

The CQ bit was checked by irmakk@google.com

[commit-bot: I haz the power, 2017-06-26 08:41:47]

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-06-26 09:45:05]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498466499222070,
"parent_rev": "87e6e751e90c9e602094ef2323434fd1d60e25c6", "commit_rev":
"c0e8613b70e3d3899c09a4e40a1ac066fe3763d2"}

[commit-bot: I haz the power, 2017-06-26 09:45:16]

Description was changed from

==========
Adding an edit button for username correction while saving new credentials for
password management.

This cl implements an edit button inside the password management bubble when
saving a new credential for a site.

The feature is added as an experiment, and the edit button which is newly added
currently does nothing. Screenshots for both rtl and ltr languages are in the
bug and the document.

The document for the feature can be found here:

https://docs.google.com/a/google.com/document/d/1zTXXiiyJ5NdYSNEUNUjPUT6jyzCq...
BUG=734965
==========

to

==========
Adding an edit button for username correction while saving new credentials for
password management.

This cl implements an edit button inside the password management bubble when
saving a new credential for a site.

The feature is added as an experiment, and the edit button which is newly added
currently does nothing. Screenshots for both rtl and ltr languages are in the
bug and the document.

The document for the feature can be found here:

https:
//docs.google.com/a/google.com/document/d/1zTXXiiyJ5NdYSNEUNUjPUT6jyzCqv52rqjsY8GXZ6II/edit?usp=sharing
Review-Url: https://codereview.chromium.org/2954093002
Cr-Commit-Position: refs/heads/master@{#482219}
Committed:
https://chromium.googlesource.com/chromium/src/+/c0e8613b70e3d3899c09a4e40a1a...
==========

[commit-bot: I haz the power, 2017-06-26 09:45:18]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c0e8613b70e3d3899c09a4e40a1a...