Update Confirmation UI for saved password change

Update Confirmation UI for saved password change

All functionality of this CL is behind a flag currently, since it will require UI review.

This CL contains implementation of 2 bubbles, that are based on PendingView:
1.For confirmation from the user that he/she changed password in case when the user has only one credentials for current site. It's the same as password save bubble with "Save"->"Update"
2.For selecting by user which credentials should be updated after change password form submission in case when the user has more than one credentials and we have no clue which credentials is correct. It contains a dropbox with a list of all credentials for current site.

Implemented UI looks the same as on mocks https://folio.googleplex.com/chrome-ux/mocks/321-password-manager/password%20update#%2F00%20password%20update%20bubbles%20-%20UI%20review.png%3Fc=show
 (UI for no accounts case will be implemented in separate CL). The only difference is that "Nope" combobox replaced with "Nope" button, since it doesn't make sense to choose never update for this site.

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

[dvadym, 2015-06-03 15:41:29]

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

[dvadym, 2015-06-03 15:41:30]

Hi Vasilii,

 Could you please review chrome/browser/ui/* changes?

Best regards,
Vadym

[dvadym, 2015-06-03 15:42:20]

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

[dvadym, 2015-06-03 15:42:21]

Hi Vaclav and Garrett,

 Could you please review this CL?

Best regards,
Vadym

[vabr (Chromium), 2015-06-03 16:33:26]

Thanks Vadym!

I'm short on time, so I only send you the initial comments.

Because I'm only back on Monday, don't wait for me if you get an approval from
Garrett or Vasilii in the meantime.

Please add screenshots, if possible (attach them to the bug, and link/mention
them in the CL description).

Also, it might be worth speaking to msramek@ about the bubble for choosing
accounts. He is designing the bubble allowing to change among different
usernames, which is basically the same thing, and I believe we should have the
same UI for it.

Cheers,
Vaclav

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6418: +     <message
name="IDS_FLAGS_ENABLE_PASSWORD_CHANGE_SUPPORT_NAME" desc="Name of the flag for
the password manager's password change support option.">
nit: +1 indent

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6422: +        Update password in the
password manager on a successful password change form submit.
I would consult a native speaker to make this sentence easier to parse (note,
e.g., the 4 nouns in a row at the end).
My suggestion would be:
"Let Chrome update passwords changed in a change-password form."

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6440: +        Do you want <ph
name="PASSWORD_MANAGER_BRAND">
nit: Any reason to break this into 3 lines?

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:97: // Returns
true if the UI for confirmation of update password is enabled.
nit: update password -> password update

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:97: // Returns
true if the UI for confirmation of update password is enabled.
Also, this should be marked as "override" and go up to the
"PasswordManagerClient implementation" section. Please move the comment to the
method's declaration in the parent interface.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:133: bool
account_for_update_selection() const {
nit: Please don't inline the definition of the method, and rename it in
CamelCase. This is no longer a simple accessor, and therefore cannot be inlined
and named in hacker_style.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:133: bool
account_for_update_selection() const {
Also, the name sounds a little cryptic (I'm not sure what it means). Would you
mind rephrasing it?

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:84: // the action
off to the FormManager. The controller MUST be in a pending
nit: Pass off means "happen or be concluded in a specified, usually satisfactory
way". Maybe you meant just "pass"?

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_password_items_view.cc:256: // 
ManagePasswordsBubbleView::CloseBubble(); // todo do we need this?
Shouldn't this go through the controller?
Also, if you don't use this line, please remove the #includes for
ManagePasswordsBubbleView.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_password_items_view.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_password_items_view.h:45:
ManagePasswordsBubbleView* bubble_view_;
This is unused.

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:908: 
nit: Any particular reason to introduce this blank line?
Keep in mind that it makes git blame harder to follow.

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:983: // 1
credentials, consider this as the same credentials.
nit: credentials -> credential
(or alternatively "piece of credentials")

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:986: return
best_matches_.end();
Why exactly end()?
Also what if best_matches_ is empty?

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:993: // Found
the second credentials with the same password, do nothing.
nit: credential

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
File components/password_manager/core/browser/password_manager_metrics_util.h
(right):

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_manager_metrics_util.h:43:
CLICKED_UPDATE,
Should you add this to the end instead, to keep the results in UMA consistent
across versions?

[vasilii, 2015-06-03 18:32:02]

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:367:
save_button_text_ = l10n_util::GetStringUTF16(
This should go to the constructor.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:133: bool
account_for_update_selection() const {
On 2015/06/03 16:33:26, vabr (away until 8 June) wrote:
> Also, the name sounds a little cryptic (I'm not sure what it means). Would you
> mind rephrasing it?

I suppose it should be something like "ShouldShowMultipleAccountUpdateUI".
Please add a comment to the method.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:159: virtual void
UpdatePasswordInternal(
The fact that this function exists means that you wanted to add a unit test but
forgot :-) Thus, please add a unit test for both  ManagePasswordsUIController
and ManagePasswordsBubbleModel.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_password_items_view.cc:8: #include
"chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h"
Remove.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_password_items_view.cc:256: // 
ManagePasswordsBubbleView::CloseBubble(); // todo do we need this?
On 2015/06/03 16:33:26, vabr (away until 8 June) wrote:
> Shouldn't this go through the controller?
> Also, if you don't use this line, please remove the #includes for
> ManagePasswordsBubbleView.

ManagePasswordItemsView definitely shouldn't think about the bubble.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_password_items_view.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_password_items_view.h:14: class
ManagePasswordsBubbleView;
Remove

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

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:379:
views::LabelButton* nope_button_;
I suggest that you go with a new UpdateView implementation so that your changes
are isolated from the current dialog.
I also don't mind if you split the update chooser from a single credential into
different View classes.

https://codereview.chromium.org/1151373006/diff/60001/components/autofill/cor...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1151373006/diff/60001/components/autofill/cor...
components/autofill/core/common/password_form.h:273: bool
is_password_change_form_without_username;
What about operator<< and PasswordFormToJSON()?

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:868:
pending_credentials_ = *best_password_match_it->second;
You found the credential with |provisionally_saved_form_->password_value|
password value. Later you pushed it into UI and then it propagated back to
PasswordFormManager::Update. Where is the place when you actually changed the
old password to a new one?

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:985: } else {
We don't write "else" after "return".

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:990: for (auto
it = best_matches_.cbegin(); it != best_matches_.cend(); ++it) {
What is the reason to go backwards?

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:341: const
base::string16& password);
A comment on the meaning of |password| is appreciated.

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
File components/password_manager/core/browser/password_manager_metrics_util.h
(right):

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_manager_metrics_util.h:43:
CLICKED_UPDATE,
On 2015/06/03 16:33:26, vabr (away until 8 June) wrote:
> Should you add this to the end instead, to keep the results in UMA consistent
> across versions?

Acknowledged.

[dvadym, 2015-06-19 15:27:30]

Thanks Vaclav and Vasilii! PTAL

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6418: +     <message
name="IDS_FLAGS_ENABLE_PASSWORD_CHANGE_SUPPORT_NAME" desc="Name of the flag for
the password manager's password change support option.">
On 2015/06/03 16:33:25, vabr (Chromium) wrote:
> nit: +1 indent

Done.

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6422: +        Update password in the
password manager on a successful password change form submit.
On 2015/06/03 16:33:25, vabr (Chromium) wrote:
> I would consult a native speaker to make this sentence easier to parse (note,
> e.g., the 4 nouns in a row at the end).
> My suggestion would be:
> "Let Chrome update passwords changed in a change-password form."

Thanks, I'll do this next week.

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6440: +        Do you want <ph
name="PASSWORD_MANAGER_BRAND">
On 2015/06/03 16:33:25, vabr (Chromium) wrote:
> nit: Any reason to break this into 3 lines?

Merged in 1 line

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:97: // Returns
true if the UI for confirmation of update password is enabled.
On 2015/06/03 16:33:26, vabr (Chromium) wrote:
> nit: update password -> password update

Done.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:97: // Returns
true if the UI for confirmation of update password is enabled.
On 2015/06/03 16:33:26, vabr (Chromium) wrote:
> Also, this should be marked as "override" and go up to the
> "PasswordManagerClient implementation" section. Please move the comment to the
> method's declaration in the parent interface.

Done.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:367:
save_button_text_ = l10n_util::GetStringUTF16(
On 2015/06/03 18:32:01, vasilii wrote:
> This should go to the constructor.

Done.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:133: bool
account_for_update_selection() const {
On 2015/06/03 18:32:02, vasilii wrote:
> On 2015/06/03 16:33:26, vabr (away until 8 June) wrote:
> > Also, the name sounds a little cryptic (I'm not sure what it means). Would
you
> > mind rephrasing it?
> 
> I suppose it should be something like "ShouldShowMultipleAccountUpdateUI".
> Please add a comment to the method.

Thanks, it was temporary name from early prototyping. I changed name for
ShouldShowMultipleAccountUpdateUI and added comment "Returns true iff the
multiple account selection prompt for account update should be presented."

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:84: // the action
off to the FormManager. The controller MUST be in a pending
On 2015/06/03 16:33:26, vabr (Chromium) wrote:
> nit: Pass off means "happen or be concluded in a specified, usually
satisfactory
> way". Maybe you meant just "pass"?

I've copied it from lines 78-80. Those lines were written by mkwst, I'll ask him
on Monday.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:159: virtual void
UpdatePasswordInternal(
On 2015/06/03 18:32:02, vasilii wrote:
> The fact that this function exists means that you wanted to add a unit test
but
> forgot :-) Thus, please add a unit test for both  ManagePasswordsUIController
> and ManagePasswordsBubbleModel.

Sure, I've just forgotten to indicate in test of CL that tests are not ready

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_password_items_view.cc:8: #include
"chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h"
On 2015/06/03 18:32:02, vasilii wrote:
> Remove.

Done.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_password_items_view.cc:256: // 
ManagePasswordsBubbleView::CloseBubble(); // todo do we need this?
On 2015/06/03 18:32:02, vasilii wrote:
> On 2015/06/03 16:33:26, vabr (away until 8 June) wrote:
> > Shouldn't this go through the controller?
> > Also, if you don't use this line, please remove the #includes for
> > ManagePasswordsBubbleView.
> 
> ManagePasswordItemsView definitely shouldn't think about the bubble.

Thanks, I forgot to remove it. Done

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_password_items_view.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_password_items_view.h:14: class
ManagePasswordsBubbleView;
On 2015/06/03 18:32:02, vasilii wrote:
> Remove

Thanks, done.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_password_items_view.h:45:
ManagePasswordsBubbleView* bubble_view_;
On 2015/06/03 16:33:26, vabr (Chromium) wrote:
> This is unused.

Done.

https://codereview.chromium.org/1151373006/diff/60001/components/autofill/cor...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1151373006/diff/60001/components/autofill/cor...
components/autofill/core/common/password_form.h:273: bool
is_password_change_form_without_username;
On 2015/06/03 18:32:02, vasilii wrote:
> What about operator<< and PasswordFormToJSON()?

I've added in PasswordFormToJSON and to operator ==.

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:868:
pending_credentials_ = *best_password_match_it->second;
On 2015/06/03 18:32:02, vasilii wrote:
> You found the credential with |provisionally_saved_form_->password_value|
> password value. Later you pushed it into UI and then it propagated back to
> PasswordFormManager::Update. Where is the place when you actually changed the
> old password to a new one?

It was implemented earlier, so I just need to use existing functionality. The
place where it tooks place is creating PasswordFormManager, namely in function 
PasswordFormManager::PasswordToSave
(https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor...).
It's happening immediately after form submit, even before we understand that
SignIn was successful.

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:908: 
On 2015/06/03 16:33:26, vabr (OOO until 29 June) wrote:
> nit: Any particular reason to introduce this blank line?
> Keep in mind that it makes git blame harder to follow.

Done.

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:983: // 1
credentials, consider this as the same credentials.
On 2015/06/03 16:33:26, vabr (OOO until 29 June) wrote:
> nit: credentials -> credential
> (or alternatively "piece of credentials")

Changed to piece of credentials

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:985: } else {
On 2015/06/03 18:32:02, vasilii wrote:
> We don't write "else" after "return".

Done.

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:986: return
best_matches_.end();
On 2015/06/03 16:33:26, vabr (OOO until 29 June) wrote:
> Why exactly end()?
> Also what if best_matches_ is empty?

.end() is returned in order to comply with standard STL conventions, if
something not found then .end() is returned. If best_matches_ is empty then
.end() is returned, that's agreed with not found case.

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:990: for (auto
it = best_matches_.cbegin(); it != best_matches_.cend(); ++it) {
On 2015/06/03 18:32:02, vasilii wrote:
> What is the reason to go backwards?

It would be rbegin() and rend() for going backwards. cbegin() and cend() return
const_iterator.

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:993: // Found
the second credentials with the same password, do nothing.
On 2015/06/03 16:33:26, vabr (OOO until 29 June) wrote:
> nit: credential

Done.

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
File components/password_manager/core/browser/password_manager_metrics_util.h
(right):

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_manager_metrics_util.h:43:
CLICKED_UPDATE,
On 2015/06/03 18:32:02, vasilii wrote:
> On 2015/06/03 16:33:26, vabr (away until 8 June) wrote:
> > Should you add this to the end instead, to keep the results in UMA
consistent
> > across versions?
> 
> Acknowledged.

Done.

[vabr (Chromium), 2015-06-19 18:03:17]

Hi Vadym,

I'm again about to be OOO :), so feel free to not wait for me with landing if
you get approvals from other reviewers.

In general, it would be helpful, if you provided screenshots for the newly
created UI. Their lack is one of the reasons I am not very sure about the UI
yet.

The rest looks good to me, but I have a number of comments, and like to see them
addressed before I can approve.

Thanks!
Vaclav

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6422: +        Update password in the
password manager on a successful password change form submit.
On 2015/06/19 15:27:28, dvadym wrote:
> On 2015/06/03 16:33:25, vabr (Chromium) wrote:
> > I would consult a native speaker to make this sentence easier to parse
(note,
> > e.g., the 4 nouns in a row at the end).
> > My suggestion would be:
> > "Let Chrome update passwords changed in a change-password form."
> 
> Thanks, I'll do this next week.

Please do consult the native speaker (or use my previous suggestion). You only
write a sentence once, but it will be read many times. Let's strive for clarity
and simplicity.

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:84: // the action
off to the FormManager. The controller MUST be in a pending
On 2015/06/19 15:27:29, dvadym wrote:
> On 2015/06/03 16:33:26, vabr (Chromium) wrote:
> > nit: Pass off means "happen or be concluded in a specified, usually
> satisfactory
> > way". Maybe you meant just "pass"?
> 
> I've copied it from lines 78-80. Those lines were written by mkwst, I'll ask
him
> on Monday.

So, what did you hear back? :)

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:986: return
best_matches_.end();
On 2015/06/19 15:27:29, dvadym wrote:
> On 2015/06/03 16:33:26, vabr (OOO until 29 June) wrote:
> > Why exactly end()?
> > Also what if best_matches_ is empty?
> 
> .end() is returned in order to comply with standard STL conventions, if
> something not found then .end() is returned. If best_matches_ is empty then
> .end() is returned, that's agreed with not found case.

Acknowledged.

https://codereview.chromium.org/1151373006/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1151373006/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:6356: +      <message
name="IDS_UPDATE_PASSWORD" desc="The title of the save password bubble when a
password can be updated.">
Note that once this goes from behind the flag, we need it to have the option of
displaying Google Smart Lock instead of Google Chrome. You should either
implement it directly (see
password_bubble_experiment::IsSmartLockBrandingEnabled), or at least put TODOs
on appropriate places to really make sure you won't forget this.

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

https://codereview.chromium.org/1151373006/diff/100001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:267:
DCHECK(form_manager);
Please remove this DCHECK, it is not very useful. If form_manager is null, the
dereference on the next line will show it.
Feel free to also remove line 259, if you want to.

https://codereview.chromium.org/1151373006/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right):

https://codereview.chromium.org/1151373006/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_password_items_view.cc:205:
!host_->model_->account_for_update_selection()
nit: Is this how clang-format indented the line? Looks like it should start at
the same column as 204.

https://codereview.chromium.org/1151373006/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:398: if
(parent->model()->account_for_update_selection()) {
Should you actually create a separate view rather than forking most of the code
for PendingView based on account_for_update_selection()?

(I'm not sure myself, please consult with vasilii@ or mkwst@.)

https://codereview.chromium.org/1151373006/diff/100001/components/autofill/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1151373006/diff/100001/components/autofill/co...
components/autofill/core/common/password_form.cc:98: original_signon_realm ==
form.original_signon_realm &&
Please keep this one equality check per line. I know this is due to
clang-format, so perhaps use
// To keep the equality checks one per line.
// clang-format off
...
// clang-format on

https://codereview.chromium.org/1151373006/diff/100001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1151373006/diff/100001/components/autofill/co...
components/autofill/core/common/password_form.h:271: // If true, then this form
is considered to be a password change form without
The comments should add value. All the information of these two lines is already
in the variable name. But it would be perhaps useful to note that this is only
used on the browser side, for communicating with the UI.

Such information is not apparent, and might explain, why this flag is not
preserved by IPC serialisation.

https://codereview.chromium.org/1151373006/diff/100001/components/autofill/co...
components/autofill/core/common/password_form.h:273: bool
is_password_change_form_without_username;
I'm not thrilled about adding a new field to PasswordForm just to use it to
communicate between PasswordManager and the UI.

http://crbug.com/498048 already lists 3 usages of PasswordForm, and the need to
split the struct. This is a 4th type.

It would be great if you could instead keep and pass this information separately
only when it is needed, so that the unused variable is not hauled along in all
renderer code, and stored in the password store.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:860: // This
is password change form without username. If username is present
Please keep the comments short. If this block is for forms without username, it
has no sense to comment on what happens to forms with usernames.

And actually, since the last if condition is verbose enough and covers the first
sentence of the comment completely, I suggest to just drop the whole comment.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:871: // We
don't care about |pending_credentials| if we didn't find best match,
nit: |pending_credentials| -> |pending_credentials_|

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:871: // We
don't care about |pending_credentials| if we didn't find best match,
grammar: best match -> the best match

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:871: // We
don't care about |pending_credentials| if we didn't find best match,
If we don't care about |pending_credentials_| if there is no best match, why
don't we just modify is_password_change_form_without_username inside the if
block above, and spare us the comment here?

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:890: // If
this was a sign-up or change password form, the names of the
nit: Is there a real reason to move the block of comments? It is already clear
that it belongs to the if-block, and plan moving and reformating it messes up
the git blame, and increases the change of this CL.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:982: // 1
credentials, consider this as the same piece of credentials.
nit: Consider replacing line 982 with
// one piece of credentials, consider it the same as is being saved.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:149: // Update
|credentials_to_update| with the user typed password.
It is not clear what it means to update a PasswordForm. Literally, it would mean
that this method just changes something in credentials_to_update, but that's
obviously false (it is a const ref).

Perhaps you wanted to say:
// Update password store entry for |credentials_to_update|, using the password
from |pending_credentials_|.
?

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:338: // Return
best candidate for password update during processing the password
I would not mention change password forms, the method is oblivious to them.
Rather, mention the criteria for selecting a result. What about:

// For non-empty |password| returns the unique entry in |best_matches_| with the
same password, if it exists, and nullptr otherwise. For empty passwords, if
there is just one entry in |best_matches_|, returns that entry, otherwise
nullptr.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:340:
autofill::PasswordFormMap::const_iterator FindBestMatchForPasswordChange(
I suggest changing the return type to
const PasswordForm*
with nullptr representing "no match found". The reason follows.

I don't think the const_iterator is useful here. Consider this snippet:

#include <iostream>
#include <map>

int main() {
  std::map<int, int*> m;
  int a = 4;
  m[5] = &a;
  auto it = m.cbegin();
  *(it->second) = 7;
  std::cout << "a=" << a << "\n";
}

It returns "a=7", because while it->second is a const pointer, it is not a
pointer to a const object. The same is true of pointers to PasswordForm, which
are the values here.

You only care about the PasswordForm pointers, not about the iterator itself, so
let's make it simpler.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:340:
autofill::PasswordFormMap::const_iterator FindBestMatchForPasswordChange(
The "Change" in the name sounds confusing. What about
FindUniqueBestMatchWithPassword
?

[dvadym, 2015-06-22 14:43:34]

Thanks! It was addressed part of comments. Please don't review yet, I'll finish
addressing of comments after vacation on July 2 or 3.

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

https://codereview.chromium.org/1151373006/diff/100001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:267:
DCHECK(form_manager);
On 2015/06/19 18:03:16, vabr (OOO until 29 June) wrote:
> Please remove this DCHECK, it is not very useful. If form_manager is null, the
> dereference on the next line will show it.
> Feel free to also remove line 259, if you want to.

Done.

https://codereview.chromium.org/1151373006/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:398: if
(parent->model()->account_for_update_selection()) {
On 2015/06/19 18:03:16, vabr (OOO until 29 June) wrote:
> Should you actually create a separate view rather than forking most of the
code
> for PendingView based on account_for_update_selection()?
> 
> (I'm not sure myself, please consult with vasilii@ or mkwst@.)

Yeah, this is a good question that constantly appears, duplication vs more clear
separate logic. We definitely need to discuss it.

https://codereview.chromium.org/1151373006/diff/100001/components/autofill/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1151373006/diff/100001/components/autofill/co...
components/autofill/core/common/password_form.cc:98: original_signon_realm ==
form.original_signon_realm &&
On 2015/06/19 18:03:16, vabr (OOO until 29 June) wrote:
> Please keep this one equality check per line. I know this is due to
> clang-format, so perhaps use
> // To keep the equality checks one per line.
> // clang-format off
> ...
> // clang-format on

Done.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:860: // This
is password change form without username. If username is present
On 2015/06/19 18:03:16, vabr (OOO until 29 June) wrote:
> Please keep the comments short. If this block is for forms without username,
it
> has no sense to comment on what happens to forms with usernames.
> 
> And actually, since the last if condition is verbose enough and covers the
first
> sentence of the comment completely, I suggest to just drop the whole comment.

Done.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:871: // We
don't care about |pending_credentials| if we didn't find best match,
On 2015/06/19 18:03:17, vabr (OOO until 29 June) wrote:
> grammar: best match -> the best match

Done.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:871: // We
don't care about |pending_credentials| if we didn't find best match,
On 2015/06/19 18:03:17, vabr (OOO until 29 June) wrote:
> nit: |pending_credentials| -> |pending_credentials_|

Done.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:871: // We
don't care about |pending_credentials| if we didn't find best match,
On 2015/06/19 18:03:17, vabr (OOO until 29 June) wrote:
> If we don't care about |pending_credentials_| if there is no best match, why
> don't we just modify is_password_change_form_without_username inside the if
> block above, and spare us the comment here?

We need to set is_password_change_form_without_username=true anyway, have we
best match or not. If we don't have best match, we will allow the user to choose
correct one later. That's why we can't set it in if block. And I think comment
is needed in order to get hint why we don't set |pending_credentials_|, but
probably we need to change wording. I would propose to discuss it offline, it
would be faster.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:890: // If
this was a sign-up or change password form, the names of the
On 2015/06/19 18:03:16, vabr (OOO until 29 June) wrote:
> nit: Is there a real reason to move the block of comments? It is already clear
> that it belongs to the if-block, and plan moving and reformating it messes up
> the git blame, and increases the change of this CL.

Just traces of intermediate implementation, where else block existed. Comments
were moved to previous place.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:982: // 1
credentials, consider this as the same piece of credentials.
On 2015/06/19 18:03:16, vabr (OOO until 29 June) wrote:
> nit: Consider replacing line 982 with
> // one piece of credentials, consider it the same as is being saved.

Done.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:149: // Update
|credentials_to_update| with the user typed password.
On 2015/06/19 18:03:17, vabr (OOO until 29 June) wrote:
> It is not clear what it means to update a PasswordForm. Literally, it would
mean
> that this method just changes something in credentials_to_update, but that's
> obviously false (it is a const ref).
> 
> Perhaps you wanted to say:
> // Update password store entry for |credentials_to_update|, using the password
> from |pending_credentials_|.
> ?

Done.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:338: // Return
best candidate for password update during processing the password
On 2015/06/19 18:03:17, vabr (OOO until 29 June) wrote:
> I would not mention change password forms, the method is oblivious to them.
> Rather, mention the criteria for selecting a result. What about:
> 
> // For non-empty |password| returns the unique entry in |best_matches_| with
the
> same password, if it exists, and nullptr otherwise. For empty passwords, if
> there is just one entry in |best_matches_|, returns that entry, otherwise
> nullptr.

I changed comments to explaining how it works.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:340:
autofill::PasswordFormMap::const_iterator FindBestMatchForPasswordChange(
On 2015/06/19 18:03:17, vabr (OOO until 29 June) wrote:
> The "Change" in the name sounds confusing. What about
> FindUniqueBestMatchWithPassword
> ?

Change to FindBestMatchForUpdatePassword. It's not fully correct, since we
matching not only by passwords. And I believe that name shouldn't contain too
much information about implementation, since implementation probably will
change.

https://codereview.chromium.org/1151373006/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:340:
autofill::PasswordFormMap::const_iterator FindBestMatchForPasswordChange(
On 2015/06/19 18:03:17, vabr (OOO until 29 June) wrote:
> I suggest changing the return type to
> const PasswordForm*
> with nullptr representing "no match found". The reason follows.
> 
> I don't think the const_iterator is useful here. Consider this snippet:
> 
> #include <iostream>
> #include <map>
> 
> int main() {
>   std::map<int, int*> m;
>   int a = 4;
>   m[5] = &a;
>   auto it = m.cbegin();
>   *(it->second) = 7;
>   std::cout << "a=" << a << "\n";
> }
> 
> It returns "a=7", because while it->second is a const pointer, it is not a
> pointer to a const object. The same is true of pointers to PasswordForm, which
> are the values here.
> 
> You only care about the PasswordForm pointers, not about the iterator itself,
so
> let's make it simpler.

Done.

[dvadym, 2015-07-23 09:45:43]

Hi Vaclav, Vasilli and Garrett,

 Could you please review this CL again?

 The main changes from previous review:
 1.Implemented separate class for update bubble (UpdatePendingView) 
 2.Implemented class for selecting credentials from combobox
(CredentialsSelectionView), thanks Martin for this.
 3.UI is made the same as in mocks
 4.Taking into consideration changes that were made for filling change password
forms https://codereview.chromium.org/1161023008

Best regards,
Vadym

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6422: +        Update password in the
password manager on a successful password change form submit.
On 2015/06/19 18:03:16, vabr (Chromium) wrote:
> On 2015/06/19 15:27:28, dvadym wrote:
> > On 2015/06/03 16:33:25, vabr (Chromium) wrote:
> > > I would consult a native speaker to make this sentence easier to parse
> (note,
> > > e.g., the 4 nouns in a row at the end).
> > > My suggestion would be:
> > > "Let Chrome update passwords changed in a change-password form."
> > 
> > Thanks, I'll do this next week.
> 
> Please do consult the native speaker (or use my previous suggestion). You only
> write a sentence once, but it will be read many times. Let's strive for
clarity
> and simplicity.

I've talked to Mike. The text is changed to "Enable updating a password in the
password manager after submitting a "change password" form."

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:84: // the action
off to the FormManager. The controller MUST be in a pending
On 2015/06/19 18:03:16, vabr (Chromium) wrote:
> On 2015/06/19 15:27:29, dvadym wrote:
> > On 2015/06/03 16:33:26, vabr (Chromium) wrote:
> > > nit: Pass off means "happen or be concluded in a specified, usually
> > satisfactory
> > > way". Maybe you meant just "pass"?
> > 
> > I've copied it from lines 78-80. Those lines were written by mkwst, I'll ask
> him
> > on Monday.
> 
> So, what did you hear back? :)

I've removed "off" in SavePassword and UpdatePassword description

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

https://codereview.chromium.org/1151373006/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:379:
views::LabelButton* nope_button_;
On 2015/06/03 18:32:02, vasilii wrote:
> I suggest that you go with a new UpdateView implementation so that your
changes
> are isolated from the current dialog.
> I also don't mind if you split the update chooser from a single credential
into
> different View classes.

It's implemented UpdatePendingView class that handles both one and multi account
case

https://codereview.chromium.org/1151373006/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1151373006/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:6356: +      <message
name="IDS_UPDATE_PASSWORD" desc="The title of the save password bubble when a
password can be updated.">
On 2015/06/19 18:03:16, vabr (Chromium) wrote:
> Note that once this goes from behind the flag, we need it to have the option
of
> displaying Google Smart Lock instead of Google Chrome. You should either
> implement it directly (see
> password_bubble_experiment::IsSmartLockBrandingEnabled), or at least put TODOs
> on appropriate places to really make sure you won't forget this.

Done.

https://codereview.chromium.org/1151373006/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right):

https://codereview.chromium.org/1151373006/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_password_items_view.cc:205:
!host_->model_->account_for_update_selection()
On 2015/06/19 18:03:16, vabr (Chromium) wrote:
> nit: Is this how clang-format indented the line? Looks like it should start at
> the same column as 204.

This change was removed.

https://codereview.chromium.org/1151373006/diff/100001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1151373006/diff/100001/components/autofill/co...
components/autofill/core/common/password_form.h:271: // If true, then this form
is considered to be a password change form without
On 2015/06/19 18:03:16, vabr (Chromium) wrote:
> The comments should add value. All the information of these two lines is
already
> in the variable name. But it would be perhaps useful to note that this is only
> used on the browser side, for communicating with the UI.
> 
> Such information is not apparent, and might explain, why this flag is not
> preserved by IPC serialisation.

This variable was removed

[vasilii, 2015-07-23 15:15:14]

The biggest question so far is "What is the contract between PasswordFormManager
and UI". It seems strange to me to analyze the number of matching credentials
and take |pending_credential| if it's 1. Why can't we just avoid
pending_credential completely?

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1093: kOsAll,
Mac and Android too?

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_state.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_state.cc:125:
.IsPossibleChangePasswordFormWithoutUsername()) {
This doesn't belong here. PasswordFormManager should think about update <->
save. In chrome/browser/ui/passwords these states should be isolated from each
other.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_view_utils.h (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_view_utils.h:26: //
|IDS_SAVE_PASSWORD_TITLE| as the |title| so that it replaces "this site" in
the comment is obsolete.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc
(right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc:65:
test_cases[i].is_smartlock_branding_enabled, false, &title,
The true value would be also nice to test.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:979: //
-------------------------------------
Match the style of other classes.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1019: View*
item;
Init

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1021: item =
selection_view_ = new CredentialsSelectionView(
Split into 2 statements.

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:990: const
base::string16& password) {
Can it be a const method?

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1009: return
best_password_match_it->second;
real danger: best_matches_.end()->second

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:149: //
password from |pending_credentials_|.
Mention that it modifies |pending_credentials_| as well.

[vabr (Chromium), 2015-07-24 08:24:29]

I have nothing substantial to add to Vasilii's review, mostly nits.

Cheers,
Vaclav

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1151373006/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6422: +        Update password in the
password manager on a successful password change form submit.
On 2015/07/23 09:45:43, dvadym wrote:
> On 2015/06/19 18:03:16, vabr (Chromium) wrote:
> > On 2015/06/19 15:27:28, dvadym wrote:
> > > On 2015/06/03 16:33:25, vabr (Chromium) wrote:
> > > > I would consult a native speaker to make this sentence easier to parse
> > (note,
> > > > e.g., the 4 nouns in a row at the end).
> > > > My suggestion would be:
> > > > "Let Chrome update passwords changed in a change-password form."
> > > 
> > > Thanks, I'll do this next week.
> > 
> > Please do consult the native speaker (or use my previous suggestion). You
only
> > write a sentence once, but it will be read many times. Let's strive for
> clarity
> > and simplicity.
> 
> I've talked to Mike. The text is changed to "Enable updating a password in the
> password manager after submitting a "change password" form."

Acknowledged.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1093: kOsAll,
On 2015/07/23 15:15:13, vasilii wrote:
> Mac and Android too?

This depends on enable-save-password-bubble, which has
kOsWin | kOsLinux | kOsCrOS | kOsMac,

and I don't see here any Mac code yet, so I guess
kOsWin | kOsLinux | kOsCrOS,
is the right choice?

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

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:210:
DCHECK(PasswordPendingUserDecision() ||
Cant this method be called when state() ==
password_manager::ui::PENDING_PASSWORD_STATE?

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:271:
DCHECK(PasswordPendingUserDecision());
Here it can also happen that state() ==
password_manager::ui::PENDING_PASSWORD_UPDATE_STATE, can't it?

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:79: // action to
the FormManager. The controller MUST be in a pending state, and
nit: PasswordFormManager or "form manager". Just FormManager looks like a class
name, but there is no such class.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:79: // action to
the FormManager. The controller MUST be in a pending state, and
nit: "must" and "will". RFCs and other standards use the upper-case to emphasise
that those terms have a specially defined meaning for the standards, but I don't
think there was any such definition for the comments here.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:31: for (const
auto& form : forms)
const auto& -> const auto*
The elements of |forms| are const pointers. It is not more expensive to copy
them by value than by reference, and it is even less expensive to use the copied
pointer than to add one level of indirection via the reference. Also, it will
make it clear that |form| indeed is a pointer.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:78:
DCHECK_EQ((int)password_forms_.size(), combobox_->model()->GetItemCount());
First, no C-style casting, please. Do use static_cast instead.
Second, please cast the int in the second argument to a size_t instead of
casting the first argument to int. The second argument is guaranteed to be
non-negative (and should be, in fact, size_t, but that's not the fault of this
CL), and so that conversion will be safe. The current conversion, although
unlikely, can overflow.

(I'm aware that this is just a DCHECK, so nothing to do with actual release
builds we ship, but we should still stick to good rules for casting.)

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.h (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
2013 -> 2015

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1081: // Multi
account case
nit: Add a full-stop at the end.

https://codereview.chromium.org/1151373006/diff/260001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1151373006/diff/260001/components/autofill/co...
components/autofill/core/common/password_form.h:280: // without username field.
nit: It might be worth copying the sentence about heuristics from line 276 to
explain the "Possible" in the name.

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1003: // Found
the second credential with the same password, do nothing.
nit: the second -> a second

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:347: //
|password|, if there is just one entry in |best_matches_| , returns that
nit: No space before ",".

[dvadym, 2015-07-24 16:42:40]

Thanks Vaclav and Vasilii, I've addressed your comments in PatchSet 15. PTAL.

Please note that tests will be in next PatchSets.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1093: kOsAll,
On 2015/07/24 08:24:28, vabr (Chromium) wrote:
> On 2015/07/23 15:15:13, vasilii wrote:
> > Mac and Android too?
> 
> This depends on enable-save-password-bubble, which has
> kOsWin | kOsLinux | kOsCrOS | kOsMac,
> 
> and I don't see here any Mac code yet, so I guess
> kOsWin | kOsLinux | kOsCrOS,
> is the right choice?

Thanks, sure now it's only Windows, Linux and CromeOS

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_state.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_state.cc:125:
.IsPossibleChangePasswordFormWithoutUsername()) {
On 2015/07/23 15:15:13, vasilii wrote:
> This doesn't belong here. PasswordFormManager should think about update <->
> save. In chrome/browser/ui/passwords these states should be isolated from each
> other.

Done.

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

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:210:
DCHECK(PasswordPendingUserDecision() ||
On 2015/07/24 08:24:28, vabr (Chromium) wrote:
> Cant this method be called when state() ==
> password_manager::ui::PENDING_PASSWORD_STATE?

Thanks, no, it can't, it was left from the previous implementation without
|PENDING_PASSWORD_UPDATE_STAT|. I've removed checking for pending_state

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:271:
DCHECK(PasswordPendingUserDecision());
On 2015/07/24 08:24:28, vabr (Chromium) wrote:
> Here it can also happen that state() ==
> password_manager::ui::PENDING_PASSWORD_UPDATE_STATE, can't it?

No it can't, I haven't added "Never for this site" button in the current update
bubble. It doesn't have big sense to have such button:
1.Never can have two meaning: "Never save" and "Never update", and it may be
confusing for the user. We can specify in text which meaning we means. But
2. Both of them not so useful for the user
  a."Never save" - but the user already has stored account and moreover it will
not prevent showing update button next time
  b."Never update" - it's also not very useful, why the user don't want to
update password in the PasswordManager if he changed it.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:79: // action to
the FormManager. The controller MUST be in a pending state, and
On 2015/07/24 08:24:29, vabr (Chromium) wrote:
> nit: "must" and "will". RFCs and other standards use the upper-case to
emphasise
> that those terms have a specially defined meaning for the standards, but I
don't
> think there was any such definition for the comments here.

Done.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:79: // action to
the FormManager. The controller MUST be in a pending state, and
On 2015/07/24 08:24:29, vabr (Chromium) wrote:
> nit: PasswordFormManager or "form manager". Just FormManager looks like a
class
> name, but there is no such class.

Done.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_view_utils.h (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_view_utils.h:26: //
|IDS_SAVE_PASSWORD_TITLE| as the |title| so that it replaces "this site" in
On 2015/07/23 15:15:13, vasilii wrote:
> the comment is obsolete.

I've changed the first phrase to: "Sets the formatted |title| in the Save
Password bubble or the Update Password bubble (depending on
|is_update_password_bubble|)." and renaming the argument variable to
is_update_password_bubble.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc
(right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_view_utils_unittest.cc:65:
test_cases[i].is_smartlock_branding_enabled, false, &title,
On 2015/07/23 15:15:13, vasilii wrote:
> The true value would be also nice to test.

Sure, the tests will be in next PatchSets

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:31: for (const
auto& form : forms)
On 2015/07/24 08:24:29, vabr (Chromium) wrote:
> const auto& -> const auto*
> The elements of |forms| are const pointers. It is not more expensive to copy
> them by value than by reference, and it is even less expensive to use the
copied
> pointer than to add one level of indirection via the reference. Also, it will
> make it clear that |form| indeed is a pointer.

Thanks

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:78:
DCHECK_EQ((int)password_forms_.size(), combobox_->model()->GetItemCount());
On 2015/07/24 08:24:29, vabr (Chromium) wrote:
> First, no C-style casting, please. Do use static_cast instead.
> Second, please cast the int in the second argument to a size_t instead of
> casting the first argument to int. The second argument is guaranteed to be
> non-negative (and should be, in fact, size_t, but that's not the fault of this
> CL), and so that conversion will be safe. The current conversion, although
> unlikely, can overflow.
> 
> (I'm aware that this is just a DCHECK, so nothing to do with actual release
> builds we ship, but we should still stick to good rules for casting.)

Done.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.h (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
On 2015/07/24 08:24:29, vabr (Chromium) wrote:
> 2013 -> 2015

Done.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:979: //
-------------------------------------
On 2015/07/23 15:15:13, vasilii wrote:
> Match the style of other classes.

Done.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1019: View*
item;
On 2015/07/23 15:15:13, vasilii wrote:
> Init

Done.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1021: item =
selection_view_ = new CredentialsSelectionView(
On 2015/07/23 15:15:13, vasilii wrote:
> Split into 2 statements.

Done.

https://codereview.chromium.org/1151373006/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1081: // Multi
account case
On 2015/07/24 08:24:29, vabr (Chromium) wrote:
> nit: Add a full-stop at the end.

Done.

https://codereview.chromium.org/1151373006/diff/260001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1151373006/diff/260001/components/autofill/co...
components/autofill/core/common/password_form.h:280: // without username field.
On 2015/07/24 08:24:29, vabr (Chromium) wrote:
> nit: It might be worth copying the sentence about heuristics from line 276 to
> explain the "Possible" in the name.

Done.

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:990: const
base::string16& password) {
On 2015/07/23 15:15:13, vasilii wrote:
> Can it be a const method?

Thanks, changed to const

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1003: // Found
the second credential with the same password, do nothing.
On 2015/07/24 08:24:29, vabr (Chromium) wrote:
> nit: the second -> a second

Done.

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1009: return
best_password_match_it->second;
On 2015/07/23 15:15:13, vasilii wrote:
> real danger: best_matches_.end()->second

Thanks, added checking on .end()

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:149: //
password from |pending_credentials_|.
On 2015/07/23 15:15:13, vasilii wrote:
> Mention that it modifies |pending_credentials_| as well.

Done.

https://codereview.chromium.org/1151373006/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:347: //
|password|, if there is just one entry in |best_matches_| , returns that
On 2015/07/24 08:24:29, vabr (Chromium) wrote:
> nit: No space before ",".

Done.

[vasilii, 2015-07-27 15:44:49]

I still have two fundamental concerns:
- The contract between the component and PasswordManagerClient isn't clear. If I
get a call PromptUserToSaveOrUpdatePassword() then what values I can extract
from PasswordFormManager and what are their meanings?
- The new methods lack unit tests. I mean the classes: PasswordFormManager,
ManagePasswordsUIController, ManagePasswordsBubbleModel, ManagePasswordsState.

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1099: kOsWin | kOsLinux | kOsCrOS,
Are you sure you wanted to change exactly this flag?

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:256:
RecordExperimentStatistics(web_contents(), dismissal_reason_);
You should call this only for the "Save?" bubble.

https://codereview.chromium.org/1151373006/diff/280001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:881:
pending_credentials_ = *best_update_match;
Didn't you just overwrite the new password value?

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

https://codereview.chromium.org/1151373006/diff/280001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:689:
.IsPossibleChangePasswordFormWithoutUsername())) {
What happens for the forms with username?

https://codereview.chromium.org/1151373006/diff/280001/components/password_ma...
File components/password_manager/core/common/password_manager_ui.h (right):

https://codereview.chromium.org/1151373006/diff/280001/components/password_ma...
components/password_manager/core/common/password_manager_ui.h:42: // We wish to
ask the user to confirm password updating.
Chrome needs to ask ...

[vabr (Chromium), 2015-07-29 15:29:19]

Hi Vadym,
Just a nits from my side, but looking forward to Vasilii's concerns being
resolved.

Cheers,
Vaclav

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

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:218:
DCHECK(state() == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE);
nit: DCHECK_EQ(password_manager::ui::PENDING_PASSWORD_UPDATE_STATE, state());

[dvadym, 2015-07-30 11:22:44]

Hi Vaclav and Vasilii,

 Thanks for comments. I discussed with Vasilii his concerns, I clarified how it
works and made some fixes. So I hope that now concerns are solved. 

I've added unit tests and I'm going to add password manager browser test in next
PatchSets

Best regards,
Vadym

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1099: kOsWin | kOsLinux | kOsCrOS,
On 2015/07/27 15:44:49, vasilii wrote:
> Are you sure you wanted to change exactly this flag?

Thanks, fixed

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:256:
RecordExperimentStatistics(web_contents(), dismissal_reason_);
On 2015/07/27 15:44:49, vasilii wrote:
> You should call this only for the "Save?" bubble.

Removed

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

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:218:
DCHECK(state() == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE);
On 2015/07/29 15:29:19, vabr (Chromium) wrote:
> nit: DCHECK_EQ(password_manager::ui::PENDING_PASSWORD_UPDATE_STATE, state());

Done.

https://codereview.chromium.org/1151373006/diff/280001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:881:
pending_credentials_ = *best_update_match;
On 2015/07/27 15:44:49, vasilii wrote:
> Didn't you just overwrite the new password value?

No, username/password/new password are in provisionall_saved_form_.

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

https://codereview.chromium.org/1151373006/diff/280001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:689:
.IsPossibleChangePasswordFormWithoutUsername())) {
On 2015/07/27 15:44:49, vasilii wrote:
> What happens for the forms with username?

They will be processed the same way as now, namely they will be updated without
asking the user.

https://codereview.chromium.org/1151373006/diff/280001/components/password_ma...
File components/password_manager/core/common/password_manager_ui.h (right):

https://codereview.chromium.org/1151373006/diff/280001/components/password_ma...
components/password_manager/core/common/password_manager_ui.h:42: // We wish to
ask the user to confirm password updating.
On 2015/07/27 15:44:49, vasilii wrote:
> Chrome needs to ask ...

Done.

[dvadym, 2015-07-31 10:56:59]

Hi Vaclav, Vasilii and Garrett,

 I've uploaded tests and addressed your comments. Could you please have a look
once more?

Best regards,
Vadym

[vabr (Chromium), 2015-07-31 12:48:13]

Hi Vadym,

Thanks for your new patches. I left some more comments.

Vaclav

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:256:
RecordExperimentStatistics(web_contents(), dismissal_reason_);
On 2015/07/30 11:22:44, dvadym wrote:
> On 2015/07/27 15:44:49, vasilii wrote:
> > You should call this only for the "Save?" bubble.
> 
> Removed

So why don't we gather the statistics here?

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

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2033:
scoped_ptr<PromptObserver> prompt_observer(
Looks unused.

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2051: }
Is there a way (here and in the other test) to check that the correct password
was saved?

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2075:
password_manager::TestPasswordStore* password_store =
Is it actually possible to just store the credentials to the store directly
without loading the page for that? I'm concerned about lengthy tests, which are
prone to flaking (also, I acknowledge that this is the case already for some
existing tests, but still would be nice to fix).

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2075:
password_manager::TestPasswordStore* password_store =
Keeping the test password store in a naked pointer is not the best practice, but
since your CL did not start this you have a choice:
(1) Fix the new tests to use scoped_refptr, or
(2) at least insert a TODO(vabr) pointing to http://crbug.com/515870.

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:32: int
best_matched_username_index = -1;
To avoid issues with int type conversion, use size_t and forms.size() as the
invalid value instead of -1.

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

https://codereview.chromium.org/1151373006/diff/380001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:691: bool
update_password = !provisional_save_manager_->best_matches().empty() &&
Can it happen that best_matches are non-empty, but the changed credential is not
one of them? I believe it can, the user might decide not to save the credential
they are currently logged in with.

[vasilii, 2015-07-31 14:38:54]

Can you specify right here the meanings of pending_credentials() and
best_matches() after PromptUserToSaveOrUpdatePassword()?

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/1151373006/diff/280001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:256:
RecordExperimentStatistics(web_contents(), dismissal_reason_);
On 2015/07/31 12:48:13, vabr (Chromium) wrote:
> On 2015/07/30 11:22:44, dvadym wrote:
> > On 2015/07/27 15:44:49, vasilii wrote:
> > > You should call this only for the "Save?" bubble.
> > 
> > Removed
> 
> So why don't we gather the statistics here?

Because it's not a save bubble and it should not affect the smart bubble
algorithm.

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

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2051:
ui_controller->state());
Shouldn't the test work with the infobar as well? You just check that the prompt
is shown.

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2056:
ASSERT_TRUE(ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled());
How is this enforced on Mac?

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2080: .get());
What about pushing the password to the store directly. Everything above looks
like a test setup and not a test logic.

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

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:579:
controller()->UpdateIconAndBubbleState(&mock);
You need to check the mock state here or just remove this line.

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:13: #include
"ui/views/layout/fill_layout.h"
Is it used?

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right):

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_password_items_view.cc:211:
host_->model_->state() ==
I think now it's easier to check agains MANAGE_STATE.

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:978: //
ManagePasswordsBubbleView::UpdatePendingView --------------------------------
One dash more than required ;-)

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1026: item =
new ManagePasswordItemsView(
DCHECK that the size is 1.

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:336:
!client_->IsUpdatePasswordUIEnabled() && is_change_password_form &&
I'd put this new expression to the end of the condition for clarity.

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:886:
pending_credentials_ = *best_update_match;
Here you overwrite pending_credentials_ so it contains the old password. Later
the UI calls Update(pending_credentials) and the old password goes to the store.

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:887: }
no need in brackets. The surrounding code doesn't use them.

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:888:
pending_credentials_.origin = provisionally_saved_form_->origin;
Why overwriting the origin?

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:149: //
password from |pending_credentials_|. It modifies |pending_credentials_|.
You should mention that |credentials_to_update| is one of the best_matches

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:1619:
// By now, the PasswordFormManager should have promoted the new password value
The comment says that it didn't happen in fact.

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:1638:
EXPECT_EQ(saved_match()->submit_element, new_credentials.submit_element);
Do I understand right that the new credential has the only different member
which is the password value. If this is the case just write
EXPECT_CALL(*mock_store(), UpdateLogin(new_credentials)); with the correct
|new_credentials|

[dvadym, 2015-08-03 15:44:08]

Hi Vasilii and Vaclav,

 I've addressed your comments (please see PatchSet 24). PTAL

Best regards,
Vadym

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

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2033:
scoped_ptr<PromptObserver> prompt_observer(
On 2015/07/31 12:48:13, vabr (Chromium) wrote:
> Looks unused.

Not it's used after adding checking of credentials

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2051: }
On 2015/07/31 12:48:13, vabr (Chromium) wrote:
> Is there a way (here and in the other test) to check that the correct password
> was saved?

Good point, I've added check for these tests

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2075:
password_manager::TestPasswordStore* password_store =
On 2015/07/31 12:48:13, vabr (Chromium) wrote:
> Keeping the test password store in a naked pointer is not the best practice,
but
> since your CL did not start this you have a choice:
> (1) Fix the new tests to use scoped_refptr, or
> (2) at least insert a TODO(vabr) pointing to http://crbug.com/515870.

Here we don't owe this TestPasswordStore, so it seems ok to use raw pointers.
Isn't it true?

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2075:
password_manager::TestPasswordStore* password_store =
On 2015/07/31 12:48:13, vabr (Chromium) wrote:
> Is it actually possible to just store the credentials to the store directly
> without loading the page for that? I'm concerned about lengthy tests, which
are
> prone to flaking (also, I acknowledge that this is the case already for some
> existing tests, but still would be nice to fix).

I've implemented such possibility and made direct adding PasswordForm to
PasswordStore

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:32: int
best_matched_username_index = -1;
On 2015/07/31 12:48:13, vabr (Chromium) wrote:
> To avoid issues with int type conversion, use size_t and forms.size() as the
> invalid value instead of -1.

Done.

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

https://codereview.chromium.org/1151373006/diff/380001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:691: bool
update_password = !provisional_save_manager_->best_matches().empty() &&
On 2015/07/31 12:48:13, vabr (Chromium) wrote:
> Can it happen that best_matches are non-empty, but the changed credential is
not
> one of them? I believe it can, the user might decide not to save the
credential
> they are currently logged in with.

Yes, it's possible, but if the user decide not to save, he/she probably will not
choose to save on password update anyway. So it seems to be good logic to
consider that the currently updated credentials are among of already stored.

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

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2051:
ui_controller->state());
On 2015/07/31 14:38:53, vasilii wrote:
> Shouldn't the test work with the infobar as well? You just check that the
prompt
> is shown.

I think it should be test only for testing change password form, for info bar
it's not implemented yet. For checking when to show or not to show
bubble/infobar w/o change password support flag we have another browser tests,
namely ChangePwdCorrect and ChangePwdIncorrect

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2056:
ASSERT_TRUE(ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled());
On 2015/07/31 14:38:53, vasilii wrote:
> How is this enforced on Mac?

I'm in progress of figuring out how it should work on Mac

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2080: .get());
On 2015/07/31 14:38:53, vasilii wrote:
> What about pushing the password to the store directly. Everything above looks
> like a test setup and not a test logic.

Done

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

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:579:
controller()->UpdateIconAndBubbleState(&mock);
On 2015/07/31 14:38:53, vasilii wrote:
> You need to check the mock state here or just remove this line.

Done.

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:13: #include
"ui/views/layout/fill_layout.h"
On 2015/07/31 14:38:53, vasilii wrote:
> Is it used?

Thanks, removed

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right):

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_password_items_view.cc:211:
host_->model_->state() ==
On 2015/07/31 14:38:53, vasilii wrote:
> I think now it's easier to check agains MANAGE_STATE.

Thanks, it's really easier

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:978: //
ManagePasswordsBubbleView::UpdatePendingView --------------------------------
On 2015/07/31 14:38:54, vasilii wrote:
> One dash more than required ;-)

Done.

https://codereview.chromium.org/1151373006/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1026: item =
new ManagePasswordItemsView(
On 2015/07/31 14:38:54, vasilii wrote:
> DCHECK that the size is 1.

Done.

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:336:
!client_->IsUpdatePasswordUIEnabled() && is_change_password_form &&
On 2015/07/31 14:38:54, vasilii wrote:
> I'd put this new expression to the end of the condition for clarity.

Done.

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:886:
pending_credentials_ = *best_update_match;
On 2015/07/31 14:38:54, vasilii wrote:
> Here you overwrite pending_credentials_ so it contains the old password. Later
> the UI calls Update(pending_credentials) and the old password goes to the
store.

This code is part of a function CreatePendingCredentials(), this is not
overwriting, this is exactly the place where pending_credentials is created.
Before this assignment pending_credentials is an empty object. A password field
is set on the line 925.

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:887: }
On 2015/07/31 14:38:54, vasilii wrote:
> no need in brackets. The surrounding code doesn't use them.

Done.

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:888:
pending_credentials_.origin = provisionally_saved_form_->origin;
On 2015/07/31 14:38:54, vasilii wrote:
> Why overwriting the origin?

Good point, we need to set origin only in case when no best match found, because
origin can't be empty.

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:149: //
password from |pending_credentials_|. It modifies |pending_credentials_|.
On 2015/07/31 14:38:54, vasilii wrote:
> You should mention that |credentials_to_update| is one of the best_matches

Done.

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:1619:
// By now, the PasswordFormManager should have promoted the new password value
On 2015/07/31 14:38:54, vasilii wrote:
> The comment says that it didn't happen in fact.

I took this comment from line 563, since I've copied part of that tests. But it
seems to be correct, |credentials| is initial credentials with new passsword,
GetPendingCredentials(&manager) returns credentials from PasswordFormManager
which are mentioned in the comment.

https://codereview.chromium.org/1151373006/diff/400001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:1638:
EXPECT_EQ(saved_match()->submit_element, new_credentials.submit_element);
On 2015/07/31 14:38:54, vasilii wrote:
> Do I understand right that the new credential has the only different member
> which is the password value. If this is the case just write
> EXPECT_CALL(*mock_store(), UpdateLogin(new_credentials)); with the correct
> |new_credentials|

No, it can contain a lot of different members, that are not related for my
changes and the behavior how they are changed can be changed in future CLs. For
example it will contain update of usage statistics (you can see the method
PasswordFormManager::UpdateLogin() which is called as result of Update()). So I
think it's better to check only relevant fields.

[vabr (Chromium), 2015-08-04 08:59:08]

LGTM with minor nits (and the expectation that any further potential Vasilii's
comments get addressed).

Cheers,
Vaclav

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

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2075:
password_manager::TestPasswordStore* password_store =
On 2015/08/03 15:44:07, dvadym wrote:
> On 2015/07/31 12:48:13, vabr (Chromium) wrote:
> > Keeping the test password store in a naked pointer is not the best practice,
> but
> > since your CL did not start this you have a choice:
> > (1) Fix the new tests to use scoped_refptr, or
> > (2) at least insert a TODO(vabr) pointing to http://crbug.com/515870.
> 
> Here we don't owe this TestPasswordStore, so it seems ok to use raw pointers.
> Isn't it true?

For scoped_refptr (unlike scoped_ptr), there is no notion of a single owner --
anybody needing the object to live should increase its refcount. We might argue
whether some other piece of code keeps the store alive long enough for us not to
care, but this does not make the code clearer. Let's follow the standard
practice of keeping refcounted objects in scoped_refptr. (Note: the situation is
very different in Blink's refcounting, where performance is a big concern.)

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

https://codereview.chromium.org/1151373006/diff/380001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:691: bool
update_password = !provisional_save_manager_->best_matches().empty() &&
On 2015/08/03 15:44:07, dvadym wrote:
> On 2015/07/31 12:48:13, vabr (Chromium) wrote:
> > Can it happen that best_matches are non-empty, but the changed credential is
> not
> > one of them? I believe it can, the user might decide not to save the
> credential
> > they are currently logged in with.
> 
> Yes, it's possible, but if the user decide not to save, he/she probably will
not
> choose to save on password update anyway. So it seems to be good logic to
> consider that the currently updated credentials are among of already stored.

Acknowledged.

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

https://codereview.chromium.org/1151373006/diff/460001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:129:
ASSERT_EQ(username, form.username_value);
EXPECT rather than ASSERT?
Unlike the above 2 asserts, a failure on lines 129 and 130 would be non-fatal,
so we may as well avoid aborting the whole test.

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

https://codereview.chromium.org/1151373006/diff/460001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:66: return false;
Also here, please insert a TODO comment noting that this should be changed to
pure virtual and implemented for infobars once http://crbug.com/359315 is done
for infobars.

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

https://codereview.chromium.org/1151373006/diff/460001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:92: const
autofill::PasswordForm& form) const {}
Please insert a TODO comment noting that this should be changed to pure virtual
and implemented for infobars once http://crbug.com/359315 is done for infobars.

[dvadym, 2015-08-04 14:34:46]

Thanks Vaclav and Vasilii for comments!

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

https://codereview.chromium.org/1151373006/diff/380001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2075:
password_manager::TestPasswordStore* password_store =
On 2015/08/04 08:59:07, vabr (Chromium) wrote:
> On 2015/08/03 15:44:07, dvadym wrote:
> > On 2015/07/31 12:48:13, vabr (Chromium) wrote:
> > > Keeping the test password store in a naked pointer is not the best
practice,
> > but
> > > since your CL did not start this you have a choice:
> > > (1) Fix the new tests to use scoped_refptr, or
> > > (2) at least insert a TODO(vabr) pointing to http://crbug.com/515870.
> > 
> > Here we don't owe this TestPasswordStore, so it seems ok to use raw
pointers.
> > Isn't it true?
> 
> For scoped_refptr (unlike scoped_ptr), there is no notion of a single owner --
> anybody needing the object to live should increase its refcount. We might
argue
> whether some other piece of code keeps the store alive long enough for us not
to
> care, but this does not make the code clearer. Let's follow the standard
> practice of keeping refcounted objects in scoped_refptr. (Note: the situation
is
> very different in Blink's refcounting, where performance is a big concern.)

Done.

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

https://codereview.chromium.org/1151373006/diff/460001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:129:
ASSERT_EQ(username, form.username_value);
On 2015/08/04 08:59:07, vabr (Chromium) wrote:
> EXPECT rather than ASSERT?
> Unlike the above 2 asserts, a failure on lines 129 and 130 would be non-fatal,
> so we may as well avoid aborting the whole test.

Done.

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

https://codereview.chromium.org/1151373006/diff/460001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.cc:66: return false;
On 2015/08/04 08:59:07, vabr (Chromium) wrote:
> Also here, please insert a TODO comment noting that this should be changed to
> pure virtual and implemented for infobars once http://crbug.com/359315 is done
> for infobars.

Done.

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

https://codereview.chromium.org/1151373006/diff/460001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_test_base.h:92: const
autofill::PasswordForm& form) const {}
On 2015/08/04 08:59:07, vabr (Chromium) wrote:
> Please insert a TODO comment noting that this should be changed to pure
virtual
> and implemented for infobars once http://crbug.com/359315 is done for
infobars.

Done.

[dvadym, 2015-08-04 14:36:32]

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

[dvadym, 2015-08-04 14:36:33]

Hi Ilya,

 Could you please review changes in histograms.xml?

Best regards,
Vadym

[Ilya Sherman, 2015-08-04 18:31:16]

histograms.xml lgtm

[vasilii, 2015-08-05 13:21:14]

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

https://codereview.chromium.org/1151373006/diff/580001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2082: #define
MAYBE_ChangePwd1AccountStored DISABLED_ChangePwd1AccountStored
You can put the whole test under #ifndef. It's not flaky.

https://codereview.chromium.org/1151373006/diff/580001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/580001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1087:
parent_->model()->OnUpdateClicked(parent_->model()->pending_password());
It should be parent->model()->local_credentials()[0].

https://codereview.chromium.org/1151373006/diff/580001/components/password_ma...
File components/password_manager/core/browser/test_password_store.h (right):

https://codereview.chromium.org/1151373006/diff/580001/components/password_ma...
components/password_manager/core/browser/test_password_store.h:36: void
AddCredentials(const autofill::PasswordForm& form);
You can use AddLogin. The only place where you use it should work fine.

[dvadym, 2015-08-05 15:18:20]

Thanks Vasilii!
 
 I've addressed your comments and changed to showing new password in update
bubble (according to discussion with UX). PTAL.

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

https://codereview.chromium.org/1151373006/diff/580001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:2082: #define
MAYBE_ChangePwd1AccountStored DISABLED_ChangePwd1AccountStored
On 2015/08/05 13:21:14, vasilii wrote:
> You can put the whole test under #ifndef. It's not flaky.

Done.

https://codereview.chromium.org/1151373006/diff/580001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/580001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1087:
parent_->model()->OnUpdateClicked(parent_->model()->pending_password());
On 2015/08/05 13:21:14, vasilii wrote:
> It should be parent->model()->local_credentials()[0].

Done.

https://codereview.chromium.org/1151373006/diff/580001/components/password_ma...
File components/password_manager/core/browser/test_password_store.h (right):

https://codereview.chromium.org/1151373006/diff/580001/components/password_ma...
components/password_manager/core/browser/test_password_store.h:36: void
AddCredentials(const autofill::PasswordForm& form);
On 2015/08/05 13:21:14, vasilii wrote:
> You can use AddLogin. The only place where you use it should work fine.

Thanks!

[vasilii, 2015-08-05 15:42:55]

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:79:
views::Label* label =
I'd get rid of |label| to avoid memory leaks

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.h (right):

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.h:10: #include
"ui/views/controls/combobox/combobox_listener.h"
Remove

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.h:15: class Label;
Remove

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1028:
forms.push_back(&parent->model()->pending_password());
This looks like a bug to me. pending_password() can be uninitialized.

[dvadym, 2015-08-05 16:40:43]

Thanks Vasilii! I've addressed your comments in PatchSet 32

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:79:
views::Label* label =
On 2015/08/05 15:42:55, vasilii wrote:
> I'd get rid of |label| to avoid memory leaks

As far as I understand |label| will be removed on destroying bubble, since it's
added to layout. Am I right?

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.h (right):

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.h:10: #include
"ui/views/controls/combobox/combobox_listener.h"
On 2015/08/05 15:42:55, vasilii wrote:
> Remove

Done.

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.h:15: class Label;
On 2015/08/05 15:42:55, vasilii wrote:
> Remove

Done.

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1028:
forms.push_back(&parent->model()->pending_password());
On 2015/08/05 15:42:55, vasilii wrote:
> This looks like a bug to me. pending_password() can be uninitialized.

Thanks you very much. I've fixed it by changing
PasswordFormManager::FindBestMatchForUpdatePassword.

[vasilii, 2015-08-05 17:34:08]

Hopefully the last round of comments :-)

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right):

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_selection_view.cc:79:
views::Label* label =
On 2015/08/05 16:40:43, dvadym wrote:
> On 2015/08/05 15:42:55, vasilii wrote:
> > I'd get rid of |label| to avoid memory leaks
> 
> As far as I understand |label| will be removed on destroying bubble, since
it's
> added to layout. Am I right?

Yes, I meant to add the view directly. The variable seems excessive.

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/600001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1028:
forms.push_back(&parent->model()->pending_password());
On 2015/08/05 16:40:43, dvadym wrote:
> On 2015/08/05 15:42:55, vasilii wrote:
> > This looks like a bug to me. pending_password() can be uninitialized.
> 
> Thanks you very much. I've fixed it by changing
> PasswordFormManager::FindBestMatchForUpdatePassword.

Acknowledged.

https://codereview.chromium.org/1151373006/diff/620001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/620001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1027:
std::vector<const autofill::PasswordForm*> forms;
Please add a DCHECK that the username is the same as in
parent->model()->local_credentials()[0]. That's important because later you
update *parent_->model()->local_credentials()[0] and not
parent->model()->pending_password().

https://codereview.chromium.org/1151373006/diff/620001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/620001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1001: // In
case when there is no old password on the form and the user has only
Update the comment

https://codereview.chromium.org/1151373006/diff/620001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1005: return
nullptr;
Are you sure? :-)

https://codereview.chromium.org/1151373006/diff/620001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/1151373006/diff/620001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:366: // entry,
otherwise nullptr.
Refresh the comment.

[dvadym, 2015-08-06 08:41:18]

Thanks Vasilii, PTAL

https://codereview.chromium.org/1151373006/diff/620001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/620001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1027:
std::vector<const autofill::PasswordForm*> forms;
On 2015/08/05 17:34:08, vasilii wrote:
> Please add a DCHECK that the username is the same as in
> parent->model()->local_credentials()[0]. That's important because later you
> update *parent_->model()->local_credentials()[0] and not
> parent->model()->pending_password().

Done.

https://codereview.chromium.org/1151373006/diff/620001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/620001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1001: // In
case when there is no old password on the form and the user has only
On 2015/08/05 17:34:08, vasilii wrote:
> Update the comment

Done.

https://codereview.chromium.org/1151373006/diff/620001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1005: return
nullptr;
On 2015/08/05 17:34:08, vasilii wrote:
> Are you sure? :-)

Thanks, done.

[vasilii, 2015-08-06 08:54:49]

https://codereview.chromium.org/1151373006/diff/640001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/640001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1028:
parent->model()->pending_password().username_value);
Here and above use DCHECK_EQ. This condition shouldn't crash the browser.

https://codereview.chromium.org/1151373006/diff/640001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/640001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1001: // In
case when there is no old password on the form and the user has only
The sentence about no old password is obsolete.

[dvadym, 2015-08-06 09:10:24]

Thanks Vasilii, PTAL.

https://codereview.chromium.org/1151373006/diff/640001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/640001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1028:
parent->model()->pending_password().username_value);
On 2015/08/06 08:54:49, vasilii wrote:
> Here and above use DCHECK_EQ. This condition shouldn't crash the browser.

Ah thanks, I made typos in DCHECK :)

https://codereview.chromium.org/1151373006/diff/640001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/640001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1001: // In
case when there is no old password on the form and the user has only
On 2015/08/06 08:54:49, vasilii wrote:
> The sentence about no old password is obsolete.

Done.

[vasilii, 2015-08-06 09:20:04]

lgtm

https://codereview.chromium.org/1151373006/diff/660001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/660001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1026:
DCHECK(parent->model()->local_credentials().size() == 1);
DCHECK_EQ

https://codereview.chromium.org/1151373006/diff/660001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/660001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1001: // In
case when the user has only one piece of credentials, consider it the
only one credential ...

[dvadym, 2015-08-06 09:34:12]

https://codereview.chromium.org/1151373006/diff/660001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1151373006/diff/660001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1026:
DCHECK(parent->model()->local_credentials().size() == 1);
On 2015/08/06 09:20:04, vasilii wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/1151373006/diff/660001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1151373006/diff/660001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1001: // In
case when the user has only one piece of credentials, consider it the
On 2015/08/06 09:20:04, vasilii wrote:
> only one credential ...

Done.

[dvadym, 2015-08-06 09:50:00]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2015-08-06 09:50:01]

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

[commit-bot: I haz the power, 2015-08-06 09:50:12]

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

[commit-bot: I haz the power, 2015-08-06 11:06:58]

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

[commit-bot: I haz the power, 2015-08-06 11:06:59]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[dvadym, 2015-08-06 11:12:51]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-08-06 11:13:01]

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

[commit-bot: I haz the power, 2015-08-06 12:49:22]

Committed patchset #36 (id:700001)

[commit-bot: I haz the power, 2015-08-06 12:50:13]

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