Remove CrApplication dependency from base

Remove CrApplication dependency from base

Add a @protocol CrAppProtocol that clients of base must implement in their NSApplication subclass, and let base depend only on this protocol.

Let MessagePumpNSApplication::DoRun() no longer initialize NSApplication (fixes a TODO).

Add a MockCrApplication that the simple unittests in base and app can use, move chrome_application to chrome/common.

Test shell might run nested run loops, so I gave it a real but simplified CrAppProtocol implementation.

BUG=62968, 46929
TEST=Everything still works. The PDF plugin prints one fewer warning when loaded.

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

[Nico, 2010-12-17 08:08:53]

Most of the files were just touched to change the include from base to
chrome/common, the actual CL is fairly small.

[Nico, 2010-12-17 08:11:58]

(looks like browser_tests also needs to call [CrApplication
sharedApplication] somewhere. Should be a 1-line fix somewhere.)

On Fri, Dec 17, 2010 at 12:08 AM,  <thakis@chromium.org> wrote:
> Reviewers: Mark Mentovai, dmac,
>
> Message:
> Most of the files were just touched to change the include from base to
> chrome/common, the actual CL is fairly small.
>
> Description:
> Remove CrApplication dependency from base
>
> Add a @protocol CrAppProtocol that clients of base must implement in their
> NSApplication subclass, and let base depend only on this protocol.
>
> Add a MockCrApplication that the simple unittests in base and app can use,
> move
> chrome_application to chrome/common.
>
> Test shell might run nested run loops, so I gave it a real but simplified
> CrAppProtocol implementation.
>
> BUG=62968,46929
> TEST=Everything still works. The PDF plugin prints one fewer warning when
> loaded.
>
> Please review this at http://codereview.chromium.org/5950003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/base
>
> Affected files:
>  M app/animation_container_unittest.cc
>  M app/test_suite.h
>  M base/base.gyp
>  M base/base.gypi
>  M base/mac_util_unittest.mm
>  M base/message_pump_mac.h
>  M base/message_pump_mac.mm
>  A base/test/mock_chrome_application_mac.h
>  A base/test/mock_chrome_application_mac.mm
>  M chrome/browser/chrome_browser_application_mac.h
>  M chrome/browser/renderer_host/render_widget_host_view_mac.mm
>  M chrome/browser/tab_contents/popup_menu_helper_mac.mm
>  M chrome/browser/tab_contents/tab_contents_view_mac.mm
>  M chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h
>  M chrome/browser/ui/cocoa/cocoa_test_helper.h
>  M chrome/chrome_browser.gypi
>  M chrome/chrome_common.gypi
>  A + chrome/common/chrome_application_mac.h
>  A + chrome/common/chrome_application_mac.mm
>  M chrome/common/sandbox_mac.mm
>  M chrome/nacl/nacl_main_platform_delegate_mac.mm
>  M chrome/plugin/plugin_main_mac.mm
>  M chrome/renderer/renderer_main_platform_delegate_mac.mm
>  M webkit/tools/test_shell/test_shell_platform_delegate_mac.mm
>
>
>

[Mark Mentovai, 2010-12-17 17:31:37]

LGTM

http://codereview.chromium.org/5950003/diff/23001/base/message_pump_mac.h
File base/message_pump_mac.h (right):

http://codereview.chromium.org/5950003/diff/23001/base/message_pump_mac.h#new...
base/message_pump_mac.h:44: class NSAutoreleasePool;
Since you just #imported AppKit.h, this is unnecessary.

(But do you need AppKit.h?)

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
File base/test/mock_chrome_application_mac.h (right):

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
base/test/mock_chrome_application_mac.h:8: 
Extra blank line.

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
base/test/mock_chrome_application_mac.h:10: #ifdef __OBJC__
Chromium code prefers #if defined(__OBJC__).

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
base/test/mock_chrome_application_mac.h:12: #import <Cocoa/Cocoa.h>
You can get away with just AppKit/AppKit.h.

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
base/test/mock_chrome_application_mac.h:27: }
}  // namespace mock_cr_app

http://codereview.chromium.org/5950003/diff/23001/webkit/tools/test_shell/tes...
File webkit/tools/test_shell/test_shell_platform_delegate_mac.mm (right):

http://codereview.chromium.org/5950003/diff/23001/webkit/tools/test_shell/tes...
webkit/tools/test_shell/test_shell_platform_delegate_mac.mm:36: BOOL b =
handlingSendEvent_;
What does b signify?

wasHandlingSendEvent?

[dmac, 2010-12-17 18:00:15]

LGTM

http://codereview.chromium.org/5950003/diff/23001/base/message_pump_mac.mm
File base/message_pump_mac.mm (left):

http://codereview.chromium.org/5950003/diff/23001/base/message_pump_mac.mm#ol...
base/message_pump_mac.mm:676: // TODO(dmaclach): Get rid of this gratuitous
sharedApplication.
thank you!

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
File base/test/mock_chrome_application_mac.h (right):

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
base/test/mock_chrome_application_mac.h:17: // on the stack. This can be used in
tests that need an NSApplication and which
I think you can get rid of both "whichs" in the last sentence.

http://codereview.chromium.org/5950003/diff/23001/webkit/tools/test_shell/tes...
File webkit/tools/test_shell/test_shell_platform_delegate_mac.mm (right):

http://codereview.chromium.org/5950003/diff/23001/webkit/tools/test_shell/tes...
webkit/tools/test_shell/test_shell_platform_delegate_mac.mm:27: -
(BOOL)isHandlingSendEvent;
Use a property to mimic what you did in chrome_application_mac? Or get rid of
the property there?

[Nico, 2010-12-17 21:46:53]

It might pass browser_tests now. Will address your comments soon.

[Nico, 2010-12-18 01:14:28]

Thanks, submitting.

http://codereview.chromium.org/5950003/diff/23001/base/message_pump_mac.h
File base/message_pump_mac.h (right):

http://codereview.chromium.org/5950003/diff/23001/base/message_pump_mac.h#new...
base/message_pump_mac.h:44: class NSAutoreleasePool;
On 2010/12/17 17:31:38, Mark Mentovai wrote:
> Since you just #imported AppKit.h, this is unnecessary.

Removed.
 
> (But do you need AppKit.h?)

Yes. (for BOOL)

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
File base/test/mock_chrome_application_mac.h (right):

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
base/test/mock_chrome_application_mac.h:8: 
On 2010/12/17 17:31:38, Mark Mentovai wrote:
> Extra blank line.

Done.

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
base/test/mock_chrome_application_mac.h:10: #ifdef __OBJC__
On 2010/12/17 17:31:38, Mark Mentovai wrote:
> Chromium code prefers #if defined(__OBJC__).

Done.

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
base/test/mock_chrome_application_mac.h:12: #import <Cocoa/Cocoa.h>
On 2010/12/17 17:31:38, Mark Mentovai wrote:
> You can get away with just AppKit/AppKit.h.

Done.

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
base/test/mock_chrome_application_mac.h:17: // on the stack. This can be used in
tests that need an NSApplication and which
On 2010/12/17 18:00:16, dmac wrote:
> I think you can get rid of both "whichs" in the last sentence.

Done.

http://codereview.chromium.org/5950003/diff/23001/base/test/mock_chrome_appli...
base/test/mock_chrome_application_mac.h:27: }
On 2010/12/17 17:31:38, Mark Mentovai wrote:
> }  // namespace mock_cr_app

Done.

http://codereview.chromium.org/5950003/diff/23001/webkit/tools/test_shell/tes...
File webkit/tools/test_shell/test_shell_platform_delegate_mac.mm (right):

http://codereview.chromium.org/5950003/diff/23001/webkit/tools/test_shell/tes...
webkit/tools/test_shell/test_shell_platform_delegate_mac.mm:27: -
(BOOL)isHandlingSendEvent;
On 2010/12/17 18:00:16, dmac wrote:
> Use a property to mimic what you did in chrome_application_mac? Or get rid of
> the property there?

Done.

http://codereview.chromium.org/5950003/diff/23001/webkit/tools/test_shell/tes...
webkit/tools/test_shell/test_shell_platform_delegate_mac.mm:36: BOOL b =
handlingSendEvent_;
On 2010/12/17 17:31:38, Mark Mentovai wrote:
> What does b signify?
> 
> wasHandlingSendEvent?

Done.