Change the password bubble on Mac so the columns are resized dynamically.

Change the password bubble on Mac so the columns are resized dynamically.

BUG=414787
Committed: https://crrev.com/6cab722144a4fb46c3990f578652f900cbf66ecc
Cr-Commit-Position: refs/heads/master@{#357070}

[vasilii, 2015-10-23 17:32:29]

Description was changed from

==========
Change the password bubble on Mac so the columns are resized dynamically.

BUG=414787
==========

to

==========
Change the password bubble on Mac so the columns are resized dynamically.

BUG=414787
==========

[vasilii, 2015-10-23 17:32:29]

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

[vasilii, 2015-10-23 17:33:38]

Hi Rachel,

please review the CL. There are actually not that many changes as it's shown. I
renamed some files.

[groby-ooo-7-16, 2015-10-23 23:57:00]

On 2015/10/23 17:33:38, vasilii wrote:
> Hi Rachel,
> 
> please review the CL. There are actually not that many changes as it's shown.
I
> renamed some files.

Wait, shouldn't the CQ pick up the moves?

[groby-ooo-7-16, 2015-10-24 00:16:23]

On 2015/10/23 23:57:00, groby wrote:
> On 2015/10/23 17:33:38, vasilii wrote:
> > Hi Rachel,
> > 
> > please review the CL. There are actually not that many changes as it's
shown.
> I
> > renamed some files.
> 
> Wait, shouldn't the CQ pick up the moves?

On reading this further, it seems the files were renamed and changed? Would you
mind breaking this into two separate CLs, since it's otherwise a bit hard to
follow what's new and what's just moved code?

Thank you!

[groby-ooo-7-16, 2015-10-24 00:17:01]

Sorry. Hit 'reply', which doesn't publish comments.

https://codereview.chromium.org/1409223005/diff/1/chrome/browser/ui/cocoa/pas...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.h
(right):

https://codereview.chromium.org/1409223005/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.h:28:
base::scoped_nsobject<NoPasswordsView> noPasswordsView_;
Since we can have only one content view, why not keep a contentView_ and cast in
the accessors?

https://codereview.chromium.org/1409223005/diff/1/chrome/browser/ui/cocoa/pas...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.mm
(right):

https://codereview.chromium.org/1409223005/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.mm:95:
forms:model_->local_credentials().get()]);
Question: Since we get the forms from the model, do we need a forms: parameter?

https://codereview.chromium.org/1409223005/diff/1/chrome/browser/ui/cocoa/pas...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller_unittest.mm
(right):

https://codereview.chromium.org/1409223005/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller_unittest.mm:90:
NSArray* items = controller().passwordsListController.itemViews;
I'm fine with either property-style syntax or message-style syntax, but we
should pick one :)

[vasilii, 2015-10-26 13:32:09]

I reverted the renaming.

https://codereview.chromium.org/1409223005/diff/1/chrome/browser/ui/cocoa/pas...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.h
(right):

https://codereview.chromium.org/1409223005/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.h:28:
base::scoped_nsobject<NoPasswordsView> noPasswordsView_;
On 2015/10/24 00:17:01, groby wrote:
> Since we can have only one content view, why not keep a contentView_ and cast
in
> the accessors?

NoPasswordsView is a View, PasswordsListViewController is a view controller. How
can I unite them?

https://codereview.chromium.org/1409223005/diff/1/chrome/browser/ui/cocoa/pas...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.mm
(right):

https://codereview.chromium.org/1409223005/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller.mm:95:
forms:model_->local_credentials().get()]);
On 2015/10/24 00:17:01, groby wrote:
> Question: Since we get the forms from the model, do we need a forms:
parameter?

We can do it. But then we do it for all the controllers simultaneously. The
logic goes to ManagePasswordsBubbleController::updateState.
ManagePasswordsBubbleController becomes suddenly very smart as it should know
how to init all the controllers (by using either pending_password(),
local_credentials() or federated_credentials()). These controllers still need
|model_|. So I don't see a win.

https://codereview.chromium.org/1409223005/diff/1/chrome/browser/ui/cocoa/pas...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller_unittest.mm
(right):

https://codereview.chromium.org/1409223005/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_manage_view_controller_unittest.mm:90:
NSArray* items = controller().passwordsListController.itemViews;
On 2015/10/24 00:17:01, groby wrote:
> I'm fine with either property-style syntax or message-style syntax, but we
> should pick one :)
> 

Done.

[vasilii, 2015-10-28 09:04:13]

Rachel?

[groby-ooo-7-16, 2015-10-28 23:08:55]

On 2015/10/28 09:04:13, vasilii wrote:
> Rachel?

Reviewing as I speak.

[groby-ooo-7-16, 2015-10-28 23:37:31]

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.h
(right):

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.h:25: -
(ManagePasswordsBubbleModel*)model;
Why not as property?

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm
(right):

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:45:
std::pair<CGFloat, CGFloat> GetResizedColumns(CGFloat maxWidth,
Hm. Since this is essentially returning the adjusted
firstColumnWidth/secondColumnWidth, can you use the same type for in and out?

Either std::pair, or CGFloat[2]?

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:78:
[[textField cell] setLineBreakMode:NSLineBreakByCharWrapping];
That's an odd line break mode for text. since you truncate, what about
NSLineBreakByTruncatingTail?

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:124:
chrome_style::GetLinkColor())];
I really dislike this formatting by clang-format. (No need to fix. Just
registering an objection. Probably not worth a bug report, either)

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:200:
if (form.federation_url.is_empty()) {
Do you mean is_empty, or would is_valid be a better choice?

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:323: -
(NSTextField*)passwordField {
Why not NSSecureTextField? Do we want to actually display the password?

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:446:
base::scoped_nsobject<NSMutableArray> items([[NSMutableArray alloc] init]);
Why not +arrayWithCapacity?

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:462:
// Create the controllers.
This doesn't create the controllers, though?

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:474:
const CGFloat curX = 0;
NSPoint cur would probably be a bit more succinct.

[vasilii, 2015-10-29 16:12:37]

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.h
(right):

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.h:25: -
(ManagePasswordsBubbleModel*)model;
On 2015/10/28 23:37:30, groby wrote:
> Why not as property?

Done.

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm
(right):

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:45:
std::pair<CGFloat, CGFloat> GetResizedColumns(CGFloat maxWidth,
On 2015/10/28 23:37:31, groby wrote:
> Hm. Since this is essentially returning the adjusted
> firstColumnWidth/secondColumnWidth, can you use the same type for in and out?
> 
> Either std::pair, or CGFloat[2]?

I'm curious how one can return CGFloat[2]?

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:78:
[[textField cell] setLineBreakMode:NSLineBreakByCharWrapping];
On 2015/10/28 23:37:30, groby wrote:
> That's an odd line break mode for text. since you truncate, what about
> NSLineBreakByTruncatingTail?

Done.

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:124:
chrome_style::GetLinkColor())];
On 2015/10/28 23:37:31, groby wrote:
> I really dislike this formatting by clang-format. (No need to fix. Just
> registering an objection. Probably not worth a bug report, either)

Acknowledged.

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:200:
if (form.federation_url.is_empty()) {
On 2015/10/28 23:37:31, groby wrote:
> Do you mean is_empty, or would is_valid be a better choice?

I mean is_empty because a PasswordForm with a non empty federation can't have a
password. We'll just show an empty field.

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:323: -
(NSTextField*)passwordField {
On 2015/10/28 23:37:31, groby wrote:
> Why not NSSecureTextField? Do we want to actually display the password?

The field contains either the password or the federated string. The password is
stored in NSSecureTextField in fact (see PasswordLabel()). The federated string
is a plain text.

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:446:
base::scoped_nsobject<NSMutableArray> items([[NSMutableArray alloc] init]);
On 2015/10/28 23:37:30, groby wrote:
> Why not +arrayWithCapacity?

Done.

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:462:
// Create the controllers.
On 2015/10/28 23:37:30, groby wrote:
> This doesn't create the controllers, though?

Indeed.

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:474:
const CGFloat curX = 0;
On 2015/10/28 23:37:31, groby wrote:
> NSPoint cur would probably be a bit more succinct.

Done.

[groby-ooo-7-16, 2015-10-29 20:44:35]

lgtm

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm
(right):

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:45:
std::pair<CGFloat, CGFloat> GetResizedColumns(CGFloat maxWidth,
On 2015/10/29 16:12:37, vasilii wrote:
> On 2015/10/28 23:37:31, groby wrote:
> > Hm. Since this is essentially returning the adjusted
> > firstColumnWidth/secondColumnWidth, can you use the same type for in and
out?
> > 
> > Either std::pair, or CGFloat[2]?
> 
> I'm curious how one can return CGFloat[2]?

In a world where C++ is sane and structs and fixed-size arrays are treated
equally. (Yeah, this won't work. Brain fart)

So std::pair or as-is. I'm not super-excited about either, so I'll leave that up
to you.

[vasilii, 2015-10-30 09:08:57]

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm
(right):

https://codereview.chromium.org/1409223005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_password_item_view_controller.mm:45:
std::pair<CGFloat, CGFloat> GetResizedColumns(CGFloat maxWidth,
On 2015/10/29 20:44:34, groby wrote:
> On 2015/10/29 16:12:37, vasilii wrote:
> > On 2015/10/28 23:37:31, groby wrote:
> > > Hm. Since this is essentially returning the adjusted
> > > firstColumnWidth/secondColumnWidth, can you use the same type for in and
> out?
> > > 
> > > Either std::pair, or CGFloat[2]?
> > 
> > I'm curious how one can return CGFloat[2]?
> 
> In a world where C++ is sane and structs and fixed-size arrays are treated
> equally. (Yeah, this won't work. Brain fart)
> 
> So std::pair or as-is. I'm not super-excited about either, so I'll leave that
up
> to you.

Done.

[vasilii, 2015-10-30 09:09:10]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2015-10-30 09:09:20]

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

[commit-bot: I haz the power, 2015-10-30 10:08:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-30 10:09:56]

Patchset 4 (id:??) landed as
https://crrev.com/6cab722144a4fb46c3990f578652f900cbf66ecc
Cr-Commit-Position: refs/heads/master@{#357070}