Add search engine selection dialog for Mac....

Add search engine selection dialog for Mac, do new-style first-run.

SearchEngineDialog.xib: creation
FirstRunDialog.xib: removal of import and cancel UI elements

BUG=47651
TEST=reset profile, run. Make sure goto/chromefirstrun is satisfied.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60699

[Avi (use Gerrit), 2010-09-24 16:48:09]

Yay finally! For your consideration.

[Miranda Callahan, 2010-09-24 19:51:29]

Great victory! :-) I have a few comments...

http://codereview.chromium.org/3223010/diff/25001/24008
File chrome/browser/cocoa/search_engine_dialog_controller.mm (right):

http://codereview.chromium.org/3223010/diff/25001/24008#newcode114
chrome/browser/cocoa/search_engine_dialog_controller.mm:114: return;
Do things get closed down properly if this happens?  And could we do the check
before getting the headerImage and setting the background color in the lines
above?

http://codereview.chromium.org/3223010/diff/25001/24008#newcode234
chrome/browser/cocoa/search_engine_dialog_controller.mm:234: [[NSButton alloc]
initWithFrame:NSMakeRect(0, 0, 100, 34)]);
Should the button be autoreleased, like the NSTextField above?

http://codereview.chromium.org/3223010/diff/25001/24009
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/3223010/diff/25001/24009#newcode491
chrome/browser/first_run/first_run.cc:491: // http://crbug.com/22142
I talked to csilv about this; could you change to TODO(mirandac) and
TODO(csilv)?  We're splitting this between ourselves.

http://codereview.chromium.org/3223010/diff/25001/24009#newcode590
chrome/browser/first_run/first_run.cc:590: bool ended_;
Could you explain briefly what the difference is between ended_ and
quit_message_loop_, and when each would be set?  I think I kind of get it, but
for the benefit of posterity.

[Evan Stade, 2010-09-24 20:11:56]

just nits

http://codereview.chromium.org/3223010/diff/25001/24009
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/3223010/diff/25001/24009#newcode491
chrome/browser/first_run/first_run.cc:491: // http://crbug.com/22142
I don't actually understand what "qualified" means in this sense

http://codereview.chromium.org/3223010/diff/25001/24009#newcode557
chrome/browser/first_run/first_run.cc:557: #if defined(OS_MACOSX) || defined
(OS_LINUX)
OS_POSIX

http://codereview.chromium.org/3223010/diff/25001/24009#newcode567
chrome/browser/first_run/first_run.cc:567: : ended_(false),
indentation here is all kinds of funny

http://codereview.chromium.org/3223010/diff/25001/24009#newcode590
chrome/browser/first_run/first_run.cc:590: bool ended_;
perhaps "should_quit_message_loop_"

http://codereview.chromium.org/3223010/diff/25001/24009#newcode621
chrome/browser/first_run/first_run.cc:621: #endif
nit: comment what if is ending

[Avi (use Gerrit), 2010-09-24 21:19:10]

http://codereview.chromium.org/3223010/diff/25001/24008
File chrome/browser/cocoa/search_engine_dialog_controller.mm (right):

http://codereview.chromium.org/3223010/diff/25001/24008#newcode114
chrome/browser/cocoa/search_engine_dialog_controller.mm:114: return;
Move: good idea.

Re closing things down: works fine in my testing.

http://codereview.chromium.org/3223010/diff/25001/24008#newcode234
chrome/browser/cocoa/search_engine_dialog_controller.mm:234: [[NSButton alloc]
initWithFrame:NSMakeRect(0, 0, 100, 34)]);
On 2010/09/24 19:51:29, Miranda Callahan wrote:
> Should the button be autoreleased, like the NSTextField above?

I usually use scoped_nsobject, but that time used autorelease since I didn't
want to think about whether a scoped ptr would work (assignment across
subclasses of objc classes).

http://codereview.chromium.org/3223010/diff/25001/24009
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/3223010/diff/25001/24009#newcode491
chrome/browser/first_run/first_run.cc:491: // http://crbug.com/22142
On 2010/09/24 19:51:29, Miranda Callahan wrote:
> I talked to csilv about this; could you change to TODO(mirandac) and
> TODO(csilv)?  We're splitting this between ourselves.

Shall I change the reference in browser/dom_ui/import_data_handler.cc from where
I copied this?

Evan: that's the original phrasing. I think that in this case, "qualified" means
"tested".

http://codereview.chromium.org/3223010/diff/25001/24009#newcode567
chrome/browser/first_run/first_run.cc:567: : ended_(false),
On 2010/09/24 20:11:56, Evan Stade wrote:
> indentation here is all kinds of funny

Gah, Xcode!

http://codereview.chromium.org/3223010/diff/25001/24009#newcode590
chrome/browser/first_run/first_run.cc:590: bool ended_;
On 2010/09/24 20:11:56, Evan Stade wrote:
> perhaps "should_quit_message_loop_"

This is all moved directly from _gtk :) but I will comment.

http://codereview.chromium.org/3223010/diff/25001/24009#newcode621
chrome/browser/first_run/first_run.cc:621: #endif
On 2010/09/24 20:11:56, Evan Stade wrote:
> nit: comment what if is ending

Done.

[Miranda Callahan, 2010-09-24 21:51:31]

LGTM!  :-D

[Evan Stade, 2010-09-24 22:01:25]

lg