Change password bubble for Mac.

Change password bubble for Mac.

This CL contains all UI changes related to change password support on Mac. On Windows/Linux UI was implemented in https://codereview.chromium.org/1151373006/ and https://codereview.chromium.org/1271283002/.

This CL contains implementation of 2 bubbles:
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.

Also in case when there are no stored credentials and no username form found username-password row is not shown in the bubble.

Screenshots of UI: https://docs.google.com/document/d/1k1SN_e5XQtSTEQnssPlpZRKz66aXC3h6WS35AXtz9BA/edit#

Update bubble is currently behind a flag until UI review.

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

[dvadym, 2015-12-15 13:42:21]

Description was changed from

==========
Change password bubble for Mac

BUG=359315
==========

to

==========
Change password bubble for Mac.
 
This CL contains all UI changes related to change password support on Mac. On
Windows/Linux UI was implemented in https://codereview.chromium.org/1151373006/
and https://codereview.chromium.org/1271283002/.

This CL contains implementation of 2 bubbles:
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.

Also in case when there are no stored credentials and no username form found
username-password row is not shown in the bubble. 

Screenshots of UI:
https://docs.google.com/document/d/1k1SN_e5XQtSTEQnssPlpZRKz66aXC3h6WS35AXtz9...

Update bubble is currently behind a flag until UI review.

BUG=359315
==========

[dvadym, 2015-12-15 14:11:23]

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

[dvadym, 2015-12-15 14:11:23]

Hi Vasilii,

 Could you please review this code from password manager point of view?

Best regards,
Vadym

[vasilii, 2015-12-15 15:58:02]

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

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2239: #endif
Shouldn't it go to the test's setup?

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/credentials_selection_view.h (right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.h:12: #import
"ui/base/cocoa/tracking_area.h"
these 3 headers seem unused.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm (right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:34:
scoped_ptr<ui::SimpleComboboxModel> model(
Looks like without the scoped pointer it will work fine for you.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:73: CGFloat curY
= kRelatedControlVerticalSpacing;
I'm not sure about this. You hardcode the margin here. Instead it's embedder's
duty to layout its views properly.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:87:
setFrameOrigin:NSMakePoint(curX, curY + kVerticalComboboxLabelSpacing)];
I don't know how it looks like if the user changes the system font. Did you try
to align the centers of two controls?

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:101:
static_cast<size_t>([usernamePopUpButton_ indexOfSelectedItem]);
It can return -1.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/passwords_list_view_controller.mm
(right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/passwords_list_view_controller.mm:291:
password_manager::ui::PENDING_PASSWORD_UPDATE_STATE)
Why do you need the new state here?

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/pending_password_view_controller.mm
(right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/pending_password_view_controller.mm:136:
NSView* password = [self createPasswordView];
It's rather a password row, not just a password

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/pending_password_view_controller.mm:182: //
Password item goes on the next row.
indent

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller.mm
(right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller.mm:43:
return nil;
A new test for this required.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller.mm
(right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller.mm:64:
return NO;
This is the default implementation.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller_unittest.mm
(right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller_unittest.mm:51:
EXPECT_CALL(*ui_controller(), UpdatePassword(testing::_));
testing:: is excessive.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller_unittest.mm:73:
}
Test that a right inner view is instantiated (combobox vs. label).

[dvadym, 2015-12-16 14:16:31]

Thanks Vasilii for comments! I've addressed all your comments, PTAL

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

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2239: #endif
On 2015/12/15 15:58:01, vasilii wrote:
> Shouldn't it go to the test's setup?

It will be removed with the flag in a couple of weeks after UI review.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/credentials_selection_view.h (right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.h:12: #import
"ui/base/cocoa/tracking_area.h"
On 2015/12/15 15:58:01, vasilii wrote:
> these 3 headers seem unused.

Done, thanks.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm (right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:34:
scoped_ptr<ui::SimpleComboboxModel> model(
On 2015/12/15 15:58:01, vasilii wrote:
> Looks like without the scoped pointer it will work fine for you.

Done.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:73: CGFloat curY
= kRelatedControlVerticalSpacing;
On 2015/12/15 15:58:01, vasilii wrote:
> I'm not sure about this. You hardcode the margin here. Instead it's embedder's
> duty to layout its views properly.

Done, I've set curY to 0

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:87:
setFrameOrigin:NSMakePoint(curX, curY + kVerticalComboboxLabelSpacing)];
On 2015/12/15 15:58:01, vasilii wrote:
> I don't know how it looks like if the user changes the system font. Did you
try
> to align the centers of two controls?

Thanks, I've replaced this constant by difference of heights of these two
elements

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:101:
static_cast<size_t>([usernamePopUpButton_ indexOfSelectedItem]);
On 2015/12/15 15:58:01, vasilii wrote:
> It can return -1.

Done.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/passwords_list_view_controller.mm
(right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/passwords_list_view_controller.mm:291:
password_manager::ui::PENDING_PASSWORD_UPDATE_STATE)
On 2015/12/15 15:58:01, vasilii wrote:
> Why do you need the new state here?

Because PasswordsListViewController is used in the Update bubble when there is
only one account to update

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/pending_password_view_controller.mm
(right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/pending_password_view_controller.mm:136:
NSView* password = [self createPasswordView];
On 2015/12/15 15:58:01, vasilii wrote:
> It's rather a password row, not just a password

Done.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/pending_password_view_controller.mm:182: //
Password item goes on the next row.
On 2015/12/15 15:58:01, vasilii wrote:
> indent

Done.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller.mm
(right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller.mm:43:
return nil;
On 2015/12/15 15:58:02, vasilii wrote:
> A new test for this required.

Tests
SavePendingPasswordViewControllerTest.ShouldShowPasswordRowWhenUsernameNonEmpty
SavePendingPasswordViewControllerTest.ShouldNotShowPasswordRowWhenUsernameEmpty
are added

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller.mm
(right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller.mm:64:
return NO;
On 2015/12/15 15:58:02, vasilii wrote:
> This is the default implementation.

Removed

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller_unittest.mm
(right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller_unittest.mm:51:
EXPECT_CALL(*ui_controller(), UpdatePassword(testing::_));
On 2015/12/15 15:58:02, vasilii wrote:
> testing:: is excessive.

Done.

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller_unittest.mm:73:
}
On 2015/12/15 15:58:02, vasilii wrote:
> Test that a right inner view is instantiated (combobox vs. label).

Tests 
UpdatePendingPasswordViewControllerTest.ShouldShowUsernameLabelWhenOneCredentials
UpdatePendingPasswordViewControllerTest.ShouldShowUsernamesComboboxWhenMultipleCredentials
are added

[vasilii, 2015-12-16 18:10:57]

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/passwords_list_view_controller.mm
(right):

https://codereview.chromium.org/1515553006/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/passwords_list_view_controller.mm:291:
password_manager::ui::PENDING_PASSWORD_UPDATE_STATE)
On 2015/12/16 14:16:31, dvadym wrote:
> On 2015/12/15 15:58:01, vasilii wrote:
> > Why do you need the new state here?
> 
> Because PasswordsListViewController is used in the Update bubble when there is
> only one account to update

Acknowledged.

https://codereview.chromium.org/1515553006/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h (right):

https://codereview.chromium.org/1515553006/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h:31: void
SetUpUpdatePendingState(bool multipleForms);
The class is C++, so keep using C++ style for naming.

https://codereview.chromium.org/1515553006/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm (right):

https://codereview.chromium.org/1515553006/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:44: [button
selectItemAtIndex:0];
Add DCHECK that forms not empty.

https://codereview.chromium.org/1515553006/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:85: CGFloat
popUpLabelHeightDifference =
Are you sure it's always positive? Otherwise the password may end up below the
parent view.

https://codereview.chromium.org/1515553006/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:94: curY =
NSMaxY([passwordField_ frame]) + kRelatedControlVerticalSpacing;
As with the code above I'd add this margin to the embedder.
One more bug is that you take NSMaxY of the password which maybe less that
NSMaxY of the combobox.

[dvadym, 2015-12-16 18:50:12]

Thanks Vasilii for comments. PTAL

https://codereview.chromium.org/1515553006/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h (right):

https://codereview.chromium.org/1515553006/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h:31: void
SetUpUpdatePendingState(bool multipleForms);
On 2015/12/16 18:10:57, vasilii wrote:
> The class is C++, so keep using C++ style for naming.

Done.

https://codereview.chromium.org/1515553006/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm (right):

https://codereview.chromium.org/1515553006/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:44: [button
selectItemAtIndex:0];
On 2015/12/16 18:10:57, vasilii wrote:
> Add DCHECK that forms not empty.

I've added at the begininng of this function   DCHECK(!forms.empty());

https://codereview.chromium.org/1515553006/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:85: CGFloat
popUpLabelHeightDifference =
On 2015/12/16 18:10:57, vasilii wrote:
> Are you sure it's always positive? Otherwise the password may end up below the
> parent view.

Done.

https://codereview.chromium.org/1515553006/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm:94: curY =
NSMaxY([passwordField_ frame]) + kRelatedControlVerticalSpacing;
On 2015/12/16 18:10:57, vasilii wrote:
> As with the code above I'd add this margin to the embedder.
> One more bug is that you take NSMaxY of the password which maybe less that
> NSMaxY of the combobox.

Thanks it makes sense

[vasilii, 2015-12-17 09:44:20]

lgtm

[dvadym, 2015-12-17 09:44:36]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-12-17 09:45:11]

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

[commit-bot: I haz the power, 2015-12-17 10:34:13]

Description was changed from

==========
Change password bubble for Mac.
 
This CL contains all UI changes related to change password support on Mac. On
Windows/Linux UI was implemented in https://codereview.chromium.org/1151373006/
and https://codereview.chromium.org/1271283002/.

This CL contains implementation of 2 bubbles:
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.

Also in case when there are no stored credentials and no username form found
username-password row is not shown in the bubble. 

Screenshots of UI:
https://docs.google.com/document/d/1k1SN_e5XQtSTEQnssPlpZRKz66aXC3h6WS35AXtz9...

Update bubble is currently behind a flag until UI review.

BUG=359315
==========

to

==========
Change password bubble for Mac.
 
This CL contains all UI changes related to change password support on Mac. On
Windows/Linux UI was implemented in https://codereview.chromium.org/1151373006/
and https://codereview.chromium.org/1271283002/.

This CL contains implementation of 2 bubbles:
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.

Also in case when there are no stored credentials and no username form found
username-password row is not shown in the bubble. 

Screenshots of UI:
https://docs.google.com/document/d/1k1SN_e5XQtSTEQnssPlpZRKz66aXC3h6WS35AXtz9...

Update bubble is currently behind a flag until UI review.

BUG=359315
==========

[commit-bot: I haz the power, 2015-12-17 10:34:14]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-17 10:35:06]

Description was changed from

==========
Change password bubble for Mac.
 
This CL contains all UI changes related to change password support on Mac. On
Windows/Linux UI was implemented in https://codereview.chromium.org/1151373006/
and https://codereview.chromium.org/1271283002/.

This CL contains implementation of 2 bubbles:
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.

Also in case when there are no stored credentials and no username form found
username-password row is not shown in the bubble. 

Screenshots of UI:
https://docs.google.com/document/d/1k1SN_e5XQtSTEQnssPlpZRKz66aXC3h6WS35AXtz9...

Update bubble is currently behind a flag until UI review.

BUG=359315
==========

to

==========
Change password bubble for Mac.

This CL contains all UI changes related to change password support on Mac. On
Windows/Linux UI was implemented in https://codereview.chromium.org/1151373006/
and https://codereview.chromium.org/1271283002/.

This CL contains implementation of 2 bubbles:
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.

Also in case when there are no stored credentials and no username form found
username-password row is not shown in the bubble.

Screenshots of UI:
https://docs.google.com/document/d/1k1SN_e5XQtSTEQnssPlpZRKz66aXC3h6WS35AXtz9...

Update bubble is currently behind a flag until UI review.

BUG=359315

Committed: https://crrev.com/5cc29b8637175db4d6a48fa7d8ea6119749fed7b
Cr-Commit-Position: refs/heads/master@{#365796}
==========

[commit-bot: I haz the power, 2015-12-17 10:35:07]

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