Make startup_tests build and work on Mac.

Make startup_tests build and work on Mac.

Also ensure that Chromium.app quits properly between the tests.

http://crbug.com/8391

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

[Paweł Hajdan Jr., 2009-03-17 21:30:40]

Please review:

pinkerton: Mac things
mark: gyp changes (I don't know how to ensure that
automation_provider_list_mac.mm will be built only on the Mac; this
include/exclude block confused me a bit)

nirnimesh: FYI; sorry for duplicating effort; my "small investigation"
transformed into a full patch surprisingly quickly

[Mark Mentovai, 2009-03-17 21:39:50]

http://codereview.chromium.org/42306/diff/1/5
File chrome/browser/automation/automation_provider_list_mac.mm (right):

http://codereview.chromium.org/42306/diff/1/5#newcode7
Line 7: #include "chrome/browser/app_controller_mac.h"
This header (but not the other) should be #import, not #include.

When you're including an Objective-C header from an Objective-C file, you should
use #import.

http://codereview.chromium.org/42306/diff/1/6
File chrome/chrome.gyp (right):

http://codereview.chromium.org/42306/diff/1/6#newcode1230
Line 1230: 'browser/automation/automation_provider_list_mac.mm',
Please take this part out.  .mm files are automatically excluded on non-Mac.

http://codereview.chromium.org/42306/diff/1/6#newcode1261
Line 1261: ['include',
'^browser/automation/automation_provider_list_mac\\.mm$'],
This is going to get hairy now because we *don't* want to re-include this file
on non-Mac.

We should actually take out this entire "Exclude most of automation" section
now, since we actually do build most of automation.  In its place, below, you
should add explicit excludes for ui_controls.cc in the non-win sources! list.

http://codereview.chromium.org/42306/diff/1/6#newcode1283
Line 1283: 'browser/app_modal_dialog_queue.cc',
Add browser/automation/ui_controls.cc here.

http://codereview.chromium.org/42306/diff/1/7
File chrome/common/chrome_constants.cc (right):

http://codereview.chromium.org/42306/diff/1/7#newcode20
Line 20: const wchar_t kBrowserProcessExecutableName[] =
L"Chromium.app/Contents/MacOS/Chromium";
Line is too long.

kBrowserProcessExecutableName is wrong if GOOGLE_CHROME_BUILD is set.  (In that
case, Chrome.app/Contents/MacOS/Chrome).

Where is this used?  TVL set up a better way to handle a lot of things like
this.

[TVL, 2009-03-17 21:45:58]

http://codereview.chromium.org/42306/diff/1/7
File chrome/common/chrome_constants.cc (right):

http://codereview.chromium.org/42306/diff/1/7#newcode20
Line 20: const wchar_t kBrowserProcessExecutableName[] =
L"Chromium.app/Contents/MacOS/Chromium";
On 2009/03/17 21:39:50, Mark Mentovai wrote:
> Line is too long.
> 
> kBrowserProcessExecutableName is wrong if GOOGLE_CHROME_BUILD is set.  (In
that
> case, Chrome.app/Contents/MacOS/Chrome).
> 
> Where is this used?  TVL set up a better way to handle a lot of things like
> this.

Looks like startup_tests.cc, it would be nice to use NSBundle over there to
properly fetch the executablePath form the bundle, but that's probably gonna
take more surgery.

[Paweł Hajdan Jr., 2009-03-18 16:06:56]

On 2009/03/17 21:39:50, Mark Mentovai wrote:
> http://codereview.chromium.org/42306/diff/1/5
> File chrome/browser/automation/automation_provider_list_mac.mm (right):
> 
> http://codereview.chromium.org/42306/diff/1/5#newcode7
> Line 7: #include "chrome/browser/app_controller_mac.h"
> This header (but not the other) should be #import, not #include.

Done.

> http://codereview.chromium.org/42306/diff/1/6
> File chrome/chrome.gyp (right):
> 
> http://codereview.chromium.org/42306/diff/1/6#newcode1230
> Line 1230: 'browser/automation/automation_provider_list_mac.mm',
> Please take this part out.  .mm files are automatically excluded on non-Mac.

Done.

> http://codereview.chromium.org/42306/diff/1/6#newcode1261
> Line 1261: ['include',
> '^browser/automation/automation_provider_list_mac\\.mm$'],
> In its place, below, you
> should add explicit excludes for ui_controls.cc in the non-win sources! list.

Much better now. Done.

> http://codereview.chromium.org/42306/diff/1/7#newcode20
> Line 20: const wchar_t kBrowserProcessExecutableName[] =
> L"Chromium.app/Contents/MacOS/Chromium";
> Line is too long.
> 
> kBrowserProcessExecutableName is wrong if GOOGLE_CHROME_BUILD is set.  (In
that
> case, Chrome.app/Contents/MacOS/Chrome).
> 
> Where is this used?  TVL set up a better way to handle a lot of things like
> this.

Fixed line length and Chromium/Chrome.app differences. After short discussion
with TVL on irc it turned out that this way is ok for now. The rework will be
needed when we want to use it for something other than tests.

[Mark Mentovai, 2009-03-18 16:13:04]

OK, looks good

[pink (ping after 24hrs), 2009-03-18 18:08:12]

LGTM

http://codereview.chromium.org/42306/diff/1008/33
File chrome/browser/automation/automation_provider_list_mac.mm (right):

http://codereview.chromium.org/42306/diff/1008/33#newcode7
Line 7: #import "chrome/browser/app_controller_mac.h"
you may be able to omit this. give it a try w/out it and see if you can.

[Nirnimesh, 2009-03-18 18:10:57]

http://codereview.chromium.org/42306/diff/1008/36
File chrome/chrome.gyp (right):

http://codereview.chromium.org/42306/diff/1008/36#newcode2154
Line 2154: 'dependencies': [
You should add 'app' as a dependency (and remove 'browser' & 'common' coz
they'll be covered by 'app')

[Mark Mentovai, 2009-03-18 18:13:29]

http://codereview.chromium.org/42306/diff/1008/36
File chrome/chrome.gyp (right):

http://codereview.chromium.org/42306/diff/1008/36#newcode2154
Line 2154: 'dependencies': [
Nirnimesh wrote:
> You should add 'app' as a dependency (and remove 'browser' & 'common' coz
> they'll be covered by 'app')

Disagree, an executable depending on an application does not imply linkage. 
Presumably we do want to link startup_tests against libbrowser and libcommon.

[Paweł Hajdan Jr., 2009-03-18 19:10:08]

On 2009/03/18 18:08:12, pink wrote:
> LGTM
> 
> http://codereview.chromium.org/42306/diff/1008/33
> File chrome/browser/automation/automation_provider_list_mac.mm (right):
> 
> http://codereview.chromium.org/42306/diff/1008/33#newcode7
> Line 7: #import "chrome/browser/app_controller_mac.h"
> you may be able to omit this. give it a try w/out it and see if you can.

It fails without this header so I left it in. It might be fixed by including
just some Cocoa header, but for simplicity...