Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
343024:
Cleans up our autorelease handling so that we don't create a layered ... (Closed)
|
Created:
11 years, 1 month ago by dmac Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, brettw+cc_chromium.org, ben+cc_chromium.org, John Grabowski, jam, pam+watch_chromium.org, darin (slow to review) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionCleans up our autorelease handling so that we don't create a layered
autorelease pool in our run loop source if there is one already on the stack
above us. This allows Cocoa to clean up all the objects at the same time as it
expects to do. There may be more "interesting" code that can be removed now
that this is in. Initially we were going to implement it by checking the
nesting levels of the runloops, but it turns out by the time sendEvent is
called at the upper level we are already out of the "CFRunloopRun" call so
our nesting count isn't a valid indicator of our state.
TEST=1) See bug 25857. 2) Start up. Open 3+ windows. Quit.
BUG=25857
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30647
Patch Set 1 #Patch Set 2 : '' #
Total comments: 17
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 15
Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #
Total comments: 8
Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 68
Patch Set 12 : '' #Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #
Total comments: 50
Patch Set 16 : '' #Patch Set 17 : '' #Patch Set 18 : '' #Patch Set 19 : '' #Patch Set 20 : '' #
Total comments: 4
Messages
Total messages: 31 (0 generated)
Sorry for the delay...turns out that the nested count didn't work, and I had a meeting to attend. Not as pretty, but still avoids running through the event loop again, and should keep things sane at the higher levels so most people won't need to worry about this stuff.
http://codereview.chromium.org/343024/diff/14/2001 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/14/2001#newcode42 Line 42: // so that we know whether or not to create an NSAutoreleasePool. I'm trying to not force my rules of grammar onto every change I review, but when you're in my file, you play by my rules. :) Revise this (and the next one) to not use "we." Most things that you can write with "we" can be written even more clearly without. Same applies to the next comment and some comments in the next file. http://codereview.chromium.org/343024/diff/14/2002 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/14/2002#newcode264 Line 264: // If we are handling another UIEvent we are going to be nested in a What is a UIEvent? Is that some kind of iPhone NSEvent? http://codereview.chromium.org/343024/diff/14/2002#newcode265 Line 265: // autorelease pool already, and we don't want to create one here. Rather than doing this three times, let's factor it out into a class that we can keep on the stack. Set up the autorelease pool if you need it in the constructor, and release it in the destructor. MaybeScopedNSAutoreleasePool? http://codereview.chromium.org/343024/diff/14/2002#newcode270 Line 270: if (![NSApp conformsToProtocol:@protocol(MessagesPumpNSApplicationProtocol)] || Watch your 80. http://codereview.chromium.org/343024/diff/14/2002#newcode271 Line 271: ![(id<MessagesPumpNSApplicationProtocol>)NSApp isHandlingUIEvent]) { I hope that we don't have any cases where we've created our MessagePumpCFRunLoopBase object prior to initializing CrApplication. I guess your use here is OK because NSApp will be nil in that case. http://codereview.chromium.org/343024/diff/14/2004 File chrome/browser/chrome_application_mac.h (right): http://codereview.chromium.org/343024/diff/14/2004#newcode15 Line 15: // Are we currently handling a UI event. Is that a question? http://codereview.chromium.org/343024/diff/14/2003 File chrome/browser/chrome_application_mac.mm (right): http://codereview.chromium.org/343024/diff/14/2003#newcode247 Line 247: handlingUIEvent_ = YES; Ineffective outside of the browser process. Only the browser is a crapplication. Is that OK? http://codereview.chromium.org/343024/diff/14/2006 File chrome/browser/cocoa/browser_window_controller.mm (right): http://codereview.chromium.org/343024/diff/14/2006#newcode265 Line 265: [self autorelease]; Nice. http://codereview.chromium.org/343024/diff/14/2005 File chrome/browser/renderer_host/render_widget_host_view_mac.mm (left): http://codereview.chromium.org/343024/diff/14/2005#oldcode292 Line 292: // We've been told to destroy. Also nice.
http://codereview.chromium.org/343024/diff/14/2005 File chrome/browser/renderer_host/render_widget_host_view_mac.mm (left): http://codereview.chromium.org/343024/diff/14/2005#oldcode292 Line 292: // We've been told to destroy. i understand all the other changes besides this one. can you elaborate?
I'm completely underqualified (misqualified? :-) ) to say something deep and insightful (maybe except for that I don't think that "we" in comments is all that bad), but: Is it possible (performance-wise etc) to collect histogram data over how many run loop iterations we run a nested loop and hence don't free the current autorelease pool? That was the major concern people seemed to have in the meeting.
Dave - can you expand on why the nesting level didn't help? If we're out of the nest loop when we the send (and autorelease) happens, how is it in the wrong timing? General comment - runloops are also per thread, this cl appear to make the behavior of all threads depend on what the main thread is doing, that seems... ;)
On 2009/10/28 21:54:51, TVL wrote: > Dave - can you expand on why the nesting level didn't help? If we're out of the > nest loop when we the send (and autorelease) happens, how is it in the wrong > timing? > > General comment - runloops are also per thread, this cl appear to make the > behavior of all threads depend on what the main thread is doing, that seems... > ;) Good call... Do people feel it would be sufficient to check CFRunLoopGetCurrent() == CFRunLoopGetMain() or should I try and twist the classes so that my new ScopedNSEventHandlerAutoreleasePool only gets created on MessagePumpNSApplications.
On 2009/10/28 22:08:18, dmac wrote:
> On 2009/10/28 21:54:51, TVL wrote:
> > Dave - can you expand on why the nesting level didn't help? If we're out of
> the
> > nest loop when we the send (and autorelease) happens, how is it in the wrong
> > timing?
The [NSApplication run] method looks something like this:
while(ad_nauseaum) {
NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init]; // This is where
the pool we care about gets created.
NSEvent *event = [self nextEventMatchingMask....]; // This is where
CFRunLoopRun gets called and increments the nest_level on entry, and the
decrements it on exit.
[self sendEvent:event]; // This is where we end up calling performClose (or
whatever else calls our CFRunLoopRun).
[self updateWindows];
[pool release]; // This is where the pool we care about gets released.
}
Does that make more sense?
On 2009/10/28 21:52:24, Nico wrote: > I'm completely underqualified (misqualified? :-) ) to say something deep and > insightful (maybe except for that I don't think that "we" in comments is all > that bad), but: > > Is it possible (performance-wise etc) to collect histogram data over how many > run loop iterations we run a nested loop and hence don't free the current > autorelease pool? That was the major concern people seemed to have in the > meeting. I didn't do a formal histogram, but I did do some testing. The helper will run almost all of its iterations in a nested loop, and almost never releases the pool. The app itself can cycle many times (I saw up to 6) when there's lots of work to be done before the autorelease pool is released.
http://codereview.chromium.org/343024/diff/14/2001 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/14/2001#newcode42 Line 42: // so that we know whether or not to create an NSAutoreleasePool. On 2009/10/28 21:40:50, Mark Mentovai wrote: > I'm trying to not force my rules of grammar onto every change I review, but when > you're in my file, you play by my rules. :) We will obey ;-) Done. http://codereview.chromium.org/343024/diff/14/2002 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/14/2002#newcode264 Line 264: // If we are handling another UIEvent we are going to be nested in a On 2009/10/28 21:40:50, Mark Mentovai wrote: > What is a UIEvent? Is that some kind of iPhone NSEvent? Done. http://codereview.chromium.org/343024/diff/14/2002#newcode265 Line 265: // autorelease pool already, and we don't want to create one here. On 2009/10/28 21:40:50, Mark Mentovai wrote: > Rather than doing this three times, let's factor it out into a class that we can > keep on the stack. Set up the autorelease pool if you need it in the > constructor, and release it in the destructor. > > MaybeScopedNSAutoreleasePool? Done. http://codereview.chromium.org/343024/diff/14/2002#newcode270 Line 270: if (![NSApp conformsToProtocol:@protocol(MessagesPumpNSApplicationProtocol)] || On 2009/10/28 21:40:50, Mark Mentovai wrote: > Watch your 80. Done. http://codereview.chromium.org/343024/diff/14/2002#newcode271 Line 271: ![(id<MessagesPumpNSApplicationProtocol>)NSApp isHandlingUIEvent]) { On 2009/10/28 21:40:50, Mark Mentovai wrote: > I hope that we don't have any cases where we've created our > MessagePumpCFRunLoopBase object prior to initializing CrApplication. > > I guess your use here is OK because NSApp will be nil in that case. Good thought, but I think we are safe. http://codereview.chromium.org/343024/diff/14/2004 File chrome/browser/chrome_application_mac.h (right): http://codereview.chromium.org/343024/diff/14/2004#newcode15 Line 15: // Are we currently handling a UI event. On 2009/10/28 21:40:50, Mark Mentovai wrote: > Is that a question? kind of ;-) Now it's a statement. http://codereview.chromium.org/343024/diff/14/2003 File chrome/browser/chrome_application_mac.mm (right): http://codereview.chromium.org/343024/diff/14/2003#newcode247 Line 247: handlingUIEvent_ = YES; On 2009/10/28 21:40:50, Mark Mentovai wrote: > Ineffective outside of the browser process. Only the browser is a > crapplication. Is that OK? I think so. Can you think of any other place that this would be an issue?
On 2009/10/28 21:43:59, pink wrote: > http://codereview.chromium.org/343024/diff/14/2005 > File chrome/browser/renderer_host/render_widget_host_view_mac.mm (left): > > http://codereview.chromium.org/343024/diff/14/2005#oldcode292 > Line 292: // We've been told to destroy. > i understand all the other changes besides this one. can you elaborate? It has been an hour or two, but as I remember it: when our nested autorelease pool was being cleaned up, destroy was being called. Objects above this still depended on this view existing, but other objects in the current pool needed it out of the view hierarchy. By doing this, Nico was effectively pushing it into the outer autoreleasepool to be cleaned up.
The first comment is substantive, the rest are mechanical beautification things. I think that this is a reasonable design but it can be cleaned up slightly: http://codereview.chromium.org/343024/diff/8003/8004 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/8003/8004#newcode89 Line 89: class ScopedRunLoopAutoreleasePool { Can we make the NSAutoreleasePool* a member of this class? Then we could do something like: ScopedRunLoopAutoreleasePool(MPCFRLB* base) : pool_(base->SetUpAutoreleasePool()) {} ~ScopedRunLoopAutoreleasePool() { [pool release]; } so the interface would just be NSAutoreleasePool* MPCFRLB::SetUpAutoreleasePool() { return [[NSAutoreleasePool alloc] init]; } or have it wrapped in custom logic in the MPNSA case. Then we don't need to keep the pool_ object on MPCFRLB at all, which seems much cleaner. The pool_ member seems like a recipe for disaster. It doesn't look like you've arranged for save and restore on nested loops. For that matter, with this change, I don't think we'd need to expose ScopedRunLoopAutoreleasePool in the header at all. We can just make it an anonymous-namespace class in the .mm file as a private implementation detail. Except we'd need SetUpAutoreleasePool to be public then, and that sort of sucks. Maybe this is a case where it makes sense to make friends. http://codereview.chromium.org/343024/diff/8003/8004#newcode91 Line 91: ScopedRunLoopAutoreleasePool(MessagePumpCFRunLoopBase *base) : base_(base) { noting star on the left of the space http://codereview.chromium.org/343024/diff/8003/8004#newcode278 Line 278: // NSAutoreleasePool. If we are in the process of handling an event we don't an event comma we don't but "we" :) http://codereview.chromium.org/343024/diff/8003/8005 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/8003/8005#newcode9 Line 9: Leave this, no reason to separate these system headers from these other system headers. http://codereview.chromium.org/343024/diff/8003/8005#newcode17 Line 17: #include "base/logging.h" Sort l before s. http://codereview.chromium.org/343024/diff/8003/8005#newcode267 Line 267: ScopedRunLoopAutoreleasePool pool(this); Leave the blank line after this one.
http://codereview.chromium.org/343024/diff/8003/8005 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/8003/8005#newcode554 Line 554: [pool_ release]; drain?!
> http://codereview.chromium.org/343024/diff/8003/8005 > File base/message_pump_mac.mm (right): > > http://codereview.chromium.org/343024/diff/8003/8005#newcode554 > Line 554: [pool_ release]; > drain?! Doesn't |drain| and |release| do the same thing when garbage collection is off?
Also... In the commit message for r29749, I left a whole bunch of test cases. You should try all of those situations out because they exercise event loops in a bunch of different configurations in both the browser and helper process. You should also give everything a good stretch while running in --single-process mode, because that's an easy way to bring up an MPNSA on the main thread for the browser and an MPNSRL on each renderer thread. I'd also appreciate it if you could add one good test case for each of the workarounds you're removing. Right now, you've just got "see bug" which is really only one known test case. We should be able to test that the workarounds you're removing are no longer necessary too.
When you start thinking about optimization, you know you're close... http://codereview.chromium.org/343024/diff/8003/8005 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/8003/8005#newcode696 Line 696: if (![NSApp conformsToProtocol:protocol] || One more. For this guy, can we do: static BOOL conformsToMessagePumpNSApplicationProtocol = [NSApp conformsToProtocol: @protocol(MessagePumpNSApplicationProtocol)]; if (!conformsToMessagePumpNSApplicationProtocol || ![(id<MessagesPumpNSApplicationProtocol>)NSApp isHandlingNSEvent]) { http://codereview.chromium.org/343024/diff/8003/8005#newcode697 Line 697: ![(id<MessagesPumpNSApplicationProtocol>)NSApp isHandlingNSEvent]) { Should be MessagePumpNSApplicationProtocol, no plural on the Message. These names are long enough as it is...
I'll work on putting some unit tests together. http://codereview.chromium.org/343024/diff/8003/8005 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/8003/8005#newcode9 Line 9: On 2009/10/29 00:11:39, Mark Mentovai wrote: > Leave this, no reason to separate these system headers from these other system > headers. Done. http://codereview.chromium.org/343024/diff/8003/8005#newcode17 Line 17: #include "base/logging.h" On 2009/10/29 00:11:39, Mark Mentovai wrote: > Sort l before s. Done. http://codereview.chromium.org/343024/diff/8003/8005#newcode267 Line 267: ScopedRunLoopAutoreleasePool pool(this); On 2009/10/29 00:11:39, Mark Mentovai wrote: > Leave the blank line after this one. Done. http://codereview.chromium.org/343024/diff/8003/8005#newcode554 Line 554: [pool_ release]; On 2009/10/29 00:12:33, Mark Mentovai wrote: > drain?! Done. http://codereview.chromium.org/343024/diff/8003/8005#newcode696 Line 696: if (![NSApp conformsToProtocol:protocol] || On 2009/10/29 01:52:00, Mark Mentovai wrote: > One more. > > For this guy, can we do: > > static BOOL conformsToMessagePumpNSApplicationProtocol = > [NSApp conformsToProtocol: > @protocol(MessagePumpNSApplicationProtocol)]; > > if (!conformsToMessagePumpNSApplicationProtocol || > ![(id<MessagesPumpNSApplicationProtocol>)NSApp isHandlingNSEvent]) { Done. http://codereview.chromium.org/343024/diff/8003/8005#newcode697 Line 697: ![(id<MessagesPumpNSApplicationProtocol>)NSApp isHandlingNSEvent]) { On 2009/10/29 01:52:00, Mark Mentovai wrote: > Should be MessagePumpNSApplicationProtocol, no plural on the Message. These > names are long enough as it is... Done.
On 2009/10/29 18:01:26, dmac wrote: > I'll work on putting some unit tests together. Still looking into this, but while I'm at it, please look at the latest incarnation. I did some sampling and we avoid posting an event well over 90% of the time for Chromium, and basically 100% of the time for Chromium helper. It should adapt to any changes that people make in the future with regards to using autorelease.
dmac, I meant to bring this up in the debugger to analyze myself, but there were a whole bunch of interruptions this afternoon. Then your CL update came in, and it was another interruption, but I was eager to see what you came up with, so I read this instead. http://codereview.chromium.org/343024/diff/8018/7006 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/8018/7006#newcode9 Line 9: #import <Foundation/NSDebug.h> Uh-oh...That's the file with the comment at the top that says "don't rely on me! but probably nothing bad will happen if you do." I hope +topAutoreleasePoolCount is stable. Using these routines is a tiny bit scary, but it'd be surprising if it went anywhere or changed behavior. Even so, you might want to put a comment by your use of +topAutoreleasePoolCount so that future readers will be aware that we consciously made the choice to use it for reasons X, Y, and Z, and we think that A, B, and C are unlikely. http://codereview.chromium.org/343024/diff/8018/7006#newcode34 Line 34: // to clean up after themselves correctly. So that makes it an interface? Interface is better than "do nothing base class so that there is a common type to pass around." Also, if you say "interface," the sentence about the virtual destructor isn't necessary either. http://codereview.chromium.org/343024/diff/8018/7006#newcode44 Line 44: // The base message pump creates and drains an autorelease pool each time This dude can go into an anonymous namespace even if you want to keep the interface in the base namespace for ease of forward declaration-ness. (The anonymous namespace would make some of the later code cleaner, where you wrote your CreateAutoreleasePoolController methods, but I guess it's a tradeoff, and you probably had a good reason to remove it.) http://codereview.chromium.org/343024/diff/8018/7006#newcode71 Line 71: NSUInteger endCount = [NSAutoreleasePool topAutoreleasePoolCount]; Concerns about +topAutoreleasePoolCount notwithstanding... This is better than I was expecting. I was afraid you'd be sending a fake NSEvent each time we went through RunWork/RunDelayedWork/RunIdleWork. With that in mind: Is there anything we can do to avoid accumulating too many fake events? Right now we'll get one each time RunWork, RunDelayedWork, and RunIdleWork fire. Since we control crapplication, there's an obvious way to coordinate this, but remember we're not a crapplication outside of the browser process (yet). Are you getting sick of this? Maybe it can be a TODO, but ideally it'd be a TODO we fix before m4 closes because of the obvious potential performance impact. (Also TODO: make non-browser processes crapplications. That one will almost definitely miss the m4 boat, though.) http://codereview.chromium.org/343024/diff/8018/7006#newcode86 Line 86: NSEvent* releaseEvent = Make this a static retained guy? I think we can eat the leak and it won't show up in valgrind. The other alternative is to have the owner pass in the NSEvent to reuse, but I don't think we need to get that crazy. I wouldn't stop you from going that crazy if you wanted to, I just don't think it's necessary. http://codereview.chromium.org/343024/diff/8018/7006#newcode87 Line 87: [NSEvent otherEventWithType:NSAppKitDefined I think we usually use NSApplicationDefined for these. There's another bogus event later in the file in MessagePumpNSApplication::Quit. Maybe we should just make MessagePumpNSApplication responsible for creating and maintaining one fake NSEvent* as a member. (I think I just changed my mind about the static, then?) http://codereview.chromium.org/343024/diff/8018/7006#newcode96 Line 96: [NSApp postEvent:releaseEvent atStart:YES]; There's no need to put it at the head of the queue. If there's real work to be done, let's let the app do that instead. http://codereview.chromium.org/343024/diff/8018/7006#newcode764 Line 764: MessagePumpCFRunLoopBase::ScopedAutoreleasePoolController* Aha. This looks weird. The indentation is all messed up! The indent on this line and the next three should be 0, 4, 2, 0. Just like you did when you implemented this in MessagePumpCFRunLoopBase.
Latest and greatest with the DeferredAutoreleasePool. I will add a pile of unittests tomorrow AM, but wanted folks to look at it and see if there were any comments before I committed more time to this. Mark I found the bug with my impl... one character killed me ;-) Note question about NSRecordAllocationEvent...
generally looks pretty nice, but some where in here I think we need to add a big old comment explaining what this is needed for. i.e.-put in something about how the NSApp runloop works, and how the local sources loop can catch something in the pool that is needed by the event that will then get processed. http://codereview.chromium.org/343024/diff/6007/6008 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/6007/6008#newcode53 Line 53: class ScopedAutoreleasePool; why not just put this in the imp file in an anon namespace? http://codereview.chromium.org/343024/diff/6007/6009 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/6007/6009#newcode75 Line 75: /* so this isn't needed now? http://codereview.chromium.org/343024/diff/6007/6009#newcode815 Line 815: deferredPool_ = [[NSMutableArray alloc] initWithCapacity:1]; why 1?
like TVL suggested, a big comment would help, but I like the idea. http://codereview.chromium.org/343024/diff/6007/6008 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/6007/6008#newcode17 Line 17: // MessagePumpCFRunLoop. For a NSRunLoop, the similar MessagePumpNSRunLoop i've lost this argument several times. people keep telling me the sound is "ehhhn" which is a vowel sound (like "endeavor"), so it should be "an", not "a". *shrug*. http://codereview.chromium.org/343024/diff/6007/6008#newcode51 Line 51: // A RAII class for controlling creation and releasing of autoreleasepools RAII? http://codereview.chromium.org/343024/diff/6007/6009 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/6007/6009#newcode785 Line 785: omit blank line http://codereview.chromium.org/343024/diff/6007/6009#newcode787 Line 787: // correctly with nested autorelease pools in runloops. a comment somewhere about why and how is needed. http://codereview.chromium.org/343024/diff/6007/6009#newcode805 Line 805: omit blank line http://codereview.chromium.org/343024/diff/6007/6009#newcode810 Line 810: omit blank line http://codereview.chromium.org/343024/diff/6007/6009#newcode829 Line 829: omit blank line http://codereview.chromium.org/343024/diff/6007/6009#newcode831 Line 831: // free on "self" and it is needed after drain. i don't understand how we are guaranteed it's still alive if we've freed self. http://codereview.chromium.org/343024/diff/6007/6009#newcode835 Line 835: ditto.
This is pretty darn clean, I have to say. There's one functional question remaining. I left that in message_pump_mac.mm:51. http://codereview.chromium.org/343024/diff/6007/6008 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/6007/6008#newcode51 Line 51: // A RAII class for controlling creation and releasing of autoreleasepools autorelease pools http://codereview.chromium.org/343024/diff/6007/6008#newcode53 Line 53: class ScopedAutoreleasePool; ScopedAutoreleasePool isn't a great name. It's too easily confused with ScopedNSAutoreleasePool. Suggestion! Extend ScopedNSAutoreleasePool to handle this for us. Add a new flavor of constructor: explicit ScopedNSAutoreleasePool(NSAutoreleasePool* autorelease_pool) Gotcha: you'll need to revise ScopedNSAutoreleasePool::Recycle to use [[[original_pool class] alloc] init] instead of always using NSAutoreleasePool. The alternative is to name this one somehow in a way that makes it clear that it's different from ScopedNSAutoreleasePool. http://codereview.chromium.org/343024/diff/6007/6008#newcode53 Line 53: class ScopedAutoreleasePool; TVL wrote: > why not just put this in the imp file in an anon namespace? Agree. http://codereview.chromium.org/343024/diff/6007/6008#newcode249 Line 249: // MessagePumpNSApplications needs to create a special autorelease pool that MessagePumpNSApplication needs or MessagePumpNSApplications need but not both. http://codereview.chromium.org/343024/diff/6007/6009 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/6007/6009#newcode1 Line 1: // Copyright (c) 2008 The Chromium Authors. All rights reserved. Pesky lice. http://codereview.chromium.org/343024/diff/6007/6009#newcode31 Line 31: // Send a null event through to the upper event loop if necessary. Why "upper event loop?" What are you contrasting "upper" with? How 'bout just "send a null NSEvent if necessary."? http://codereview.chromium.org/343024/diff/6007/6009#newcode33 Line 33: // If there is already an event waiting in the queue there is no need queue comma there http://codereview.chromium.org/343024/diff/6007/6009#newcode36 Line 36: if (![NSApp nextEventMatchingMask:NSAnyEventMask Very classy! http://codereview.chromium.org/343024/diff/6007/6009#newcode41 Line 41: [[NSEvent otherEventWithType:NSApplicationDefined Nit: continuation lines get a 4-, not 2-space indent. http://codereview.chromium.org/343024/diff/6007/6009#newcode50 Line 50: [NSApp postEvent:wakeUpEvent atStart:NO]; But yeah, very classy. http://codereview.chromium.org/343024/diff/6007/6009#newcode51 Line 51: } There's one way I can think of that causes things not to be released at the end of an NSApplication loop cycle. Let's say that RunWork does some work and some stuff gets autoreleased to a DeferredAutoreleasePool. Good. Immediately after that, in the same pass of the -[NSApplication run] loop, RunDelayedWork does some work and winds up spinning its own sub-loop reasons known only to itself. It pumps the wakeUpEvent we posted right out of the queue and then it returns to RunDelayedWork and we make it back to the outer run loop. Now we no longer have anything sitting in the NSApplication NSEvent queue, but we've still got the stuff that RunWork has autoreleased via its deferred pool waiting to be freed. http://codereview.chromium.org/343024/diff/6007/6009#newcode63 Line 63: // RAII class for dealing with autoreleasepools when using messagepumps. autotrelease<space>pools message<space>pumps http://codereview.chromium.org/343024/diff/6007/6009#newcode66 Line 66: ScopedAutoreleasePool(MessagePumpCFRunLoopBase* base) : base sounds like you're referring to a base class or something. How about owner? http://codereview.chromium.org/343024/diff/6007/6009#newcode72 Line 72: DISALLOW_COPY_AND_ASSIGN(ScopedAutoreleasePool); DISALLOW_COPY_AND_ASSIGN should always be the last thing in the private: section. http://codereview.chromium.org/343024/diff/6007/6009#newcode445 Line 445: Too much whitespace. http://codereview.chromium.org/343024/diff/6007/6009#newcode785 Line 785: I don't think you generally need the whitespace line before when the comment starts at an increased indentation level. http://codereview.chromium.org/343024/diff/6007/6009#newcode808 Line 808: DCHECK([NSGarbageCollector defaultCollector] == nil); This should just be an early return, then? Well, only if you implement -release, I guess. This is OK as-is. http://codereview.chromium.org/343024/diff/6007/6009#newcode812 Line 812: // in init because we use the existence of the pool as a flag to we, wheee! http://codereview.chromium.org/343024/diff/6007/6009#newcode814 Line 814: // the app to release its autoreleasepool. autorelease<space>pool http://codereview.chromium.org/343024/diff/6007/6009#newcode815 Line 815: deferredPool_ = [[NSMutableArray alloc] initWithCapacity:1]; TVL wrote: > why 1? The way this is structured now, deferredPool_ creation is deferred until there's something to add. This is correct as dmac has written it. http://codereview.chromium.org/343024/diff/6007/6009#newcode818 Line 818: // Store the object in our pool. Don't call [super addObject] because the our is the same as we http://codereview.chromium.org/343024/diff/6007/6009#newcode825 Line 825: // testing and set some breakpoints and nothing seems to call it. I was wondering about this too. It should only impact debugging, maybe it's only called when certain debugging modes are enabled. http://codereview.chromium.org/343024/diff/6007/6009#newcode835 Line 835: pink wrote: > ditto. Yeah, I think this is my fault. I know I told you "blank line before a comment" but I meant for that to apply to comments where you were staying at the same indentation level. I think that the expected Google and Chrome style in this case would be to put the blank line before the conditional, so we'd have: - (void)drain { // Store off NSMutableArray* deferredPool [super drain] if (deferredPool) { // This autorelease pool http://codereview.chromium.org/343024/diff/6007/6009#newcode844 Line 844: // Must call drain on DeferredAutoreleasePools. I would have implemented this for completeness, but I guess this is fine. You've already made this purpose-specific enough by putting the wake-up stuff right in this class.
http://codereview.chromium.org/343024/diff/6007/6008 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/6007/6008#newcode17 Line 17: // MessagePumpCFRunLoop. For a NSRunLoop, the similar MessagePumpNSRunLoop pink wrote: > i've lost this argument several times. people keep telling me the sound is > "ehhhn" which is a vowel sound (like "endeavor"), so it should be "an", not "a". > *shrug*. I agree with Mike. NSRunLoop is pronounced "en ess run loop," not "nissrunloop."
http://codereview.chromium.org/343024/diff/6007/6009 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/6007/6009#newcode815 Line 815: deferredPool_ = [[NSMutableArray alloc] initWithCapacity:1]; On 2009/10/30 14:18:56, Mark Mentovai wrote: > TVL wrote: > > why 1? > > The way this is structured now, deferredPool_ creation is deferred until there's > something to add. This is correct as dmac has written it. right, but nothing says it won't get more things, so why not let it to its normal default allocation size to be ready for more objects?
http://codereview.chromium.org/343024/diff/6007/6008 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/6007/6008#newcode17 Line 17: // MessagePumpCFRunLoop. For a NSRunLoop, the similar MessagePumpNSRunLoop On 2009/10/30 14:20:53, Mark Mentovai wrote: > I agree with Mike. NSRunLoop is pronounced "en ess run loop," not > "nissrunloop." I agree with Mike, and Mark, and the Apple documentation. From the NSRunLoop page: "An NSRunLoop object processes input for sources such as mouse and keyboard events from the window system, NSPort objects, and NSConnection objects." It's clear that the canonical pronunciation of "NS(.*)" is "en ess \1" and therefore it gets an "an".
Think I covered everything. http://codereview.chromium.org/343024/diff/6007/6008 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/6007/6008#newcode17 Line 17: // MessagePumpCFRunLoop. For a NSRunLoop, the similar MessagePumpNSRunLoop On 2009/10/30 14:30:53, Avi wrote: > On 2009/10/30 14:20:53, Mark Mentovai wrote: > > I agree with Mike. NSRunLoop is pronounced "en ess run loop," not > > "nissrunloop." > > I agree with Mike, and Mark, and the Apple documentation. From the NSRunLoop > page: > > "An NSRunLoop object processes input for sources such as mouse and keyboard > events from the window system, NSPort objects, and NSConnection objects." > > It's clear that the canonical pronunciation of "NS(.*)" is "en ess \1" and > therefore it gets an "an". Agreed. Sorry. Just saw an An followed by a N and it was late. http://codereview.chromium.org/343024/diff/6007/6008#newcode51 Line 51: // A RAII class for controlling creation and releasing of autoreleasepools On 2009/10/30 14:06:06, pink wrote: > RAII? http://www.hackcraft.net/raii/ http://codereview.chromium.org/343024/diff/6007/6008#newcode51 Line 51: // A RAII class for controlling creation and releasing of autoreleasepools On 2009/10/30 14:18:56, Mark Mentovai wrote: > autorelease pools Done. http://codereview.chromium.org/343024/diff/6007/6008#newcode53 Line 53: class ScopedAutoreleasePool; On 2009/10/30 14:18:56, Mark Mentovai wrote: > TVL wrote: > > why not just put this in the imp file in an anon namespace? > > Agree. Because it was calling a protected function within MessagePumpCFRunLoopBase. It no longer does this so I could move it into anon. http://codereview.chromium.org/343024/diff/6007/6008#newcode53 Line 53: class ScopedAutoreleasePool; On 2009/10/30 14:18:56, Mark Mentovai wrote: > ScopedAutoreleasePool isn't a great name. It's too easily confused with > ScopedNSAutoreleasePool. > > Suggestion! > > Extend ScopedNSAutoreleasePool to handle this for us. Add a new flavor of > constructor: > > explicit ScopedNSAutoreleasePool(NSAutoreleasePool* autorelease_pool) > > Gotcha: you'll need to revise ScopedNSAutoreleasePool::Recycle to use > [[[original_pool class] alloc] init] instead of always using NSAutoreleasePool. > > The alternative is to name this one somehow in a way that makes it clear that > it's different from ScopedNSAutoreleasePool. Fixed by changing the name. I put a comment in the file as to why I didn't extend ScopedNSAutoreleasePool. http://codereview.chromium.org/343024/diff/6007/6008#newcode249 Line 249: // MessagePumpNSApplications needs to create a special autorelease pool that On 2009/10/30 14:18:56, Mark Mentovai wrote: > MessagePumpNSApplication needs > > or > > MessagePumpNSApplications need > > but not both. Done. http://codereview.chromium.org/343024/diff/6007/6009 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/6007/6009#newcode1 Line 1: // Copyright (c) 2008 The Chromium Authors. All rights reserved. On 2009/10/30 14:18:56, Mark Mentovai wrote: > Pesky lice. Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode31 Line 31: // Send a null event through to the upper event loop if necessary. On 2009/10/30 14:18:56, Mark Mentovai wrote: > Why "upper event loop?" What are you contrasting "upper" with? How 'bout just > "send a null NSEvent if necessary."? Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode33 Line 33: // If there is already an event waiting in the queue there is no need On 2009/10/30 14:18:56, Mark Mentovai wrote: > queue comma there Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode36 Line 36: if (![NSApp nextEventMatchingMask:NSAnyEventMask On 2009/10/30 14:18:56, Mark Mentovai wrote: > Very classy! thx http://codereview.chromium.org/343024/diff/6007/6009#newcode41 Line 41: [[NSEvent otherEventWithType:NSApplicationDefined On 2009/10/30 14:18:56, Mark Mentovai wrote: > Nit: continuation lines get a 4-, not 2-space indent. !@#$!@#$ Xcode copy/paste http://codereview.chromium.org/343024/diff/6007/6009#newcode51 Line 51: } On 2009/10/30 14:18:56, Mark Mentovai wrote: > There's one way I can think of that causes things not to be released at the end > of an NSApplication loop cycle. > > Let's say that RunWork does some work and some stuff gets autoreleased to a > DeferredAutoreleasePool. Good. > > Immediately after that, in the same pass of the -[NSApplication run] loop, > RunDelayedWork does some work and winds up spinning its own sub-loop reasons > known only to itself. It pumps the wakeUpEvent we posted right out of the queue > and then it returns to RunDelayedWork and we make it back to the outer run loop. > > Now we no longer have anything sitting in the NSApplication NSEvent queue, but > we've still got the stuff that RunWork has autoreleased via its deferred pool > waiting to be freed. Fixed I think. http://codereview.chromium.org/343024/diff/6007/6009#newcode63 Line 63: // RAII class for dealing with autoreleasepools when using messagepumps. On 2009/10/30 14:18:56, Mark Mentovai wrote: > autotrelease<space>pools > > message<space>pumps Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode66 Line 66: ScopedAutoreleasePool(MessagePumpCFRunLoopBase* base) : On 2009/10/30 14:18:56, Mark Mentovai wrote: > base sounds like you're referring to a base class or something. How about > owner? Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode72 Line 72: DISALLOW_COPY_AND_ASSIGN(ScopedAutoreleasePool); On 2009/10/30 14:18:56, Mark Mentovai wrote: > DISALLOW_COPY_AND_ASSIGN should always be the last thing in the private: > section. Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode75 Line 75: /* On 2009/10/30 13:29:11, TVL wrote: > so this isn't needed now? Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode445 Line 445: On 2009/10/30 14:18:56, Mark Mentovai wrote: > Too much whitespace. Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode785 Line 785: On 2009/10/30 14:06:06, pink wrote: > omit blank line Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode785 Line 785: On 2009/10/30 14:18:56, Mark Mentovai wrote: > I don't think you generally need the whitespace line before when the comment > starts at an increased indentation level. Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode787 Line 787: // correctly with nested autorelease pools in runloops. On 2009/10/30 14:06:06, pink wrote: > a comment somewhere about why and how is needed. Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode805 Line 805: On 2009/10/30 14:06:06, pink wrote: > omit blank line Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode810 Line 810: On 2009/10/30 14:06:06, pink wrote: > omit blank line Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode812 Line 812: // in init because we use the existence of the pool as a flag to On 2009/10/30 14:18:56, Mark Mentovai wrote: > we, wheee! Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode814 Line 814: // the app to release its autoreleasepool. On 2009/10/30 14:18:56, Mark Mentovai wrote: > autorelease<space>pool Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode815 Line 815: deferredPool_ = [[NSMutableArray alloc] initWithCapacity:1]; On 2009/10/30 14:24:14, TVL wrote: > On 2009/10/30 14:18:56, Mark Mentovai wrote: > > TVL wrote: > > > why 1? > > > > The way this is structured now, deferredPool_ creation is deferred until > there's > > something to add. This is correct as dmac has written it. > > right, but nothing says it won't get more things, so why not let it to its > normal default allocation size to be ready for more objects? Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode818 Line 818: // Store the object in our pool. Don't call [super addObject] because the On 2009/10/30 14:18:56, Mark Mentovai wrote: > our is the same as we no it's not... our is three letters long, and we is only two. Duh! Fixed. http://codereview.chromium.org/343024/diff/6007/6009#newcode825 Line 825: // testing and set some breakpoints and nothing seems to call it. On 2009/10/30 14:18:56, Mark Mentovai wrote: > I was wondering about this too. It should only impact debugging, maybe it's > only called when certain debugging modes are enabled. I did some experimentation and didn't see anything. Took a look at the disassembly and didn't see anything there either. I'm going to leave the comment for future generations to appreciate that we thought about it at least. http://codereview.chromium.org/343024/diff/6007/6009#newcode829 Line 829: On 2009/10/30 14:06:06, pink wrote: > omit blank line Done. http://codereview.chromium.org/343024/diff/6007/6009#newcode835 Line 835: On 2009/10/30 14:18:56, Mark Mentovai wrote: > pink wrote: > > ditto. > > Yeah, I think this is my fault. I know I told you "blank line before a comment" > but I meant for that to apply to comments where you were staying at the same > indentation level. > > I think that the expected Google and Chrome style in this case would be to put > the blank line before the conditional, so we'd have: > > - (void)drain { > // Store off > NSMutableArray* deferredPool > [super drain] > > if (deferredPool) { > // This autorelease pool Done.
Very nice. Almost done. Functionally, this is almost perfect now. There's a missing flag-clear that you need to address somehow. http://codereview.chromium.org/343024/diff/25/2024 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/25/2024#newcode1 Line 1: // Copyright (c) 2008-2009 The Chromium Authors. All rights reserved. DOH I was just kidding when I said "pesky lice." As a matter of civil disobedience, I never update these. I *think* that we're just supposed to put the most recent year in here now, so just 2009? They keep changing the rules on us, though. That's why I never update the dates: that's what the rule once was, and that's the rule I like best. (Practically, it doesn't matter, and as much as I'd like to say IANAL, well, in this instance...) http://codereview.chromium.org/343024/diff/25/2024#newcode253 Line 253: void set_needs_event_loop_wakeup(); The implementation should be inline. The implementation should use true, not YES, because the member is declared as bool, not BOOL. Also, this looks like a standard mutator which would accept a bool argument, but it's actually a "true-ifier". Maybe rename it to set_needs_event_loop_wakeup_true(), or just let it take the bool argument. http://codereview.chromium.org/343024/diff/25/2024#newcode271 Line 271: // when the runloop is exiting. run loop http://codereview.chromium.org/343024/diff/25/2024#newcode272 Line 272: bool needs_event_loop_wake_up_; Do you want needs_event_loop_wakeup (as in the method) or needs_event_loop_wake_up (as here)? http://codereview.chromium.org/343024/diff/25/2025 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/25/2025#newcode21 Line 21: // autorelease pool stack when -[drain] is called. See its implementation -drain or -[DeferredAutoreleasePool drain] or |drain|, lately I've just been using -drain. http://codereview.chromium.org/343024/diff/25/2025#newcode29 Line 29: // Informs the message pump that it has been drained. I like the separation. http://codereview.chromium.org/343024/diff/25/2025#newcode51 Line 51: // misuse, and the case where you pass in a kind of NSAutoreleasePool that you you is like we and our http://codereview.chromium.org/343024/diff/25/2025#newcode53 Line 53: class AutoreleasePoolOwner { Good choice on the name. http://codereview.chromium.org/343024/diff/25/2025#newcode55 Line 55: AutoreleasePoolOwner(NSAutoreleasePool *pool) : pool_(pool) {} Mark constructors taking a single argument as "explicit" to avoid implicit conversions. http://codereview.chromium.org/343024/diff/25/2025#newcode68 Line 68: No reason to add me. http://codereview.chromium.org/343024/diff/25/2025#newcode582 Line 582: On all of the functions in here, I put a quick comment before the comment saying who and where the callers are. It sort of helps follow the flow. This should be something like // Called on the run loop thread by RunWork, RunDelayedWork, and RunIdleWork. http://codereview.chromium.org/343024/diff/25/2025#newcode583 Line 583: NSAutoreleasePool* MessagePumpCFRunLoopBase::CreateAutoreleasePool() { Just put a quick comment inside this method: "By default, a normal NSAutoreleasePool can be used for each source as it fires." http://codereview.chromium.org/343024/diff/25/2025#newcode709 Line 709: // Wake up the event loop. This comment isn't helpful now that the function's got an awesome descriptive name. http://codereview.chromium.org/343024/diff/25/2025#newcode717 Line 717: } Uh-oh, needs_event_loop_wake_up_ needs to be reset to false now. http://codereview.chromium.org/343024/diff/25/2025#newcode721 Line 721: // Return an instance of a special autorelease pool that deals Per e-mail. http://codereview.chromium.org/343024/diff/25/2025#newcode790 Line 790: needs_event_loop_wake_up_ = YES; Comments in the other file, where this should move. http://codereview.chromium.org/343024/diff/25/2025#newcode823 Line 823: void SetNeedsEventLoopWakeUp(MessagePumpNSApplication *pump) { What side is the * on? http://codereview.chromium.org/343024/diff/25/2025#newcode839 Line 839: // the app to release its autoreleasepool. autorelease pool http://codereview.chromium.org/343024/diff/25/2025#newcode843 Line 843: // Store the object in local pool. Don't call [super addObject] because the Store the object in (what?) local pool. Store the object in deferredPool_? Nah, that's kind of obvious from the next line. But you might want a comment saying why we release, because it might not be immediately obvious why we're releasing something in the method that's supposed to be handling AUTOreleasing. http://codereview.chromium.org/343024/diff/25/2025#newcode850 Line 850: // some testing and set some breakpoints and nothing seems to call it. Nice. http://codereview.chromium.org/343024/diff/25/2025#newcode855 Line 855: // free on "self" and it is needed after drain. NSAutoreleasepools are "it" can refer back to several things this late in the sentence, including 'self" and "free" and maybe even "-[super drain]", but what you really want is deferredPool_. BONUS: NSAutoreleasePools http://codereview.chromium.org/343024/diff/25/2025#newcode859 Line 859: // deferredPool_ on the stack before calling drain a good reference to calling drain<comma> a good reference http://codereview.chromium.org/343024/diff/25/2025#newcode863 Line 863: if (deferredPool) { This check isn't needed. It's fine to just do [deferredPool autorelease] outright. http://codereview.chromium.org/343024/diff/25/2025#newcode876 Line 876: // Asserting that dealloc is truely never called on a NSAutoreleasePool. truly http://codereview.chromium.org/343024/diff/25/2025#newcode877 Line 877: - (void)dealloc { Is this necessary? -[NSAutoreleasePool dealloc] already has this check, and it complains really loudly. Seems like this is just a few extra bytes of code to reimplement what super already does. http://codereview.chromium.org/343024/diff/25/2025#newcode895 Line 895: // Set flag in pump_ so that it wakes up the event loop when exiting its Set a flag http://codereview.chromium.org/343024/diff/25/2025#newcode896 Line 896: // runloop. Calling through C-func trampoline to get around problem where run loop C-func isn't strictly accurate and it doesn't really contribute anything here, just leave it out or replace it with the word "a". Unless it's an NSTrampoline, then say "an"
/me Calling Guinness to see if there is a record for most comments on a single CL. http://codereview.chromium.org/343024/diff/25/2024 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/25/2024#newcode1 Line 1: // Copyright (c) 2008-2009 The Chromium Authors. All rights reserved. On 2009/10/30 20:48:37, Mark Mentovai wrote: > DOH > > I was just kidding when I said "pesky lice." As a matter of civil disobedience, > I never update these. > > I *think* that we're just supposed to put the most recent year in here now, so > just 2009? They keep changing the rules on us, though. That's why I never > update the dates: that's what the rule once was, and that's the rule I like > best. > > (Practically, it doesn't matter, and as much as I'd like to say IANAL, well, in > this instance...) Done. http://codereview.chromium.org/343024/diff/25/2024#newcode253 Line 253: void set_needs_event_loop_wakeup(); On 2009/10/30 20:48:37, Mark Mentovai wrote: > The implementation should be inline. > > The implementation should use true, not YES, because the member is declared as > bool, not BOOL. > > Also, this looks like a standard mutator which would accept a bool argument, but > it's actually a "true-ifier". Maybe rename it to > set_needs_event_loop_wakeup_true(), or just let it take the bool argument. Done. http://codereview.chromium.org/343024/diff/25/2024#newcode271 Line 271: // when the runloop is exiting. On 2009/10/30 20:48:37, Mark Mentovai wrote: > run loop Done. http://codereview.chromium.org/343024/diff/25/2024#newcode272 Line 272: bool needs_event_loop_wake_up_; On 2009/10/30 20:48:37, Mark Mentovai wrote: > Do you want needs_event_loop_wakeup (as in the method) or > needs_event_loop_wake_up (as here)? I'm confused. If you are asking if it is wake_up or wakeup, wakeup isn't a word. If you are asking if I should implement an accessor, I decided not to as needs_event_loop_wake_up_ is used privately. http://codereview.chromium.org/343024/diff/25/2025 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/25/2025#newcode21 Line 21: // autorelease pool stack when -[drain] is called. See its implementation On 2009/10/30 20:48:37, Mark Mentovai wrote: > -drain or -[DeferredAutoreleasePool drain] or |drain|, lately I've just been > using -drain. Done. http://codereview.chromium.org/343024/diff/25/2025#newcode51 Line 51: // misuse, and the case where you pass in a kind of NSAutoreleasePool that you On 2009/10/30 20:48:37, Mark Mentovai wrote: > you is like we and our Done. http://codereview.chromium.org/343024/diff/25/2025#newcode55 Line 55: AutoreleasePoolOwner(NSAutoreleasePool *pool) : pool_(pool) {} On 2009/10/30 20:48:37, Mark Mentovai wrote: > Mark constructors taking a single argument as "explicit" to avoid implicit > conversions. Done. http://codereview.chromium.org/343024/diff/25/2025#newcode68 Line 68: On 2009/10/30 20:48:37, Mark Mentovai wrote: > No reason to add me. Done. http://codereview.chromium.org/343024/diff/25/2025#newcode582 Line 582: On 2009/10/30 20:48:37, Mark Mentovai wrote: > On all of the functions in here, I put a quick comment before the comment saying > who and where the callers are. It sort of helps follow the flow. This should > be something like > > // Called on the run loop thread by RunWork, RunDelayedWork, and RunIdleWork. Done. http://codereview.chromium.org/343024/diff/25/2025#newcode583 Line 583: NSAutoreleasePool* MessagePumpCFRunLoopBase::CreateAutoreleasePool() { On 2009/10/30 20:48:37, Mark Mentovai wrote: > Just put a quick comment inside this method: "By default, a normal > NSAutoreleasePool can be used for each source as it fires." Done. http://codereview.chromium.org/343024/diff/25/2025#newcode709 Line 709: // Wake up the event loop. On 2009/10/30 20:48:37, Mark Mentovai wrote: > This comment isn't helpful now that the function's got an awesome descriptive > name. Done. http://codereview.chromium.org/343024/diff/25/2025#newcode717 Line 717: } On 2009/10/30 20:48:37, Mark Mentovai wrote: > Uh-oh, needs_event_loop_wake_up_ needs to be reset to false now. Done. http://codereview.chromium.org/343024/diff/25/2025#newcode790 Line 790: needs_event_loop_wake_up_ = YES; On 2009/10/30 20:48:37, Mark Mentovai wrote: > Comments in the other file, where this should move. Done. http://codereview.chromium.org/343024/diff/25/2025#newcode823 Line 823: void SetNeedsEventLoopWakeUp(MessagePumpNSApplication *pump) { On 2009/10/30 20:48:37, Mark Mentovai wrote: > What side is the * on? The right side (in both meanings of the word) IMHO ;-) Sorry. http://codereview.chromium.org/343024/diff/25/2025#newcode839 Line 839: // the app to release its autoreleasepool. On 2009/10/30 20:48:37, Mark Mentovai wrote: > autorelease pool Done. http://codereview.chromium.org/343024/diff/25/2025#newcode843 Line 843: // Store the object in local pool. Don't call [super addObject] because the On 2009/10/30 20:48:37, Mark Mentovai wrote: > Store the object in (what?) local pool. > > Store the object in deferredPool_? Nah, that's kind of obvious from the next > line. > > But you might want a comment saying why we release, because it might not be > immediately obvious why we're releasing something in the method that's supposed > to be handling AUTOreleasing. Done. http://codereview.chromium.org/343024/diff/25/2025#newcode855 Line 855: // free on "self" and it is needed after drain. NSAutoreleasepools are On 2009/10/30 20:48:37, Mark Mentovai wrote: > "it" can refer back to several things this late in the sentence, including > 'self" and "free" and maybe even "-[super drain]", but what you really want is > deferredPool_. > > BONUS: NSAutoreleasePools Done. http://codereview.chromium.org/343024/diff/25/2025#newcode859 Line 859: // deferredPool_ on the stack before calling drain a good reference to On 2009/10/30 20:48:37, Mark Mentovai wrote: > calling drain<comma> a good reference Done. http://codereview.chromium.org/343024/diff/25/2025#newcode863 Line 863: if (deferredPool) { On 2009/10/30 20:48:37, Mark Mentovai wrote: > This check isn't needed. It's fine to just do [deferredPool autorelease] > outright. Performance optimization???? Fixed. http://codereview.chromium.org/343024/diff/25/2025#newcode876 Line 876: // Asserting that dealloc is truely never called on a NSAutoreleasePool. On 2009/10/30 20:48:37, Mark Mentovai wrote: > truly Done. http://codereview.chromium.org/343024/diff/25/2025#newcode877 Line 877: - (void)dealloc { On 2009/10/30 20:48:37, Mark Mentovai wrote: > Is this necessary? -[NSAutoreleasePool dealloc] already has this check, and it > complains really loudly. Seems like this is just a few extra bytes of code to > reimplement what super already does. Done. http://codereview.chromium.org/343024/diff/25/2025#newcode895 Line 895: // Set flag in pump_ so that it wakes up the event loop when exiting its On 2009/10/30 20:48:37, Mark Mentovai wrote: > Set a flag Done.
http://codereview.chromium.org/343024/diff/25/2024 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/25/2024#newcode272 Line 272: bool needs_event_loop_wake_up_; dmac wrote: > I'm confused. If you are asking if it is wake_up or wakeup, wakeup isn't a word. > If you are asking if I should implement an accessor, I decided not to as > needs_event_loop_wake_up_ is used privately. I meant that you named the setter with "wakeup" (no underscore) but you named this with "wake_up" (underscore).
In the TEST= section of the description, please list the three test cases for this bug. One of them is the test case from bug 25857. The other two are the test cases for the workarounds you're removing. LGTM with this very small number of changes. Congratulations, you've qualified for Chromium Readability! http://codereview.chromium.org/343024/diff/8024/8025 File base/message_pump_mac.h (right): http://codereview.chromium.org/343024/diff/8024/8025#newcode237 Line 237: // ObjC objects and C++ objects can't be friends, so this C function remove letter C http://codereview.chromium.org/343024/diff/8024/8026 File base/message_pump_mac.mm (right): http://codereview.chromium.org/343024/diff/8024/8026#newcode20 Line 20: // This pool passes the objects stored in it to the pool above it in the pool above or pool below? Stacks are usually described as growing up, right? (But the pointers decrease. Horrors!) The way you've used "above" here seems contrary to how you've used it on line 764. I think this should be "below." http://codereview.chromium.org/343024/diff/8024/8026#newcode48 Line 48: // This class is independant from scoped_nsautoreleasepool because having The name of the class is actually ScopedNSAutoreleasePool. http://codereview.chromium.org/343024/diff/8024/8026#newcode801 Line 801: // MessagePumpNSApplication::EnterExitRunLoop. We send the event just as the Final we. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
