Cleans up our autorelease handling so that we don't create a layered ...

Cleans up our autorelease pool handling by making sure that an autorelease pool isn't created while the app is handling an event sent via -[NSApp sendEvent].

Branches browser/chrome_application_mac into browser/chrome_browser_application and base/chrome_application. Renderers will run as chrome_applications, and browsers will run as chrome_browser_applications. 

BUG=26418, 25462, 25463, 25465
TEST=1) See bug 25857. 2) Start up. Open 3+ windows. 3)Quit. See bugs for other repro cases.


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

[dmac, 2009-11-03 19:12:41]

I thought the horse was dead, but obviously more whipping is/was required.

[Mark Mentovai, 2009-11-03 19:49:36]

http://codereview.chromium.org/345051/diff/2001/3001
File base/message_pump_mac.h (right):

http://codereview.chromium.org/345051/diff/2001/3001#newcode279
Line 279: // True if an event should be sent to the event loop to cause it to
spin
It's funny to say "true if" for type std::vector<something>, just clarify it by
saying that an element is true if…

http://codereview.chromium.org/345051/diff/2001/3001#newcode281
Line 281: std::vector<bool> needs_event_loop_wake_up_;
Excellent.

http://codereview.chromium.org/345051/diff/2001/3002
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/345051/diff/2001/3002#newcode28
Line 28: // Class for dealing with scoping autorelease pools when they need to
be a
This comment isn't so accurate now.  Everything will always be an
NSAutoreleasePool, the only difference from ScopedNSAutoreleasePool is that this
gives the ability to use an extant NSAutoreleasePool instead of letting the
scoper create it.

http://codereview.chromium.org/345051/diff/2001/3002#newcode48
Line 48: @interface NSAutoreleasePool (ChromeAutoreleaseAdditions)
Of course they're autorelease additions, it's an autorelease pool! 
(ChromeAdditions)?

http://codereview.chromium.org/345051/diff/2001/3002#newcode458
Line 458: void MessagePumpCFRunLoopBase::EnterExitObserver(CFRunLoopObserverRef
observer,
I was planning on changing the contract here so that nesting_level() will always
be equal to the level being entered or exited when calling EnterExitRunLoop,
because I think that's cleaner from the perspective of EnterExitRunLoop.  Did
you consider doing that?

http://codereview.chromium.org/345051/diff/2001/3002#newcode496
Line 496: self->EnterExitRunLoop(activity);
…was gonna get rid of this and put it into each |case| above in the right spot…

http://codereview.chromium.org/345051/diff/2001/3002#newcode778
Line 778: // PerformClick: spins a nested run loop. If the pool created in
DoWork is a
lowercase p on performClick:.

http://codereview.chromium.org/345051/diff/2001/3002#newcode786
Line 786: // The current solution is to have a deferred autorelease pool that
collects
No it's not.

Not really as it's written here, anyway.  This comment still seems to describe
what we did last Friday, but that's changed now.  We only care about
+[DeferredAutoreleasePool load] and the rest of the stuff is handled in the
stuff we've done in our category.

http://codereview.chromium.org/345051/diff/2001/3002#newcode811
Line 811: if (![NSApp nextEventMatchingMask:NSAnyEventMask
Bullet point 6 in bug 26418, and bug 26399 comment 1.

This will result in terrible ugly stack exhaustion, terrible ugly reentrancy,
probably crashes, and really weird difficult-to-figure-out "tall" stacks that
are also terrible.

http://codereview.chromium.org/345051/diff/2001/3002#newcode840
Line 840: pump->set_needs_event_loop_wake_up_true();
The _true name isn't really valid any more now that this guy is all vectorific.

http://codereview.chromium.org/345051/diff/2001/3002#newcode843
Line 843: // Storage for the NSAutoreleasePool (ChromeAutoreleaseAdditions)
category
Rather than doing all of these guys as statics in namespace base, why not just
end namespace base here and put these guys into an anonymous namespace?  Good
for grouping.

http://codereview.chromium.org/345051/diff/2001/3002#newcode848
Line 848: // Key for retrieving the array of autoreleased objects from
dictionary stored
Performance might be slightly better if we didn't store dicts but did one of...

…simple objects with a couple of fields…
…a different container for the properties we need to store (currently only two)…
…C structs (but you'd need to use STL containers and not Foundation ones)…

What do you think?

http://codereview.chromium.org/345051/diff/2001/3002#newcode860
Line 860: NSMutableDictionary* dictionary = nil;
This ought to be a DCHECK, right?  Also, use +isMainThread, because we don't
care about the pre-10.5 world.

DCHECK([NSThread isMainThread]);
return (NSMutableDictionary*)NSMapGet(gAutoreleaseAddOnsMapTable, pool);

right?

http://codereview.chromium.org/345051/diff/2001/3002#newcode872
Line 872: if ([[NSThread currentThread] isEqual:[NSThread mainThread]]) {
+isMainThread again.

Maybe just DCHECK it, though.

Unless you're actually expecting these guys to be called from other threads?

http://codereview.chromium.org/345051/diff/2001/3002#newcode1001
Line 1001: DCHECK([NSGarbageCollector defaultCollector] == nil);
I think we usually use ! instead of == nil?

http://codereview.chromium.org/345051/diff/2001/3002#newcode1021
Line 1021: method_exchangeImplementations(m1, m2);
hooray!

http://codereview.chromium.org/345051/diff/2001/3002#newcode1025
Line 1025: @end
This change doesn't undo the band-aids I put back in place when I did the
backout on Saturday.  Will you rip those band-aids off in a separate change? 
(That's fine.)

[dmac, 2009-11-04 22:02:10]

Horse was not responding well to whipping. Decided to use large stick instead.
Sort of wandered into uncharted territory for me, so please take a close look at
this one please.

[Mark Mentovai, 2009-11-04 22:08:39]

I don't know if it's a good (or clean) idea to abuse MessageLoop in this way. 
It doesn't seem like you actually need to do any of this in MessageLoop.  Can
you keep track of sending_event elsewhere?

[dmaclach1, 2009-11-04 22:29:13]

On Nov 4, 2009, at 2:08 PM, mark@chromium.org wrote:

> I don't know if it's a good (or clean) idea to abuse MessageLoop in  
> this way.
> It doesn't seem like you actually need to do any of this in  
> MessageLoop.  Can
> you keep track of sending_event elsewhere?
>
> http://codereview.chromium.org/345051

I was keeping track of it in the app, but then that added a dependency  
on base (message_pump) up to the application level. I couldn't see a  
way of getting the message pump from the app, but I could get at the  
message loop.

If you have a better suggestion on where to put it and keep the  
dependencies clean I'd be happy to hear it...

http://codereview.chromium.org/345051

[Mark Mentovai, 2009-11-04 22:31:29]

dmac wrote:
> I was keeping track of it in the app, but then that added a dependency  
> on base (message_pump) up to the application level. I couldn't see a  
> way of getting the message pump from the app, but I could get at the  
> message loop.
> 
> If you have a better suggestion on where to put it and keep the  
> dependencies clean I'd be happy to hear it...

Perhaps we move CrApplication into base then?

We might need to make our unit tests CrApplications too, remember: this might
not be the only dependency on CrApplication that a change like this would result
in.

[Scott Hess - ex-Googler, 2009-11-04 23:31:38]

This seems awful intrusive.  I don't understand things well enough to know for
certain, but doesn't having handling_send_event() set on the message loop
correspond exactly to having the message be pumped by
MessagePumpNSApplication()?

[Scott Hess - ex-Googler, 2009-11-04 23:44:55]

Nevermind, I think I'm going to go hide under my table or something.
Just like I feel every other time I look at message loop code :-).

On Wed, Nov 4, 2009 at 4:31 PM,  <shess@chromium.org> wrote:
> This seems awful intrusive. =A0I don't understand things well enough to k=
now
> for
> certain, but doesn't having handling_send_event() set on the message loop
> correspond exactly to having the message be pumped by
> MessagePumpNSApplication()?
>
> http://codereview.chromium.org/345051
>

[Scott Hess - ex-Googler, 2009-11-05 00:11:24]

http://codereview.chromium.org/345051/diff/8003/7006
File base/message_loop.h (right):

http://codereview.chromium.org/345051/diff/8003/7006#newcode255
Line 255: #endif  // OS_MACOSX
Rather than putting this here, could you define MessageLoop::pump_mac()
(parallel to pump_win), and then add the flag there?

http://codereview.chromium.org/345051/diff/8003/7005
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/345051/diff/8003/7005#newcode70
Line 70: };
This seems excessive, at this point.  You could add
MessagePumpCFRunLoopBase::GetAutoreleasePool(), and have it return nil if
handling_send_event.  Then at the point of usage you could write:

scoped_nsobject<NSAutoreleasePool> pool(GetAutoreleasePool());

http://codereview.chromium.org/345051/diff/8003/7011
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/345051/diff/8003/7011#newcode136
Line 136: chrome_application_mac::ScopedSendingEvent scoper();
I have a CL out for review which will conflict with this, I think. 
http://codereview.chromium.org/362013

Anyhow, a thing an earlier pass through that CL brought up was whether uncaught
NSExceptions could pass through here.  I _think_ they're handled in
-[NSApplication sendEvent:], but I could be wrong, in which case scoper isn't
sufficient.  Having the sending-event setting persist past this function would
be bad, I think.

http://codereview.chromium.org/345051/diff/8003/7017
File chrome/common/chrome_application_mac.h (right):

http://codereview.chromium.org/345051/diff/8003/7017#newcode46
Line 46: CrApplication* app_;
Can this be used?

http://codereview.chromium.org/345051/diff/8003/7016
File chrome/common/chrome_application_mac.mm (left):

http://codereview.chromium.org/345051/diff/8003/7016#oldcode141
Line 141: // exceptions.
Just to be clear, one of the reasons this code wasn't already shared is because
I have no idea whether it will do the right things in the plugin and renderer
processes :-).  I think the -reportException: code is probably completely safe,
but I'm not very sure about the NSException-DCHECK code (though since it's
DCHECK, the downside is ultimately limited).

[Mark Mentovai, 2009-11-05 02:59:11]

http://codereview.chromium.org/345051/diff/6049/9053
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/345051/diff/6049/9053#newcode136
Line 136: chrome_application_mac::ScopedSendingEvent scoper;
I like the scoper better than what's in Scott's change (or even how I suggested
he modify it.)

http://codereview.chromium.org/345051/diff/6049/9057
File chrome/browser/chrome_browser_application_mac_unittest.mm (right):

http://codereview.chromium.org/345051/diff/6049/9057#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
Um, it looks like this file is blank except for the LICE.  I wouldn't even
bother adding it if it's going to be empty like this.

http://codereview.chromium.org/345051/diff/6049/9063
File chrome/browser/cocoa/cocoa_test_helper.h (right):

http://codereview.chromium.org/345051/diff/6049/9063#newcode159
Line 159: //                 of any dependencies on this. 11/30/09 at the
latest.
That's the spirit!

http://codereview.chromium.org/345051/diff/6049/9059
File chrome/common/chrome_application_mac.h (right):

http://codereview.chromium.org/345051/diff/6049/9059#newcode8
Line 8: #ifdef __OBJC__
I wonder: do we ever need to #include this file in non-Obj-C code?

http://codereview.chromium.org/345051/diff/6049/9059#newcode12
Line 12: #import "base/message_loop.h"
#include

but this #include should fall out of here.

http://codereview.chromium.org/345051/diff/6049/9059#newcode33
Line 33: class ScopedSendingEvent {
Well, I like the scoper, but not while it's implemented on top of MessageLoop.

http://codereview.chromium.org/345051/diff/6049/9065
File chrome/renderer/renderer_main_platform_delegate_mac.mm (right):

http://codereview.chromium.org/345051/diff/6049/9065#newcode38
Line 38: // Initialize Cocoa.  Without this call, drawing of native UI
I think we should do this first.  We've got no idea what's going to happen
inside InitWebCoreSystemInterface and maybe even SandboxWarmup.

http://codereview.chromium.org/345051/diff/6049/9065#newcode39
Line 39: // elements (e.g. buttons) in WebKit will explode.
You should also DCHECK([NSApp isKindOfClass:[CrApplication class]]) as is done
in the browser.

[dmac, 2009-11-05 06:53:37]

I think I've taken care of all of Scott and Mark's comments. Please take another
look. Thanks!

[Scott Hess - ex-Googler, 2009-11-05 07:54:19]

http://codereview.chromium.org/345051/diff/9093/9094
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/345051/diff/9093/9094#newcode16
Line 16: #import "base/chrome_application_mac.h"
Sort these.

http://codereview.chromium.org/345051/diff/9093/9094#newcode56
Line 56: // returning it to its previous value once the event has been sent.
indent.

http://codereview.chromium.org/345051/diff/9093/9094#newcode61
Line 61: [(id)NSApp isHandlingSendEvent])) {
Is this called frequently enough to make it worth putting a category on
NSApplication to return a default for -isHandlingSendEvent?

Wait a minute ... can you be doing this?  What if this isn't on the UI thread? 
Shouldn't this only be checked if you're using MessagePumpNSApplication?

http://codereview.chromium.org/345051/diff/9093/9105
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/345051/diff/9093/9105#newcode19
Line 19: #import "chrome/browser/chrome_browser_application_mac.h"
This seems odd.  We aren't refering to CrApplication anywhere, is this line
necessary these days?

http://codereview.chromium.org/345051/diff/9093/9102
File chrome/browser/browser_main_mac.mm (right):

http://codereview.chromium.org/345051/diff/9093/9102#newcode34
Line 34: [BrowserCrApplication sharedApplication];
Would it make sense to push the -isKindOfClass: test into +sharedApplication
itself?  Since BrowserCrApplication isKindOf CrApplication, it could do the
NSApp test after calling super.  I'm not committed to this, I just see the same
basic code in two places and assume that means we should think about whether it
belongs in one.  I think that you'd only have to test in +[CrApplication
sharedApplication], since self will be [BrowserCrApplication class] at the
appropriate times.

http://codereview.chromium.org/345051/diff/9093/9099
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/345051/diff/9093/9099#newcode67
Line 67: };
ScopedCrashKey seems to have fallen out of the anonymous namespace.  Was that
intentional?

http://codereview.chromium.org/345051/diff/9093/9099#newcode150
Line 150: - (void)reportException:(NSException *)anException {
Would prefer to leave this in the same general location in the file so that the
diff looks cleaner.  Am assuming that it's the same as it was before.

http://codereview.chromium.org/345051/diff/9093/9106
File chrome/browser/cocoa/cocoa_test_helper.h (right):

http://codereview.chromium.org/345051/diff/9093/9106#newcode149
Line 149: [CrApplication sharedApplication];
Any reason not to replace "// Look ..." through -sharedApplication with a call
to CocoaTest::BootstrapCocoa()?

http://codereview.chromium.org/345051/diff/9093/9108
File chrome/renderer/renderer_main_platform_delegate_mac.mm (right):

http://codereview.chromium.org/345051/diff/9093/9108#newcode32
Line 32: << [[NSApp className] UTF8String];
<< should line up, I think.

[dmac, 2009-11-05 17:54:52]

Updated with shess' comments

http://codereview.chromium.org/345051/diff/9093/9094
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/345051/diff/9093/9094#newcode16
Line 16: #import "base/chrome_application_mac.h"
On 2009/11/05 07:54:19, shess wrote:
> Sort these.

Done.

http://codereview.chromium.org/345051/diff/9093/9094#newcode56
Line 56: // returning it to its previous value once the event has been sent.
On 2009/11/05 07:54:19, shess wrote:
> indent.

Done.

http://codereview.chromium.org/345051/diff/9093/9094#newcode61
Line 61: [(id)NSApp isHandlingSendEvent])) {
On 2009/11/05 07:54:19, shess wrote:
> Is this called frequently enough to make it worth putting a category on
> NSApplication to return a default for -isHandlingSendEvent?
> 
> Wait a minute ... can you be doing this?  What if this isn't on the UI thread?

> Shouldn't this only be checked if you're using MessagePumpNSApplication?

Done.

http://codereview.chromium.org/345051/diff/9093/9105
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/345051/diff/9093/9105#newcode19
Line 19: #import "chrome/browser/chrome_browser_application_mac.h"
On 2009/11/05 07:54:19, shess wrote:
> This seems odd.  We aren't refering to CrApplication anywhere, is this line
> necessary these days?

Done.

http://codereview.chromium.org/345051/diff/9093/9102
File chrome/browser/browser_main_mac.mm (right):

http://codereview.chromium.org/345051/diff/9093/9102#newcode34
Line 34: [BrowserCrApplication sharedApplication];
On 2009/11/05 07:54:19, shess wrote:
> Would it make sense to push the -isKindOfClass: test into +sharedApplication
> itself?  Since BrowserCrApplication isKindOf CrApplication, it could do the
> NSApp test after calling super.  I'm not committed to this, I just see the
same
> basic code in two places and assume that means we should think about whether
it
> belongs in one.  I think that you'd only have to test in +[CrApplication
> sharedApplication], since self will be [BrowserCrApplication class] at the
> appropriate times.

Done.

http://codereview.chromium.org/345051/diff/9093/9099
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/345051/diff/9093/9099#newcode67
Line 67: };
On 2009/11/05 07:54:19, shess wrote:
> ScopedCrashKey seems to have fallen out of the anonymous namespace.  Was that
> intentional?

Done.

http://codereview.chromium.org/345051/diff/9093/9099#newcode150
Line 150: - (void)reportException:(NSException *)anException {
On 2009/11/05 07:54:19, shess wrote:
> Would prefer to leave this in the same general location in the file so that
the
> diff looks cleaner.  Am assuming that it's the same as it was before.

Done.

http://codereview.chromium.org/345051/diff/9093/9106
File chrome/browser/cocoa/cocoa_test_helper.h (right):

http://codereview.chromium.org/345051/diff/9093/9106#newcode149
Line 149: [CrApplication sharedApplication];
On 2009/11/05 07:54:19, shess wrote:
> Any reason not to replace "// Look ..." through -sharedApplication with a call
> to CocoaTest::BootstrapCocoa()?

Done.

http://codereview.chromium.org/345051/diff/9093/9108
File chrome/renderer/renderer_main_platform_delegate_mac.mm (right):

http://codereview.chromium.org/345051/diff/9093/9108#newcode32
Line 32: << [[NSApp className] UTF8String];
On 2009/11/05 07:54:19, shess wrote:
> << should line up, I think.

Done.

[Scott Hess - ex-Googler, 2009-11-05 18:48:33]

LGTM.

http://codereview.chromium.org/345051/diff/6077/6078
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/345051/diff/6077/6078#newcode60
Line 60: [static_cast<CrApplication*>(NSApp) isHandlingSendEvent])) {
Consider this non-blocking, as I realize I'm flogging a pony, and I think it's
more of a follow-up/to-do:

If MessagePumpCFRunLoopBase vended a virtual method to create an
NSAutoreleasePool, with the default implementation being to create it, then
MessagePumpNSApplication could override with a version to test before creating
(and that test would not need to check which thread it's on).  The resulting
ownership could be handled by scoped_nsobject.  I think it's six of one and half
a dozen of the other, but when I see something like this I feel like it's
manually implementing late method binding.

[Mark Mentovai, 2009-11-05 19:01:46]

http://codereview.chromium.org/345051/diff/6106/9135
File base/chrome_application_mac.h (right):

http://codereview.chromium.org/345051/diff/6106/9135#newcode16
Line 16: @property(readonly, getter=isHandlingSendEvent) BOOL handlingSendEvent;
Can this be nonatomic?

http://codereview.chromium.org/345051/diff/6106/9135#newcode21
Line 21: // Namespace for exception-reporting helper functions.
Not really related to exceptions at all anymore.

http://codereview.chromium.org/345051/diff/6106/9135#newcode24
Line 24: // Controls the state of |handlingSendEvent_| in the event loop so that
it is
Does this need to be exposed beyond the .mm file?  Seems like it could just go
into the .mm in an anonymous namespace.

http://codereview.chromium.org/345051/diff/6106/9134
File base/chrome_application_mac.mm (right):

http://codereview.chromium.org/345051/diff/6106/9134#newcode9
Line 9: @interface CrApplication ()
Give the category a name like Private?

http://codereview.chromium.org/345051/diff/6106/9134#newcode10
Line 10: @property(readwrite) BOOL handlingSendEvent;
nonatomic?

http://codereview.chromium.org/345051/diff/6106/9134#newcode23
Line 23: << " not " << [[NSApp className] UTF8String];
nit: ", not "

http://codereview.chromium.org/345051/diff/6106/9118
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/345051/diff/6106/9118#newcode60
Line 60: [static_cast<CrApplication*>(NSApp) isHandlingSendEvent])) {
Doing this blindly is a little scary.  I know you've taken precautions to avoid
winding up here without a CrApplication, but this is an easy thing to screw up
later.

Also, I kind of liked how you had structured things before with the virtual
factory to create the autorelease pool, and the separate Owner class.  This
seems like a step backwards.

With the special virtual factory, you wouldn't need the isMainThread check
(except maybe as a DCHECK).  You could also use a static variable to check if
NSApp is CrApplication or a subclass as a slight optimization.

http://codereview.chromium.org/345051/diff/6106/9118#newcode65
Line 65: [pool_ release];
drain?

http://codereview.chromium.org/345051/diff/6106/9118#newcode619
Line 619: MessagePumpScopedAutoreleasePool autorelease_pool;
This can stay ScopedNSAutoreleasePool.  MPCFRL shouldn't ever be on the NSApp
thread.

It can also stay as-is, I guess if you're using the virtual factory method that
you had before, it doesn't really matter.

http://codereview.chromium.org/345051/diff/6106/9119
File chrome/app/app-Info.plist (left):

http://codereview.chromium.org/345051/diff/6106/9119#oldcode190
Line 190: <string>CrApplication</string>
Aww, bye-bye!

http://codereview.chromium.org/345051/diff/6106/9121
File chrome/browser/chrome_browser_application_mac.h (right):

http://codereview.chromium.org/345051/diff/6106/9121#newcode11
Line 11: #import "base/histogram.h"
Whoa, really?

http://codereview.chromium.org/345051/diff/6106/9121#newcode12
Line 12: #import "base/logging.h"
Really?

http://codereview.chromium.org/345051/diff/6106/9121#newcode13
Line 13: #import "base/scoped_nsobject.h"
Really?

http://codereview.chromium.org/345051/diff/6106/9121#newcode14
Line 14: #import "base/sys_string_conversions.h"
and the rest too.

http://codereview.chromium.org/345051/diff/6106/9123
File chrome/browser/chrome_browser_application_mac.mm (right):

http://codereview.chromium.org/345051/diff/6106/9123#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
We will actually want the renderer to share some other stuff from here, but we
can make those decisions after this goes in.

http://codereview.chromium.org/345051/diff/6106/9132
File chrome/renderer/renderer_main_platform_delegate_mac.mm (right):

http://codereview.chromium.org/345051/diff/6106/9132#newcode28
Line 28: // drawing of native UI elements
Reflow comment.

[dmac, 2009-11-05 19:38:31]

http://codereview.chromium.org/345051/diff/6106/9135
File base/chrome_application_mac.h (right):

http://codereview.chromium.org/345051/diff/6106/9135#newcode16
Line 16: @property(readonly, getter=isHandlingSendEvent) BOOL handlingSendEvent;
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> Can this be nonatomic?

Done.

http://codereview.chromium.org/345051/diff/6106/9135#newcode21
Line 21: // Namespace for exception-reporting helper functions.
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> Not really related to exceptions at all anymore.

Done.

http://codereview.chromium.org/345051/diff/6106/9135#newcode24
Line 24: // Controls the state of |handlingSendEvent_| in the event loop so that
it is
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> Does this need to be exposed beyond the .mm file?  Seems like it could just go
> into the .mm in an anonymous namespace.

Yes, because BrowserCrapplication needs it

http://codereview.chromium.org/345051/diff/6106/9134
File base/chrome_application_mac.mm (right):

http://codereview.chromium.org/345051/diff/6106/9134#newcode9
Line 9: @interface CrApplication ()
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> Give the category a name like Private?

Apparently @property redefinitions can only occur in anonymous categories. Need
to do more testing here, but I will do it post getting this CL in.

http://codereview.chromium.org/345051/diff/6106/9134#newcode10
Line 10: @property(readwrite) BOOL handlingSendEvent;
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> nonatomic?

Done.

http://codereview.chromium.org/345051/diff/6106/9134#newcode23
Line 23: << " not " << [[NSApp className] UTF8String];
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> nit: ", not "

Done.

http://codereview.chromium.org/345051/diff/6106/9118
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/345051/diff/6106/9118#newcode60
Line 60: [static_cast<CrApplication*>(NSApp) isHandlingSendEvent])) {
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> Doing this blindly is a little scary.  I know you've taken precautions to
avoid
> winding up here without a CrApplication, but this is an easy thing to screw up
> later.
> 
> Also, I kind of liked how you had structured things before with the virtual
> factory to create the autorelease pool, and the separate Owner class.  This
> seems like a step backwards.
> 
> With the special virtual factory, you wouldn't need the isMainThread check
> (except maybe as a DCHECK).  You could also use a static variable to check if
> NSApp is CrApplication or a subclass as a slight optimization.

Done.

http://codereview.chromium.org/345051/diff/6106/9118#newcode65
Line 65: [pool_ release];
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> drain?

Done.

http://codereview.chromium.org/345051/diff/6106/9121
File chrome/browser/chrome_browser_application_mac.h (right):

http://codereview.chromium.org/345051/diff/6106/9121#newcode11
Line 11: #import "base/histogram.h"
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> Whoa, really?

Done.

http://codereview.chromium.org/345051/diff/6106/9121#newcode12
Line 12: #import "base/logging.h"
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> Really?

Done.

http://codereview.chromium.org/345051/diff/6106/9121#newcode13
Line 13: #import "base/scoped_nsobject.h"
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> Really?

Done.

http://codereview.chromium.org/345051/diff/6106/9121#newcode14
Line 14: #import "base/sys_string_conversions.h"
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> and the rest too.

Done.

http://codereview.chromium.org/345051/diff/6106/9132
File chrome/renderer/renderer_main_platform_delegate_mac.mm (right):

http://codereview.chromium.org/345051/diff/6106/9132#newcode28
Line 28: // drawing of native UI elements
On 2009/11/05 19:01:46, Mark Mentovai wrote:
> Reflow comment.

Done.

[Mark Mentovai, 2009-11-05 19:53:40]

LGTM % a couple of nitty things.

http://codereview.chromium.org/345051/diff/9157/9174
File base/chrome_application_mac.mm (right):

http://codereview.chromium.org/345051/diff/9157/9174#newcode49
Line 49: }  // chrome_application_mac
nit: "// namespace chrome_applicaiton_mac"

http://codereview.chromium.org/345051/diff/9157/9158
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/345051/diff/9157/9158#newcode35
Line 35: explicit MessagePumpScopedAutoreleasePool(MessagePumpCFRunLoopBase*
runLoop) :
Nit: run_loop, because we follow this_style in C++ classes instead of thisStyle
in Objective-C classes.

Further nit: in a past life, you named the variable pump, which is much more
descriptive than run_loop.  Use pump again?

http://codereview.chromium.org/345051/diff/9157/9161
File chrome/browser/chrome_browser_application_mac.h (right):

http://codereview.chromium.org/345051/diff/9157/9161#newcode11
Line 11: #import "base/histogram.h"
I thought you said you got rid of these unnecessary headers?  No?

These headers all (or almost all) belong in the .mm file, not in the .h.

http://codereview.chromium.org/345051/diff/9157/9163
File chrome/browser/chrome_browser_application_mac.mm (left):

http://codereview.chromium.org/345051/diff/9157/9163#oldcode7
Line 7: #import "base/histogram.h"
Restore these.  I meant to take them out of the .h, not the .mm.

http://codereview.chromium.org/345051/diff/9157/9167
File chrome/browser/chrome_browser_application_mac_unittest.mm (right):

http://codereview.chromium.org/345051/diff/9157/9167#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
Thanks for fixing the svn property here, too.

http://codereview.chromium.org/345051/diff/9157/9172
File chrome/renderer/renderer_main_platform_delegate_mac.mm (right):

http://codereview.chromium.org/345051/diff/9157/9172#newcode9
Line 9: #include "base/chrome_application_mac.h"
#import this one.

[Mark Mentovai, 2009-11-05 19:54:16]

Higher-level comment: this came out cleaner than I expected, and maybe cleaner
than you expected too.  Here's hoping this one's a keeper.  Best of luck.

[dmac, 2009-11-05 20:22:04]

Final (fingers crossed) cleanup for Mark. Awaiting the tree opening again.

http://codereview.chromium.org/345051/diff/6077/6078
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/345051/diff/6077/6078#newcode60
Line 60: [static_cast<CrApplication*>(NSApp) isHandlingSendEvent])) {
On 2009/11/05 18:48:34, shess wrote:
> Consider this non-blocking, as I realize I'm flogging a pony, and I think it's
> more of a follow-up/to-do:
> 
> If MessagePumpCFRunLoopBase vended a virtual method to create an
> NSAutoreleasePool, with the default implementation being to create it, then
> MessagePumpNSApplication could override with a version to test before creating
> (and that test would not need to check which thread it's on).  The resulting
> ownership could be handled by scoped_nsobject.  I think it's six of one and
half
> a dozen of the other, but when I see something like this I feel like it's
> manually implementing late method binding.

Done.

http://codereview.chromium.org/345051/diff/9157/9158#newcode35
Line 35: // #3 [NSRunLoop run]
On 2009/11/05 19:53:40, Mark Mentovai wrote:
> Nit: run_loop, because we follow this_style in C++ classes instead of
thisStyle
> in Objective-C classes.
> 
> Further nit: in a past life, you named the variable pump, which is much more
> descriptive than run_loop.  Use pump again?

Done.

http://codereview.chromium.org/345051/diff/9157/9161
File chrome/browser/chrome_browser_application_mac.h (right):

http://codereview.chromium.org/345051/diff/9157/9161#newcode11
Line 11: #import "base/histogram.h"
On 2009/11/05 19:53:40, Mark Mentovai wrote:
> I thought you said you got rid of these unnecessary headers?  No?
> 
> These headers all (or almost all) belong in the .mm file, not in the .h.

Done.

http://codereview.chromium.org/345051/diff/9157/9172
File chrome/renderer/renderer_main_platform_delegate_mac.mm (right):

http://codereview.chromium.org/345051/diff/9157/9172#newcode9
Line 9: #include "base/chrome_application_mac.h"
On 2009/11/05 19:53:40, Mark Mentovai wrote:
> #import this one.

Done.

[Mark Mentovai, 2009-11-05 20:28:39]

Yes yes yes yes yes yes yes

LGTM

http://codereview.chromium.org/345051/diff/9179/9180
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/345051/diff/9179/9180#newcode36
Line 36: pool_(pump->CreateAutoreleasePool()) {
Final really tiny minor insignificant nit that you can fix and not need a new
LGTM for...

The preferred format is to put the : on the second line and have it be indented
by four spaces.

  explicit MessagePumpScopedAutoreleasePool(MessagePumpCFRunLoopBase* pump)
      : pool_(pump->CreateAutoreleasePool()) {

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

[Scott Hess - ex-Googler, 2009-11-05 20:36:12]

LGTM!

http://codereview.chromium.org/345051/diff/9179/9180
File base/message_pump_mac.mm (right):

http://codereview.chromium.org/345051/diff/9179/9180#newcode38
Line 38: ~MessagePumpScopedAutoreleasePool() {
Nit: indentation.