Close the account chooser and auto sign-in opt in with ESC on Mac.

Close the account chooser and auto sign-in opt in with ESC on Mac.

BUG=609810
Committed: https://crrev.com/4d237e3b85e855b9620ba3e94df06095ad3ecc07
Cr-Commit-Position: refs/heads/master@{#392298}

[vasilii, 2016-05-09 09:53:27]

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

[vasilii, 2016-05-09 09:53:28]

Hi Vaclav,

please review.

[vabr (Chromium), 2016-05-09 11:16:17]

LGTM with a question and some comments.

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

https://codereview.chromium.org/1961023002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/autosignin_prompt_view_controller.mm:39:
@property (nonatomic, copy) BOOL (^escHandler)(NSEvent* theEvent);
Is this settable so that it can be a no-op before the -loadView call? Or are
there other reasons against just hard-coding handleEscPress inside
-performKeyEquivalent?

https://codereview.chromium.org/1961023002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/autosignin_prompt_view_controller.mm:52:
return TRUE;
TRUE or YES?

https://codereview.chromium.org/1961023002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/autosignin_prompt_view_controller.mm:187:
return TRUE;
ditto here and on line 189: YES/NO instead of TRUE/FALSE?

[vasilii, 2016-05-09 11:45:54]

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

https://codereview.chromium.org/1961023002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/autosignin_prompt_view_controller.mm:39:
@property (nonatomic, copy) BOOL (^escHandler)(NSEvent* theEvent);
On 2016/05/09 11:16:17, vabr (Chromium) wrote:
> Is this settable so that it can be a no-op before the -loadView call? Or are
> there other reasons against just hard-coding handleEscPress inside
> -performKeyEquivalent?

It's done to avoid back pointers to the bridge or
AutoSigninPromptViewController. There is a danger of cyclic dependencies.
Of course there is one |weakSelf| below but at least AutoSigninPromptView
doesn't depend on anything.

https://codereview.chromium.org/1961023002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/autosignin_prompt_view_controller.mm:52:
return TRUE;
On 2016/05/09 11:16:17, vabr (Chromium) wrote:
> TRUE or YES?

YES!

https://codereview.chromium.org/1961023002/diff/1/chrome/browser/ui/cocoa/pas...
chrome/browser/ui/cocoa/passwords/autosignin_prompt_view_controller.mm:187:
return TRUE;
On 2016/05/09 11:16:17, vabr (Chromium) wrote:
> ditto here and on line 189: YES/NO instead of TRUE/FALSE?

Done.

[vasilii, 2016-05-09 11:46:05]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-05-09 11:46:05]

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/1961023002/#ps20001
(title: "comments")

[commit-bot: I haz the power, 2016-05-09 11:46:12]

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

[commit-bot: I haz the power, 2016-05-09 12:13:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-09 12:15:23]

Description was changed from

==========
Close the account chooser and auto sign-in opt in with ESC on Mac.

BUG=609810
==========

to

==========
Close the account chooser and auto sign-in opt in with ESC on Mac.

BUG=609810

Committed: https://crrev.com/4d237e3b85e855b9620ba3e94df06095ad3ecc07
Cr-Commit-Position: refs/heads/master@{#392298}
==========

[commit-bot: I haz the power, 2016-05-09 12:15:24]

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