Integrate the account chooser dialog on Mac.

Integrate the account chooser dialog on Mac.

BUG=550922
Committed: https://crrev.com/26173e3e8ab8da1050ee7260dac3d2be714ad5f8
Cr-Commit-Position: refs/heads/master@{#370691}

[vasilii, 2016-01-20 16:33:18]

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

[vasilii, 2016-01-20 16:33:18]

Hi Vaclav,

please review the CL.

[vabr (Chromium), 2016-01-21 10:02:10]

Thank you, Vasilii, this CL LGTM with some minor nits below.


The UI code is rather complex, being different on different platforms, and with
the number of UI elements increasing. You have a good vision of what is the
structure of the classes, and what is the appropriate abstraction (platform
independent vs. dependent code) and separation (models vs. controllers, etc.).

It would be very valuable, if you could share this knowledge with the team, for
example through a tech talk or some kind of documentation. There is neither
pressure nor deadlines for this :), but please give it a thought; I and many
others would appreciate it.

Thanks!
Vaclav

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

https://codereview.chromium.org/1610653002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/account_chooser_view_controller.h:18: class
PasswordDialogController;
nit: I would move line 18 above the "namespace net" block, and separate by blank
lines from code above and below. Usually, non-namespaced forward declarations
are before namespaced ones, and line 13 confirms that in this file.

https://codereview.chromium.org/1610653002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/account_chooser_view_controller.h:38:
protected:
nit: Blank line above.

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

https://codereview.chromium.org/1610653002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/password_prompt_view_bridge.h:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
nit: No "(c)" in the copyright message.

http://dev.chromium.org/developers/coding-style#TOC-File-headers

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

https://codereview.chromium.org/1610653002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/password_prompt_view_bridge.mm:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
nit: No "(c)" in the copyright message.

[vasilii, 2016-01-21 12:48:50]

I'm continuing to master the Cocoa part. Once I feel confident enough I can do a
techtalk ;-)

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

https://codereview.chromium.org/1610653002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/account_chooser_view_controller.h:18: class
PasswordDialogController;
On 2016/01/21 10:02:10, vabr (Chromium) wrote:
> nit: I would move line 18 above the "namespace net" block, and separate by
blank
> lines from code above and below. Usually, non-namespaced forward declarations
> are before namespaced ones, and line 13 confirms that in this file.

Done.

https://codereview.chromium.org/1610653002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/account_chooser_view_controller.h:38:
protected:
On 2016/01/21 10:02:09, vabr (Chromium) wrote:
> nit: Blank line above.

Done.

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

https://codereview.chromium.org/1610653002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/password_prompt_view_bridge.h:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
On 2016/01/21 10:02:10, vabr (Chromium) wrote:
> nit: No "(c)" in the copyright message.
> 
> http://dev.chromium.org/developers/coding-style#TOC-File-headers

Done.

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

https://codereview.chromium.org/1610653002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/password_prompt_view_bridge.mm:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
On 2016/01/21 10:02:10, vabr (Chromium) wrote:
> nit: No "(c)" in the copyright message.

Done.

[vasilii, 2016-01-21 12:49:06]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-01-21 12:49:07]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1610653002/#ps20001
(title: "comments")

[commit-bot: I haz the power, 2016-01-21 12:49:52]

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

[vabr (Chromium), 2016-01-21 13:21:09]

LGTM.

On 2016/01/21 12:48:50, vasilii wrote:
> I'm continuing to master the Cocoa part. Once I feel confident enough I can do
a
> techtalk ;-)

Great! I appreciate it and am looking forward to it. :)

Cheers,
Vaclav

[commit-bot: I haz the power, 2016-01-21 14:03:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-21 14:05:17]

Description was changed from

==========
Integrate the account chooser dialog on Mac.

BUG=550922
==========

to

==========
Integrate the account chooser dialog on Mac.

BUG=550922

Committed: https://crrev.com/26173e3e8ab8da1050ee7260dac3d2be714ad5f8
Cr-Commit-Position: refs/heads/master@{#370691}
==========

[commit-bot: I haz the power, 2016-01-21 14:05:17]

Patchset 2 (id:??) landed as
https://crrev.com/26173e3e8ab8da1050ee7260dac3d2be714ad5f8
Cr-Commit-Position: refs/heads/master@{#370691}