[autofill] First step towards autofill dialog on OSX.

[autofill] First step towards autofill dialog on OSX.

This CL implements a blank tab-modal dialog that gets invoked when
requestAutocomplete is used.

TBR=jhawkins@chromium.org
R=sail@chromium.org, estade@chromium.org
BUG=157274
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195286

[groby-ooo-7-16, 2013-04-02 19:52:49]

Hey Sailesh, currently working on a tab-modal dialog for OSX/autofill. Since
that whole area is in flux - would you mind taking a quick look and letting me
know if this is taking the right approach?

[sail, 2013-04-02 22:02:12]

On 2013/04/02 19:52:49, groby wrote:
> Hey Sailesh, currently working on a tab-modal dialog for OSX/autofill. Since
> that whole area is in flux - would you mind taking a quick look and letting me
> know if this is taking the right approach?

Looks perfect. LoginhandlerMac is a good example to follow in this case. Note,
you'll want to use ConstrainedWindowCustomWindow as the class for your Window in
the .xib file.

Also, if the UI is very simple consider creating the UI in code instead of using
a .xib file. (Up to you).

[groby-ooo-7-16, 2013-04-02 22:32:19]

There's going to be a lot of auto-generation code in there already, so I might
as well. Mostly opted for xib because all the other ConstrainedWindow cases were
doing it. (All the cool kids are doing it! ;)

[groby-ooo-7-16, 2013-04-09 18:33:05]

sail:PTAL

I have no idea why the review diff for the new files choses the diff base it
chooses - uploaded with -similarity 100

[sail, 2013-04-10 02:36:56]

Looks really good, just some nits.
Could you add a unit test to exercise this code? Just enough to layout the
dialog would be ok.

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h (right):

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h:6: #define
CHROME_BROWSER_UI_VIEWS_AUTOFILL_AUTOFILL_DIALOG_COCOA_H_
UI_VIEWS -> UI_COCOA

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h:52: void
PerformClose();
new line after

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h:78: -
(IBAction)closeSheet:(id)sender;
end line after

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h:81: #endif
// CHROME_BROWSER_UI_COCOA_AUTOFILL_AUTOFILL_DIALOG_COCOA_H_

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right):

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:21: // TODO(groby):
Should this be shared with ConstrainedWindowAlert?
We could move this to constrained_window_control_utils.h
Keeping it here and removing the comment would be ok too.

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:38:
autofillDialog:this]);
align on ":"

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:117: @implementation
AutofillDialogWindowController
new line after

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:152:
ui::kWindowSizeDeterminedLater]);
This definitely works. An alternative is to use NSZeroRect and then do a
sizeToFit. (If you did this could you also get rid of the setFrame call on line
185).

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:153: [button
setTitle:l10n_util::GetNSString(IDS_CANCEL)];
GetNSStringWithFixup (more below)

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:154: [button
setKeyEquivalent:@"\e"];
use kKeyEquivalentEscape?

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:164: [button
setKeyEquivalent:@"\r"];
use kKeyEquivalentReturn?

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:179:
GTMUILocalizerAndLayoutTweaker* layoutTweaker =
scoped_nsobject?

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:186: }
new line after?

[sail, 2013-04-10 02:37:26]

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h (right):

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h:78: -
(IBAction)closeSheet:(id)sender;
On 2013/04/10 02:36:56, sail wrote:
> end line after

oops, should be "new line after"

[groby-ooo-7-16, 2013-04-12 03:13:39]

sail: PTAL
estade: chrome/browser/ui/autofill, please

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h (right):

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h:6: #define
CHROME_BROWSER_UI_VIEWS_AUTOFILL_AUTOFILL_DIALOG_COCOA_H_
Ooops :)
Done.
On 2013/04/10 02:36:56, sail wrote:
> UI_VIEWS -> UI_COCOA

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h:52: void
PerformClose();
On 2013/04/10 02:36:56, sail wrote:
> new line after

Done.

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h:78: -
(IBAction)closeSheet:(id)sender;
On 2013/04/10 02:37:26, sail wrote:
> On 2013/04/10 02:36:56, sail wrote:
> > end line after
> 
> oops, should be "new line after"

Done.

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h:81: #endif
On 2013/04/10 02:36:56, sail wrote:
> // CHROME_BROWSER_UI_COCOA_AUTOFILL_AUTOFILL_DIALOG_COCOA_H_

Done.

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right):

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:21: // TODO(groby):
Should this be shared with ConstrainedWindowAlert?
On 2013/04/10 02:36:56, sail wrote:
> We could move this to constrained_window_control_utils.h
> Keeping it here and removing the comment would be ok too.

Done.

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:38:
autofillDialog:this]);
On 2013/04/10 02:36:56, sail wrote:
> align on ":"

Done.

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:117: @implementation
AutofillDialogWindowController
On 2013/04/10 02:36:56, sail wrote:
> new line after

Done.

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:152:
ui::kWindowSizeDeterminedLater]);
I take it NSControls can be created with zero size, then?
Done.
On 2013/04/10 02:36:56, sail wrote:
> This definitely works. An alternative is to use NSZeroRect and then do a
> sizeToFit. (If you did this could you also get rid of the setFrame call on
line
> 185).

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:153: [button
setTitle:l10n_util::GetNSString(IDS_CANCEL)];
On 2013/04/10 02:36:56, sail wrote:
> GetNSStringWithFixup (more below)

Done.

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:154: [button
setKeyEquivalent:@"\e"];
On 2013/04/10 02:36:56, sail wrote:
> use kKeyEquivalentEscape?

Done.

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:164: [button
setKeyEquivalent:@"\r"];
On 2013/04/10 02:36:56, sail wrote:
> use kKeyEquivalentReturn?

Done.

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:179:
GTMUILocalizerAndLayoutTweaker* layoutTweaker =
On 2013/04/10 02:36:56, sail wrote:
> scoped_nsobject?

Done.

https://codereview.chromium.org/13470023/diff/7004/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:186: }
On 2013/04/10 02:36:56, sail wrote:
> new line after?

Done.

[Evan Stade, 2013-04-12 03:51:27]

lgtm, but please switch the logic/comment to just if defined(TOOLKIT_GTK) return
null

[groby-ooo-7-16, 2013-04-12 17:37:13]

I need to at least check for OS_ANDROID, too.  Also, currently running a compile
test to see what happens for OS_IOS :)

[groby-ooo-7-16, 2013-04-12 17:50:25]

estade: Ignore last message. Caffeine levels were insufficient. Fix uploaded.

[groby-ooo-7-16, 2013-04-13 03:11:54]

sail: Added test to exercise dialog. PTAL

[Evan Stade, 2013-04-15 19:04:36]

https://chromiumcodereview.appspot.com/13470023/diff/6005/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_view.cc (right):

https://chromiumcodereview.appspot.com/13470023/diff/6005/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_view.cc:14: // TODO(estade):
implement the dialog on other platforms. See
s/other platforms/gtk

[sail, 2013-04-15 19:09:53]

LGTM

https://codereview.chromium.org/13470023/diff/6005/chrome/browser/ui/cocoa/au...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/13470023/diff/6005/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa_browsertest.mm:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/13470023/diff/6005/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa_browsertest.mm:82:
//controller_->ViewClosed();
remove?

https://codereview.chromium.org/13470023/diff/6005/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa_browsertest.mm:93:
DISALLOW_COPY_AND_ASSIGN(AutofillDialogCocoaBrowserTest);
wrong indent?

[groby-ooo-7-16, 2013-04-15 22:12:07]

Submitting whenever the CQ re-awakens ;)

https://codereview.chromium.org/13470023/diff/6005/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_view.cc (right):

https://codereview.chromium.org/13470023/diff/6005/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_view.cc:14: // TODO(estade):
implement the dialog on other platforms. See
On 2013/04/15 19:04:37, Evan Stade wrote:
> s/other platforms/gtk

Done.

https://codereview.chromium.org/13470023/diff/6005/chrome/browser/ui/cocoa/au...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/13470023/diff/6005/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa_browsertest.mm:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2013/04/15 19:09:54, sail wrote:
> 2013

Done.

https://codereview.chromium.org/13470023/diff/6005/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa_browsertest.mm:82:
//controller_->ViewClosed();
On 2013/04/15 19:09:54, sail wrote:
> remove?

Done.

https://codereview.chromium.org/13470023/diff/6005/chrome/browser/ui/cocoa/au...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa_browsertest.mm:93:
DISALLOW_COPY_AND_ASSIGN(AutofillDialogCocoaBrowserTest);
On 2013/04/15 19:09:54, sail wrote:
> wrong indent?

Done.

[commit-bot: I haz the power, 2013-04-18 00:10:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/13470023/37001

[commit-bot: I haz the power, 2013-04-18 00:10:40]

Presubmit check for 13470023-37001 failed and returned exit status 1.

INFO:root:Found 6 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/chrome_browser_ui.gypi

Presubmit checks took 1.4s to calculate.

[groby-ooo-7-16, 2013-04-18 17:37:46]

TBR'd jhawkins for gypi change

[commit-bot: I haz the power, 2013-04-18 17:38:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/13470023/37001

[commit-bot: I haz the power, 2013-04-18 18:36:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-19 00:36:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/13470023/37001

[commit-bot: I haz the power, 2013-04-19 02:36:50]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[groby-ooo-7-16, 2013-04-19 18:21:38]

sail: PTAL at browsertest - needed to switch to a MessageLoopRunner.

[sail, 2013-04-19 18:53:56]

LGTM

[commit-bot: I haz the power, 2013-04-19 18:57:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/13470023/58001

[commit-bot: I haz the power, 2013-04-19 21:17:35]

Change committed as 195286